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 command-line editing support for interactive pprof #362

Merged
merged 3 commits into from
Apr 18, 2018

Conversation

rauls5382
Copy link
Contributor

@rauls5382 rauls5382 commented Apr 17, 2018

Implement basic command-line editing, using github.com/chzyer/readline. This will not affect the golang
distribution as it uses its own pprof driver, with its own driver.

Fixes #57.
Only tested on Linux.
TODO: Implement auto-completion and persistent history.

This implements basic command-line editing, using
github.com/chzyer/readline. This will not affect the golang
distribution as it uses its own pprof driver, with its own
driver.

Only tested on Linux.
TODO: Implement auto-completion and persistent history.
@codecov-io
Copy link

codecov-io commented Apr 17, 2018

Codecov Report

Merging #362 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #362   +/-   ##
=======================================
  Coverage   66.64%   66.64%           
=======================================
  Files          36       36           
  Lines        7448     7448           
=======================================
  Hits         4964     4964           
  Misses       2082     2082           
  Partials      402      402

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6167805...ab67fe3. Read the comment docs.

@aalexand
Copy link
Collaborator

@rauls5382 Did you consider using https://github.com/golang/crypto/blob/master/ssh/terminal/terminal.go? Go team confirmed that can be vendored into Go tree so Go pprof users would benefit as well.

@aalexand
Copy link
Collaborator

Also, regarding "This will not affect the golang distribution as it uses its own pprof driver, with its own driver." - does Go distribution excludes the pprof.go file? If no, the dependency is technically is still there (e.g. would be effective if "go get" is done against the pprof vendored location).

@rauls5382
Copy link
Contributor Author

I did not consider using other terminal implementations. I'm flexible if there's consensus.
chzyer/readline seems to be popular, though, and not sure if adding dependences into something in crypto/.*/ssh would have other downsides.

In any case, terminal support has to be explicitly ported/reimplemented into golang/go's custom pprof.go: we're technically not constrained to using the same terminal implementation.

The golang distribution does not exclude pprof.go from google/pprof, but I believe it is not compiled when building the distro. They could exclude this file if they really wanted to scrub the dependence.

Adding @hyangah for their opinion.

@hyangah
Copy link
Contributor

hyangah commented Apr 18, 2018

Please go ahead.

As @rauls5382 said, the pprof cmd included in go distribution is built from cmd/pprof/pprof.go and we need to create the similar change there. The vendored main package is unused. I just briefly tested and the additional dependency in the vendored pprof seems ok.

aalexand
aalexand previously approved these changes Apr 18, 2018
pprof.go Outdated
fmt.Fprintf(os.Stderr, "pprof: %v\n", err)
os.Exit(2)
}
}

// readlineUI implements the driver.UI interface using the
// github.com/chzyer/readline library
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add a sentence to summarize what we discussed which is "It is contained in pprof.go to avoid the readline dependency in the vendored copy of pprof in Go distribution (which does not use this file)".

@aalexand aalexand merged commit a1a3fd8 into google:master Apr 18, 2018
@rauls5382 rauls5382 deleted the readline branch April 19, 2018 16:16
KISSMonX pushed a commit to KISSMonX/pprof that referenced this pull request Jun 14, 2018
* 'master' of github.com:google/pprof: (32 commits)
  record diff base profile label key/value constant (google#384)
  apply additional command overrides based on report format (google#381)
  profile: fix legacy format Go heap profile parsing (google#382)
  add -diff flag for better profile comparision (google#369)
  Use -u flag in pprof installation command so that it updates if needed. (google#376)
  Add GetBase support for ASLR kernel mappings (google#371)
  Add show_from profile filter. (google#372)
  Update README.md due to 8dff45d (google#375)
  Remove stale docs, move useful ones. (google#374)
  internal/driver: skip tests requiring tcp on js (google#373)
  Add "trim path" option which can be used to relocate sources. (google#366)
  Move update_d3flamegraph.sh script from the root directory. (google#370)
  Skip unsymbolizable mapping during symbolz pass. (google#368)
  remove -positive_percentages flag (google#365)
  Render icicle graph in "Flame Graph" view (google#367)
  Add command-line editing support for interactive pprof (google#362)
  Improve profile filter unit tests, fix show filtering of mappings (google#354)
  Fake mappings should be merged into a single one during merging (google#357)
  Hack the code so that both existing Go versions and current Go master format it the same (google#358)
  moved filter tests into their own test file which matches the implementation file, filter.go. (google#356)
  ...
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
Add command-line editing support for interactive pprof

This implements basic command-line editing, using
github.com/chzyer/readline. This will not affect the golang
distribution as it uses its own pprof driver, with its own
driver.

Only tested on Linux.
TODO: Implement auto-completion and persistent history.
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.

5 participants