Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capture transaction name for errors (v8) #10846

Closed
22 tasks done
mydea opened this issue Feb 28, 2024 · 6 comments
Closed
22 tasks done

Capture transaction name for errors (v8) #10846

mydea opened this issue Feb 28, 2024 · 6 comments
Assignees

Comments

@mydea
Copy link
Member

mydea commented Feb 28, 2024

We have a transaction field on events (e.g. error events) which is used in-product for a bunch of things.

We need a way to also ensure this works in v8. We've deprecated scope.setTransactionName() but maybe need to revisit this.

Status Quo

In the JS SDK, we currently only set event.transaction in the request data integration - so only in server environments.
In Browser SDKs, we never set this today - we only set a transaction tag.

Some constraints:

  • This needs to work with and without performance/spans
  • Ideally we capture this even if a span is over.

Solution

Update: After an internal meeting, we settled on the following solution:

We un-deprecate scope.setTransactionName(), which can be used with or without performance.
In our own auto-instrumentation, we can use that to automatically set the transaction name on the scope. This can happen based on spans, or not.

Users can always overwrite it on the given scope.

Important: The scope's transaction name will no longer be coupled to root spans or spans in general. Meaning, it's purpose is to associate errors and other events with location (i.e. a transaction) and not 1:1 with a root span name. In many cases, when we auto instrument things, the names will be the same but there can be cases when these two diverge.

Examples:

  • In browser tracing integration, we update the transaction name.
  • In OTEL, we update it in http.server middlewares
  • Possibly we can also generally set it whenever a root span is started via startSpan APIs (=a span without a parent)

Undeprecation and Decoupling

Preview Give feedback
  1. Lms24
  2. lforst

Some caveats:

  • In OTEL, unsampled spans (=the majority of spans) are NonRecordingSpans which have no name. So we can't read it off the active span (which additionally also has the downside that we can't read it once the active span is over, e.g. browser tracing).
@Lms24
Copy link
Member

Lms24 commented Feb 28, 2024

I agree with the proposed solution. Undeprecating is probably the right way to go. I don't have a better name for it and assuming we move further away from the transaction event in the coming months, the ambiguity between event.transaction and transactions (as in root spans) is going to become less of a concern.

One thought:

In browser tracing integration, we update the transaction name.

To slightly decouple calling scope.setTransactionName from browserTracingIntegration, the integration could emit a client hook (e.g. onNavigation naming tbd). This might also be helpful for replay where we could also listen to the hook 🤔

Regardless, automatically updating the transaction on the scope is already step 2 for this issue so we can still discuss this.

@Lms24
Copy link
Member

Lms24 commented Mar 8, 2024

RE automatically updating the scope.transactionName: We’re gonna run into the same infamous timing issues as with Dynamic Sampling when updating the name on a navigation (probably also on the server side). Events could be associated with the old or unparameterized name if they are thrown during a navigation or simply before we found the parameterized route name. Given that grouping isn't affected by event.transaction we'll nevertheless move forward for now. We can revisit the automatic updating in the future if it turns out to be problematic or undesireable.

@Lms24
Copy link
Member

Lms24 commented Mar 11, 2024

UPDATE: For now, we'll disregard the concern outlined in this answer but I'm leaving it here b/c it explains the precedence dilemma well and it's important to be aware of the concern. We can always revisit the behaviour.

After a shower thought, I'm not entirely sure anymore if decoupling event.transaction entirely from a potentially active root span is the correct way to go. Don't we want to still associate e.g. an error event with the active transaction?

I think we need to look at four cases to properly assign event.transaction:

  1. !scope.transactionName && !rootSpan.name => event.transaction === undefined
  2. !scope.transactionName && rootSpan.name => event.transaction === rootSpan.name
  3. scope.transactionName && !rootSpan.name => event.transaction === scope.transactionName
  4. scope.transactionName && rootSpan.name => event.transaction === scope.transactionName

Obviously, 4 could go both ways and there are pros/cons to both of them:

  • scope.transactionName
      • allows users to always override
      • (error) events are not associated with the active transaction event(?)
      • if we automatically update scope.transactionName this means that we're responsible for divergences between the active root span and the transactionName on the scope.
  • rootSpan.name
      • (error) events always associated with active transaction event
      • no way for users to override

technical note:

If we go with 4 taking scope.transactionName, we can reduce the cases to:

event.transaction = scope.transactionName ? scope.transactionName : rootSpan?.name || undefined

otherwise, it boils down to

event.transaction = rootSpan?.name ?  rootSpan?.name : scope.transactionName || undefined

@mydea
Copy link
Member Author

mydea commented Mar 11, 2024

Events should still/anyhow be linked to transactions through the trace though, shouldn't they? This is just about what the transaction field on the event should be - and there, IMHO, it's OK to decouple them? Esp. if in 90% of cases they will not be decoupled (when we set them consistently).

I'm also OK with making the root span name take precedence over the scope transactionName, but keep in mind that this will also lead to inconsistent transaction names (possibly) on the same event because the root span will sometimes exist, sometimes not, so you'll get different event.transaction values for exactly the same error event, which also seems not ideal to me as it could break grouping etc...?

@Lms24
Copy link
Member

Lms24 commented Mar 11, 2024

Events should still/anyhow be linked to transactions through the trace though, shouldn't they?

I think so, yes. Actually I strongly hope so 😅

I'll test things a bit more to be sure but yeah, I'll just continue with the initial plan. We can always revisit...

@Lms24
Copy link
Member

Lms24 commented Mar 12, 2024

Added a quite elaborate answer about our plan and the precedence dilemma here: #11025 (comment)

Lms24 added a commit that referenced this issue Apr 4, 2024
Super small PR to check that an error event has a valid `transaction`
field in NextJS.
Besides testing, there was nothing specifically to do for NextJS since
as far as I can tell we never update a pageload or navigation span name.

ref #10846
Lms24 added a commit that referenced this issue Apr 9, 2024
…11510)

This PR adds scope transactionName updating to our NestJS
instrumentation. Similarly to Hapi and Fastify, we can use a
framework-native component, an Interceptor, to assign the parameterized
route.

ref #10846
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue Apr 19, 2024
…entry#11424)

Super small PR to check that an error event has a valid `transaction`
field in NextJS.
Besides testing, there was nothing specifically to do for NextJS since
as far as I can tell we never update a pageload or navigation span name.

ref getsentry#10846
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue Apr 19, 2024
…etsentry#11510)

This PR adds scope transactionName updating to our NestJS
instrumentation. Similarly to Hapi and Fastify, we can use a
framework-native component, an Interceptor, to assign the parameterized
route.

ref getsentry#10846
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants