getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
584 stars 206 forks source link

Consider setting the transaction on the current scope automatically #2399

Open mattjohnsonpint opened 1 year ago

mattjohnsonpint commented 1 year ago

Problem Statement

When manually instrumenting a transaction, it's advised to add it to the current scope:

SentrySdk.ConfigureScope(scope => scope.Transaction = transaction);

This is in the docs: https://docs.sentry.io/platforms/dotnet/performance/instrumentation/custom-instrumentation/

And samples:

https://github.com/getsentry/sentry-dotnet/blob/a674815a4ed0fe807b0bec4205c4ea48c085a988/samples/Sentry.Samples.Console.Basic/Program.cs#L38-L40

Adding the transaction to the scope is required for methods like SentrySDK.GetSpan() to work - which is useful for then calling methods like StartChild() to instrument a specific portion of the app. If the transaction is not set on the scope, then GetSpan() returns null.

It's very easy to forget to do this. It would be better if it happened automatically when starting a transaction, instead of having to call a separate API.

Solution Brainstorm

Consider when calling StartTransaction, if there's an active scope that doesn't already have a transaction set, that we could set it on the scope at that time.

Likewise, when a transaction is finished, if it is the one on the scope then we can clear it from the scope.

We should be careful that any existing code that is setting it manually doesn't break, by only setting it when null, and only clearing it if we were the one to set it.

mattjohnsonpint commented 1 year ago

With regard to potentially more than one concurrent transaction, only the first would be set on the scope.

Also, with regard to #2066 - we should not be setting transactions bridged from otel spans on the scope at all.

jamescrosswell commented 8 months ago

Also, with regard to #2066 - we should not be setting transactions bridged from otel spans on the scope at all.

Would love to know the rational for this as I've possibly already gone and broken things:

jamescrosswell commented 7 months ago

Would love to know the rational for this as I've possibly already gone and broken things:

I think I might have had an Aha moment. Folks can set Scope.Transaction manualy and the Sentry SDK often sets this automatically via our various integrations (e.g. our ASP.NET and ASP.NET Core middleware).

When we set this in the SpanProcessor we are potentially overwriting a value that might have been set by either our SDK or the user.

In the case of our SDK Integrations, I don't think this is a problem (yet) since we wouldn't generally mix any of the Integrations that set the transaction automatically (Sentry.AspNet, Sentry.AspNetCore, Sentry.Azure.Functions.Worker) and Otel (you either instrument with one or the other... but not both).

So I think the situation in which this would be a problem would be if someone had configured Sentry to use OpenTelemetry and had also manually instrumented part of their code using Sentry's instrumentation API... If an OpenTelemetry Span hit the span processor while they were in the middle of a SentryTransaction (e.g. if they were using OpenTelemetry to instrument GraphQL or something but using Sentry for the rest of their application) their Scope.Transaction could potentially be overwritten by the SpanProcessor logic.

All of this should hopefully be resolved by:

If OTEL is being used everywhere under the hood, there would no longer be a difference between the Sentry instrumentation and the OTEL instrumentation and Scope.Transaction could be replaced by Activity.Current.