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

pprof: create HTTP endpoint for setting MutexProfileFraction #5527

Merged
merged 2 commits into from
Sep 27, 2018

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Sep 26, 2018

Allows to dynamically change the MutexProfileFraction to enable and
disable mutex profiling. It should be very useful for detecting
deadlocks, lock contention and general concurrency problems.

How to use:
To enable: curl -X POST -v 'localhost:5001/debug/pprof-mutex/?fraction=10
To disable: curl -X POST -v 'localhost:5001/debug/pprof-mutex/?fraction=0'

Fraction defines which fraction of events will be profiled. Higher it is
the lower performance impact but less reliable the result.

To fetch the result use:
go tool pprof $PATH_TO_IPFS_BIN http://localhost:5001/debug/pprof/mutex

License: MIT

@ghost ghost assigned Kubuxu Sep 26, 2018
@ghost ghost added the status/in-progress In progress label Sep 26, 2018
@Kubuxu Kubuxu added the need/review Needs a review label Sep 26, 2018
Allows to dynamically change the MutexProfileFraction to enable and
disable mutex profiling. It should be very useful for detecting
deadlocks, lock contention and general concurrency problems.

How to use:
To enable run: curl -X POST -v 'localhost:5001/debug/pprof-mutex/?fraction=10
To disable: curl -X POST -v 'localhost:5001/debug/pprof-mutex/?fraction=0'

Fraction defines which fraction of events will be profiled. Higher it is
the lower performance impact but less reliable the result.

To fetch the result use:
go tool pprof $PATH_TO_IPFS_BIN http://localhost:5001/debug/pprof/mutex

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Code LGTM, but I wonder if it wouldn't be better to do this as a more generalized endpoint to multiple internal knobs / hardcoded values. Also, is there a reason this can't be just a command (like ipfs debug set)?

@magik6k magik6k removed the need/review Needs a review label Sep 26, 2018
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 26, 2018

It could be a command, but is there a reason for it to be a command? No normal user is expected to use it directly and if we are asking someone for pprof dumps we send them commands either way (in future we can create collection util).

As for more generic endpoint we can think about it but please, let's not block this PR on figuring out a general solution.

@Stebalien
Copy link
Member

I actually prefer this over a command as commands tend to become a part of our API. This is really just a debugging endpoint.

Now, at some point in the future we may add a debug command. At that point, we can reconsider.


@Kubuxu this should have at least one sharness test (just turn it on and off and make sure nothing errors).

@Stebalien
Copy link
Member

Otherwise, LGTM.

@Kubuxu Kubuxu added need/review Needs a review and removed need_tests labels Sep 27, 2018
@ghost ghost added the status/in-progress In progress label Sep 27, 2018
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 27, 2018

Tests added.

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 27, 2018

Fixed the tabs, not even sure where they were.

@Stebalien
Copy link
Member

@magik6k are you fine with merging this for now (given that we already have a debug endpoint like this) or do you feel that it really needs to be a command?

@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 27, 2018

From another side, IMO, his shouldn't be an API. It is specific to Go only, and only because this option is not exposed under existing pprof endpoints.

It is the essence of implementation detail as an implementation detail can be.

@Stebalien
Copy link
Member

t is specific to Go only, and only because this option is not exposed under existing pprof endpoints.

I agree that it's a go-ipfs specific implementation detail but that doesn't necessarily mean we can't expose it over the HTTP API (like our other ipfs diag commands).

Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

LGTM, It was more of a suggestion about the command.

@Stebalien Stebalien merged commit 86559e9 into master Sep 27, 2018
@ghost ghost removed the status/in-progress In progress label Sep 27, 2018
@Stebalien Stebalien deleted the feat/pprof-lock branch September 27, 2018 18:49
@Stebalien
Copy link
Member

Yeah, we should probably come up with a comprehensive story around debugging. I just kind of doubt it'll get done any time soon.

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

Successfully merging this pull request may close these issues.

3 participants