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
Periodically send status to systemd #3006
Conversation
e98e6d7
to
3008009
Compare
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 @QWYNG for taking my work and taking it over the finish line.
It does look the message gets trimmed so that could be a future improvement. Still, I think this is ready to merge.
lib/puma/plugin/systemd.rb
Outdated
messages = Array.new(workers) do |i| | ||
common_message(stats[:worker_status][i][:last_status]) |
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.
I didn't look at the actual source, but would it be possible to iterate over stats[:worker_status]
instead? Untested, but something like:
messages = Array.new(workers) do |i| | |
common_message(stats[:worker_status][i][:last_status]) | |
messages = stats[:worker_status].values do |worker| | |
common_message(worker[:last_status]) |
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.
Thank you!
stats[:worker_status] is Array instanse.
So I modified it as follows
messages = stats[:worker_status].map do |worker|
common_message(worker[:last_status])
end.join(',')
With systemd it's possible to send a status line. This will show up when users run systemctl status puma.service. Most of this code is based on puma-plugin-systemd.
3008009
to
91fa294
Compare
lib/puma/plugin/systemd.rb
Outdated
@@ -10,7 +10,7 @@ | |||
Puma::Plugin.create do | |||
def start(launcher) | |||
begin | |||
require 'sd_notify' | |||
require_relative '../sd_notify' | |||
rescue LoadError | |||
launcher.log_writer.log "Systemd integration failed. It looks like you're trying to use systemd notify but don't have sd_notify gem installed" |
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.
Now that it's vendored (6d8b728), I don't think there's a chance that it will fail to load. Certainly the message is wrong now since the sd_notify
gem is no longer needed.
lib/puma/systemd.rb
Outdated
@@ -29,6 +29,29 @@ def start_watchdog | |||
end |
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.
Something went wrong in your rebase. This file is supposed to be deleted in the Rewrite systemd integration as a plugin
commit.
The primary motiviation for this is that plugins have native integration for background threads. This is much cleaner since it allows tracking of those. For example, it's possible to clean up those threads in the test suite.
c644273
to
338ef29
Compare
@dentarg |
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!
Hi @dentarg, @nateberkopec |
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.
Looks good to me, haven't used it myself, but I trust people from the community who seems to have tried this out!
Description
Hi, Thank you for great gem.
Add test for #2833. Thank you @ekohl
it was not able to retrieve information from
Puma.stats_hash
properly.So I have changed the logic in
lib/puma/plugin/systemd.rb
a little.Description from #2833
Your checklist for this pull request
[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.