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

Flame Graph support on Web UI #188

Merged
merged 45 commits into from Nov 21, 2017

Conversation

Projects
None yet
8 participants
@spiermar
Contributor

spiermar commented Aug 10, 2017

Implements #166

Details:

  • Basic flame graph implementation under http output.
  • Tested with both CPU and Memory profiles.
  • Endpoint is /flamegraph, and there's a link in /,
  • Opted not to use the directed graph to generate the flame graph since it doesn't have the detailed information required. Parsed the raw samples instead.
  • Not doing any filtering in the samples.
  • Output is mostly self-contained and could be saved. It should also be easy to add an option to export the html file.
  • Making available all sample types in the UI, via a drop-down. e.g.: CPU profile will have samples and cpu.
  • Displaying some profile metadata like profile time and duration.
  • Converting nanoseconds to seconds. I'm not sure if this is ideal, but same thing is being done in the main view. Happy to revert it if necessary.
  • Using a bootstrap layout now, but happy to revert it if you believe it would be better to keep a simple layout without the bootstrap dependency.
  • Implements all features mentioned by @brendangregg, except different palettes, which is under development and requires changes in the D3 plugin.
  • We are continuously adding more features to the D3 plugin, and those should be easily assimilated by this.

A demo can be found in pprof_flame.html

Happy to make any changes deemed necessary.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 10, 2017

Codecov Report

Merging #188 into master will increase coverage by 0.55%.
The diff coverage is 95.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   65.29%   65.84%   +0.55%     
==========================================
  Files          34       35       +1     
  Lines        7212     7346     +134     
==========================================
+ Hits         4709     4837     +128     
- Misses       2109     2113       +4     
- Partials      394      396       +2
Impacted Files Coverage Δ
internal/driver/webui.go 58.44% <100%> (+0.19%) ⬆️
internal/driver/webhtml.go 100% <100%> (ø) ⬆️
internal/driver/flamegraph.go 85% <85%> (ø)

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 71d5bad...7b1f77e. Read the comment docs.

codecov-io commented Aug 10, 2017

Codecov Report

Merging #188 into master will increase coverage by 0.55%.
The diff coverage is 95.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   65.29%   65.84%   +0.55%     
==========================================
  Files          34       35       +1     
  Lines        7212     7346     +134     
==========================================
+ Hits         4709     4837     +128     
- Misses       2109     2113       +4     
- Partials      394      396       +2
Impacted Files Coverage Δ
internal/driver/webui.go 58.44% <100%> (+0.19%) ⬆️
internal/driver/webhtml.go 100% <100%> (ø) ⬆️
internal/driver/flamegraph.go 85% <85%> (ø)

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 71d5bad...7b1f77e. Read the comment docs.

@aalexand

This comment has been minimized.

Show comment
Hide comment
@aalexand

aalexand Aug 10, 2017

Collaborator

Given that pprof is vendored into Golang sources and given that it's used internally at Google, I am skeptical about us being able to accept the source which <script>'s a number of locations on the Internet. @rsc @rauls5382 @rakyll

Collaborator

aalexand commented Aug 10, 2017

Given that pprof is vendored into Golang sources and given that it's used internally at Google, I am skeptical about us being able to accept the source which <script>'s a number of locations on the Internet. @rsc @rauls5382 @rakyll

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Aug 10, 2017

Contributor

@aalexand If that's a concern, it should be possible to source those files from pprof's http server.

Contributor

spiermar commented Aug 10, 2017

@aalexand If that's a concern, it should be possible to source those files from pprof's http server.

@rauls5382

This comment has been minimized.

Show comment
Hide comment
@rauls5382

rauls5382 Aug 10, 2017

Collaborator

Thank you. This looks pretty cool.

I agree we should freeze the version of all scripts/plugins and serve them from the pprof http server. That way pprof won't be broken if there are incompatible changes in the future.

The Go distribution would have to vendor any dependent packages as well, similar to what they already do to pick up the ianlancetaylor/demangle package. If they decide against it (which I doubt), we could find a way to make it easy to trim that functionality via plugins.

Collaborator

rauls5382 commented Aug 10, 2017

Thank you. This looks pretty cool.

I agree we should freeze the version of all scripts/plugins and serve them from the pprof http server. That way pprof won't be broken if there are incompatible changes in the future.

The Go distribution would have to vendor any dependent packages as well, similar to what they already do to pick up the ianlancetaylor/demangle package. If they decide against it (which I doubt), we could find a way to make it easy to trim that functionality via plugins.

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Aug 10, 2017

Contributor

Thanks @rauls5382

Versioning shouldn't theoretically be a problem, since all external resource references are versioned CDN urls. Security-wise, the external resource could be compromised, so I've added integrity check to all resources to be on the safe side.

If vendor the dependent packages is a better alternative, there are a few different ways of getting this done, and I'm not sure what would be best option, from a pprof perspective. Let me know and I can make the changes.

Contributor

spiermar commented Aug 10, 2017

Thanks @rauls5382

Versioning shouldn't theoretically be a problem, since all external resource references are versioned CDN urls. Security-wise, the external resource could be compromised, so I've added integrity check to all resources to be on the safe side.

If vendor the dependent packages is a better alternative, there are a few different ways of getting this done, and I'm not sure what would be best option, from a pprof perspective. Let me know and I can make the changes.

@rakyll

This comment has been minimized.

Show comment
Hide comment
@rakyll

rakyll Aug 10, 2017

Member

@spiermar Can you compile all the external scripts in a single one and embed the final artifact in a Go file? Having said that, I am not sure how licensing will work. @aalexand, @rauls5382, do we need a separate in the repo with a licensing file to be able to vendor the external dependencies?

Member

rakyll commented Aug 10, 2017

@spiermar Can you compile all the external scripts in a single one and embed the final artifact in a Go file? Having said that, I am not sure how licensing will work. @aalexand, @rauls5382, do we need a separate in the repo with a licensing file to be able to vendor the external dependencies?

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Aug 10, 2017

Contributor

@rakyll already have the scripts to create combined and minified vendor.js and vendor.css files. Licensing is the part needs checking. The flame graph plugin is Apache 2.0, so not a problem. d3 is BSD-3 and d3-tip is MIT. Already removed the lodash dependency in the last commit, so not a problem anymore. Bootstrap could be removed.

Contributor

spiermar commented Aug 10, 2017

@rakyll already have the scripts to create combined and minified vendor.js and vendor.css files. Licensing is the part needs checking. The flame graph plugin is Apache 2.0, so not a problem. d3 is BSD-3 and d3-tip is MIT. Already removed the lodash dependency in the last commit, so not a problem anymore. Bootstrap could be removed.

@aalexand

This comment has been minimized.

Show comment
Hide comment
@aalexand

aalexand Aug 14, 2017

Collaborator

@rakyll Yes, I would probably expect something like third_party/d3 and third_party/flame-something-something for the dependencies along with the LICENSE files in there. It would be good to pre-check with the Go team maybe. What concerns me the most is having pprof which is a local tool fetch any resources from elsewhere on the web. Security aside, pprof and "go tool pprof" should be functional without requiring internet access.

Collaborator

aalexand commented Aug 14, 2017

@rakyll Yes, I would probably expect something like third_party/d3 and third_party/flame-something-something for the dependencies along with the LICENSE files in there. It would be good to pre-check with the Go team maybe. What concerns me the most is having pprof which is a local tool fetch any resources from elsewhere on the web. Security aside, pprof and "go tool pprof" should be functional without requiring internet access.

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Aug 14, 2017

Contributor

I'll make the changes later today

Contributor

spiermar commented Aug 14, 2017

I'll make the changes later today

@aalexand

This comment has been minimized.

Show comment
Hide comment
@aalexand
Collaborator

aalexand commented Aug 14, 2017

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Aug 15, 2017

Contributor

JavaScript resources are inline now and I've also removed the bootstrap dependency. License files are also checked in.
Considered using the minified versions of the scripts, but decided to follow the same pattern from the svgPan third party library.
Same for inline vs. independent resource endpoints. Inlining is not ideal for multiple reasons, but followed the same pattern as svgPan. On the plus side, the HTML can be saved this way.
View could benefit from a few UI/UX improvements, but it's working.

Contributor

spiermar commented Aug 15, 2017

JavaScript resources are inline now and I've also removed the bootstrap dependency. License files are also checked in.
Considered using the minified versions of the scripts, but decided to follow the same pattern from the svgPan third party library.
Same for inline vs. independent resource endpoints. Inlining is not ideal for multiple reasons, but followed the same pattern as svgPan. On the plus side, the HTML can be saved this way.
View could benefit from a few UI/UX improvements, but it's working.

@aalexand

This comment has been minimized.

Show comment
Hide comment
@aalexand

aalexand Aug 15, 2017

Collaborator

Thanks. Some comments:

  • The code coverage went down. Can you add tests to cover the new code?
  • I tried opening the flame graph with a 5 MB profile.pb.gz file (which I can't immediately share), the graph view choked on that. Do you want to test with some large profiles to see whether there are some optimizations to do?
  • I would expect a bit more unification between the graph view and the flame graph view. More specifically:
    • The metric selection (e.g. samples vs. cpu) should be present on both pages.
    • The filtering like focus / ignore / hide / show should be present on both pages.
    • There should be a way back from the flame view back to the graph.
Collaborator

aalexand commented Aug 15, 2017

Thanks. Some comments:

  • The code coverage went down. Can you add tests to cover the new code?
  • I tried opening the flame graph with a 5 MB profile.pb.gz file (which I can't immediately share), the graph view choked on that. Do you want to test with some large profiles to see whether there are some optimizations to do?
  • I would expect a bit more unification between the graph view and the flame graph view. More specifically:
    • The metric selection (e.g. samples vs. cpu) should be present on both pages.
    • The filtering like focus / ignore / hide / show should be present on both pages.
    • There should be a way back from the flame view back to the graph.
@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Aug 15, 2017

Contributor
  • Yes, had it on my list to add a few tests.
  • The sluggishness on large flame graphs comes from the large number of rect that need to be drawn. Generally because of tons of small value elements that can hardly be seen without zooming. I'm looking into hiding small frames (spiermar/d3-flame-graph#51), but that's not done yet. A temporary fix might be filtering before creating the data structure, like the directed graph, but that reduces accuracy. I can add filtering, but would also like to give the user an option to disable it.
  • Agree with the metric selection on both views. I just need to understand how to get that out of the report to add a selection in the other view.
  • The focus/ignore/hide/show filtering is a bit more tricky since they are not implemented in the plugin and generally not how users interact with flame graphs. Zooming and searching are the basic interactions. @brendangregg, any thoughts?
  • I will add a link back to the graph.
Contributor

spiermar commented Aug 15, 2017

  • Yes, had it on my list to add a few tests.
  • The sluggishness on large flame graphs comes from the large number of rect that need to be drawn. Generally because of tons of small value elements that can hardly be seen without zooming. I'm looking into hiding small frames (spiermar/d3-flame-graph#51), but that's not done yet. A temporary fix might be filtering before creating the data structure, like the directed graph, but that reduces accuracy. I can add filtering, but would also like to give the user an option to disable it.
  • Agree with the metric selection on both views. I just need to understand how to get that out of the report to add a selection in the other view.
  • The focus/ignore/hide/show filtering is a bit more tricky since they are not implemented in the plugin and generally not how users interact with flame graphs. Zooming and searching are the basic interactions. @brendangregg, any thoughts?
  • I will add a link back to the graph.

spiermar added some commits Nov 19, 2017

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Nov 19, 2017

Contributor

@aalexand Finally got a bit of time to get back to this. The call_tree option is done, and also the percentage of total. That's all I had on my list. Let me know how that looks.

Contributor

spiermar commented Nov 19, 2017

@aalexand Finally got a bit of time to get back to this. The call_tree option is done, and also the percentage of total. That's all I had on my list. Let me know how that looks.

@aalexand

Thanks!

Is the current code rebased to the latest master?

Show outdated Hide outdated third_party/d3tip/d3_tip.go
Show outdated Hide outdated third_party/d3tip/d3_tip.go
Show outdated Hide outdated internal/driver/flamegraph.go
Show outdated Hide outdated internal/driver/flamegraph.go
@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Nov 19, 2017

Contributor

@aalexand Yes, it was rebased.

Contributor

spiermar commented Nov 19, 2017

@aalexand Yes, it was rebased.

@aalexand

I also wonder if the size of the dependencies can be further reduced. I saw d3 is composed of microlibraries. Do we need all of d3 here or we can limit the dependency to a couple of those?

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Nov 19, 2017

Contributor

I could create a custom D3 build. Let me check how much that would reduce the bundle size.

Contributor

spiermar commented Nov 19, 2017

I could create a custom D3 build. Let me check how much that would reduce the bundle size.

@aalexand

This comment has been minimized.

Show comment
Hide comment
@aalexand

aalexand Nov 19, 2017

Collaborator

@spiermar Great, thanks a lot, that's much smaller! Can you add a README.md in the d3 directory which documents how the steps you did can be repeated to upgrade from upstream when we need to?

Collaborator

aalexand commented Nov 19, 2017

@spiermar Great, thanks a lot, that's much smaller! Can you add a README.md in the d3 directory which documents how the steps you did can be repeated to upgrade from upstream when we need to?

@aalexand

This comment has been minimized.

Show comment
Hide comment
@aalexand

aalexand Nov 20, 2017

Collaborator

@spiermar Overall I think it looks good now. One thing: would you be open (and would it be difficult) to switch the flame graph to grow down rather than up? We found recently that with deep flame graphs it looks more natural in the UI when they grow down and you have to scroll to get the finer level of details (deeper elements) rather than having to scroll just to see the root. See the attached example.
6p3qt0nedam

Collaborator

aalexand commented Nov 20, 2017

@spiermar Overall I think it looks good now. One thing: would you be open (and would it be difficult) to switch the flame graph to grow down rather than up? We found recently that with deep flame graphs it looks more natural in the UI when they grow down and you have to scroll to get the finer level of details (deeper elements) rather than having to scroll just to see the root. See the attached example.
6p3qt0nedam

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Nov 20, 2017

Contributor

@aalexand it's in the roadmap for the d3 flame graph plugin (spiermar/d3-flame-graph#73), but I don't have a target date for the release yet.

Contributor

spiermar commented Nov 20, 2017

@aalexand it's in the roadmap for the d3 flame graph plugin (spiermar/d3-flame-graph#73), but I don't have a target date for the release yet.

@brendangregg

This comment has been minimized.

Show comment
Hide comment
@brendangregg

brendangregg Nov 20, 2017

My original flamegraph.pl did this layout too (icicle), but here's how I interpret each flame graph:

  1. sweep top down and look for what's on-CPU, especially any large plateaus, to identify what the CPU is doing.
  2. then sweep bottom-up to see how we got there, which is generally quicker after knowing (1).

Last week I had large plateau of UTF-8 processing (51%), then swept bottom up to look for UTF-8 or string-related functions, skipping the rest (a lot of framework frames). This meant I could find the relevant UTF-8/string frames more quickly, after knowing the on-CPU context first.

Another recent one was gzip. Another was kernel network interrupt processing -- reading that bottom up would be a waste of time, since what's ultimately burning CPU is unrelated to the user-level program that's running.

Reading bottom-up (root first) still gets the job done. My point is that knowing some on-CPU context first can make that quicker, as you have a clue as to what you're looking for.

I've used the icicle layout when reversing the stack order as well, so that the top frame remains what is on-CPU, and beneath it is ancestry.

That's one of the big reasons we wanted to rewrite this in d3, so that flipping the layout direction and merge direction could be button presses in an interactive d3 graph. Doing it with my Perl program meant rerunning the Perl program.

brendangregg commented Nov 20, 2017

My original flamegraph.pl did this layout too (icicle), but here's how I interpret each flame graph:

  1. sweep top down and look for what's on-CPU, especially any large plateaus, to identify what the CPU is doing.
  2. then sweep bottom-up to see how we got there, which is generally quicker after knowing (1).

Last week I had large plateau of UTF-8 processing (51%), then swept bottom up to look for UTF-8 or string-related functions, skipping the rest (a lot of framework frames). This meant I could find the relevant UTF-8/string frames more quickly, after knowing the on-CPU context first.

Another recent one was gzip. Another was kernel network interrupt processing -- reading that bottom up would be a waste of time, since what's ultimately burning CPU is unrelated to the user-level program that's running.

Reading bottom-up (root first) still gets the job done. My point is that knowing some on-CPU context first can make that quicker, as you have a clue as to what you're looking for.

I've used the icicle layout when reversing the stack order as well, so that the top frame remains what is on-CPU, and beneath it is ancestry.

That's one of the big reasons we wanted to rewrite this in d3, so that flipping the layout direction and merge direction could be button presses in an interactive d3 graph. Doing it with my Perl program meant rerunning the Perl program.

@aalexand

This comment has been minimized.

Show comment
Hide comment
@aalexand

aalexand Nov 20, 2017

Collaborator

@spiermar Note a couple of other open comments I made.

Collaborator

aalexand commented Nov 20, 2017

@spiermar Note a couple of other open comments I made.

@spiermar

This comment has been minimized.

Show comment
Hide comment
@spiermar

spiermar Nov 20, 2017

Contributor

@aalexand regarding the D3 build, sure. I used rollup to build the distribution and the configuration is on my Github page (https://github.com/spiermar/d3-pprof). With that it's just a single command to generate the build. If it's Ok to link to the repo, I'll add the instructions to a README.md file on pprof and also update the repo with the instructions.

Contributor

spiermar commented Nov 20, 2017

@aalexand regarding the D3 build, sure. I used rollup to build the distribution and the configuration is on my Github page (https://github.com/spiermar/d3-pprof). With that it's just a single command to generate the build. If it's Ok to link to the repo, I'll add the instructions to a README.md file on pprof and also update the repo with the instructions.

@aalexand

This comment has been minimized.

Show comment
Hide comment
@aalexand

aalexand Nov 20, 2017

Collaborator

@spiermar I'd prefer to include self-contained instructions on how to do the build inside of pprof than point to another repo. This is to keep the deps minimal, even if just for the build process.

Collaborator

aalexand commented Nov 20, 2017

@spiermar I'd prefer to include self-contained instructions on how to do the build inside of pprof than point to another repo. This is to keep the deps minimal, even if just for the build process.

@aalexand aalexand merged commit 80fc05d into google:master Nov 21, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment