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 flag for block profiling #1681

Merged
merged 3 commits into from
Jul 14, 2016
Merged

Add flag for block profiling #1681

merged 3 commits into from
Jul 14, 2016

Conversation

jml
Copy link
Contributor

@jml jml commented Jul 14, 2016

Fixes #1672

Test by adding --app.block.profile.rate=1, running scope in
multi-tenant mode and browsing to http:///debug/pprof/


This change is Reviewable

Fixes #1672

Test by adding `--app.block.profile.rate=1`, running scope in
multi-tenant mode and browsing to http://<scope-url>/debug/pprof/
@2opremio
Copy link
Contributor

2opremio commented Jul 14, 2016

Test by adding --app.block.profile.rate=1

Isn't this flag a bit fine-grained? How about simply enabling block profiling with a default value when supplying --debug?

Also, could you please update site/building.md?

@2opremio
Copy link
Contributor

Another option is adding something like --debug.block-profiling-rate=n which would affect both in the app and the probe.

(these are just suggestions, it's ultimately up to you)

@2opremio
Copy link
Contributor

Thinking about it a bit more, in the documentation it seems we separate debugging (showing more info in the logs) from profiling so feel free to ignore my comments about the flag names.

@jml
Copy link
Contributor Author

jml commented Jul 14, 2016

Isn't this flag a bit fine-grained? How about simply enabling block profiling with a default value when supplying --debug

I anticipate that we'll get the default value wrong & will want to iterate quickly. Flags are the best way we currently have of doing that.

Another option is adding something like --debug.block-profiling-rate=n which would affect both in the app and the probe.

Having one flag for both app & probe seems like a good idea. Does your other comment about separating debug from profiling preclude this as a flag name?

@2opremio
Copy link
Contributor

Does your other comment about separating debug from profiling preclude this as a flag name?

Probably, at least we separate them in the docs.

Scope with `--app.block.profile.rate=N` (where `N` is the number of
nanoseconds between samples) and then:

go tool pprof http://localhost:4040/debug/pprof/

This comment was marked as abuse.

@2opremio
Copy link
Contributor

LGTM (provided you get a green from circle)

@jml jml merged commit cdb651f into master Jul 14, 2016
@jml jml deleted the block-profiling-1672 branch July 14, 2016 14:54
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