Conversation
size-limit report 📦
|
❌ Unsupported file format
|
ccbd697 to eb3c0bc Compare Uh oh!
There was an error while loading. Please reload this page.
eb3c0bc to cb8e92e Compare | @@ -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.
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.
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.
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.
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.
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.
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.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.