Pageload transaction names for dynamic catch-all routes is the filepath instead of the url pathname

[kylemh]

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/nextjs

SDK Version

6.17.5

Framework Version

6.17.5

Link to Sentry event

https://sentry.io/organizations/capsule-corporation/performance/summary/?environment=production&project=1219269&query=transaction.duration%3A%3C15m+transaction.op%3Apageload&statsPeriod=1h&transaction=%2F%5B...app%5D&unselectedSeries=p100%28%29

Steps to Reproduce

1. Create a new Next.js application with the Sentry plug-in configured.
2. Create a dynamic catch-all page file. Let's say it's pages/[...app].tsx.
3. Deploy, visit url.com/some-random-page-which-loads-the-dynamic-catch-all-route and watch for issues on the Web Vitals tab of the Performance dashboard.
4. Note that all pageload transactions for the above URL have a transaction name of /[...app].

Expected Result

Pageload transaction names should be the result of the requested pathname. Not the Next.js filepath.

Actual Result

In dev tools, I see:
Sentry Logger [Log]: [Tracing] starting pageload transaction - /[...app]
Sentry Logger [Log]: [Tracing] Finishing pageload transaction: /[...app].

[AbhiPrasad]

Hey, thanks for writing in. This is on purpose, we paramaterize our transactions (using the NextJS filesystem router) to group them in the UI so you can get better aggregate statistics.

I agree that maybe we can do better with catch-all routes, but we basically are limited to what the Next Router is giving us

sentry-javascript/packages/nextjs/src/performance/client.ts

Lines 37 to 44 in 0b2221d

	if (startTransactionOnPageLoad) {
	prevTransactionName = Router.route !== null ? stripUrlQueryAndFragment(Router.route) : global.location.pathname;
	activeTransaction = startTransactionCb({
	name: prevTransactionName,
	op: 'pageload',
	tags: DEFAULT_TAGS,
	});
	}

I'm going to mark this as a potential improvement and backlog.

Note, you should still be able to see URLs on individual events.

For the time being, if you want to specifically change something, you are free to use the beforeNavigate option to mutate transaction names in any way you want. https://docs.sentry.io/platforms/javascript/guides/nextjs/performance/instrumentation/automatic-instrumentation/#beforenavigate

[kylemh]

Hey @AbhiPrasad

Just brainstorming a solution here: Couldn't you track the transaction info, but delay the transaction info send if the pathname starts with /[... or /[[.... Then you can send the correct data after a small delay when router.isReady is true and asPath doesn't start with /[... or /[[...?

For the workaround you described with beforeNavigate. That makes sense, but I'm not sure where in my project I should intercept? sentry.client.config.js doesn't have access to the router context.

[AbhiPrasad]

Just brainstorming a solution here: Couldn't you track the transaction info, but delay the transaction info send if the pathname starts with /[... or /[[.... Then you can send the correct data after a small delay when router.isReady is true and asPath doesn't start with /[... or /[[...?

Will there ever be a problem with unparamaterized routes possibly? Like bad grouping of transaction names this way? I think your /[...//[[... heuristic is probably the direction we need to take here but we need to make sure we maintain a good UX for the out of the box experience.

btw PRs welcome - we might not get around to this for a bit.

For the workaround you described with beforeNavigate. That makes sense, but I'm not sure where in my project I should intercept? sentry.client.config.js doesn't have access to the router context.

You can use window.location.pathname (or a processed version of that) and just replace the transaction name.

// sentry.client.config.js
Sentry.init({
  // ...
  integrations: [
    new BrowserTracing({
      beforeNavigate: context => {
        return {
          ...context,
          name: generateTransactionName(window.location.pathname),
        };
      },
    }),
  ],
});

[kylemh]

Last question regarding the workaround: is generateTransactionName just an example or is it an import? I was curious how I can specifically target pageload transactions from this callback.

Will there ever be a problem with unparamaterized routes possibly?

I'm interested in eventually opening a PR! So, let me make sure I've got this question answered. Are you asking for non-dynamic routes? I think it's gauranteed that if pathname includes either an /[... or an /[[... it is either a catch-all or an optional catch-all route. I'll try making a pagefile like so: pages/[...test.tsx to make sure. I'd wager that Next.js will throw an error.

[AbhiPrasad]

Last question regarding the workaround: is generateTransactionName just an example or is it an import? I was curious how I can specifically target pageload transactions from this callback.

generateTransactionName was just an example function.

I'm interested in eventually opening a PR! So, let me make sure I've got this question answered. Are you asking for non-dynamic routes? I think it's gauranteed that if pathname includes either an /[... or an /[[... it is either a catch-all or an optional catch-all route. I'll try making a pagefile like so: pages/[...test.tsx to make sure. I'd wager that Next.js will throw an error.

I think it's gauranteed that if pathname includes either an /[... or an /[[... it is either a catch-all or an optional catch-all route.

I think this addresses my concerns pretty well, then we should be good to go to use the square bracket heuristic!

[lforst]

Since we want transactions to be low cardinality for a good user experience on the product, this is actually intended.