Make health probe server more general purpose

[jklina]

Howdy and thanks for the great library! 👋 I was looking into a way of adding an endpoint for monitoring metrics on job servers and saw that there was already an existing server used for health checks that uses the Rack interface. I did a quick spike that makes the health check server more general purpose and allows users to configure their own Rack apps for running on instances. Just wanted to get some input before pursuing it further and cleaning things up.

---

The health check and catchall logic are moved into simple Rack middleware that can be composed by users however they like and be used to preserve existing health check behavior while transitioning to a more general purpose utility server.

Additionally, this allows users to use WEBrick for the probe server in order to have a fully Rack compliant option.

All and all this pattern will allow users to add whatever functionality they like to GoodJob's web server by composing Rack apps and using GoodJob's configuration to pass in users' Rack apps. IE:

config.good_job.middleware = Rack::Builder.app do
  use GoodJob::Middleware::MyCustomMiddleware
  use GoodJob::Middleware::PrometheusExporter
  use GoodJob::Middleware::Healthcheck
  run GoodJob::Middleware::CatchAll
end
config.good_job.middleware_port = 7001

This could help resolve:

• Options to monitor good_job in production? #750
• Add debugging and telemetry stats #532

[bensheldon]

I have mixed feelings:

• I'm worried that folks will want more and more out of the probe server (it's a single thread / blocking) and that's not where I want to spend me development time. The extreme reaction (which I realize you're not proposing but is my fear) is that if someone needs a webserver, they should be running GoodJob in Puma, not recreating Puma in GoodJob.
• I'm not sure how defensive to be if folks are running arbitrary Rack Apps. Like should GoodJob be including ActionDispatch::Executor middleware in case someone adds customize middleware that touches Rails autoloaded objects or Active Record even though GoodJob doesn't need it itself. Because otherwise there might be deadlocks or leaked database connections which I imagine will be a support burden.
• I recognize that being able to add prometheus metrics or other custom healthchecks would be nice without having to spin up yet-another-process (though maybe if you're already running kube and prometheus that shouldn't be a dealbreaker). But I get it, that would be helpful for advanced systems.

So maybe my objections here could be overcome fully by defensive naming:

• Leaving the name of it as ProbeServer implying the the purpose is for probing the process (I think "Utility" is too vague).
• "Middleware" is a bit overloaded in the background job space, as it's also used for describing job customization (though not in GoodJob). We should keep it namespaced as ProbeServer::Middleware::....,
• Let's call the configuration for the Rack app: config.good_job.probe_server_app =

Otherwise I like the PR 😄 I think it cleaves the behaviors in the right place and allows flexibility without necessitating it.

[shouichi]

Is Good::HttpServer meant to be rack compliant? The current implementation doesn't seem so. Rack specifies various mandatory fields and some rack apps assume those fields to exist. Thus some apps won't work.

[jklina]

@shouichi probably not. The more I think about this the more I'm worried it'll open a can of worms. I'm thinking of just closing this for now and if there's more demand down the line, I'd be happy to revisit and see it through making its limitations very visible.

[bensheldon]

oh, the Rack spec is kind of a lot: https://github.com/rack/rack/blob/main/SPEC.rdoc

I do like the idea of allowing it to be extensible, but agree that I don't want to set the expectation that anything more than a trivial rack middleware will work.

[shouichi]

I tried a "trivial" example from https://www.rubydoc.info/gems/rack/Rack/Builder but didn't work.

app = Rack::Builder.new do
  use Rack::CommonLogger
  map "/ok" do
    run lambda { |env| [200, {'content-type' => 'text/plain'}, ['OK']] }
  end
end

run app

The spec is a lot, implementing it in GoodJob might be a rabbit hole. I guess it's better to use an existing server (e.g., webrick).

Other than the spec, error handling is not trivial work. I recently opened #1083 but there are potentially more corner cases. Search Errno in webrick/puma, you'll see a lot of them.

• https://github.com/ruby/webrick/blob/b56700865bfaeba2b8415a189e3e1c4421bbcb07/lib/webrick/server.rb
• https://github.com/puma/puma/blob/7b1a2db7e85242ac598fb21859ab7493146f10a8/lib/puma/server.rb

[bensheldon]

I think I do like the idea of pluggable middleware; it would be nice if any Rack middleware would work. I think that has me lean towards reintroducing webrick.

Would it be possible to keep the array of middleware and add an additional option that was like (ugh, I don't like the name):

config.good_job.probe_server_server = :good_job # or webrick

And then folks could choose and we'd just have a warning in the readme of like "the default :good_job is very barebones and likely won't work if you get complicated.

[jklina]

I'm wondering, if Webrick is going to become a dependency of GoodJob anyway, do we keep the current homespun server or nix it completely? Removing the homespun server would be one less configuration option to worry about (I feel like there might be some confusion about selecting servers) and anything that'd work with the homespun server should work with Webrick.

If that's the case maybe we break this into two parts:

• Reverting Replace Webrick with custom simple http server #1030
• And finishing up this PR for use with Webrick

[bensheldon]

if Webrick is going to become a dependency of GoodJob anyway

Because it would be optional, I wouldn't add Webrick to the gemspec, and instead simply require it, if configured, and rescue the LoadError if it hasn't been added.

I hear you about complexity though 😰

[jklina]

I went ahead and took a stab at implementing the suggestions. I'll keep my eyes open for any further suggestions during review!

[bensheldon]

I like this! I just pushed up a few tweaks and I will accept this.