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

Fix gauge aggregation #6

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

andreaswachowski
Copy link
Contributor

I noticed that the aggregation-parameter for gauges did nothing in a multi-process setting (the pid-attribute was still present). This PR should fix that.

@ollym
Copy link

ollym commented Apr 12, 2023

@Envek ?

@Envek
Copy link
Member

Envek commented Apr 17, 2023

Hey, thanks a lot for the pull request and sorry for long reply.

The problem I can see here is that aggregation option of Yabeda gauge was made to support official Prometheus client's aggregation setting which supports more modes than GitLab's Prometheus mmap client does. The missing one is most_recent mode which is used e.g. by yabeda-sidekiq here. If pass existing modes as is to prometheus-mmap then it will probably raise an error.

Most probably we will need to map existing values (sum to livesum at least) and just not pass most_recent. But let me think a bit about it. Details of your use case or your thoughts are more than welcome!

@ollym
Copy link

ollym commented Dec 3, 2023

@Envek use case for us is switching to https://github.com/Shopify/pitchfork (from Puma) and we need to provide our own stats:

Yabeda.configure do

  group :pitchfork do
    gauge :workers, aggregation: :max, comment: 'Number of pitchfork workers'
    gauge :active_workers, aggregation: :max, comment: 'Number of active pitchfork workers'
    gauge :request_backlog, aggregation: :max,  comment: 'Number of requests waiting to be processed by a pitchfork worker'
  end

  collect do
    if defined?(Raindrops::Linux.tcp_listener_stats)
      if listener_address = Pitchfork.listener_names.first
        stats = Raindrops::Linux.tcp_listener_stats([listener_address])[listener_address]
        pitchfork.active_workers.set({}, stats.active)
        pitchfork.request_backlog.set({}, stats.queued)
      end

      pitchfork.workers.set({}, File.read("/proc/#{Process.ppid}/task/#{Process.ppid}/children").split.count - 1) if Process.ppid > 0
    end
  end
end

Problem is this results in something like this:

pitchfork_workers{pid="314"} 29
pitchfork_workers{pid="318"} 29
pitchfork_workers{pid="324"} 29
pitchfork_workers{pid="34"} 29
pitchfork_workers{pid="41"} 29
pitchfork_workers{pid="45"} 29

So instead of just 29 we have 174

* master:
  Upgrade CI rubies, upgrade Rubocop and move it to separate CI job
  Enhance Compatibility with Rack 3 and Maintain Support for Rack 2 (yabeda-rb#8)
@Envek Envek merged commit 9fdbff3 into yabeda-rb:master Dec 11, 2023
5 checks passed
@Envek
Copy link
Member

Envek commented Dec 11, 2023

Sorry for the late reply and thanks for heads up.

Merged and released in 0.4.0. Enjoy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants