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 support for saving the current configuration settings #535

Merged
merged 4 commits into from
May 7, 2020

Conversation

ghemawat
Copy link
Contributor

@ghemawat ghemawat commented May 6, 2020

Add support for saving the current configuration settings as a named
configuration and reloading such a configuration later.

All of the configurations are stored in a settings file, by default:
~/.config/pprof.json

A struct named config replaces the old map[string]string which used
to store the set of configurable options. Instead, each option is
now a field in the config struct.

The UI now has a new 'Config' menu with

Save as: prompts for a name and saves current config.
Default: applies default config.
$X: for every config named X: applies settings from X.

The currently selected config is highlighted.

Every named config has a delete button to control deletion.

Some semi-related changes:

  1. Both filefunctions and functions in the granularity group had an
    initial value of true, which is incorrect since these are mutually
    incompatible choices. Set filefunctions' initial value to false.
  2. Renamed the group for sorting from "cumulative" to "sort".
  3. Store testing.T in TestUI instead of leaving the field nil.

configuration and reloading such a configuration later.

All of the configurations are stored in a settings file, by default:
~/.config/pprof.json

A struct named config replaces the old map[string]string which used
to store the set of configurable options. Instead, each option is
now a field in the config struct.

The UI now has a new 'Config' menu with

Save as: prompts for a name and saves current config.
Default: applies default config.
$X: for every config named X: applies settings from X.

The currently selected config is highlighted.

Every named config has a delete button to control deletion.

Some semi-related changes:
1. Both filefunctions and functions in the granularity group had an
   initial value of true, which is incorrect since these are mutually
   incompatible choices. Set filefunctions' initial value to false.
2. Renamed the group for sorting from "cumulative" to "sort".
3. Store testing.T in TestUI instead of leaving the field nil.
@rsc
Copy link
Contributor

rsc commented May 6, 2020

Nice. Can you use os.UserConfigDir to find the config dir? That will do all the things in the code right now but play nicer on Macs (using $HOME/Library/Application Support).

@ghemawat
Copy link
Contributor Author

ghemawat commented May 6, 2020

Nice. Can you use os.UserConfigDir to find the config dir? That will do all the things in the code right now but play nicer on Macs (using $HOME/Library/Application Support).

Thanks for the suggestion.

I think UserConfigDir was added in Go 1.13. Alex, which older versions of Go is pprof promising to support currently. Would it be a problem to require 1.13 or newer?

@aalexand
Copy link
Collaborator

aalexand commented May 6, 2020

Nice. Can you use os.UserConfigDir to find the config dir? That will do all the things in the code right now but play nicer on Macs (using $HOME/Library/Application Support).

Thanks for the suggestion.

I think UserConfigDir was added in Go 1.13. Alex, which older versions of Go is pprof promising to support currently. Would it be a problem to require 1.13 or newer?

@ghemawat We follow https://golang.org/doc/devel/release.html#policy, so since 1.14 is the current we support 1.13 and 1.14, so requiring 1.13+ should be fine.

@codecov-io
Copy link

Codecov Report

Merging #535 into master will increase coverage by 1.24%.
The diff coverage is 84.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
+ Coverage   67.05%   68.30%   +1.24%     
==========================================
  Files          37       39       +2     
  Lines        7629     8008     +379     
==========================================
+ Hits         5116     5470     +354     
- Misses       2105     2117      +12     
- Partials      408      421      +13     
Impacted Files Coverage Δ
internal/driver/commands.go 30.21% <0.00%> (-13.79%) ⬇️
internal/driver/webui.go 56.20% <64.51%> (-1.70%) ⬇️
internal/driver/interactive.go 63.86% <68.51%> (+2.21%) ⬆️
internal/driver/settings.go 69.33% <69.33%> (ø)
internal/driver/cli.go 78.65% <88.00%> (-0.19%) ⬇️
internal/driver/driver.go 71.35% <88.13%> (+1.29%) ⬆️
internal/driver/config.go 88.23% <88.23%> (ø)
internal/driver/driver_focus.go 86.06% <100.00%> (ø)
internal/driver/flamegraph.go 87.75% <100.00%> (+0.79%) ⬆️
internal/driver/webhtml.go 100.00% <100.00%> (ø)
... and 2 more

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 160c429...f33251f. Read the comment docs.

Also fix an issue with embedded UIs where the deleteconfig and
saveconfig requests were being sent with absolute URLs instead
of relative URLs.
@ghemawat
Copy link
Contributor Author

ghemawat commented May 6, 2020

Switched to using os.UserConfigDir() as suggested by Russ.

Copy link
Collaborator

@aalexand aalexand left a comment

Choose a reason for hiding this comment

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

I'll merge once the CI passes.

@aalexand aalexand merged commit 427632f into google:master May 7, 2020
giordano added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Nov 13, 2020
* Update pprof to latest revision

Bump from 20191205061153 => 20201109224723

My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`)

Includes:
- google/pprof#564
- google/pprof#575
- google/pprof#574
- google/pprof#571
- google/pprof#572
- google/pprof#570
- google/pprof#562
- google/pprof#561
- google/pprof#565
- google/pprof#560
- google/pprof#563
- google/pprof#557
- google/pprof#554
- google/pprof#552
- google/pprof#545
- google/pprof#549
- google/pprof#547
- google/pprof#541
- google/pprof#534
- google/pprof#542
- google/pprof#535
- google/pprof#531
- google/pprof#530
- google/pprof#528
- google/pprof#522
- google/pprof#525
- google/pprof#527
- google/pprof#519
- google/pprof#520
- google/pprof#517
- google/pprof#518
- google/pprof#514
- google/pprof#513
- google/pprof#510
- google/pprof#508
- google/pprof#506
- google/pprof#509
- google/pprof#504

* Update P/pprof/build_tarballs.jl - use a real version number

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

* Remove now unused `timestamp`

* [pprof] Use `GitSource`

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
Add support for saving the current configuration settings as a named
configuration and reloading such a configuration later.

All of the configurations are stored in a settings file, by default:
~/.config/pprof.json

A struct named config replaces the old map[string]string which used
to store the set of configurable options. Instead, each option is
now a field in the config struct.

The UI now has a new 'Config' menu with

Save as: prompts for a name and saves current config.
Default: applies default config.
$X: for every config named X: applies settings from X.

The currently selected config is highlighted.

Every named config has a delete button to control deletion.

Some semi-related changes:
1. Both filefunctions and functions in the granularity group had an
   initial value of true, which is incorrect since these are mutually
   incompatible choices. Set filefunctions' initial value to false.
2. Renamed the group for sorting from "cumulative" to "sort".
3. Store testing.T in TestUI instead of leaving the field nil.
@ghemawat ghemawat deleted the configs branch April 12, 2022 21:32
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