-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Detect redirects when emitting navigation spans #16324
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
base: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
❌ Unsupported file format
|
ccbd697
to
eb3c0bc
Compare
eb3c0bc
to
cb8e92e
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.
Makes sense to me! There aren't many product areas in performance that specifically rely on navigations so I think this should be fine (and I think we'd consider surfacing redirects in those areas a bug anyways).
@@ -371,6 +396,10 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio | |||
startTrackingInteractions(); | |||
} | |||
|
|||
if (detectRedirects && optionalWindowDocument) { | |||
addEventListener('click', () => (lastClickTimestamp = timestampInSeconds()), { capture: true, passive: true }); |
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.
are there other events, such as key presses, that could indicate a user manually navigating?
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, keypress might also be a good candidate, agreed.
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.
👍 also looking at keypress
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.
Sorry for the late review, but LGTM! I think we probably need to widen the timespan a bit because 300ms feel a bit fast to me (thinking of the endless redirects I get when doing SSO or stuff like this). But maybe it's good enough for now. I'd say its something we adjust on a per-feedback basis.
@@ -371,6 +396,10 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio | |||
startTrackingInteractions(); | |||
} | |||
|
|||
if (detectRedirects && optionalWindowDocument) { | |||
addEventListener('click', () => (lastClickTimestamp = timestampInSeconds()), { capture: true, passive: true }); |
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, keypress might also be a good candidate, agreed.
19d02d3
to
67791e9
Compare
startBrowserTracingNavigationSpan( | ||
client, | ||
{ | ||
name: parsed?.pathname || WINDOW.location.pathname, |
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.
nit(ish): It looks like we are only using this variable once.
Is there any reason to declare it at all? 🙂
name: parsed?.pathname || WINDOW.location.pathname, | |
name: parseStringToURLObject(to)?.pathname || WINDOW.location.pathname, |
return false; | ||
} | ||
|
||
return true; |
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.
Since there is nothing between the condition and the default return, I think we could just set this condition as a final return value:
return true; | |
return !(lastInteractionTimestamp && now - lastInteractionTimestamp <= REDIRECT_THRESHOLD); |
removing the above (L757-L759):
if (lastInteractionTimestamp && now - lastInteractionTimestamp <= REDIRECT_THRESHOLD) {
return false;
}
|
||
// More than 300ms since last navigation/pageload span? | ||
// --> never consider this a redirect | ||
const startTimestamp = spanData.start_timestamp; |
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.
Same as my other comment we are only using spanData
once it looks like. 🙂
const startTimestamp = spanData.start_timestamp; | |
const startTimestamp = spanToJSON(activeSpan).start_timestamp; |
67791e9
to
e2018b5
Compare
Closes #15286
This PR adds a new option to
browserTracingIntegration
,detectRedirects
, which is enabled by default. If this is enabled, the integration will try to detect if a navigation is actually a redirect based on a simple heuristic, and in this case, will not end the ongoing pageload/navigation, but instead let it run and create anavigation.redirect
zero-duration span instead.An example trace for this would be: https://sentry-sdks.sentry.io/explore/discover/trace/95280de69dc844448d39de7458eab527/?dataset=transactions&eventId=8a1150fd1dc846e4ac8420ccf03ad0ee&field=title&field=project&field=user.display&field=timestamp&name=All%20Errors&project=4504956726345728&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=5m×tamp=1747646096&yAxis=count%28%29

Where the respective index route that triggered this has this code:
The used heuristic is:
this limit was chosen somewhat arbitrarily, open for other suggestions too.
While this logic will not be 100% bullet proof, it should be reliable enough and likely better than what we have today. Users can opt-out of this logic via
browserTracingIntegration({ detectRedirects: false })
, if needed.