Add -tagroot and -tagleaf options

[mhansen]

These add synthetic stack frames to samples.

Example usage:

$ pprof -tagroot thread pprof.proto

Will add synthetic stack frames at the root like thread:UIThread and
thread:BackgroundPool-1.

Closes #558

[aalexand]

This looks very nice, thanks for adding this! Except the comment on formatting / scaling the numeric tag values this review is a bunch of style comments.

[aalexand]

All are nits except a question about handling NumLabel in the root tag loop.

[aalexand]

Apologies about more comments, I should have tried to identify all these things earlier. I think this is very close though.

[aalexand]

@mhansen looks like there are test failures.

[mhansen]

Sorry about all the re-pushing and reapprovals necessary. I think I needed to upgrade my go version to get gofmt to work the same as CI.

[codecov-commenter]

Codecov Report

Merging #649 (f556682) into master (1096026) will increase coverage by 0.29%.
The diff coverage is n/a.

❗ Current head f556682 differs from pull request most recent head 6174693. Consider uploading reports for the commit 6174693 to get more accurate results

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   67.89%   68.19%   +0.29%     
==========================================
  Files          40       41       +1     
  Lines        7313     7388      +75     
==========================================
+ Hits         4965     5038      +73     
- Misses       1908     1909       +1     
- Partials      440      441       +1     

Impacted Files	Coverage Δ
.../github.com/google/pprof/internal/driver/config.go	86.50% <0.00%> (ø)
...ithub.com/google/pprof/internal/driver/commands.go	25.83% <0.00%> (ø)
...github.com/google/pprof/internal/driver/tagroot.go	100.00% <0.00%> (ø)
.../github.com/google/pprof/internal/driver/driver.go	70.74% <0.00%> (+0.35%)	⬆️

---

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 1096026...6174693. Read the comment docs.

[aalexand]

Looks like the CI tests fail on Go tip. We should probably fix this before we can submit this.

[aalexand]

Filed #653.

[mhansen]

I've been playing around with this a little, and I'm not so sure any more about prefixing with labelname:. On the flamegraph view, where horizontal space is limited, this usually pushes out the interesting bit (the label value) into the ellipsised section, meaning you have to hover to see the value.

This isn't a showstopper, I just think it might be suboptimal, there might be a better solution hiding, and maybe we should discuss options.

Let's generate some options:

1. Add two frames, one for the label key, one for the label value. Then we can see both. e.g. threadpool in one frame, then below that, DefaultPool%d. This makes the graph kinda self-explanatory in a nice way I think, and vertical space is cheap. But then again, the label is kinda meaningless to select, and it will visually associate with the frame above, rather than the frames below, as it's larger.
2. A variation on the above: show one frame threadpool:DefaultPool%d above, then DefaultPool%d below. This is just like the above, but ensures the label visually groups with the value by making them the same width. It feels a bit hacky to duplicate it though.
3. Only show the label value. Given we're adding frames unconditionally, even if the label value is empty, it's implicit that the first frames will be the label values. There's some prior art here: Brendan Gregg's ACM Queue flamegraph article shows a flamegraph where the root has frames like javac and swapper -- thread comm names.
4. Instead of labelKey:labelValue invert the positioning somehow, like labelValue (labelKey) or labelValue [labelKey]? That might give trouble with regex search. Or we could get creative, labelValue←labelKey. Are there any programming languages with keys on the right hand side of hashtables? What's their syntax?
5. Keep labelKey:labelValue. This is similar to what felixge's pprofutils does: https://github.com/felixge/pprofutils#example-1-add-root-frames-for-pprof-label-values.

Thoughts? (there's probably other options!) I think I'm leaning towards "Only show the label value" and perhaps "Add two frames, one for key, one for value"

[aalexand]

(why is there no way to reply to a comment in GitHub that is not a code review comment?..)

Thoughts? (there's probably other options!) I think I'm leaning towards "Only show the label value" and perhaps "Add two frames, one for key, one for value"

I need to sit on this a bit longer but my initial reaction is that I don't like two frame approach for how weirdly it will interplay with the filtering - e.g. -ignore=<tag name> will non-atomically filter out the frames corresponding to the label name which seems weird to my engineer's mind...

[nolanmar511]

Thoughts? (there's probably other options!) I think I'm leaning towards "Only show the label value" and perhaps "Add two frames, one for key, one for value"

If it would work, I might suggest storing tag value as the "function name" and the tag key as the "filename". For the flame graph, it should be be roughly equivalent visually "only show the label value", but the function name should still show up in the tooltip and be considered when filtering.

[aalexand]

If it would work, I might suggest storing tag value as the "function name" and the tag key as the "filename". For the flame graph, it should be be roughly equivalent visually "only show the label value", but the function name should still show up in the tooltip and be considered when filtering.

I think it's a great idea to explore!

[mhansen]

If it would work, I might suggest storing tag value as the "function name" and the tag key as the "filename". For the flame graph, it should be be roughly equivalent visually "only show the label value", but the function name should still show up in the tooltip and be considered when filtering.

I think this is a great idea! I've implemented it, and I like it. It looks great in our internal pprof flamegraph viewer (e.g. http://go/pprof-649) where the tooltip shows the filename.

FYI, in the pprof web flamegraph the tooltip doesn't show the filename (yet):

I think I still like this approach the best, using the filename, and perhaps we should file a separate issue to show the filename in the tooltip in the web UI's flamegraph.

[mhansen]

I should add, thank you for your ideas! This is a fun collaboration for me :-), I'm enjoying the brainstorming

[mhansen]

In my latest commit, I've changed the test strings to be https://github.com/brendangregg/FlameGraph "Folded Stack" format, which I'm fairly used to and I think demonstrates the empty stack frames (with semicolon-delimited frames) better than spaces.

And I've gotten rid of the two layers of interning. You were right Alexey, I should have done this a while ago when you suggested it.

[aalexand]

I think this looks good. A few bikeshedding comments but LGTM otherwise - I think it's ready to be merged.