Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAMEL-19648: readiness/liveness probes #1010

Merged
merged 2 commits into from
Nov 20, 2023
Merged

Conversation

Croway
Copy link
Contributor

@Croway Croway commented Nov 17, 2023

No description provided.

management.endpoint.health.group.readiness.include=readinessState,camelReadinessState
----

Using camel specific readiness and liveness health indicators, the probes will be augmented with camel components
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use upper case Camel

@@ -0,0 +1,34 @@
package org.apache.camel.spring.boot.actuate.health;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add license header

@@ -0,0 +1,39 @@
package org.apache.camel.spring.boot.actuate.health.liveness;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add license header

@@ -0,0 +1,51 @@
package org.apache.camel.spring.boot.actuate.health.readiness;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add license header

if (!HealthCheck.State.UP.equals(result.getState())) {
isUp = false;

result.getError().ifPresent(error -> log.warn(result.getCheck().getId(), error));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm do you want WARN logs every time this is invoked and there is some kind of error.

I think either this need to be an option your can turn on|off

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can lower the log level to debug, I think it makes sense to log the error since by default the readiness/liveness probe shows only UP or DOWN without further information.

Anyway I'll add an option CamelHealthCheckConfigurationProperties to enable/disable the log

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does spring boot not have more details you can set/enrich to the object your return. It should not be every custom check that has to do their own kind of logging or whatever - SB should really deal with this generally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you can configure management.endpoint.health.group.liveness.show-details=always but, it will only add information regarding the beans status, ex:

{
   "status":"UP",
   "components":{
      "camelLivenessState":{
         "status":"UP"
      },
      "livenessState":{
         "status":"UP"
      }
   }
}

nothing really interesting for debug, anyway, kubernetes probes does not need it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the message itself, is the id (result.getCheck().getId()) good enough as message content?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davsclaus in case of a disconnected kafka consumer, the error is logged only when http://localhost:8080/actuator/health/readiness or http://localhost:8080/actuator/health/ are invoked, but the console is full of warns from kafka, do you think it is worth to have a flag to enable/disable the warn for the probes?

public class CamelAvailabilityCheckAutoConfiguration {

@Bean
@ConditionalOnMissingBean(CamelLivenessStateHealthIndicator.class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case, can it be useful to allow a custom creation of a CamelLivenessStateHealthIndicator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think there is a real usecase, but we have the ConditionalOnMissingBean for almost all the beans in camel (health check one for example), but I agree with you, I do not see you someone should use a custom implementation, I will remove it

@Bean
@ConditionalOnMissingBean(CamelLivenessStateHealthIndicator.class)
public CamelLivenessStateHealthIndicator camelLivenessStateHealthIndicator(ApplicationAvailability applicationAvailability,
CamelContext camelContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's reformat it please

}

@Bean
@ConditionalOnMissingBean({CamelReadinessStateHealthIndicator.class})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto + the braces can be removed here

protected AvailabilityState getState(ApplicationAvailability applicationAvailability) {
Collection<HealthCheck.Result> results = HealthCheckHelper.invokeLiveness(camelContext);

boolean isLive = CamelReadinessStateHealthIndicator.checkState(results, LOG);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling a method from CamelReadinessStateHealthIndicator in CamelLivenessStateHealthIndicator seems confusing, maybe the method should be moved into an utility class instead, WDYT?


private static final Logger LOG = LoggerFactory.getLogger(CamelLivenessStateHealthIndicator.class);

private CamelContext camelContext;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be final


private static final Logger LOG = LoggerFactory.getLogger(CamelReadinessStateHealthIndicator.class);

private CamelContext camelContext;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

if (!HealthCheck.State.UP.equals(result.getState())) {
isUp = false;

result.getError().ifPresent(error -> log.warn(result.getCheck().getId(), error));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the message itself, is the id (result.getCheck().getId()) good enough as message content?

@Croway Croway force-pushed the CAMEL-19648 branch 2 times, most recently from 5d4fee3 to c080185 Compare November 17, 2023 15:32
@Croway
Copy link
Contributor Author

Croway commented Nov 17, 2023

thanks @essobedo for your comment, I refactored the logging and the class structure, this is the log in case of failing kafka consumer:

Probe in group 'camel', with id 'consumer:kafka route' failed with message 'KafkaConsumer is not ready (recovery in progress using 5s0ms intervals).'

where kafka route is the id of the route.

@Croway Croway marked this pull request as ready for review November 17, 2023 15:39
@Croway
Copy link
Contributor Author

Croway commented Nov 17, 2023

This is a small reproducer
reproducer.zip

@davsclaus
Copy link
Contributor

@Croway we can look at that WARN thing later. Lets first complete this work and ensure CSB works nicely with SB k8s ready/live checks.

You are welcome to create a JIRA so we wont forget about that other thing

@Croway Croway merged commit 93ea895 into apache:main Nov 20, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants