fix(core): Stop clobbering existing transaction name with scope value

[lobsterkatie]

Draft until we figure out if this is actually what we want to do.

---

This prevents the transaction name stored on the scope from overwriting an event's existing transaction value. Since the code in question runs before either event processors or beforeSend, the only way for an event to already have a transaction value at that point are

a) to be a transaction, or
b) to be a custom event captured by the user.

In the case of a), we don't want to overwrite the existing transaction value because we've just done a bunch of work to make sure that it's a good, parameterized value coming in. In the case of b), the user has actively provided a name, which we should respect.

Resolves #5660

[github-actions]

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.47 KB (+0.03% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	60.24 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.09 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	53.17 KB (+0.03% 🔺)
@sentry/browser - Webpack (gzipped + minified)	19.85 KB (+0.03% 🔺)
@sentry/browser - Webpack (minified)	64.51 KB (+0.03% 🔺)
@sentry/react - Webpack (gzipped + minified)	19.88 KB (+0.03% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	44.78 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.92 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.36 KB (+0.01% 🔺)

[lforst]

Can we add a comment here on why we did this? I.e. a one- or two-liner saying that we don't want to set event.transaction for transaction events and that it works because transaction events have a transaction field set from the get-go.

[lforst]

Shouldn't this resolve #5660 (as opposed to resolving only a part of it)?

[lobsterkatie]

Shouldn't this resolve #5660 (as opposed to resolving only a part of it)?

My take was that there's still an open question of what setTransactionName should actually do. (Like, should it actually set the name of the transaction (assuming there's an active one on the scope)? Can we in fact get away with removing it in JS (even if not across the board in all SDKs)? etc)

[lforst]

Shouldn't this resolve #5660 (as opposed to resolving only a part of it)?

My take was that there's still an open question of what setTransactionName should actually do. (Like, should it actually set the name of the transaction (assuming there's an active one on the scope)? Can we in fact get away with removing it in JS (even if not across the board in all SDKs)? etc)

Ah I guess that's also problematic. I originally opened the issue in the context of dynamic sampling where it simply messed up transaction names + source, which is fixed by this PR. So I would just close that issue and open up a new one that basically says "Scope.setTransactionName is confusing" and we discuss how to proceed with the API. Okay for you?

[lobsterkatie]

So I would just close that issue and open up a new one that basically says "Scope.setTransactionName is confusing" and we discuss how to proceed with the API. Okay for you?

I'm fine with that, but I think you could easily get away with not opening a new issue and just commenting on #5718.

[AbhiPrasad]

Can we add a test showing we’ll set a transaction name if it’s not defined?

[lobsterkatie]

Pausing on this because I couldn't quite bring myself to write "this makes scope.setTransactionName no longer set the name of the transaction on the scope" in the changelog, because that makes no sense as an improvement. Before proceeding, we're going to try to gather data on how often this is being used in the wild. Moving this to draft so it doesn't get merged in the meantime.