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 a default health controller #46936
Conversation
Load balancers and uptime monitors all need a basic endpoint to tell whether the app is up. Here's a good starting point that'll work in many situations.
cc @rubys, not sure what Fly is doing for default health check, but would be neat if this default would match. So it's even easier to go from "rails new" to on fly, with health checks running. |
This looks like easymon or rails-healthcheck. Do you plan to add any way to add checks? Active Record, Active Job, Cache, etc. I don't think it provides much value without checks. What do you think? I currently use rails-healthcheck to deploy in Fly, although I could very well use easymon, and it works very well. |
I polled around fly.io, and came up with a bunch of disjoint responses. TL;DR: we work without active checks, but having this would be useful.
|
@brunoprietog No plans for other checks at the moment. I'm not convinced that it's a good idea to have the app's health status, which determines whether it'll get yanked from an LB pool, for example, dependent on secondary storage. Those elements should have their own health checks. In any case, it's easy enough to map 'up' to a custom controller for those who want something more sophisticated than this. |
Anyway, going to proceed for now, but will remain open to feedback on the points raised above. |
@dhh what is the purpose of this then if it does not detect if the application is really healthy? For example, it could happen that it starts routing traffic to the new application even if there are pending migrations, and the controller says that the application is healthy but it really is not. This is just an example. Here it always returns 200 regardless of whether things are OK. I say this because for anyone who doesn't read the code, might think that this route is really going to return 503 if something isn't working right, something like a false sense of security. Maybe in that case it would be good to make a disclaimer, what do you think? |
App health is a spectrum. This basic controller will test that the whole stack (os/docker/network/whatever) up until the app, which includes booting successfully with all the configuration in the app. That's about as much as we can standardize without configuration. If you want to include database connections (of which there may be several, to different DBs) or redis connections (of which there may be fallbacks or whatever) or even internal network connections to microservices you rely on, well yeah, you're going to have to do that yourself. But that does not diminish the usefulness of a default health check that can work for deployments to tell if the new version is ready etc. I'd be happy to see all this explained in a guide or as documentation for Rails::HealthController! Please do look into a patch for that, if you fancy. |
I was also making a pr, great! |
@brunoprietog Feel free to add onto that PR if you want! |
FWIW this could be a better approach vs https://devcenter.heroku.com/articles/preboot for example. (Instead of waiting 3 minutes to switch over to the new deploy, wait until /up returns 200.) |
Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
[#46936] Add documentation for Rails::HealthController
end | ||
|
||
def render_down | ||
render html: html_status(color: "red"), status: 500 |
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.
Wouldn't 503 Service Unavailable
be a more appropriate HTTP code in this case?
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.
Yes.
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.
Here is a PR to address this: #47061
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.
💯
@dhh It's probably too late but what are your thoughts on changing the "up" response to be Typically this health check endpoint is being hit every few seconds by various probes and monitors. Doing the least amount of work possible is beneficial. Machines don't need to see an HTML rendered response with colors and a human can quickly see if it's valid or not by looking at the status code, especially if you hit the endpoint in curl which is often the case if you're manually checking to see if a site is up for an unexpected situation. That's the approach I've taken in https://github.com/nickjj/docker-rails-example/blob/main/app/controllers/up_controller.rb for years and it works out nicely. |
If the controller is named Less surprising; easier to figure out how they are related. Also, https://www.airbnb.com/health |
@nickjj Even if you're polling this every second, creating a string is not something that's going to put any load at all on this server or even request. Thousands of strings were created just to get to the action. And I like the pleasing look of green when I hit the endpoint directly 😄 @sandstorm The relation is spelled out explicitly in routes.rb. Feel free to rename it to /health on your application 👍 |
Also have generator comment out Rails 7.1 health check. See also: * superfly/flyctl#1678 * rails/rails#47217 * rails/rails#46936
Load balancers and uptime monitors all need a basic endpoint to tell whether the app is up. Here's a good starting point that'll work in many situations.