-
Notifications
You must be signed in to change notification settings - Fork 618
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
internal/driver/webui.go
Outdated
"/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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
f69f379
to
4d87e8c
Compare
Fixes #777.