Txn source: "url" even though set manually in beforeNavigate()

[maxkosty]

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

SDK Version

7.51.1

Framework Version

No response

Link to Sentry event

*** See linked Jira ticket ***

SDK Setup

Sentry.init({
 dsn: _dsn,
 
 integrations: [
 new Sentry.BrowserTracing({
 tracingOrigins: [
 // ...
 ],
 beforeNavigate: (context) => {
 return {
 ...context,
 name: transactionName()
 };
 }
 }),
 
 // ...

 routingInstrumentation: Sentry.reactRouterV5Instrumentation(
 history,
 REACT_ROUTER_ROUTES,
 matchPath
 ),

 // ...

 });

// ...

export const transactionName = () => {
 const { pathname } = window.location;

 // ...

 // temporarily fixes <<unparameterized>> grouping as recommended by sentry engineers
 // this unblocks analytics performance monitoring until sentry fixes an issue
 // with transaction grouping on their end. 
 // ref - https://develop.sentry.dev/transaction-clustering/
 if (pathname === '/') {
 return '/';
 }

 // ...

};

Steps to Reproduce

Go to customer website *** see linked Jira ticket ***
Capture a few dozen pageloads until transaction is sampled (their tracesSampleRate is 0.1), get the id
Look at txn in Sentry.
Examine their sentry.js if needed for more detail

Expected Result

transaction.source: "custom"

Actual Result

transaction.source: "url"

ROOT CAUSE: https://github.com/getsentry/sentry-javascript/blob/ff4b1a85b3c04d2782880629b0d5be591568deaf/packages/tracing-internal/src/browser/browsertracing.ts#LL297C49-L297C49

Original PR: #5396

┆Issue is synchronized with this Jira Improvement by Unito

[Lms24]

What should the change look like? Removing the name change check would always set the source to custom even if users change around different stuff in beforeNavigate but don't change the transaction name.

[maxkosty]

@lforst what do you think?

[lforst]

@realkosty I don't think the code section you identified as the root cause is the actual problem - unless I am missing somthing ofc. Can you elaborate a bit how "url" may end up in the new transaction source here?

[maxkosty]

@lforst this behavior happens specifically when name set in beforeNavigate is identical to name derived from URL. The !== condition in the code results in source: custom not being set.

[Lms24]

Hmm I honestly still think this is fine generally but I can see how this is a problem for users who manually try to parameterize their routes and come across routes that don't have parameters at all. Like /.

The problem I see with removing this check is that if users leave urls with params as-is (e.g. /users/123), we mark an unparameterized URL as parameterized by always setting the source to custom as soon as beforeNavigate was invoked.

[lforst]

I am not sure if we should change the current behaviour. What the user could do however is something like:

beforeNavigate: (context) => {
 return {
 ...context,
	metadata: {
		source: transactionName() === '/' ? 'custom' : undefined
	}
 };
}

Does that sound reasonable?

In general, I am wondering if we should set transactions with the name "/" to custom always.

[Lms24]

Unless we want to remove the equality check (i.e. assume calling beforeNavigate always results in a custom txn name source), I agree that users should set the source manually to custom.

In general, I am wondering if we should set transactions with the name "/" to custom always.

It's worth a thought IMO but probably quite intransparent for users ("i.e. why is / shown as parameterized while /users is unparamterized - both have no params?!")

[maxkosty]

I can see how this may be tricky to implement but don't get the principal objections:

• If users sets txn name explicitly in beforeSend source should be "custom".
• If txn name is automatically derived from URL source should be "url".

What's so controversial about this?

[lforst]

What's so controversial about this?

Nothing's controversial. Maybe the tone of the conversation didn't transfer well here ^^ I also get the frustration.

I think the reason Lukas and I are a bit careful and not jumping to fixes is because this has gone through a 💩-ton of discussions already, and we basically settled that we were gonna have server-side heuristics in relay to parameterize transactions in cases where it's not possible to parameterize with 100% certainty (like in this case).

As for "/" it is the only case where I would say we can make an exception. The other cases you mentioned ("/asdf" etc.) are unfixable from the SDK side of things. IMO it's more correct if we continue sending "/" with source "url" and let relay make the special casing here.

Btw if "/" shows up under unparameterized, we likely have an issue in Relay with the auto-grouping logic. These transactions should very likely be promoted to parameterized. cc @jjbayer @iker-barriocanal

[maxkosty]

@lforst Got it, thanks for the context. I understand the pushback/caution since changing this may have all sorts of implications and might miss something important that came up in those past discussions.

Btw if "/" shows up under unparameterized, we likely have an issue in Relay with the auto-grouping logic. These transactions should very likely be promoted to parameterized. cc @jjbayer @iker-barriocanal

There is indeed an issue and Relay team is aware of it. Sorry if I didn't make that clear. But there always be edge-cases - after all it's heuristic, don't you think? Customers need a way to override it by manually specifying txn name.

Right now in the when you click on << unparameterized >> in the transactions list you get this callout:

that links to https://docs.sentry.io/platforms/javascript/performance/instrumentation/automatic-instrumentation/#beforenavigate