Shopify / pitchfork

Other
674 stars 22 forks source link

Access to listener_names within before_service_worker_ready #139

Open ollym opened 1 month ago

ollym commented 1 month ago

We're trying to setup a monitoring service within before_service_worker_ready however when we try and Pitchfork.listener_names from within this block we get an error. How do you advise we can do this?

casperisfine commented 1 month ago

Yes, the service worker has the listeners closed, so that it doesn't appear as listening: https://github.com/Shopify/pitchfork/blob/ec75abc15c4da4c2d0520bca9038b51d6e7e2015/lib/pitchfork/http_server.rb#L930

You didn't give any detail on what solution you are using for monitoring. Is it Raindrops or something else?

For us it isn't too much of a problem because we receive the listeners as environment, so it's not really duplication.

Right now you can work around this via:

      listener_names = nil

      after_mold_fork do |_, _|
        listener_names = Pitchfork.listener_names
      end

      before_service_worker_ready do |server, service|
        $stderr.puts listener_names
      end

It's a bit hacky, but I just tested it, it works.

I guess Pitchfork could be changed to record Pitchfork.listener_names before closing it so it remain accessible for the service worker, so you wouldn't need such a workaround.

ollym commented 1 month ago

Hi yea we're using Raindrops, for now we simplified it by hard coding the bind address and stopped relying on Pitchfork.listener_names entirely.

How are you monitoring pitchfork? We're using yabeda + prometheus and our collection script now looks a bit like this:

pitchfork.workers.set({}, Pitchfork::Info.workers_count)
pitchfork.live_workers.set({}, live_workers_count = Pitchfork::Info.live_workers_count)

if defined?(Raindrops::Linux.tcp_listener_stats)
  listener_address = "0.0.0.0:#{ENV.fetch('PORT', 3000)}"
  stats = Raindrops::Linux.tcp_listener_stats([listener_address])[listener_address]
  pitchfork.active_workers.set({}, stats.active)
  pitchfork.request_backlog.set({}, stats.queued)

  pitchfork.live_capacity.set({}, live_workers_count - stats.active)
  pitchfork.live_busy_percentage.set({}, live_workers_count.zero? ? 100.0 : (stats.active * 100.0) / live_workers_count)
end
casperisfine commented 1 month ago

More or less the same. Raindrops and some StatsD metrics.

One thing you may want to be aware of, is that currently every call to tcp_listener_stats allocate an IO instance and doesn't eagerly close it, so you you do this a lot in a process that doesn't do much more, it can accumulate a lot of file descriptors until GC finally trigger: https://yhbt.net/raindrops-public/20230926214000.M564322@dcvr/T/#e73072adeb83bb8bc74ac6829d1446e316309edc4

So we use this fork for now: https://github.com/casperisfine/raindrops/tree/file-descriptor-leak

casperisfine commented 1 month ago

As for Pitchfork.listener_names, I think it would make sense to just compute it before closing the listeners so it can continue to return the same value. PR welcome, but otherwise I might do it at some point.

ollym commented 1 month ago

What are your thoughts on inlining listener_stats to pitchfork? I've seen you dropped the dependency on raindrops but maybe just inline the function it was providing?

Pitchfork.listener_stats

And taking care of the leak in the process? This kind of monitoring feels pretty mission critical that everyone will need to do at some point.

casperisfine commented 1 month ago

Yeah, I considered some form of that. Was more thinking of doing a companion gem as it could be used by other servers, but I'd like to stop depending on raindrops at Shopify, just haven't gotten around to do it.