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

feat: add an "ipfs diag profile" command #8291

Merged
merged 7 commits into from
Jul 22, 2021
Merged

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jul 21, 2021

This should replace the "collect-profiles.sh" script and allow users to easily collect profiles.

Usage: ipfs diag profile or ipfs diag profile -o my-profile.zip.

At the moment, it just dumps all profiles into a single zip file. It does this server-side so it's easy fetch them with curl.

In the future, it would be nice to add:

  1. Progress indicators (cpu profiles take 30 seconds).
  2. An option to specify which profiles to collect.

But we can handle that later.

Unfortunately, this command doesn't produce symbolized svgs as I didn't want to depend on having a local go compiler.

@Stebalien Stebalien force-pushed the feat/diag-profile branch 2 times, most recently from 01188ce to 4117aed Compare July 21, 2021 21:22
@Stebalien

This comment has been minimized.

@Stebalien Stebalien force-pushed the feat/diag-profile branch 2 times, most recently from 5df2f5e to 9270680 Compare July 21, 2021 21:50
This should replace the "collect-profiles.sh" script and allow users to
easily collect profiles.

At the moment, it just dumps all profiles into a single zip file. It
does this server-side so it's easy fetch them with curl.

In the future, it would be nice to add:

1. Progress indicators (cpu profiles take 30 seconds).
2. An option to specify which profiles to collect.

But we can handle that later.

Unfortunately, this command doesn't produce symbolized svgs as I didn't
want to depend on having a local go compiler.
sys diag includes private information (like the interface addresses)
Comment on lines +183 to +184
// Collect binary
if fi, err := openIPFSBinary(); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is worth doing by default. We have the version info and it bloats the size of the zip. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need it for the symbols in case the user deployed a custom binary. I guess we could include it iff the version is "dirty"?

Note: the zip is ~26MiB. So it's not terrible.

Copy link
Contributor

@aschmahmann aschmahmann Jul 22, 2021

Choose a reason for hiding this comment

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

It's not, although I think I've seen maximum sizes for emails and Slack attachments from external parties be lower. I'm not sure what the maximum GitHub issue attachment size is (I think I saw 25MB at one point, but I have no idea if that's still true)

Even if it's not the default we should probably have a flag here just in case

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Awesome!

As mentioned above I'm a little concerned about the zip file sizes containing binaries causing problems for us. Let's figure out what we want to do there (maybe it's just having a flag defaulted to including the binaries and then switching the defaults later if we're running into problems). Aside from that though LGTM.

docs/debug-guide.md Outdated Show resolved Hide resolved
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
@Stebalien
Copy link
Member Author

As mentioned above I'm a little concerned about the zip file sizes containing binaries causing problems for us. Let's figure out what we want to do there (maybe it's just having a flag defaulted to including the binaries and then switching the defaults later if we're running into problems). Aside from that though LGTM.

The current "collect-profiles.sh" script does the same thing. Unfortunately, without the binary, we don't get the symbols. And I couldn't find a way to extract just the symbols.

Well, a quick google turns up https://stackoverflow.com/questions/42554900/how-to-extract-own-symbol-table. But that seems like it'll be unreliable, especially on other operating systems.

@aschmahmann
Copy link
Contributor

Fair enough. It's certainly not worse than the current script and if it's a problem we can deal with it later. Merge when ready.

And I couldn't find a way to extract just the symbols.

You could in theory clone most of the code used by go tool nm https://github.com/golang/go/blob/master/src/cmd/nm/nm.go to get out the symbol table (it relies on an internal package, so not fun). I'm not totally sure how you make that information available to pprof though once you have it though.

@Stebalien
Copy link
Member Author

I'm not totally sure how you make that information available to pprof though once you have it though.

Well, there's probably a way to strip out everything except the symbol table, and make pprof think we're passing it the real binary.

@Stebalien Stebalien merged commit 5bf5a58 into master Jul 22, 2021
@Stebalien Stebalien deleted the feat/diag-profile branch July 22, 2021 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants