Transaction names are not properly parameterized with react-router v6

[jamesbvaughan]

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 package are you using?

@sentry/react

SDK Version

7.8.0

Framework Version

18.1.0

Link to Sentry event

https://sentry.io/organizations/arcol-og/performance/summary/?environment=dev&project=6576795&query=transaction.duration%3A%3C15m+transaction.op%3Apageload&statsPeriod=1h&transaction=%2Fprojects%2Fe7962d8a-e041-4006-9f44-d84ffabbdad9%2Fviews%2F982ebfaa-1e51-4726-9ca4-b04869e395b5&unselectedSeries=p100%28%29

Steps to Reproduce

Here's a simplified version of my relevant react-router (react-router-dom version 6.2.1) and Sentry config:

Sentry.init({
  ...
  integrations: [
    new BrowserTracing({
      routingInstrumentation: Sentry.reactRouterV6Instrumentation(
        useEffect,
        useLocation,
        useNavigationType,
        createRoutesFromChildren,
        matchRoutes,
      ),
    }),
  ],
});

const SentryRoutes = Sentry.withSentryReactRouterV6Routing(Routes);

const MyRoutes = () => {
  return (
    <SentryRoutes>
      <Route index element={<Navigate to="/projects" />} />

      <Route path="/account" element={<AccountPage />} />

      <Route path="/projects">
        <Route index element={<ProjectBrowser />} />

        <Route path=":projectId" element={<ProjectPage />}>
          <Route index element={<ProjectPageRoot />} />

          <Route element={<EditorPage />}>
            <Route path="views/:viewId" element={<ViewCanvas />} />
            <Route path="spaces/:spaceId" element={<SpaceCanvas />} />
          </Route>
        </Route>
      </Route>

      <Route path="*" element={<NoMatchPage />} />
    </SentryRoutes>
  );
};

export default MyRoutes

As far as I can tell, I'm not doing anything unusual, except possibly the use of react router's Outlets and Layout Routes.

Expected Result

With this config, I'd expect to get transactions with names like /projects/:projectId/views/:viewId.

Actual Result

Instead, I'm getting transactions with names like /projects/e7962d8a-e041-4006-9f44-d84ffabbdad9/views/982ebfaa-1e51-4726-9ca4-b04869e395b5.

[AbhiPrasad]

Thanks for writing in! I wrote up a quick test to validate this, and seems the behaviour is a bit off:

it.only('works with nested paths with parameters', () => {
  const [mockStartTransaction] = createInstrumentation();
  const SentryRoutes = withSentryReactRouterV6Routing(Routes);

  render(
    <MemoryRouter initialEntries={['/']}>
      <SentryRoutes>
        <Route index element={<Navigate to="/projects/123/views/234" />} />
        <Route path="account" element={<div>Account Page</div>} />
        <Route path="projects">
          <Route index element={<div>Project Index</div>} />
          <Route path=":projectId" element={<div>Project Page</div>}>
            <Route index element={<div>Project Page Root</div>} />
            <Route element={<div>Editor</div>}>
              <Route path="views/:viewId" element={<div>View Canvas</div>} />
              <Route path="spaces/:spaceId" element={<div>Space Canvas</div>} />
            </Route>
          </Route>
        </Route>

        <Route path="*" element={<div>No Match Page</div>} />
      </SentryRoutes>
    </MemoryRouter>,
  );

  expect(mockStartTransaction).toHaveBeenCalledTimes(2);
  expect(mockStartTransaction).toHaveBeenLastCalledWith({
    name: 'views/:viewId',
    op: 'navigation',
    tags: { 'routing.instrumentation': 'react-router-v6' },
    metadata: { source: 'route' },
  });
});

Notice I removed the / in front of /account and /projects, which matches the conventions from the React Router docs.

When navigating to /projects/123/views/234, it picks views/:viewId as the transaction name - which is only partly correct :/

If you look at our previous tests, we tested with the full path:

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

Lines 178 to 189 in 6f4ad56

	render(
	<MemoryRouter initialEntries={['/']}>
	<SentryRoutes>
	<Route path="/stores" element={<div>Stores</div>}>
	<Route path="/stores/:storeId" element={<div>Store</div>}>
	<Route path="/stores/:storeId/products/:productId" element={<div>Product</div>} />
	</Route>
	</Route>
	<Route path="/" element={<Navigate to="/stores/foo/products/234" />} />
	</SentryRoutes>
	</MemoryRouter>,
	);

, which works very well - but here we would have to construct the path based on what was rendered before.

Going to explore this further - but any suggestions is greatly appreciated!

[AbhiPrasad]

I've managed to fix this by adjusting the matching algo.

diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx
index 8f2c2825a..1d3dd8f3b 100644
--- a/packages/react/src/reactrouterv6.tsx
+++ b/packages/react/src/reactrouterv6.tsx
@@ -20,9 +20,23 @@ type Params<Key extends string = string> = {
   readonly [key in Key]: string | undefined;
 };
 
+// https://github.com/remix-run/react-router/blob/9fa54d643134cd75a0335581a75db8100ed42828/packages/react-router/lib/router.ts#L114-L134
 interface RouteMatch<ParamKey extends string = string> {
+  /**
+   * The names and values of dynamic parameters in the URL.
+   */
   params: Params<ParamKey>;
+  /**
+   * The portion of the URL pathname that was matched.
+   */
   pathname: string;
+  /**
+   * The portion of the URL pathname that was matched before child routes.
+   */
+  pathnameBase: string;
+  /**
+   * The route object that was used to match.
+   */
   route: RouteObject;
 }
 
@@ -94,13 +108,25 @@ function getNormalizedName(
 
   const branches = matchRoutes(routes, location);
 
+  let pathBuilder = '';
   if (branches) {
     // eslint-disable-next-line @typescript-eslint/prefer-for-of
     for (let x = 0; x < branches.length; x++) {
-      if (branches[x].route && branches[x].route.path && branches[x].pathname === location.pathname) {
-        const path = branches[x].route.path;
+      const branch = branches[x];
+      const route = branch.route;
+      if (route) {
+        // Early return if index route
+        if (route.index) {
+          return [branch.pathname, 'route'];
+        }
+
+        const path = route.path;
         if (path) {
-          return [path, 'route'];
+          const newPath = path[0] === '/' ? path : `/${path}`;
+          pathBuilder += newPath;
+          if (branch.pathname === location.pathname) {
+            return [pathBuilder, 'route'];
+          }
         }
       }
     }

Will write some more tests, and then open a patch to fix.

Appreciate the detailed bug report - thank you for your patience @jamesbvaughan!

[AbhiPrasad]

Had to adjust the algo slightly to account for other cases, PR has been opened to fix!

[stspnu]

Hi, @AbhiPrasad thanks for fixing it. Faced a similar issue a week ago and when was opening a PR with fix noticed that you are already fixed it :)

I curious why not to use flattenRoutes and flattenRoutes from https://github.com/remix-run/react-router/blob/main/packages/react-router/lib/router.ts#L155 in order to create ranked branches and then use matchPath in order to find first matching path.

function getNormalizedName(
  routes: RouteObject[],
  location: Location,
  matchRoutes: MatchRoutes,
): [string, TransactionSource] {
  if (!routes || routes.length === 0 || !matchRoutes) {
    return [location.pathname, 'url'];
  }

  const todoFlattenRoutes = flattenRoutes(routes);
  rankRouteBranches(todoFlattenRoutes);

  const [routeMatch] = matchRoutes(routes, location);
  const result = todoFlattenRoutes.find(path => _matchPath(path, routeMatch.pathname));

  if (result) {
    return [result.path, 'route'];
  }

  return [location.pathname, 'url'];
}

React Router doesn't export flattenRoutes and flattenRoutes so you need to copy paste it manually but IMO it better to use the same mechanism to find matched path as used in React Router.