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 option to have /healthz probes to be tied to database(s) connectivity #143

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

alsin
Copy link

@alsin alsin commented Nov 27, 2024

It could be useful to fail sql-exporter by means of K8s health probing by simply checking all configured DB connections could be established, failing /healthz probing otherwise.

@alsin alsin marked this pull request as ready for review December 4, 2024 12:47
@alsin
Copy link
Author

alsin commented Jan 9, 2025

Hello, any chance this feature request could be merged? We'd pretty much appreciate it for our setup. Thank you.

@dewey
Copy link
Member

dewey commented Jan 9, 2025

Hey, I'm not sure it's a good idea to kill the service if one database can't connect. That way your metrics would drop out for all databases just because one database has a connection issue / being restarted.

A better way to do that would be to write an alert based on the absence of a metric, or an outdated timestamp if you export one through a metric.

@alsin
Copy link
Author

alsin commented Jan 9, 2025

Hi @dewey, thanks for the quick reply. The point of this behavior is that we have some database password rotation carried out regularly and having this feature enabled allows auto-restart of the sql_exporter container (we use K8s for container provisioning) which all of a sudden loses ability to connect to the database as the respective secret has changed.

I can imagine a situation when sql_exporter has only jobs configured for night metrics scraping, thus such metrics would be outdated during the day and nobody would figure out there's something wrong with sql_exporter as it doesn't need to connect to DB until night hours.

@dewey
Copy link
Member

dewey commented Jan 9, 2025

Thanks for describing your use case @alsin, I understand your reasoning! In this case I still think it's not a good fit to merge as for your use case it would work well, but for others that could be very unexpected.

I'd maybe look into triggering a restart of the service through other means in that case.

@alsin
Copy link
Author

alsin commented Jan 9, 2025

...but for others that could be very unexpected.

That's why my PR suggests an optional behavior which is off by default and to use it one should intentionally enable it by setting the db.connectivity-as-healthz flag to true. Otherwise it stays off and the healthcheck endpoint works just as previously.

@alsin
Copy link
Author

alsin commented Jan 10, 2025

As an alternative we could require all databases to be unreachable and only after that try to kill the service.

@dewey
Copy link
Member

dewey commented Jan 16, 2025

I think that would make more sense. The code would also need to be formatted a bit, I think there's a gofmt missing as there's a lot of new lines being introduced.

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.

2 participants