fix(otel): Fix Dynamic Sampling context propagation for nested spans #559
Conversation
otel/propagator.go
Outdated
| if sentrySpan != nil { | ||
| // Dynamic sampling context is set only on the (root) transaction. | ||
| sentryTransaction := sentrySpan.GetTransaction() | ||
| if sentryTransaction != nil { | ||
| sentryBaggageStr = sentryTransaction.ToBaggage() | ||
| } |
There was a problem hiding this comment.
This is the actual fix.
tonyo
added
Type: Improvement
Topic: OpenTelemetry
Codecov ReportBase: 75.48% // Head: 75.62% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #559 +/- ##
==========================================
+ Coverage 75.48% 75.62% +0.13%
==========================================
Files 37 37
Lines 3765 3770 +5
==========================================
+ Hits 2842 2851 +9
+ Misses 808 805 -3
+ Partials 115 114 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Alright, found another issue: in the propagator's |
|
What issue, in particular, you're seeing? Freezing the baggage during creation is possible today, what would a more explicit way improve? |
| t.Error(err) | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(baggageGot, baggageWant) { |
There was a problem hiding this comment.
I would always use https://pkg.go.dev/github.com/google/go-cmp/cmp, had quite a few issues with reflect in the past.
There was a problem hiding this comment.
Agreed, will include things like this in the planned test utils refactoring.
| transaction := sentry.StartTransaction( | ||
| ctx, | ||
| emptyContextWithSentry(), |
There was a problem hiding this comment.
| emptyContextWithSentry(), | |
| emptyCtxWithSentry(), |
There was a problem hiding this comment.
It's a helper function that's used in multiple places already, do we really want this?
| if withSpan { | ||
| span := transaction.StartChild("op") | ||
| // We want the child to have the SpanID from transactionContext, so | ||
| // we "swap" span IDs from the transaction and the child span. | ||
| transaction.SpanID = span.SpanID |
There was a problem hiding this comment.
Not sure I fully understand this.
There was a problem hiding this comment.
When withSpan is passed, we want to create a transation and a child span. We also want the child span to have the predefined SpanID (passed via transactionContext). It means that we have to update the SpanID for the transaction. We can generate a new random one, or we can just take the already-generated one from the child span (span), that will be replaced with SpanIDFromHex(transactionContext.spanID) in the next line anyway.
|
I see the following issues:
Ideally we should have convenient ways of explicitly generating the DSC/baggage in a proper format (as baggage values, or as a string). I don't have a super clear picture yet how it will look like, though. |
|
The DSC APIs are something I want to avoid to expose to publicly, as it has lots of nitty-gritty details. Taking some extra steps for Otel is fine IMO.
You would use this function to get the value of the header string for outgoing HTTP requests. |
Ok, fair, but there's an assumption that the DSC was already populated/frozen at some point before. When you start a transaction manually, and then call |
|
You are right, we actually need to create a DSC when calling |
|
Cool, will think about how to improve this (as a separate task) |
Thanks to #558, we can now fix the DSC/baggage propagation for non-transaction spans in the OTel propagator.
Fixes #553.