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

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

Merged
merged 6 commits into from Apr 28, 2021

Conversation

davidtaylorhq
Copy link
Contributor

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:

  • You want to view flamegraphs for AJAX requests made by a Javascript application. By supplying pp=async-flamegraph, flamegraph links for every AJAX request will be made available in the mini-profiler UI.

  • You want to see the HTML result of a request, and view the flamegraph later. The existing ?pp=flamegraph option hides the true output.

  • You are performing the request via a tool like curl, and would like to view the flamegraph later in the browser (you can extract the X-MiniProfiler-Ids header from the response, then view flamegraph in the browser)


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.

Using `?pp=async-flamegraph` causes the flamegraph data to be stored in the page_struct, 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:

- You want to view flamegraphs for AJAX requests made by a Javascript application. By supplying `pp=async-flamegraph`, flamegraph links for every AJAX request will be made available in the mini-profiler UI.

- You want to see the HTML result of a request, and view the flamegraph later. The existing `?pp=flamegraph` option hides the true output.

- You are performing the request via a tool like `curl`, and would like to view the flamegraph later in the browser (you can extract the X-MiniProfiler-Ids header from the response, then view flamegraph in the browser)
@davidtaylorhq davidtaylorhq marked this pull request as draft April 27, 2021 22:40
davidtaylorhq added a commit to discourse/discourse that referenced this pull request Apr 27, 2021
This updates the preview_theme_id preservation logic to use more recent, robust, browser APIs. It also adds support for preserving the `?pp=async-flamegraph` parameter which is proposed in MiniProfiler/rack-mini-profiler#494
@SamSaffron
Copy link
Member

SamSaffron commented Apr 27, 2021

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
Copy link
Contributor Author

davidtaylorhq commented Apr 27, 2021

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 discourse/discourse#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
Copy link
Member

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)

Copy link
Collaborator

@OsamaSayegh OsamaSayegh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks excellent to me 👌 Just one minor comment/question.

lib/html/includes.tmpl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2021

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%) ⬇️
lib/mini_profiler/timer_struct/page.rb 88.88% <100.00%> (+0.36%) ⬆️

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.

All we need for the `results` route is a boolean. The `/flamegraph` route takes care of the actual flamegraph data
@davidtaylorhq davidtaylorhq marked this pull request as ready for review April 28, 2021 14:55
@davidtaylorhq
Copy link
Contributor Author

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

@OsamaSayegh
Copy link
Collaborator

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.

@OsamaSayegh OsamaSayegh merged commit 020cd4f into MiniProfiler:master Apr 28, 2021
@davidtaylorhq davidtaylorhq deleted the async-flamegraph branch April 28, 2021 18:17
davidtaylorhq added a commit to discourse/discourse that referenced this pull request Apr 30, 2021
This updates the preview_theme_id preservation logic to use more recent, robust, browser APIs. It also adds support for preserving the `?pp=async-flamegraph` parameter which is proposed in MiniProfiler/rack-mini-profiler#494
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants