-
Notifications
You must be signed in to change notification settings - Fork 11
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
Autoredirect remote profiles #108
Conversation
d741463
to
30ae916
Compare
Instead of logging profile viewing URLs, we can just redirect to them when the developer wants to see them. Saves copy-pasting the path.
30ae916
to
84e9d86
Compare
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 am with you on making autoredirect the default behaviour.
I added a commit to make the behaviour default instead of opting into it with autoredirect. If we agree, I'll squash it and merge. |
@@ -17,9 +17,15 @@ def initialize(profile) | |||
@profile = profile | |||
end | |||
|
|||
def view | |||
def view(response: nil, autoredirect: nil, async: false) |
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 am not sure if we need autoredirect: nil, async: false
here?
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.
Because we're passing in the parameters from the view action (from the middleware), we can expect these to be provided. If we don't mention them, Ruby will raise an ArgumentError and our public API will be broken if we don't provide defaults.
Support autoredirection on remote profile views. Previously, the remote speedscope viewer would log a line like
Profile available at /app_profiler/<profile-id>
if a profile was initiated (eg.my.host/some/url?profile=wall
). When autoredirection is enabled via the query param, header, or configuration (eg.my.host/some/url?profile=wall&autoredirect=1
), a redirect is added to just redirect to/app_profiler/<profile-id>
instead of having to copy-paste the log into the browser.Should we just make this the new default behaviour? It is much better DX.