feat(browser): Detect redirects when emitting navigation spans#16324
feat(browser): Detect redirects when emitting navigation spans#16324
Conversation
size-limit report 📦
|
❌ Unsupported file format
|
ccbd697 to
eb3c0bc
Compare
eb3c0bc to
cb8e92e
Compare
edwardgou-sentry
left a comment
There was a problem hiding this comment.
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).
| } | ||
|
|
||
| if (detectRedirects && optionalWindowDocument) { | ||
| addEventListener('click', () => (lastClickTimestamp = timestampInSeconds()), { capture: true, passive: true }); |
There was a problem hiding this comment.
are there other events, such as key presses, that could indicate a user manually navigating?
There was a problem hiding this comment.
yes, keypress might also be a good candidate, agreed.
There was a problem hiding this comment.
👍 also looking at keypress
Lms24
left a comment
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| if (detectRedirects && optionalWindowDocument) { | ||
| addEventListener('click', () => (lastClickTimestamp = timestampInSeconds()), { capture: true, passive: true }); |
There was a problem hiding this comment.
yes, keypress might also be a good candidate, agreed.
19d02d3 to
67791e9
Compare
67791e9 to
e2018b5
Compare
e2018b5 to
9dec9c3
Compare
Co-authored-by: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com>
9dec9c3 to
da0cffe
Compare
There was a problem hiding this comment.
Bug: Navigation URL Metadata Update Fails
The scope.setSDKProcessingMetadata is not updated for navigation spans if the URL is falsy or if the navigation is detected as a redirect. This prevents subsequent events from having the correct URL information on the scope. Additionally, the redirect detection logic uses inconsistent timestamp functions (timestampInSeconds vs dateTimestampInSeconds), which can lead to inaccurate timing comparisons.
packages/browser/src/tracing/browserTracingIntegration.ts#L469-L780
sentry-javascript/packages/browser/src/tracing/browserTracingIntegration.ts
Lines 469 to 780 in e82bf79
Bug: Browser Tracing Integration Event Listener Leak
The browserTracingIntegration introduces a memory leak by adding global click and keydown event listeners for redirect detection without ever removing them. This causes listeners to accumulate when the integration is reinitialized or multiple instances are created, such as in SPAs, hot module reloading, or test environments. A cleanup mechanism is required to prevent this accumulation.
packages/browser/src/tracing/browserTracingIntegration.ts#L467-L475
sentry-javascript/packages/browser/src/tracing/browserTracingIntegration.ts
Lines 467 to 475 in e82bf79
Was this report helpful? Give feedback by reacting with 👍 or 👎
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.redirectzero-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.