Repository navigation
React router integration should use route name instead of location #10196
Description
Is there an existing issue for this?
- I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
- I have reviewed the documentation https://docs.sentry.io/
- I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/react
SDK Version
7.93.0
Framework Version
18.2.0
Link to Sentry event
No response
SDK Setup
Sentry.init({
dsn: '<dsn here>',
integrations: [
new RewriteFrames({
iteratee: (frame) => ({
...frame,
filename: frame.filename?.replace(document.baseURI, ''),
}),
}),
new Sentry.BrowserTracing({
routingInstrumentation: Sentry.reactRouterV6Instrumentation(
useEffect,
useLocation,
useNavigationType,
createRoutesFromChildren,
matchRoutes,
),
}),
new Sentry.Replay(generateSentryReplayConfig()),
],
})
Steps to Reproduce
Initialize sentry with react router v6.4 data API according to docs. E.g. above sentry init and:
const sentryCreateBrowserRouter =
Sentry.wrapCreateBrowserRouter(createBrowserRouter);
export function createRouter() {
return sentryCreateBrowserRouter(
routes,
{ basename },
);
}When running this in our environment, we see transactions with the current location name show up. E.g.:
baseurl/product/1234-hash-of-this-product-id
Expected Result
We expected transactions to by grouped by the route path as defined in react router, e.g:
product/:productId
As well as without their baseurl.
The source of this seems to be:
I believe a correct implementation would involve checking the matched routes and using the route path instead:
router.subscribe((state: RouterState) => {
const targetRoute = state.matches
.findLast(match => match.route.path != null)
?.route.path;
if (targetRoute) {
handleNavigation(location, routes, state.HistoryAction, undefined, basename)
}
Actual Result
n/a
Metadata
Metadata
Assignees
Projects
- Status
Activity
Lms24 commentedon Jan 17, 2024
Hi @ruudandriessen thanks we'll take a look!
onurtemizkan commentedon Jan 17, 2024
Hi @ruudandriessen, I could not reproduce the issue.
Route lookup does not work correctly if
Sentry.initruns aftersentryCreateBrowserRouter. Could you please check if that's the reason?ruudandriessen commentedon Jan 17, 2024
I can confirm our
Sentry.initruns beforesentryCreateBrowserRouter. We have a baseURL that differs for each organization, so it may be that that's the primary cause? For example, we're seeing transactions like:While I expected routes
pathto be used (e.g.app/page/:pageId. I can attempt to create a minimal reproduction case if that will help.ruudandriessen commentedon Jan 17, 2024
I believe I have found out what the exact issue is. It indeed works when the base contains just a regular string. However, it's a problem when a base URL is given with a
/in it (in that case the baseURL is included in the transaction instead). Which is the case for us, since our baseURL is in the form of/<organization>/<tenant>/...I have a minimal reproduction case for you here:
https://codesandbox.io/p/devbox/lgl3ls?file=%2Fsrc%2FApp.tsx%3A29%2C34
2 remaining items
onurtemizkan commentedon Jan 22, 2024
Hi @ruudandriessen, thanks for the reproduction.
So, we're including the given
basenameinto the transaction names in the current implementation. So it's the expected behaviour and not a bug.Test for reference:
sentry-javascript/packages/react/test/reactrouterv6.4.test.tsx
Lines 354 to 360 in 5f0b506
That said, we can add a flag for users wanting to opt-out from including
basenameinto transaction names, if the team agrees.What might be a bug is,
seeing
baseurl/product/<resolved-product-id>instead ofbaseurl/product/:productId. But I'm still not able to reproduce that.cc: @Lms24
Lms24 commentedon Jan 22, 2024
So to summarize (because I'm no React expert at all) we want to add an option to exclude the basename from the transaction/span name? Sounds good to me.
Let's just make sure to cover this behaviour with tests. thanks!
ruudandriessen commentedon Jan 22, 2024
Thanks! Having an option to include/exclude the basename sounds sensible.
I can also confirm that
This is no longer happening. I think it may have been an older transaction. Our latest transactions only have the baseURL in them while we expected these to be stripped.
ruudandriessen commentedon Jan 25, 2024
Just checked out the latest release, can confirm the transactions that are being send now work as expected. We'll do a full end to end check, but I think that should turn out well now! Thanks for the quick turnaround time @onurtemizkan & @Lms24 🙏