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

Add /healthcheck endpoint that doesn't collect PDU data #6

Merged

Conversation

tomelliff
Copy link
Contributor

@tomelliff tomelliff commented Apr 30, 2024

When the exporter is deployed in a system that healthchecks running processes, such as Kubernetes, it's useful to have a healthcheck endpoint that can return quickly which still catching errors where eg the web server is hanging.

This basic /healthcheck endpoint returns a 200 and a short message, avoiding the call to collect which would normally gather all of the PDU data and can be slow to return. All other endpoints will still return the PDU data in Prometheus format.

Closes #5.

@nhjjr
Copy link
Collaborator

nhjjr commented May 8, 2024

Thank you for the PR! This looks good and after some tests on our setup it seems to run without issues. There are two points I'd like you to address before I merge this PR:

  • There is a flake8 error because a line is too long (main.py:127:80), that should be a quick fix to have this PR pass tests
  • Can you add a small section to README.md explaining this new feature?

When the exporter is deployed in a system that healthchecks running
processes, such as Kubernetes, it's useful to have a healthcheck
endpoint that can return quickly which still catching errors where eg
the web server is hanging.

This basic /healthcheck endpoint returns a 200 and a short message,
avoiding the call to `collect` which would normally gather all of the
PDU data and can be slow to return. All other endpoints will still
return the PDU data in Prometheus format.

Closes
psyinfra#5.
@tomelliff tomelliff force-pushed the telliffoshea/provide-healthcheck-endpoint branch from 9c826a2 to 774e0b0 Compare May 12, 2024 18:19
@tomelliff
Copy link
Contributor Author

Ah, sorry, just realised I didn't hit comment on this when I added the changes. I think that's right now although running the tests locally was failing for me so was just going to rely on the CI here unless it fails again.

@aqw
Copy link

aqw commented May 16, 2024

I approved the CI run. :-)

@nhjjr
Copy link
Collaborator

nhjjr commented May 16, 2024

Nice work @tomelliff! I'll merge the PR now.

@nhjjr nhjjr merged commit 04bc065 into psyinfra:master May 16, 2024
6 checks passed
@aqw
Copy link

aqw commented May 16, 2024

Thanks for the patch!

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.

Expose a healthcheck endpoint
3 participants