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.

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 pumactl command to print thread backtraces #2054

Merged

Conversation

composerinteralia
Copy link
Contributor

@composerinteralia composerinteralia commented Oct 25, 2019

Completes 1 of 2 items from #1964

This commit adds an endpoint to the status app to print thread
backtraces, and control cli command to call that endpoint.

I tried this locally by starting a server with:

bundle exec bin/puma test/rackup/hello.ru \
  --control-url="unix://test.sock" \
  --control-token="token"

and then printing the backtraces with:

bundle exec bin/pumactl thread-backtraces \
  --control-url="unix://test.sock" \
  --control-token="token"
  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] to all commit messages.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the commit messages.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@composerinteralia composerinteralia force-pushed the pumactrl-thread-backtraces branch 3 times, most recently from a52fd51 to 7eb7ad8 Compare October 26, 2019 01:46
@composerinteralia composerinteralia marked this pull request as ready for review October 26, 2019 01:50
when /\/thread-backtraces$/
strings = Puma::Events.strings
@cli.log_thread_status(strings)
rack_response(200, strings.stdout.string)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be JSON instead of the raw string?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. All current responses are json, so we're going to need to turn this into json object.

Maybe something like:

threads: [
  { name: "", backtrace: "" },
  { name: "", backtrace: "" }
]

when /\/thread-backtraces$/
strings = Puma::Events.strings
@cli.log_thread_status(strings)
rack_response(200, strings.stdout.string)
Copy link
Member

Choose a reason for hiding this comment

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

Yes. All current responses are json, so we're going to need to turn this into json object.

Maybe something like:

threads: [
  { name: "", backtrace: "" },
  { name: "", backtrace: "" }
]

@@ -11,7 +11,8 @@
module Puma
class ControlCLI

COMMANDS = %w{halt restart phased-restart start stats status stop reload-worker-directory gc gc-stats}
COMMANDS = %w{halt restart phased-restart start stats status stop reload-worker-directory gc gc-stats thread-backtraces}
PRINTABLE_COMMANDS = %w{gc-stats stats thread-backtraces}
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

@@ -205,6 +205,21 @@ def close_binder_listeners
@binder.close_listeners
end

def log_thread_status(events)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you'll need to refactor this out a bit so that you have two methods - one that prepares a Ruby object representation of threads and backtraces, then one method that logs that info to events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nateberkopec Should we always use the object representation, or do you want to preserve the existing format for the SIGINFO and use the object representation only for the status app?

Copy link
Member

Choose a reason for hiding this comment

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

I think what matters is that the current logged output remains the same, the status app outputs JSON, and that they both use the same method underneath for obtaining that info.

Completes 1 of 2 items from puma#1964

This commit adds an endpoint to the status app to print thread
backtraces, and control cli command to call that endpoint.

I tried this locally by starting a server with:

```sh
bundle exec bin/puma test/rackup/hello.ru \
  --control-url="unix://test.sock" \
  --control-token="token"
```

and then printing the backtraces with:

```sh
bundle exec bin/pumactl thread-backtraces \
  --control-url="unix://test.sock" \
  --control-token="token"
```
With this commit the status app sends the thread backtraces as an array
of objects with `name` and `backtrace` keys, rather than as a string
matching the SIGINFO output.

While working on this I noticed that we logged the thread TID twice.
This commit simplifies that so we only log the thread TID once, with
both the label (I don't know when the label would get set) and name if
they are available.
@composerinteralia
Copy link
Contributor Author

Ready for another review

@nateberkopec nateberkopec merged commit 39d16fa into puma:master Nov 11, 2019
@nateberkopec
Copy link
Member

Nice work!

@composerinteralia
Copy link
Contributor Author

Woah, that was fast! Thanks!

@composerinteralia composerinteralia deleted the pumactrl-thread-backtraces branch November 11, 2019 05:09
@olleolleolle
Copy link
Contributor

From the sidelines: this is super helpful! The tab that was added to each backtrace line makes for an ergonomic developer experience.

@jondavidschober
Copy link

Is there a timeline for releasing this?

@nateberkopec
Copy link
Member

This was merged 3 days after 4.3 was released, so I realize it's been in the backlog for a while. You can see progress on 5.0 here: https://github.com/puma/puma/milestone/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants