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 a default health controller #46936

Merged
merged 4 commits into from Jan 10, 2023
Merged

Add a default health controller #46936

merged 4 commits into from Jan 10, 2023

Conversation

dhh
Copy link
Member

@dhh dhh commented Jan 9, 2023

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.

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.
@rails-bot rails-bot bot added the railties label Jan 9, 2023
@dhh
Copy link
Member Author

dhh commented Jan 9, 2023

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.

@brunoprietog
Copy link
Contributor

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.

@rubys
Copy link
Contributor

rubys commented Jan 10, 2023

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.

  1. Many customers don't like their health checks to be publicly available, so perhaps it isn't wise to enable this by default. That being said, such information would be useful to us to route around busted instances or restart misbehaving apps.
  2. If you are looking for the closest thing to a standard, the defacto one is https://stackoverflow.com/questions/43380939/where-does-the-convention-of-using-healthz-for-application-health-checks-come-f
  3. we have a config file (fly.toml) where we describe things like these, so at launch time if we detect a rails 7.1 app we can fill in the configuration to match whatever you come up with: https://fly.io/docs/reference/configuration/#services-http_checks

@dhh
Copy link
Member Author

dhh commented Jan 10, 2023

@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.

@dhh
Copy link
Member Author

dhh commented Jan 10, 2023

  1. I've seen that sentiment, but it doesn't make sense to me, certainly as long as the check is based on Ruby code only (and doesn't include timing information from DB or whatever). But could be swayed by specific, informed security information to the contrary. Also, easy enough to wrap this route in something that guards it. And also, given that it's exposed by default, easy enough to turn it off.
  2. Interesting point on /healthz. But eeks, it's ugly 😄.
  3. 👍

Anyway, going to proceed for now, but will remain open to feedback on the points raised above.

@dhh dhh merged commit 5c830a8 into main Jan 10, 2023
@dhh dhh deleted the add-basic-health-controller branch January 10, 2023 09:47
@brunoprietog
Copy link
Contributor

@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?

@dhh
Copy link
Member Author

dhh commented Jan 11, 2023

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.

@zzak
Copy link
Member

zzak commented Jan 12, 2023

@dhh I took a stab at the docs in #46972 if you wouldn't mind taking a look, probably needs some word smithing 🙏

@brunoprietog
Copy link
Contributor

I was also making a pr, great!

@zzak
Copy link
Member

zzak commented Jan 12, 2023

@brunoprietog Feel free to add onto that PR if you want!

@trevorturk
Copy link
Contributor

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.)

zzak added a commit to zzak/rails that referenced this pull request Jan 16, 2023
Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
zzak added a commit that referenced this pull request Jan 16, 2023
[#46936] Add documentation for Rails::HealthController
end

def render_down
render html: html_status(color: "red"), status: 500
Copy link

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link

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

Choose a reason for hiding this comment

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

💯

@nickjj
Copy link
Contributor

nickjj commented Jan 22, 2023

@dhh It's probably too late but what are your thoughts on changing the "up" response to be head :ok?

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.

@sandstrom
Copy link
Contributor

If the controller is named HealthController, why not align the path and controller name?

Less surprising; easier to figure out how they are related.

Also, /health seems to be pretty common, for example:

https://www.airbnb.com/health
https://digitalocean.com/health

@dhh
Copy link
Member Author

dhh commented Jan 23, 2023

@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 👍

rubys added a commit to rubys/rails-healthcheck that referenced this pull request Feb 7, 2023
Also have generator comment out Rails 7.1 health check.

See also:
* superfly/flyctl#1678
* rails/rails#47217
* rails/rails#46936
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants