-
Notifications
You must be signed in to change notification settings - Fork 18
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 requests_count
measure
#31
base: master
Are you sure you want to change the base?
Conversation
These CI failures seem related to this project's CI matrix including end-of-life rubies, how the CI task installs bundler, and bundler's policy for dropping support for End-of-Life rubies.
FWIW, Ruby 2.7 reached end-of-life on 2023-03-31 and is even outside of its security maintenance window -- https://www.ruby-lang.org/en/downloads/branches/ I've not found any policy documentation from this project on compatibility expectations. Any guidance from the maintainers on how to proceed? Would it be appropriate for the next release of the yabeda-puma-plugin to drop unsupported / end-of-life'ed rubies and puma versions from its CI compatibility matrix? |
Thanks for the pull request! Ignore CI failures for outdated rubies, I will drop them from the CI matrix. I will review your pull request soon, hopefully in a few days. |
* master: Switch to RubyGems Trusted publishing in CI release workflow Refresh CI setup: test against newer Rubies, use official actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the pull request! I fixed tests, they are passing (and this is worrying, see below).
My concerns are as follows:
-
It is a gauge, however actually it should be a counter. I'm not sure whether e.g. Prometheus will be able to correctly handle gauge reset to 0 on puma restarts (as it do for counters). See metric types docs for details.
But of course we can't make it counter in current way (parsing value from control app), unless there is a way to hook into Puma processing request (like by maybe prepending here)
-
This feature was added in Puma 5.0, but test for Puma 4 passed fine. Probably because we're using mocked control app response.
I'm totally fine with dropping Puma 4 support, as Puma 5 was released almost 4 years ago already.
The way the
Puma, itself, handles the incrementing a resets of the measure; the Given the comment:
...I think we're more aligned than not, and probably saying the same thing from different angles. Please let me know if I've misunderstood the argument, or if there are other recommendations / considerations for moving this particular conversation forward.
I feel like I'm not fully understanding this concern. Given Prometheus' guidance on how to implement gauges in client libraries, it is unclear to me why a this value being reset to 0 or decreasing after a process restart would be detrimental. Would you mind providing a bit more context on the concern?
I suppose the mantra "Only mock the things you own." is relevant here 😉. Perhaps I missed some integration level examples? |
Hi, great job to add puma_requests_count metric introduced in Puma 5.0 (changelog) 👍 I think that is ok to use gauge, instead of counter, becase we are not controlling that value and in fact are not interested into saving previous value to calculate the difference |
Summary
Closes: #19
Expose the number of requests a puma worker has processed.
Context
Since puma/puma#2106, puma workers keep track of how many requests they have processed. This exposes Puma's
requests_count
stat via the yabeda-puma-plugin.Examples
Standalone mode
Clustered mode (4 workers)