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 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 composerinteralia:pumactrl-thread-backtraces branch 3 times, most recently from a52fd51 to 7eb7ad8 Oct 25, 2019
@composerinteralia composerinteralia marked this pull request as ready for review Oct 26, 2019
when /\/thread-backtraces$/
strings = Puma::Events.strings
@cli.log_thread_status(strings)
rack_response(200, strings.stdout.string)

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Oct 26, 2019

Author Contributor

Should this be JSON instead of the raw string?

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Oct 31, 2019

Member

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)

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Oct 31, 2019

Member

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}

This comment has been minimized.

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

def log_thread_status(events)

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Oct 31, 2019

Member

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.

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Nov 8, 2019

Author Contributor

@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?

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Nov 9, 2019

Member

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 #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"
```
@composerinteralia composerinteralia force-pushed the composerinteralia:pumactrl-thread-backtraces branch from 7eb7ad8 to e87d82d Nov 11, 2019
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 composerinteralia force-pushed the composerinteralia:pumactrl-thread-backtraces branch from e87d82d to a344d75 Nov 11, 2019
@composerinteralia
Copy link
Contributor Author

composerinteralia commented Nov 11, 2019

Ready for another review

@nateberkopec nateberkopec merged commit 39d16fa into puma:master Nov 11, 2019
12 checks passed
12 checks passed
build
Details
OS: ubuntu-16.04 Ruby: 2.3.x
Details
OS: ubuntu-18.04 Ruby: 2.4.x
Details
OS: ubuntu-18.04 Ruby: 2.5.x
Details
OS: ubuntu-18.04 Ruby: 2.6.x
Details
OS: macos Ruby: 2.4.x
Details
OS: macos Ruby: 2.5.x
Details
OS: macos Ruby: 2.6.x
Details
OS: windows-latest Ruby: 2.4.x
Details
OS: windows-latest Ruby: 2.5.x
Details
OS: windows-latest Ruby: 2.6.x
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nateberkopec
Copy link
Member

nateberkopec commented Nov 11, 2019

Nice work!

@composerinteralia
Copy link
Contributor Author

composerinteralia commented Nov 11, 2019

Woah, that was fast! Thanks!

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

olleolleolle commented Nov 11, 2019

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

@Littlejd1997
Copy link

Littlejd1997 commented Apr 1, 2020

Is there a timeline for releasing this?

@nateberkopec
Copy link
Member

nateberkopec commented Apr 1, 2020

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.