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.

Sign up for GitHub

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

Jump to bottom

Puma.stats is now a hash, not JSON. #2086

Merged
merged 1 commit into from Dec 17, 2019
+41 −18
Merged

Puma.stats is now a hash, not JSON. #2086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions History.md
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
* Features
* Add pumactl `thread-backtraces` command to print thread backtraces (#2053)
* Configuration: `environment` is read from `RAILS_ENV`, if `RACK_ENV` can't be found (#2022)
* `Puma.stats` now returns a Hash instead of a JSON string (#2086)

* Bugfixes
* Your bugfix goes here (#Github Number)
Expand Down
2 changes: 1 addition & 1 deletion lib/puma/app/status.rb
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@ def call(env)
rack_response(200, GC.stat.to_json)

when /\/stats$/
rack_response(200, @cli.stats)
rack_response(200, @cli.stats.to_json)

when /\/thread-backtraces$/
backtraces = []
Expand Down
31 changes: 26 additions & 5 deletions lib/puma/cluster.rb
View file
Open in desktop
Expand Up @@ -73,7 +73,7 @@ def initialize(idx, pid, phase, options)
@first_term_sent = nil
@started_at = Time.now
@last_checkin = Time.now
@last_status = '{}'
@last_status = {}
@term = false
end

Expand All @@ -94,7 +94,11 @@ def term?

def ping!(status)
@last_checkin = Time.now
@last_status = status
captures = status.match(/{ "backlog":(?<backlog>\d*), "running":(?<running>\d*), "pool_capacity":(?<pool_capacity>\d*), "max_threads": (?<max_threads>\d*) }/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just deserialize the JSON response, really?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to have been an intentional decision made in the past to not let Puma rely on the JSON gem. I don't have the context as to why but since this is an internal message being passed I figured it wasn't worth adding the dependency as we don't need more robust parsing. I can change it if you still think it is better to do so.

@last_status = captures.names.inject({}) do |hash, key|
hash[key.to_sym] = captures[key].to_i
hash
end
end

def ping_timeout?(which)
Expand Down Expand Up @@ -352,9 +356,26 @@ def reload_worker_directory
# the master process.
def stats
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is so much more readable now ❤️

old_worker_count = @workers.count { |w| w.phase != @phase }
booted_worker_count = @workers.count { |w| w.booted? }
worker_status = '[' + @workers.map { |w| %Q!{ "started_at": "#{w.started_at.utc.iso8601}", "pid": #{w.pid}, "index": #{w.index}, "phase": #{w.phase}, "booted": #{w.booted?}, "last_checkin": "#{w.last_checkin.utc.iso8601}", "last_status": #{w.last_status} }!}.join(",") + ']'
%Q!{ "started_at": "#{@started_at.utc.iso8601}", "workers": #{@workers.size}, "phase": #{@phase}, "booted_workers": #{booted_worker_count}, "old_workers": #{old_worker_count}, "worker_status": #{worker_status} }!
worker_status = @workers.map do |w|
{
started_at: w.started_at.utc.iso8601,
pid: w.pid,
index: w.index,
phase: w.phase,
booted: w.booted?,
last_checkin: w.last_checkin.utc.iso8601,
last_status: w.last_status,
}
end

{
started_at: @started_at.utc.iso8601,
workers: @workers.size,
phase: @phase,
booted_workers: worker_status.count { |w| w[:booted] },
old_workers: old_worker_count,
worker_status: worker_status,
}
end

def preload?
Expand Down
12 changes: 7 additions & 5 deletions lib/puma/single.rb
View file
Open in desktop
Expand Up @@ -14,11 +14,13 @@ module Puma
# that this inherits from.
class Single < Runner
def stats
b = @server.backlog || 0
r = @server.running || 0
t = @server.pool_capacity || 0
m = @server.max_threads || 0
%Q!{ "started_at": "#{@started_at.utc.iso8601}", "backlog": #{b}, "running": #{r}, "pool_capacity": #{t}, "max_threads": #{m} }!
{
started_at: @started_at.utc.iso8601,
backlog: @server.backlog || 0,
running: @server.running || 0,
pool_capacity: @server.pool_capacity || 0,
max_threads: @server.max_threads || 0,
}
end

def restart
Expand Down
2 changes: 1 addition & 1 deletion test/test_app_status.rb
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@ def halt
end

def stats
"{}"
{}
end
end

Expand Down
11 changes: 5 additions & 6 deletions test/test_cli.rb
View file
Open in desktop
Expand Up @@ -57,8 +57,7 @@ def test_control_for_tcp
body = s.read
s.close

assert_match(/{ "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 }/, body.split(/\r?\n/).last)
assert_match(/{ "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 }/, Puma.stats)
assert_match(/{"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}/, body.split(/\r?\n/).last)

ensure
cli.launcher.stop
Expand Down Expand Up @@ -92,9 +91,9 @@ def test_control_for_ssl
body = http.request(req).body
end

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 }/
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}/
assert_match(expected_stats, body.split(/\r?\n/).last)
assert_match(expected_stats, Puma.stats)
assert_equal([:started_at, :backlog, :running, :pool_capacity, :max_threads], Puma.stats.keys)

ensure
cli.launcher.stop
Expand Down Expand Up @@ -137,7 +136,7 @@ def test_control_clustered
body = s.read
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 \} \},\{ "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 \} \}\] \}/, body.split("\r\n").last)
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\}\},\{"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\}\}\]\}/, body.split("\r\n").last)
ensure
if UNIX_SKT_EXIST && HAS_FORK
cli.launcher.stop
Expand Down Expand Up @@ -172,7 +171,7 @@ def test_control
body = s.read
s.close

assert_match(/{ "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 }/, body.split("\r\n").last)
assert_match(/{"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}/, body.split("\r\n").last)
ensure
if UNIX_SKT_EXIST
cli.launcher.stop
Expand Down