Add health checks for Mongodb, Kafka and Neo4j #5222

loicmathieu · 2019-11-05T15:18:57Z

This fixes #5073

This is a draft PR as the Kafka health check is subject to discussion:

it instanciate a new AdminClient as the check is on quarkus-kafka-client that didn't provides any Kafka client inside CDI.
it uses the (standard) property kafka.bootstrap.servers as a temporary solution : a better solution should be to scan all properties and find the one that defines a Kafka cluster (from mp-messaging or kafka-stream config) and creates an AdminClient for each.
it is disabled by default because I think it's the best things to do (see the rational on the issue description).

When #3343 lands in, we must impact the MongoDB health check to check all the mongo clients.

gastaldi · 2019-11-05T21:17:26Z

This looks great.

Here is an idea: would it be possible to enable them automatically if the quarkus-smallrye-health extension is present? I think this would save the developer the trouble of having to define the health=enabled flag for these services.

gastaldi · 2019-11-05T21:18:33Z

Ah ignore my idea, I think you already do that :) Awesome!

machi1990 · 2019-11-05T21:52:15Z

extensions/neo4j/deployment/src/main/java/io/quarkus/neo4j/deployment/Neo4jBuildTimeConfig.java

+@ConfigRoot(name = "neo4j", phase = ConfigPhase.BUILD_TIME)
+public class Neo4jBuildTimeConfig {
+    /**
+     * Whether or not an healt check is published in case the smallrye-health extension is present (default to true).


Maybe we can remove the (default to true) from the description. It will be automatically picked from ConfigItem during doc generation.

Yes but the JavaDoc is also usefull for Quarkus developpers :)
Maybe I could but best is to make it after to keep all health check enabled comments the same (agroal, these 3 new ones, kafka-streams)

loicmathieu · 2019-11-12T10:00:21Z

@gunnarmorling can you review the Kafka health check implementation and some question I raised inside the PR description ?

jmartisk · 2019-11-14T10:34:17Z

extensions/neo4j/runtime/src/main/java/io/quarkus/neo4j/runtime/heath/Neo4jHealthCheck.java

+            driver.verifyConnectivity();
+            return builder.build();
+        } catch (Exception e) {
+            return builder.down().withData("reason", e.getMessage()).build();


Not that it matters much, so just nitpicking that it is a little weird if you first call up() and then change it to down(). To me reading this, the first impression was "So this always returns 'up', why?". You could call up() only after verifyConnectivity() passes. Otherwise a big +1 from me

This constuct is inspired by the Agroal health check because of multiple datasource support. When we will support multiple MongoDB connection/ Kafka connection / ... this construct will be needed (because you will fails the health check if one of the multiple connection fails).
So better keep it this way.

loicmathieu · 2019-11-28T16:34:43Z

Can someone review the Kafka part and gives feedback of the fact that I choose to disabled the health check by default (rational is in the issue description)? Maybe @gunnarmorling or @cescoffier ?

gunnarmorling

Nice work, @loicmathieu; a few comments on the Kafka part.

docs/src/main/asciidoc/kafka.adoc

...client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaBuildTimeConfig.java

...kafka-client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaProcessor.java

extensions/neo4j/deployment/src/main/java/io/quarkus/neo4j/deployment/Neo4jBuildTimeConfig.java

...ions/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/health/KafkaHealthCheck.java

gsmet · 2019-11-29T10:20:41Z

Just a heads up that I started a review of this one.

I'm working on the Qute PR today but will get back to this one soon.

loicmathieu · 2019-12-04T09:08:10Z

@gunnarmorling WDYT regarding my latests comments on this PR?

...client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaBuildTimeConfig.java

loicmathieu · 2019-12-06T11:36:25Z

I rebase on master to fix a conflict.

@gsmet did you have time to review it as you intended ?

gsmet · 2019-12-08T14:09:20Z

I adjusted a few things.

gsmet · 2019-11-28T18:31:00Z

docs/src/main/asciidoc/mongodb.adoc

+
+So when you access the `/health/ready` endpoint of your application you will have information about the connection validation status.
+
+This behavior can be disabled via the property `quarkus.mongodb.health.enabled`.


Suggested change

This behavior can be disabled via the property `quarkus.mongodb.health.enabled`.

This behavior can be disabled by setting the property `quarkus.mongodb.health.enabled` to `false` in your `application.properties`.

Same for the others.

gsmet · 2019-11-28T18:31:29Z

...client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaBuildTimeConfig.java

+@ConfigRoot(name = "kafka", phase = ConfigPhase.BUILD_TIME)
+public class KafkaBuildTimeConfig {
+    /**
+     * Whether or not an health check is published in case the smallrye-health extension is present (default to false).


Suggested change

* Whether or not an health check is published in case the smallrye-health extension is present (default to false).

* Whether or not an health check is published in case the smallrye-health extension is present (defaults to false).

gsmet · 2019-12-08T14:57:48Z

Merged, thanks.

loicmathieu · 2019-12-09T08:47:32Z

@gsmet you move the bootstrap server property from the standard one to a quarkus specific one.
According to me it's an issue as the standard one is already used when using smallrye-reactive-messaging Kafka (as it uses standard producer and consumer properties).
So if a user uses the quarkus-smallrye-reactive-messaging-kafka extension it will needs to provides the boostrap-server in two properties: a quarkus one and a standard one.

zeljkot · 2020-01-11T22:47:20Z

Having two separate properties for reactive streams and health is very confusing. The need for additional property in "Guides" can be easily overlooked. Please emphasize that in the documentation, or add all messaging bootstrap servers automatically. IMHO health check should not be general Kafka health check, but rather reactive messaging specific one; otherwise, whoever writes a Kafka client should somehow register it with health check functionality.

machi1990 reviewed Nov 5, 2019

View reviewed changes

jmartisk reviewed Nov 14, 2019

View reviewed changes

loicmathieu mentioned this pull request Nov 22, 2019

Health support for 50% of the relevant extensions #5701

Closed

gunnarmorling reviewed Nov 28, 2019

View reviewed changes

loicmathieu force-pushed the feat/mongodb-kafka-neo4j-health branch from 4d8f254 to d3c1314 Compare November 29, 2019 10:14

loicmathieu marked this pull request as ready for review November 29, 2019 10:16

gunnarmorling reviewed Dec 4, 2019

View reviewed changes

...client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaBuildTimeConfig.java Outdated Show resolved Hide resolved

loicmathieu force-pushed the feat/mongodb-kafka-neo4j-health branch from d3c1314 to 3043525 Compare December 4, 2019 13:22

loicmathieu added this to the 1.1.0 milestone Dec 6, 2019

loicmathieu force-pushed the feat/mongodb-kafka-neo4j-health branch from 3043525 to c033695 Compare December 6, 2019 11:36

loicmathieu and others added 4 commits December 8, 2019 12:11

feat: mongodb health check

670e224

feat: kafka client health check

aa601cd

feat: neo4j health check

ec426da

Various editorializations for the health checks documentation

94b59e3

gsmet force-pushed the feat/mongodb-kafka-neo4j-health branch from c033695 to 94b59e3 Compare December 8, 2019 11:18

gsmet added 2 commits December 8, 2019 13:21

Separate Kafka and Kafka Streams integration tests

da20cf7

Make the Kafka health check configuration more future proof

397424e

gsmet approved these changes Dec 8, 2019

View reviewed changes

gsmet merged commit b76f837 into quarkusio:master Dec 8, 2019

loicmathieu deleted the feat/mongodb-kafka-neo4j-health branch December 9, 2019 09:37

gsmet changed the title ~~Add health check for mongodb, kafka and neo4j~~ Add health checks for Mongodb, Kafka and Neo4j Dec 9, 2019

gsmet added the release/noteworthy-feature label Dec 9, 2019


		So when you access the `/health/ready` endpoint of your application you will have information about the connection validation status.

		This behavior can be disabled via the property `quarkus.mongodb.health.enabled`.

	This behavior can be disabled via the property `quarkus.mongodb.health.enabled`.
	This behavior can be disabled by setting the property `quarkus.mongodb.health.enabled` to `false` in your `application.properties`.

	* Whether or not an health check is published in case the smallrye-health extension is present (default to false).
	* Whether or not an health check is published in case the smallrye-health extension is present (defaults to false).

Add health checks for Mongodb, Kafka and Neo4j #5222

Add health checks for Mongodb, Kafka and Neo4j #5222

Uh oh!

Conversation

loicmathieu commented Nov 5, 2019

Uh oh!

gastaldi commented Nov 5, 2019

Uh oh!

gastaldi commented Nov 5, 2019

Uh oh!

machi1990 Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

loicmathieu Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

loicmathieu commented Nov 12, 2019

Uh oh!

jmartisk Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loicmathieu Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

loicmathieu commented Nov 28, 2019

Uh oh!

gunnarmorling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsmet commented Nov 29, 2019

Uh oh!

loicmathieu commented Dec 4, 2019

Uh oh!

Uh oh!

loicmathieu commented Dec 6, 2019

Uh oh!

gsmet commented Dec 8, 2019

Uh oh!

gsmet Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

gsmet Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

gsmet commented Dec 8, 2019

Uh oh!

loicmathieu commented Dec 9, 2019

Uh oh!

zeljkot commented Jan 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jmartisk Nov 14, 2019 •

edited

Loading