JSON parse cluster worker stats instead of regex #2124
Conversation
wjordan
force-pushed
the
json_cluster_stats
branch
4 times, most recently
from
21efbc4
to
3745bae
Compare
|
Hmmm... I wonder if those windows failures are related or not. I feel like I've seen stalls on that test before, though not specifically on Windows. |
|
Pinging @MSP-Greg for the possible test stability issue here. |
| @@ -5,6 +5,7 @@ | |||
| require 'puma/plugin' | |||
|
|
|||
| require 'time' | |||
| require 'json' | |||
There was a problem hiding this comment.
Currently we only require json for the status app, which isn't that big of a deal. This would definitely require json for most people running Puma.
There was a problem hiding this comment.
For example, this would, I believe, introduce a new native extension during startup, which I believe affects other open PRs.
There was a problem hiding this comment.
I thought requiring json would be straightforward as part of the standard library, but I did find it unusual that it was being intentionally avoided. If loading json does introduce some known stability/performance issue on any of the supported platforms (I'm less familiar with non-linux/MRI platforms) it makes sense to not accept this change.
There was a problem hiding this comment.
I checked with Evan and there was no special reason. This is good to 🚢
lib/puma/server.rb
Outdated
| @@ -1045,5 +1045,10 @@ def self.current | |||
| def shutting_down? | |||
| @status == :stop || @status == :restart | |||
| end | |||
|
|
|||
| def stats | |||
| stat_names = %i(backlog running pool_capacity max_threads requests_count) | |||
There was a problem hiding this comment.
Would prefer these stats in a constant somewhere.
test/test_cli.rb
Outdated
| @@ -93,7 +93,7 @@ def test_control_for_ssl | |||
|
|
|||
| expected_stats = /{"started_at":"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z","backlog":0,"running":0,"pool_capacity":16,"max_threads":16,"requests_count":0}/ | |||
| assert_match(expected_stats, body.split(/\r?\n/).last) | |||
| assert_equal([:started_at, :backlog, :running, :pool_capacity, :max_threads, :requests_count], Puma.stats.keys) | |||
| assert_equal(JSON.parse(body.split(/\r?\n/).last, symbolize_names: true), Puma.stats) | |||
There was a problem hiding this comment.
We already checked the entire response on the previous line. IMO, this test is redundant.
test/test_cli.rb
Outdated
| @@ -137,6 +137,7 @@ def test_control_clustered | |||
| s.close | |||
|
|
|||
| assert_match(/\{"started_at":"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z","workers":2,"phase":0,"booted_workers":2,"old_workers":0,"worker_status":\[\{"started_at":"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z","pid":\d+,"index":0,"phase":0,"booted":true,"last_checkin":"[^"]+","last_status":\{"backlog":0,"running":2,"pool_capacity":2,"max_threads":2,"requests_count":0\}\},\{"started_at":"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z","pid":\d+,"index":1,"phase":0,"booted":true,"last_checkin":"[^"]+","last_status":\{"backlog":0,"running":2,"pool_capacity":2,"max_threads":2,"requests_count":0\}\}\]\}/, body.split("\r\n").last) | |||
| assert_equal(Puma.stats, JSON.parse(body.split("\r\n").last, symbolize_names: true)) | |||
There was a problem hiding this comment.
Same here, redundant test.
There was a problem hiding this comment.
Maybe we can test Puma.stats output separately in another test if we don't do that explicitly anywhere else?
There was a problem hiding this comment.
Yeah, I added these lines because my first iteration of this PR unintentionally changed keys in Puma.stats from symbols to strings (JSON.parse default) without failing any tests, so I thought that behavior should be covered by an extra assertion somewhere. I can move it to a new test to make it more clear.
nateberkopec
added
waiting-for-changes
|
I did notice those tests failures. I'll have a look. |
|
I grabbed the PR, and ran tests in my fork, both standard and using the I then quickly tried using 2.6.5 locally, and things got weird. It had Bundler 1.7.2 installed, when I upgraded both RubyGems & Bundler with But, all the master/head are passing. Just thought I'd point that out. Also, maybe PR #2126 |
|
Starting a test re-run. Still needs my code concerns addressed. I'm discussing the json thing with Evan. EDIT: We're good to require |
wjordan
force-pushed
the
json_cluster_stats
branch
6 times, most recently
from
5a42fc3
to
e755788
Compare
wjordan
force-pushed
the
json_cluster_stats
branch
from
e755788
to
f5c2dbb
Compare
Description
Refactoring PR to simplify the cluster worker stats code using
JSON.parseand.to_jsonin place of regex-parsing and string interpolation.Also includes refactoring of duplicated server-stats logic into a
Server#statsmethod.Your checklist for this pull request
[changelog skip]the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.