-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
Set the scope transaction for Otel transactions #3072
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3072 +/- ##
==========================================
+ Coverage 76.42% 76.45% +0.03%
==========================================
Files 351 351
Lines 13263 13259 -4
Branches 2646 2644 -2
==========================================
+ Hits 10136 10137 +1
+ Misses 2450 2446 -4
+ Partials 677 676 -1 ☔ View full report in Codecov by Sentry. |
@@ -275,14 +275,6 @@ internal TransactionTracer(IHub hub, ITransactionContext context, TimeSpan? idle | |||
internal ISpan StartChild(SpanId? spanId, SpanId parentSpanId, string operation, | |||
Instrumenter instrumenter = Instrumenter.Sentry) | |||
{ | |||
if (instrumenter != _instrumenter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it was ever needed to be honest.
If the instrumenter is OpenTelemetry then we don't want our own Asp.NET Core integration also trying to instrument the same requests... so I can see why we might throw if an attempt is made to create a new transaction using the Sentry Hub.
I can't see any reason to prevent adding children to transactions that have already been created however.
I think this code was Matt's originally (I took it over part way through the PR)... so I'm not sure what his original reasoning was (I'm just guessing at it).
The alternative would be for calling methods in our own code to check what the current instrumenter is and pass that in as a parameter so as to sidestep the above check... that seemed like a weird solution. Those spans aren't actually coming from OpenTelemetry (they're coming from a Sentry integration). It seemed more honest just to admit that actually we're going to have Sentry child spans of OpenTelemetry transaction spans.
Resolves #3061
Problem Summary
The
SentrySpanProcessor
wasn't setting theScope.Transaction
... so when using OpenTelemetry this was returning null:sentry-dotnet/src/Sentry.DiagnosticSource/Internal/DiagnosticSource/SentrySqlListener.cs
Lines 75 to 79 in 84925a9
Consequently the integrations weren't able to create DB Spans under the DB Root.