discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
532 stars 154 forks source link

Puma instrumentation starts one monitor thread per worker #68

Open siebertm opened 5 years ago

siebertm commented 5 years ago

Given you configure the puma instrumentation as per the readme instructions And you use puma in clustered mode, Then PrometheusExporter::Instrumentation::Puma.start is called n times, where n is the number of worker processes configured (workers option in puma.rb)

For a puma with 2 workers, this leads to 2 monitoring threads:

ps -T -p <PUMA MASTER PID>
  PID  SPID TTY          TIME CMD
26778 26778 pts/10   00:00:03 ruby
26778 26836 pts/10   00:00:00 process.rb:22
26778 26839 pts/10   00:00:00 client.rb:149
26778 26859 pts/10   00:00:00 tmp_restart.rb*
26778 26860 pts/10   00:00:00 auto_reap.rb:12
26778 26867 pts/10   00:00:00 puma.rb:9     # THIS
26778 26888 pts/10   00:00:00 ruby-timer-thr
26778 26892 pts/10   00:00:00 puma.rb:9     # AND THAT

There should be only one of these threads

SamSaffron commented 5 years ago

Does puma have a similar mechanism to unicorn here so you can figure this out and only run on the master?

siebertm commented 5 years ago

Since puma 3.0, there's plugins which can provide that hook.

We might get this done with a simple plugin:

Puma::Plugin.create do
  def start
    require 'prometheus_exporter/instrumentation'
    PrometheusExporter::Instrumentation::Puma.start
  end
end

There's also in_background which seems like it would create and manage the thread, so we could even leverage this (by making PrometheusExporter::Instrumentation::PumaaPuma::Plugin`).

I'm pretty sure that this could also be used to hook in the process stats, so puma worker + process monitoring is a one-shot thing (see also the heroku-puma plugin code which essentially does this)

Fivell commented 5 years ago

@SamSaffron , from readme

# in unicorn/puma/passenger be sure to run a new process instrumenter after fork
after_fork do
  require 'prometheus_exporter/instrumentation'
  PrometheusExporter::Instrumentation::Process.start(type:"web")
end

but

# puma.rb config
after_worker_boot do
  require 'prometheus_exporter/instrumentation'
  PrometheusExporter::Instrumentation::Puma.start
end

 why PrometheusExporter::Instrumentation::Puma.start can't be launched inside after_fork ?

Fivell commented 5 years ago

btw puma has before_fork, after_fork is not even defined

booleanbetrayal commented 3 years ago

Wondering if there has been any traction on this. Should this be run as a plugin? Any changes needed to the instrumentation implement to facilitate that, or just wrap it in a Puma::Plugin.create?

iusztin commented 2 years ago

I'm wondering, wouldn't it be sufficient to use before_fork in cluster mode? Or am I overlooking something?

https://github.com/puma/puma/blob/v5.5.2/lib/puma/dsl.rb#L510-L527 => before_fork runs only once, on the master process, before any workers are forked.

According to Puma's documentation Puma.stats would be available: https://puma.io/puma/file.stats.html

Content of Puma.stats looks like this in before_fork:

{ "started_at": "2021-11-09T06:26:35Z", "workers": 0, "phase": 0, "booted_workers": 0, "old_workers": 0, "worker_status": [] }

Looks like Prometheus exporter would support that particular Puma.stats state?https://github.com/discourse/prometheus_exporter/blob/v0.8.1/lib/prometheus_exporter/instrumentation/puma.rb#L53-L56 Only some metrics wouldn't be available yet, but appear after the workers have started: https://github.com/discourse/prometheus_exporter/blob/v0.8.1/lib/prometheus_exporter/instrumentation/puma.rb#L64-L74

And after testing (e.g. $ curl http://localhost:9394/metrics right after $ pumactl restart + again after waiting for workers) I didn't notice anything unexpected.