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

Remove old d3 flamegraph code and d3 dependency. #825

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

aalexand
Copy link
Collaborator

Fixes #777.

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ec68065) 66.97% compared to head (4d87e8c) 66.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #825      +/-   ##
==========================================
- Coverage   66.97%   66.83%   -0.14%     
==========================================
  Files          45       44       -1     
  Lines        9865     9800      -65     
==========================================
- Hits         6607     6550      -57     
+ Misses       2797     2790       -7     
+ Partials      461      460       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ghemawat
ghemawat previously approved these changes Dec 18, 2023
Copy link
Contributor

@ghemawat ghemawat left a comment

Choose a reason for hiding this comment

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

Thanks!

"/source": http.HandlerFunc(ui.source),
"/peek": http.HandlerFunc(ui.peek),
"/flamegraph": http.HandlerFunc(ui.stackView),
// TODO: Remove supporting this older flamegraph2 URL at some point.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we perhaps change this to a permanent HTTP redirect to avoid people bookmarking /flamegraph2?

Also, do we want to preserve support for /flamegraphold (either redirect or an alias) to avoid breaking existing bookmarks, links, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, makes sense, but after spending 10 minutes on this it turned out to be trickier than I thought - http.Redirect("flamegraph") doesn't work since we have this higher level stripping of "/ui" and I assume we don't want to hardcode "/ui" in the redirect path since this defeats the purpose of this prefix being transparent to the implementation. I tried using a relative path in the redirect location but that didn't work either for some reason, need to look into this more. Maybe the StripPrefix() handler need to inspect the response and recognize redirects adding the /ui prefix back, but that means that whoever embeds this server in their server under a custom prefix will have to do the same and meanwhile the redirects will be broken for them. I might be missing a trivial solution? Anyway, will need to look more into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get the current request's URL and replace a trailing /flamegraph2 with /flamegraph to form the new URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, StripPrefix() modifies the request URL destructively.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, /foo redirects to /ui/foo. Then /ui/foo is handled by stripping the /ui prefix and then looking up the handler in the handler map.

What if we drop the StripPrefix call and have the handler map keys start with /ui? But perhaps some clients are doing something weirder?

Alternative: instead of calling http.StripPrefix, we provide our own wrapper that first examines the original URL and either (a) redirects from flamegraph2 to flamegraph, or (b) returns the precomputed http.StripPrefix() result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ended up using a small bit of code to return a relative path redirect instead of using http.Redirect() which does not support that. This is similar to this code in Go runtime itself so I think this is reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great!

@aalexand aalexand merged commit 5aaadb5 into google:main Dec 29, 2023
31 checks passed
This was referenced Feb 16, 2024
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.

Switch to the new flame graph implementation by default and remove d3 dependency
3 participants