Add ThreadPool#stats and adjust Server#stats to use it #3527
Conversation
|
A few questions: There are several methods that can be removed with this. Should we mark then as deprecated as of Puma 7? Several of current 'stats' tests check the generated 'stats' string. These have long, hard to read match strings/regexes. Are we concerned about the string representation, or should we be checking them as objects returned via In addition to #3517, I'd like to add two more items to stats, These look at Puma behavior when it may see 'bursts' of traffic, which is the main cause of the |
| running: @spawned, | ||
| pool_capacity: @waiting (@max - @spawned) | ||
| } | ||
| end |
There was a problem hiding this comment.
here's the approach i was thinking, in order to avoid repeating code (naming can change of course). WDYT?
def backlog_logic
@todo.size
end
def backlog
with_mutext { backlog_logic }
end
def stats
with_mutex do
{ backlog: backlog_logic,
...
}
end
endThere was a problem hiding this comment.
Actually, the three methods exist only so Server#stats can obtain the information. There is one exception, and that may be removed with work currently being looked at to help with the 'long tail' issue.
So, thanks for the example. As I mentioned, they may be deprecated and removed in Puma 7.
There was a problem hiding this comment.
ahh gotcha, i thought others were used for business logic like busy_threads
There was a problem hiding this comment.
concern: does existing test suite really test the behavior of stats method fully after this PR? could we, in this PR, remove the mutex from backlog, invoke the method from stats instead of copy/pasting the logic there, and keep things like that? (possibly moving the two methods into private in the future)
if we don't do that, suggestion: add comments above the methods which are now never invoked by puma code, indicating this, maybe linking to this PR, maybe saying maybe they should be deprecated in the future (or can the deprecation decision be made now in this PR?)
As mentioned in PR #3528, until all the stats hash values are unique, not really. The purpose of this code was to simplify gathering the metrics from ThreadPool, so Server isn't doing them in multiple method calls, currently with more than one lock. Re issues like Re all these stat related methods, the 'long-tail' issue may result in changes as to what metrics are used by Puma for request routing/processing. Hence, my opinion is to leave as is, and fix up for Puma v7. |
|
Sorry, I should have been more clear (and should have checked first) - I assumed that In any case - I think this PR is ready to go! (unless you want to add comments to, or even get the ball rolling on deprecating, those 3 methods in this PR.) |
| def stats | ||
| STAT_METHODS.map {|name| [name, send(name) || 0]}.to_h |
There was a problem hiding this comment.
STAT_METHODS is now only referenced from the test suite
Description
Many of the variables in ThreadPool are locked when changing their values. Current stats code may enter that lock more than once. See #3517 (comment).
This PR creates a
ThreadPool#statsmethod that returns a hash of the stats data that comes from ThreadPool, using a single lock.Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.