React router integration should use route name instead of location

[ruudandriessen]

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:

sentry-javascript/packages/react/src/reactrouterv6.tsx

Line 293 in 56206f5

const location = state.location;

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

[Lms24]

Hi @ruudandriessen thanks we'll take a look!

[onurtemizkan]

Hi @ruudandriessen, I could not reproduce the issue.

Route lookup does not work correctly if Sentry.init runs after sentryCreateBrowserRouter. Could you please check if that's the reason?

[ruudandriessen]

Hi @ruudandriessen, I could not reproduce the issue.

Route lookup does not work correctly if Sentry.init runs after sentryCreateBrowserRouter. Could you please check if that's the reason?

I can confirm our Sentry.init runs before sentryCreateBrowserRouter. 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:

<organization>/<tenant>/app/page/*

While I expected routes path to be used (e.g. app/page/:pageId. I can attempt to create a minimal reproduction case if that will help.

[ruudandriessen]

Hi @ruudandriessen, I could not reproduce the issue.

Route lookup does not work correctly if Sentry.init runs after sentryCreateBrowserRouter. Could you please check if that's the reason?

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

[ruudandriessen]

@onurtemizkan @Lms24 does the above reproduction case confirm this as a bug? Is there anything more I can do to help?

[onurtemizkan]

Hi @ruudandriessen, thanks for the reproduction.

So, we're including the given basename into 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

	expect(mockStartTransaction).toHaveBeenLastCalledWith({
	name: '/admin/:orgId/users/:userId',
	op: 'navigation',
	origin: 'auto.navigation.react.reactrouterv6',
	tags: { 'routing.instrumentation': 'react-router-v6' },
	metadata: { source: 'route' },
	});

That said, we can add a flag for users wanting to opt-out from including basename into transaction names, if the team agrees.

What might be a bug is,

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

seeing baseurl/product/<resolved-product-id> instead of baseurl/product/:productId. But I'm still not able to reproduce that.

cc: @Lms24

[Lms24]

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]

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!

Thanks! Having an option to include/exclude the basename sounds sensible.

I can also confirm that

seeing baseurl/product/<resolved-product-id> instead of baseurl/product/:productId. But I'm still not able to reproduce 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]

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 🙏