Support different display granularities in flame graph view. #896
Conversation
Previously, flame-graph view could only display filefunctions granularity. We can now support all the available granularities. This allows the user to supply a granularity either via a command-line flag (e.g., --files), or as a URL parameter (e.g., ?g=files). Details * Made the default initial granularity "" so that we can stick with the default flame-graph granularity of "filefunctions" while also allowing overrides. * Use different computation of the list of shorter display names for file names (strip off leading path components instead of package prefixes). * Similarly compute color based on directory name instead of package name. * Include line number and column number in displayed names if the granularity includes line numbers.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #896 +/- ##
==========================================
+ Coverage 66.86% 67.41% +0.54%
==========================================
Files 44 44
Lines 9824 9851 +27
==========================================
+ Hits 6569 6641 +72
+ Misses 2794 2762 -32
+ Partials 461 448 -13 ☔ View full report in Codecov by Sentry. |
| @@ -68,7 +68,7 @@ func defaultConfig() config { | |||
| Trim: true, | |||
| DivideBy: 1.0, | |||
| Sort: "flat", | |||
| Granularity: "functions", | |||
There was a problem hiding this comment.
One potentially unintended consequence of this change is that the default is now unclear in the output of the "o" command in the interactive shell.
Before:
$ pprof profile.pb.gz
...
(pprof) o
...
granularity = functions //: [addresses | filefunctions | files | functions | lines]
...
With the change:
$ go run pprof.go profile.pb.gz
...
(pprof) o
...
granularity = //: [addresses | filefunctions | files | functions | lines]
...
There was a problem hiding this comment.
Maybe this is reasonable given that the change proposes having different default granularities for different views.
There was a problem hiding this comment.
Or maybe we should at least show a string like "default" here? Showing empty string just looks a bit odd.
There was a problem hiding this comment.
We now show:
granularity = (default) //: [addresses | filefunctions | files | functions | lines]
| } | ||
|
|
||
| for _, c := range []testCase{ | ||
| test("empty", "", "."), |
There was a problem hiding this comment.
Is "." expected here? I'm worried a little we'll show this dot in the UI when the profile does not have filenames (e.g. binary had symbols but no debug info).
There was a problem hiding this comment.
It is an artifact of the implementation leaking through.
I can fix this in a subsequent change. Any suggestions for a better thing to display. Maybe "?" if neither func/file is available?
There was a problem hiding this comment.
At a minimum I'd probably just leave it empty for now. I.e. just not turn empty strings into the dot. For the "?" - maybe, not sure, would need to look at it first. I believe we also use "unknown" and "" placeholders in some places, if we'd want to start using "?" I think three different ones might be too many :)
Previously, flame-graph view could only display filefunctions granularity. We can now support all the available granularities.
This allows the user to supply a granularity either via a command-line flag (e.g., --files), or as a URL parameter (e.g., ?g=files).
Details