MiniProfiler / rack-mini-profiler

Profiler for your development and production Ruby rack apps.
MIT License
3.7k stars 402 forks source link

FEATURE: Introduce `pp=async-flamegraph` for asynchronous flamegraphs #494

Closed davidtaylorhq closed 3 years ago

davidtaylorhq commented 3 years ago

Using ?pp=async-flamegraph causes the flamegraph data to be placed in long-term storage, and made available via a link in the mini_profiler UI. Flamegraph data will also be recorded and stored for all AJAX requests with ?pp=async-flamegraph in the Referer header. This is useful in a few situations:


When the pp=async-flamegraph parameter is supplied, a new "flamegraph" link is added to the UI:

Screenshot 2021-04-27 at 23 27 00

Clicking the link will take you to a URL like /mini-profiler-resources/flamegraph?id=t0x70kt7hy3cmitx7adx, which displays the flamegraph UI.

SamSaffron commented 3 years ago

I see what you did here! looks great. Some feedback

  1. How sticky is Referer? For something like Discourse do we want to clear it or maintain it between interior link?

  2. We need some documentation in the README

  3. We need some tests

  4. Do we want a "ninja" option of "Please gather me 3 flamegraphs for routes matching /my/amazing/page ? This is extremely powerful for debugging and pretty usable. (similar to how we do sampling)

@OsamaSayegh can you have a look as well?

davidtaylorhq commented 3 years ago

How sticky is Referer? For something like Discourse do we want to clear it or maintain it between interior link?

Yeah I think we want to make the parameter sticky for internal links. I made a draft for that in https://github.com/discourse/discourse/pull/12863

Agreed on 2 & 3 - will look at those things tomorrow.

Do we want a "ninja" option of "Please gather me 3 flamegraphs for routes matching /my/amazing/page ? This is extremely powerful for debugging and pretty usable. (similar to how we do sampling)

Sounds cool! How do you imagine the UX working here? I guess flamegraphs could be added to the sampling feature, although it is quite a lot of data so we probably wouldn't want to be shipping it to other sites like we do with the other sampled data.

SamSaffron commented 3 years ago

Sounds cool! How do you imagine the UX working?

I guess I would start backwards :) My use case is:

  1. I am on the site awesome.com I have MiniProfiler access on the site
  2. I would like to get 10 flamegraphs for the route /homepage from random users.

Even though I can get a flamegraph today for /homepage it will include some extra "admin" and "logged on user" data. This may be less interesting for a particular profiling session.

I am thinking I would use a special route to "enable collection" for a path. Then I would head to another page to see all the current collected results.

A "list" UI is generally useful anyway for the REFERER solution cause it allows you to easily dig through history.

I would say that data forwarding (like we do for sampling) may be a v3 kind of feature. (list UI and strategic route based collection can also be a v2 thing, it should not hold up this PR)

codecov-commenter commented 3 years ago

Codecov Report

Merging #494 (41398c8) into master (aef9b23) will decrease coverage by 1.10%. The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
- Coverage   88.70%   87.59%   -1.11%     
==========================================
  Files          18       18              
  Lines        1257     1274      +17     
==========================================
+ Hits         1115     1116       +1     
- Misses        142      158      +16     
Impacted Files Coverage Δ
lib/mini_profiler/profiler.rb 84.07% <34.78%> (-2.99%) :arrow_down:
lib/mini_profiler/timer_struct/page.rb 88.88% <100.00%> (+0.36%) :arrow_up:

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 aef9b23...41398c8. Read the comment docs.

davidtaylorhq commented 3 years ago

Thanks @SamSaffron and @OsamaSayegh. Updated based on your comments, now with tests, and updates to the README/CHANGELOG.

OsamaSayegh commented 3 years ago

Ok, I'm merging this in, thanks @davidtaylorhq ❤️

We will need @SamSaffron to push a new version on rubygems because I don't think I have push access there.