Health support #130

danielkec · 2021-02-24T15:35:49Z

Fixes #47

Signed-off-by: Daniel Kec daniel.kec@oracle.com

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>

eclipse-microprofile-bot · 2021-02-24T15:40:02Z

Can one of the admins verify this patch?

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>

spec/src/main/asciidoc/architecture.asciidoc

tomas-langer · 2021-02-25T10:55:18Z

spec/src/main/asciidoc/architecture.asciidoc

+
+=== Liveness
+
+Liveness check `messaging` gets DOWN only when `cancel` or `onError` signals are detected on any of the messaging channels.


Similar to Readiness - define if it starts in UP or DOWN state, then define states that trigger change

What should happen if a message gets nacked?

I don't believe acknowledgement should be related to the ability of the channel to send messages as messaging itself doesn't leverage acking/nacking anyhow. It should be responsibility of the publishers and subscribers(connectors) as that requirement can change case by case. In other words such checks should be present in the connectors or business code if or when needed.

spec/src/main/asciidoc/architecture.asciidoc

tck/pom.xml

...clipse/microprofile/reactive/messaging/tck/health/HealthCancelledConnectorInChannelTest.java

spec/src/main/asciidoc/architecture.asciidoc

* Rewording definition of state transition description of liveness and readiness * Move config key to messaging context * Document test cases * Utilize ArchiveExtender Signed-off-by: Daniel Kec <daniel.kec@oracle.com>

* Revert trailing whitespaces Signed-off-by: Daniel Kec <daniel.kec@oracle.com>

cescoffier

I did a quick first pass.

In addition to the comments, I made in the code, the TCK does not seem to cover Emitter, unmanaged channels, and nacked messages.

The TCK relies on HTTP, which is a bit annoying, as it makes running the TCK much more framework-specific. Typically, I would need to use Quarkus (and so create a cycle between out implementation and Quarkus), instead of just Weld.

cescoffier · 2021-03-04T08:23:32Z

spec/src/main/asciidoc/architecture.asciidoc

@@ -1331,6 +1331,44 @@ The connector is responsible for the acknowledgment (positive or negative) of th
 * An outgoing connector must acknowledge the incoming `org.eclipse.microprofile.reactive.messaging.Message` once it has successfully dispatched the message.
 * An outgoing connector must acknowledge negatively the incoming `org.eclipse.microprofile.reactive.messaging.Message` if it cannot be dispatched.

+== Health
+
+When MicroProfile Reactive Messaging is used in an environment where MicroProfile Health is enabled, implicit readiness


readiness might be misleading. Do you mean "ready to get traffic" or "started successfully"? Kubernetes has readiness and startup checks, and readiness is focusing on traffic, which most of the time is ignored by messaging protocol (as the probe are used for HTTP request routing).

I ve been thinking about use-cases of producing/consuming RS channels with websocket or SSE

cescoffier · 2021-03-04T08:26:46Z

spec/src/main/asciidoc/architecture.asciidoc

+== Health
+
+When MicroProfile Reactive Messaging is used in an environment where MicroProfile Health is enabled, implicit readiness
+and liveness checks `messaging` are produced.


messaging might not be explicit enough, or conflict with user checks. What about MicroProfile Reactive Messaging?

You are right, I just can't find any definitive convention for naming health checks, maybe mp-reactive-messaging would make it more machine readable?

cescoffier · 2021-03-04T08:28:09Z

spec/src/main/asciidoc/architecture.asciidoc

+  ]
+}
+----
+


In the case, we don't pass the checks, what extra data should we provide? None won't be very useful (even if these checks tend to be consumed by machines ignoring the extra data). Should we list the unsatisfied channels? Is it implementation specific?

I was playing with that idea but couldn't come with any sufficient reason for requiring channel listing in the checks

cescoffier · 2021-03-04T08:29:30Z

spec/src/main/asciidoc/architecture.asciidoc

+=== Liveness
+
+State of the liveness check `messaging` is UP until `cancel` or `onError` signals are detected on any of the messaging
+channels.


Not sure about cancellations. We have seen applications using cancellations as part of their regular lifecycle.
Failure definitely.

We have also seen use-cases with failures as part of its regular lifecycle (onErrorResumeWith, ...), but as I see it channel is a simple processor which is not mutating the stream(except for un/wrapping), so any cancel or onError signal passing thru channel renders it DOWN for good

Problem with channels where retries/re-subscriptions are expected should be solvable with exclusions(inclusions?)

cescoffier · 2021-03-04T08:38:56Z

spec/src/main/asciidoc/architecture.asciidoc

+
+State of the liveness check `messaging` is UP until `cancel` or `onError` signals are detected on any of the messaging
+channels.
+


Wondering about "all channels" while it seems to only require checking the final subscribers (subscriber methods, outgoing channels).

Also, about @Channel injection (unmanaged stream). It might be hard to track, as in this case, the subscription is delegated to the users. So, all the used channels may not receive their subscription, because the user has not subscribed yet (and it does not mean it's not ready).

Maybe it should focus on:

subscriber methods:

for readiness, must have received a subscription,

for liveness, a received failure would trigger the liveness check to be DOWN

outgoing connector

for readiness, must have received a subscription,

for liveness, a received failure would trigger the liveness check to be DOWN, an unrecoverable (like serialization) failure in the connector logic would trigger the liveness check to be DOWN.

incoming connector

for liveness, any unrecoverable failure would set the liveness check to DOWN

for readiness, it can be tricky, as we may not have a downstream subscription yet (because of unmanaged streams) (we are actually seen reports about that in Quarkus)

Intermediate channels can recover from failures, and implement retry logic, so it should not be reported as a DOWN.

This would be quite a different angle than the one I was trying to take.

There are not much parts of reactive pipelines we are actually managing in the MP Messaging impls and those are the channels. Channels are basically processors which are not mutating the stream (except for un/wrapping) so if cancel or 'onError' signal passes thru it, it can be considered as failed. Any recovery made by unmanaged streams is just re-routing the stream thru different chain of operators or retry/resubscribe.

I am not sure that the health of connectors, methods and unmanaged streams should be in the scope of MP Messaging implicit health check. I would count those as business code where health and readiness can be monitored with standard MP Health API.

Also implicit checks should be consistent, if we differentiate its behavior by its pub/sub kinds, we risk user confusion. Again custom check with exclusion/inclusion can help in such cases.

cescoffier · 2021-03-04T08:40:10Z

spec/src/main/asciidoc/architecture.asciidoc

+[source, properties]
+----
+mp.messaging.health.live.exclude=<channel-name>,<channel-name>
+mp.messaging.health.ready.exclude=<channel-name>,<channel-name>


If we follow the logic I described above, this would only make sense for subscriber methods. For connectors, it would be better to have a specific attribute and keep the connector configuration co-localized.

cescoffier · 2021-03-04T08:42:52Z

tck/src/main/java/org/eclipse/microprofile/reactive/messaging/tck/health/HealthAssertions.java

+     */
+    static HealthAssertions create(String url) {
+        try {
+            HttpURLConnection con = (HttpURLConnection) new URL(url).openConnection();


Do we have to use HTTP to verify the checks? Doesn't Microprofile Health expose an API (asking the question as I never checked).

That would be great, I didn't found other way. MP Health TCKs use same approach

Emily-Jiang · 2021-03-04T09:52:39Z

...clipse/microprofile/reactive/messaging/tck/health/HealthCancelledConnectorInChannelTest.java

+ * </ul>
+ */
+@RunWith(Arquillian.class)
+public class HealthCancelledConnectorInChannelTest extends HealthBase {


The test should only run if MP Health is present. One of the solutions is to load Readiness api to check whther MP Health is enabled or not. This comment apply to all of the tests.

IMHO if the feature is required when mp health is present, mp health needs to be required by TCKs. Other way around feature would be effectively optional.

The MP RM feature does not need MP Health to be enabled. Only this integration comes into action if MP health is enabled. My point is that the tck will fail if a vendor does not support MP Health. However, MP Heath is not mandatory hence the tcks should only be activated if MP health api can be loaded in the runtime.

Implicit health check eclipse#47

d0f0c9c

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>

Timeout cleanup

4cba0f5

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>

This was referenced Feb 24, 2021

WIP: Messaging implicit health exclusions helidon-io/helidon#2790

Closed

Health support #47

Open

Azquelt reviewed Feb 25, 2021

View reviewed changes

spec/src/main/asciidoc/architecture.asciidoc Outdated Show resolved Hide resolved

tomas-langer suggested changes Feb 25, 2021

View reviewed changes

Emily-Jiang reviewed Feb 25, 2021

View reviewed changes

spec/src/main/asciidoc/architecture.asciidoc Outdated Show resolved Hide resolved

Emily-Jiang reviewed Feb 25, 2021

View reviewed changes

spec/src/main/asciidoc/architecture.asciidoc Outdated Show resolved Hide resolved

danielkec added 2 commits February 25, 2021 21:50

Review issues

1e3caf6

* Rewording definition of state transition description of liveness and readiness * Move config key to messaging context * Document test cases * Utilize ArchiveExtender Signed-off-by: Daniel Kec <daniel.kec@oracle.com>

Review issues

923a105

* Revert trailing whitespaces Signed-off-by: Daniel Kec <daniel.kec@oracle.com>

cescoffier requested changes Mar 4, 2021

View reviewed changes

Emily-Jiang reviewed Mar 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Health support #130

Health support #130

danielkec commented Feb 24, 2021

eclipse-microprofile-bot commented Feb 24, 2021

tomas-langer Feb 25, 2021

Emily-Jiang Feb 25, 2021

danielkec Feb 25, 2021

cescoffier left a comment •

edited

Loading

cescoffier Mar 4, 2021

danielkec Mar 5, 2021

cescoffier Mar 4, 2021

danielkec Mar 5, 2021

cescoffier Mar 4, 2021

danielkec Mar 4, 2021

cescoffier Mar 4, 2021

danielkec Mar 4, 2021

danielkec Mar 4, 2021

cescoffier Mar 4, 2021

danielkec Mar 4, 2021

cescoffier Mar 4, 2021

cescoffier Mar 4, 2021

danielkec Mar 5, 2021

Emily-Jiang Mar 4, 2021

danielkec Mar 5, 2021

Emily-Jiang Apr 28, 2021


		=== Liveness

		Liveness check `messaging` gets DOWN only when `cancel` or `onError` signals are detected on any of the messaging channels.


		State of the liveness check `messaging` is UP until `cancel` or `onError` signals are detected on any of the messaging
		channels.

Health support #130

Are you sure you want to change the base?

Health support #130

Conversation

danielkec commented Feb 24, 2021

eclipse-microprofile-bot commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cescoffier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cescoffier left a comment •

edited

Loading