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

webhtml: reenable sort option for flamegraphs #491

Merged
merged 3 commits into from
Oct 25, 2019

Conversation

bbrks
Copy link
Contributor

@bbrks bbrks commented Oct 21, 2019

This sorts flamegraph elements lexicographically such that two similar profiles can be compared side-by-side to eyeball differences in flamegraph profile shapes and sizes.

Flamegraph elements prior to #367 were sorted, but since then, it has been difficult to eyeball 2 similar profiles side-by-side, as the elements appear in a somewhat random order.

Before sorting profile comparison:

Screenshot 2019-10-21 at 22 50 25

After sorting profile comparison:

Screenshot 2019-10-21 at 22 50 18

This sorts flamegraph elements lexographiaclly such that two similar
profiles can be compared side-by-side to eyeball differences in
flamegraph profile shapes and sizes.
@nolanmar511
Copy link
Contributor

nolanmar511 commented Oct 21, 2019

I know there are differing opinions for whether the flame graph should be sorted by total value for each node, or whether it should be sorted by function name (mainly depending on the use case), but it looks like the order of nodes now is random. This change strikes me as positive (especially given that profile comparison is not supported in pprof's flame graph view).

@aalexand or @kalyanac -- This change LGTM, but I'll let either of you decide if this should be approved and merged,

@aalexand
Copy link
Collaborator

@nolanmar511 Yes, I think the change is an improvement. I am not sure whether this implementation of the flame graph is capable of sorting by the total value, but whatever sorting is better than no sorting.

Something is up with the CI tests though: looks like Travis CI is going to fail with

+++staticcheck github.com/google/pprof github.com/google/pprof/driver github.com/google/pprof/fuzz github.com/google/pprof/internal/binutils github.com/google/pprof/internal/driver github.com/google/pprof/internal/elfexec github.com/google/pprof/internal/graph github.com/google/pprof/internal/measurement github.com/google/pprof/internal/plugin github.com/google/pprof/internal/proftest github.com/google/pprof/internal/report github.com/google/pprof/internal/symbolizer github.com/google/pprof/internal/symbolz github.com/google/pprof/internal/transport github.com/google/pprof/profile github.com/google/pprof/third_party/d3 github.com/google/pprof/third_party/d3flamegraph github.com/google/pprof/third_party/svgpan
internal/driver/interactive_test.go:308:42: possible nil pointer dereference (SA5011)
The command "./test.sh" exited with 1.

which we need to fix and looks like AppVeyor build didn't start at all.

@codecov-io
Copy link

codecov-io commented Oct 25, 2019

Codecov Report

Merging #491 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #491      +/-   ##
=========================================
+ Coverage    67.1%   67.1%   +<.01%     
=========================================
  Files          37      37              
  Lines        7602    7603       +1     
=========================================
+ Hits         5101    5102       +1     
  Misses       2097    2097              
  Partials      404     404
Impacted Files Coverage Δ
internal/driver/webhtml.go 100% <100%> (ø) ⬆️

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 5260658...75f3d0f. Read the comment docs.

@aalexand aalexand merged commit a8b9f9d into google:master Oct 25, 2019
@bbrks bbrks deleted the sorted_flamegraphs branch October 25, 2019 15:21
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
This sorts flamegraph elements lexographiaclly such that two similar
profiles can be compared side-by-side to eyeball differences in
flamegraph profile shapes and sizes.
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