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

Support individual health check endpoints #173

Merged
merged 22 commits into from
Jan 29, 2021
Merged

Support individual health check endpoints #173

merged 22 commits into from
Jan 29, 2021

Conversation

mkrueger-sabio
Copy link
Contributor

I implemented the test but can't get them running locally. Don't know how to set them up.

@michaelklishin
Copy link
Owner

Thank you.

I would agree to add these as undocumented. They have been deprecated and replaced with more focussed health checks which IIRC now have their HTTP API equivalents in 3.8. Ideally we should support just those endpoints.

We (Team RabbitMQ) certainly don't want to promote either of these endpoints. They are either too simplistic (the aliveness check) or too intrusive (the original health check), and do not provide a lot of context in the response.

@michaelklishin
Copy link
Owner

Also, I think these can go into a single file, e.g. healthchecks sounds reasonable to me.

@michaelklishin
Copy link
Owner

I'd be happy to take a look at adding support for rabbitmq/rabbitmq-management#854 to this client. Thank you for the reminder 👍

@mkrueger-sabio
Copy link
Contributor Author

Ah, ok, I didn't know that there was a change in the api. In this case, I will drop this pr and implement the new checks, unless you want it to be merged.

@michaelklishin michaelklishin changed the title Add aliveness-test and healthchecks endpoint Support individual health check endpoints Jan 25, 2021
@michaelklishin
Copy link
Owner

I will QA this today. Thank you very much for picking this up and putting in all the effort to re-do it around the new API endpoints!

health_checks.go Outdated Show resolved Hide resolved
Copy link
Owner

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

This is definitely on the right track. We have some room to make it even better and easier to use, though.

health_checks.go Outdated Show resolved Hide resolved
health_checks.go Outdated Show resolved Hide resolved
@michaelklishin
Copy link
Owner

I am finishing this today.

@mkrueger-sabio
Copy link
Contributor Author

Yes, thanks, that would be nice. I have not the time at the moment to work on this properly.

@michaelklishin michaelklishin merged commit 17799f0 into michaelklishin:master Jan 29, 2021
@michaelklishin
Copy link
Owner

@mkrueger-sabio I have merged an updated version after some suggestions from other RabbitMQ engineers. I hope this modified API looks reasonable to you.

@michaelklishin michaelklishin added this to the 2.7.0 milestone Jan 30, 2021
@mkrueger-sabio
Copy link
Contributor Author

@michaelklishin Yes, I'm fine with that. Thanks.

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