-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(react-router): Ensure that all browser spans have source=route
#16984
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
Conversation
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.
Some tests are failing but otherwise looks good
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.
Thank you!
fa36b0b
to
4dda614
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.
Bug: Router Patching Dependent on Pageload Span
The router.navigate
patching logic was moved inside the if (pageloadSpan)
block. Consequently, if no active pageload span exists when the router is instrumented, router.navigate
is not patched, preventing navigation transaction creation. This also creates an inconsistency, as the router subscription for updating navigation spans remains outside this block, attempting to update spans that were never created. Navigation patching should occur independently of pageload span existence.
packages/react-router/src/client/hydratedRouter.ts#L54-L66
sentry-javascript/packages/react-router/src/client/hydratedRouter.ts
Lines 54 to 66 in 4dda614
// Patching navigate for creating accurate navigation transactions | |
if (typeof router.navigate === 'function') { | |
const originalNav = router.navigate.bind(router); | |
router.navigate = function sentryPatchedNavigate(...args) { | |
maybeCreateNavigationTransaction( | |
String(args[0]) || '<unknown route>', // will be updated anyway | |
'url', // this also will be updated once we have the parameterized route | |
); | |
return originalNav(...args); | |
}; | |
} | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
…16984) This updates the browser tracing implementation in react router to ensure that we always have parametrized routes. In the previous implementation, if the parametrized route was the same as the "initial" one, we would just do nothing and the source remains URL. We would also not set the origin of the spans. this is not correct, because we actually ensured this is parametrized (there are simply no parameters in, but still!), so the source should still be `route` in this case.
This updates the browser tracing implementation in react router to ensure that we always have parametrized routes.
In the previous implementation, if the parametrized route was the same as the "initial" one, we would just do nothing and the source remains URL. We would also not set the origin of the spans. this is not correct, because we actually ensured this is parametrized (there are simply no parameters in, but still!), so the source should still be
route
in this case.