-
Notifications
You must be signed in to change notification settings - Fork 573
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
JBPM-6740 - Expose readiness and liveness checks in KIE Server #1332
Conversation
|
||
} | ||
|
||
private String calculateUptime() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider using java.time.Duration for these calculations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did and the problem is that it does not take into account already used parts of it, see example:
0 days, 0 hours, 30 minutes, 1800 seconds
0 days, 0 hours, 30 minutes, 0 seconds
first one is done with duration.toMinutes and duration.toMillis()/1000 as there seems to be missing toSeconds method.
Or did you have something else in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Was thinking about using some formatter to convert Duration to some string representation, but it seems that there isn't any out of the box.
Current approach seems ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use plain toString method on a duration, it will generate for example PT41M29.064S. It is ISO 8601 format. In fact, Duration's toString method does it manually with div and mod too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarianMacik correct, but it's way less readable in my opinion, especially for those that are not familiar with ISO date format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also easy to use things DurationFormatUtils in commons-lang3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhrcek you rock! replaced with DurationFormatUtils, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks. One idea included in comment.
2666487
to
acdc678
Compare
response=Void.class, code=200) | ||
@ApiResponses(value = { @ApiResponse(code = 503, message = "Service not yet available") }) | ||
@GET | ||
@Path("readiness") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rename this to "readycheck" (same for method name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
response=Void.class, code=200) | ||
@ApiResponses(value = { @ApiResponse(code = 503, message = "If any of the checks failed") }) | ||
@GET | ||
@Path("health") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rename this to "healthcheck" (same for method name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if (server.isKieServerReady()) { | ||
return Response.status(Response.Status.OK).build(); | ||
} | ||
return serviceUnavailable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all "check/monitoring" methods should have always OK response status and show issues as messages to user in case of error. Might be better if users build monitoring apps against these checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these endpoints are meant for machine reading rather than human - so the most important is to communicate clearly over status codes with minimal payload that's why by default they always return just status and no payload. When user wants to see what is not healthy then there is ?report=true query param that will return payload with messages regardless of the status code (both 200 and 503)
@etirelli @ge0ffrey @tarilabs @sutaakar
here is the implementation that aims at providing basic readiness and liveness checks for KIE Server and all extensions activated.
In general:
response codes for health check are same as for readiness. Meaning that any error found will result in response 503, regardless if that is failed container, failed extension or not ready yet
Health check can be invoked in two modes:
Now I did implement some checks in extensions but not sure if the make sense or more stuff should be added or not. So please take a moment and let me know what you would like to health check in extension you deal with.