Problematic API: `Scope.setTransactionName()`

[lforst]

We've identified this API, or rather its implementation as problematic.

Scope.setTransactionName() will populate the transaction field of any event that comes after that call on that scope (and any scopes that were cloned from that scope).

The transaction field is used for associating an error with a transaction (not an actual transaction event in Sentry but rather a route or error origin) when dealing with an error event, and it contains the name of a transaction when dealing with a transaction event.

While this sounds nice and all, overwriting the name of transactions like this is a bit problematic:

1. It's a bit opaque that this is the behaviour of Scope.setTransactionName()
2. Often times we already have a high-quality transaction name through auto-instrumentation (source: route) and we don't want to have it overridden by the user. While we should provide users to overwrite high quality transaction names if they really want to, they probably shouldn't do it with Scope.setTransactionName(). It's very likely that this function just causes side-effects that the user didn't intend to introduce.
3. If transaction is overriden by Scope.setTransactionName() we're not updating the source of the transaction to custom. We need source to be correct for Dynamic Sampling.

[lforst]

In my opinion, the problem with this API is that it does completely different things for errors and transaction. While the changing of the transaction field is quite inconsequential for error events, it completely messes up transactions when used wrong.

I'd approach it like the following:

• Keep the behaviour for error events.
• Don't update the name for transaction events.

[Lms24]

I'd approach it like the following:

• Keep the behaviour for error events.
• Don't update the name for transaction events.

I +100 this. Seems like we can't get rid of this weird API entirely so let's at least make sure that the field isn't applied to the transaction event anymore.

[lobsterkatie]

IMHO this is part of a larger problem we have, which is that we have a number of generations of behavior with respect to the transaction field (both on the scope and in events), with the result that the end result is hard to reason about, inconsistent between error types, packages, etc, and prone to problems such as the one in this issue. (Probably that should be written up as a separate issue, though. UPDATE: I did end up writing it up: #5718) As for @lforst's suggestion...

• Don't update the name for transaction events.

That can be done just by changing

sentry-javascript/packages/hub/src/scope.ts

Line 468 in efd32f0

event.transaction = this._transactionName;

to

sentry-javascript/packages/hub/src/scope.ts

Line 468 in 99100ce

event.transaction = event.transaction || this._transactionName;

(This works because transaction events always have a transaction value from the very start:

sentry-javascript/packages/tracing/src/transaction.ts

Lines 142 to 161 in efd32f0

	const transaction: Event = {
	contexts: {
	trace: this.getTraceContext(),
	},
	spans: finishedSpans,
	start_timestamp: this.startTimestamp,
	tags: this.tags,
	timestamp: this.endTimestamp,
	transaction: this.name,
	type: 'transaction',
	sdkProcessingMetadata: {
	...metadata,
	baggage: this.getBaggage(),
	},
	...(metadata.source && {
	transaction_info: {
	source: metadata.source,
	},
	}),
	};

)

Making this change would bring the behavior in line with what happens in addRequestDataToEvent here:

sentry-javascript/packages/utils/src/requestdata.ts

Lines 393 to 397 in efd32f0

	if (include.transaction && !event.transaction) {
	// TODO do we even need this anymore?
	// TODO make this work for nextjs
	event.transaction = extractTransaction(req, include.transaction);
	}

[lforst]

We will look at data to determine if this problem is as big as we think.