Keep transaction name in error event with request data extension

[jeengbe]

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

SDK Version

7.106.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

Sentry.initSentry({
  // ...
    new Sentry.Integrations.RequestData({})
})

Steps to Reproduce

1. Start transaction
2. Rename transaction Sentry.getActiveSpan()?.updateName("a");
3. Set Sentry.getCurrentScope().setSDKProcessingMetadata({ request: ctx.request }); from koa request
4. Sentry.captureException(new Error())

Expected Result

I expect error events created in the transaction to inherit the transaction name from the span/transaction (not sure what the difference is with recent deprecations). Instead, the RequestData integration overwrites the transaction name from the currently active transaction.

I see there is a check in

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

Line 352 in d51fbd3

if (include.transaction && !event.transaction) {

that tries !event.transasction, but that property is not set on that event. Instead, there's a tags.transaction tag.

Actual Result

See above.

---

The workaround of setting:

    new Sentry.Integrations.RequestData({
      // @ts-expect-error
      transactionNamingScheme: false,
    }),

seems to work (I assume transaction linking is done in the backend?)

[Lms24]

Hey @jeengbe thanks for writing in!

So you discovered one of the unfortunate edge cases in our SDKs which we're actually trying to solve right now.

Before we get into details, would you mind sharing a link to a Sentry event that has the wrong transaction name? I'm curious where in the product this is visible.

---

Let me go through the general issue:

event.transaction - in non-transaction events (e.g. errors) - is only set to give the error a "location" if you will. For example, it will be displayed in the issues list, next to the error message in gray:

(In cases where we don't set event.transaction, this "location" will fall back to stack trace file names or symbols)

Now that we know the purpose of the field, we have to think about how to set it correctly. What's important to understand is that there's an API on the Scope, setTransactionName which allows you (SDK users) to store a name on the the currently active scope which should be assigned to event.transaction.

I expect error events created in the transaction to inherit the transaction name from the span/transaction (not sure what the difference is with recent deprecations).

So, generally that sounds reasonable. However, the question arises which fields should take presedence, when both, scope.transactionName as well as an active root span/transaction with a name are available. Unfortunately, you can argue this in both directions, as I outlined in #10846 (comment). This question becomes especially important if/when we start to automatically update scope.transactionName in the future because then, this field will be populated a lot more often than at them moment.

not sure what the difference is with recent deprecations

That's fair and something we need to communicate better once we have a stable v8 release. The gist is: We're moving away from the concept of a "Transaction" as in root span/container around spans. Instead, the "outermost" span will just be a span that you can access via Sentry.getRootSpan.
Given that in the future, the SDK won't use this "Transaction" concept anymore, the scope.transactionName API will become less ambiguous and it's easier to decouple the two.

So to sum up, our current plan is to:

1. decouple scope.transactionName and an active root span's name (I already opened feat(core): Decouple scope.transactionName from root spans #10991)
2. assign event.transaction based on the scope's transactionName and not anymore based on a potentially active root span name
3. update the scope's transactionName property automatically in sensible places in the SDK. For example, during browser pageloads or navigations; or on the server when we start a new request span. This should keep the two names generally in sync but there will be cases when these go out of sync.

To adress one last question: Does this mean, errors are no longer reliably associated with a root span? No. We still associate error events with a traceId and spanId and with the trace explorer, you are still able to navigate to the trace. I'm not yet sure, how the transaction tag's future in the issues detail page will look like but ideally, we also find a good strategy for that.

What do you think about this plan? Does this solve your use case? If you have concrete feedback, please feel free to post it in #10846 because that's the place where we're tracking all of this work. For more specific stuff around your issue, let's keep discussing here. Thanks :)

[jeengbe]

Here's the link to such an event: https://prisjakt-ab.sentry.io/issues/5059101435/events/cc92170cf69c47639c97728f9d8bbf88/. Note that the transaction tag is set to the request URL.

This is what happens right at this line:

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

Line 358 in d51fbd3

return event;

Interestingly, that is only a problem with error events. type=transaction have the transaction value set properly. See #11029 (reply in thread) for how I set up Sentry.

[jeengbe]

What do you think about this plan? Does this solve your use case? If you have concrete feedback, please feel free to post it in #10846 because that's the place where we're tracking all of this work. For more specific stuff around your issue, let's keep discussing here. Thanks :)

Technically, my problem is already solved with the shared expect-error workaround. As for the long explainer you shared, I must admit I don't follow entirely, nothing wrong with the text, though. I believe that's mainly due to confusion around the basics and how things (scopes/transactions/spans) work together in this, that's all part of a bigger picture that I also partly touched on in what this comment replies to: #11029 (reply in thread). I believe thread to be a mere symptom thereof.

[jeengbe]

I have little understanding of the SDK's internals, I just know how I would like to use them, and unambiguous transaction data is part of that.

Why not just remove .transaction entirely and keep the tag only? 🙂 There are many other thoughts I have about the SDK's hilly API landscape that'd I'd be happy to share when I get the time for it. ☹️ Also, this isn't the place for that.

[Lms24]

Why not just remove .transaction entirely and keep the tag only?

Ultimately that's gonna end in the same question: What value do we set on the tag :)

Regardless, too much of the Sentry product relies on event.transaction on non-error events so we can't remove it easily. To be accurate, this field predates Sentry's performance monitoring Transaction concept, where its purpose simply was to assign a "location" other than the stack trace filename to an error.