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

Override healthcheck HTTP status #1821

Closed
wants to merge 3 commits into from
Closed

Override healthcheck HTTP status #1821

wants to merge 3 commits into from

Conversation

maffe
Copy link
Contributor

@maffe maffe commented Mar 23, 2021

Some monitoring tools only parse responses with status 200. With this change status 500 can be disabled.

Some monitoring tools only parse responses with status 200. With this change status 500 can be disabled.
@maffe maffe requested review from a team as code owners March 23, 2021 15:23
Copy link
Member

@joschi joschi left a comment

Choose a reason for hiding this comment

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

@maffe Thanks for your contribution! 🙂

I've added a few remarks about your PR.

@@ -124,7 +124,8 @@ protected void doGet(HttpServletRequest req,
if (results.isEmpty()) {
resp.setStatus(HttpServletResponse.SC_NOT_IMPLEMENTED);
} else {
if (isAllHealthy(results)) {
final boolean alwaysOk = Boolean.parseBoolean(req.getParameter("alwaysOk"));
Copy link
Member

Choose a reason for hiding this comment

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

Could we rename the parameter to overrideStatusCode?

The name alwaysOk implies that the health check result would be always healthy. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I planned to name it similar to your proposal, but then I thought this would suggest the parameter value should be the desired status code (overrideStatusCode=200) instead of a boolean. I included OK because this is the official name for HTTP status 200. Also, I think OK differs from healthy enough to not suggest it would affect those entries.

Maybe httpStatusOk=true?

@joschi joschi requested a review from a team March 23, 2021 15:51
@joschi joschi added this to the 4.1.19 milestone Mar 23, 2021
@pstackle
Copy link

This approach is a bit different than what I'd expect. It seems a little strange to have the client be able to dynamically influence the response status code of the HealthCheckServlet via a query string parameter. One of the main points of the HealthCheckServlet is to indicate the overall health of the running instance by having that servlet return a 200 status code if everything is healthy but return a server error (5xx) if it's not healthy.

If the behavior was to be configurable, I would expect it to be configured through the ServletConfig via the init method instead of via a query string parameter. Supporting config from the calling client might be setting a bad precedent.

@maffe Can you elaborate a bit on your use case and which monitoring tools you are having trouble with?

Alternatively, a workaround could be to use a servlet filter to swallow the 500 status code from the HealthCheckServlet response and turn it into a 200 status code instead.

@maffe
Copy link
Contributor Author

maffe commented Mar 24, 2021

Currently a PRTG HTTP Sensor is used to retrieve the overall status. The goal is to monitor each healthcheck individually, but the REST Custom Sensor just shows gaps for everything when the status is 500.

Disabling the status 500 via query parameter would make it possible to use both types of sensors with minimal code change in Dropwizard. Using static configuration would also work when the basic HTTP Sensor is not needed.

@joschi joschi modified the milestones: 4.1.19, 4.1.20 Apr 8, 2021
joschi added a commit that referenced this pull request Apr 15, 2021
- Allow overriding the `ObjectMapper` instance used in `HealthCheckServlet` with the servlet attribute `com.codahale.metrics.servlets.HealthCheckServlet.mapper`
- Allow setting the HTTP status code used to indicate health to 200 (OK) with the servlet attribute `com.codahale.metrics.servlets.HealthCheckServlet.httpStatusIndicator`, or with the HTTP query parameter `httpStatusIndicator` per request

Refs #1821
joschi added a commit that referenced this pull request Apr 15, 2021
- Allow overriding the `ObjectMapper` instance used in `HealthCheckServlet` with the servlet attribute `com.codahale.metrics.servlets.HealthCheckServlet.mapper`
- Allow setting the HTTP status code used to indicate health to 200 (OK) with the servlet attribute `com.codahale.metrics.servlets.HealthCheckServlet.httpStatusIndicator`, or with the HTTP query parameter `httpStatusIndicator` per request

Refs #1821
@joschi
Copy link
Member

joschi commented Apr 15, 2021

@maffe @pstackle I've included this change in #1871 and made it possible to set the general behavior with a Servlet attribute.

@joschi
Copy link
Member

joschi commented Apr 18, 2021

Resolved in #1871.

@joschi joschi closed this Apr 18, 2021
joschi added a commit that referenced this pull request Apr 18, 2021
- Allow overriding the `ObjectMapper` instance used in `HealthCheckServlet` with the servlet attribute `com.codahale.metrics.servlets.HealthCheckServlet.mapper`
- Allow setting the HTTP status code used to indicate health to 200 (OK) with the servlet attribute `com.codahale.metrics.servlets.HealthCheckServlet.httpStatusIndicator`, or with the HTTP query parameter `httpStatusIndicator` per request

Refs #1821
@maffe maffe deleted the patch-1 branch July 20, 2023 13:51
joschi added a commit that referenced this pull request Jan 24, 2024
- Allow overriding the `ObjectMapper` instance used in `HealthCheckServlet` with the servlet attribute `io.dropwizard.metrics.servlets.HealthCheckServlet.mapper`
- Allow setting the HTTP status code used to indicate health to 200 (OK) with the servlet attribute `io.dropwizard.metrics.servlets.HealthCheckServlet.httpStatusIndicator`, or with the HTTP query parameter `httpStatusIndicator` per request

Refs #1821
Refs #1871
joschi added a commit that referenced this pull request Jan 24, 2024
- Allow overriding the `ObjectMapper` instance used in `HealthCheckServlet` with the servlet attribute `io.dropwizard.metrics.servlets.HealthCheckServlet.mapper`
- Allow setting the HTTP status code used to indicate health to 200 (OK) with the servlet attribute `io.dropwizard.metrics.servlets.HealthCheckServlet.httpStatusIndicator`, or with the HTTP query parameter `httpStatusIndicator` per request

Refs #1821
Refs #1871
joschi added a commit that referenced this pull request Jan 24, 2024
- Allow overriding the `ObjectMapper` instance used in `HealthCheckServlet` with the servlet attribute `io.dropwizard.metrics.servlets.HealthCheckServlet.mapper`
- Allow setting the HTTP status code used to indicate health to 200 (OK) with the servlet attribute `io.dropwizard.metrics.servlets.HealthCheckServlet.httpStatusIndicator`, or with the HTTP query parameter `httpStatusIndicator` per request

Refs #1821
Refs #1871
Fixes #3918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants