feat(tracing): Introduce span.SetContext #599
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #599 +/- ##
==========================================
+ Coverage 78.88% 78.93% +0.04%
==========================================
Files 38 38
Lines 3860 3869 +9
==========================================
+ Hits 3045 3054 +9
Misses 712 712
Partials 103 103
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. |
There was a problem hiding this comment.
Added some comments. We need to find a solution for https://github.com/getsentry/sentry-go/blob/master/otel/span_processor.go#L148 as well, as the initial issue with not having any otel context being present will persist.
| @@ -236,6 +241,10 @@ func (s *Span) SetData(name, value string) { | |||
| s.Data[name] = value | |||
| } | |||
|
|
|||
| func (s *Span) SetContext(key string, value Context) { | |||
| s.contexts[key] = value | |||
There was a problem hiding this comment.
// span context, can only be set on transactions
Should we guard against this?
There was a problem hiding this comment.
I thought about it, but decided against this for now since this field is a) private and b) only used when calling .toEvent().
| for k, v := range s.contexts { | ||
| contexts[k] = v | ||
| } | ||
| contexts["trace"] = s.traceContext().Map() |
There was a problem hiding this comment.
This might overwrite the trace key, is this a concern?
There was a problem hiding this comment.
Nope, we want to make sure the trace key is always being set by the transaction itself, and not overwritten by setContext.
| @@ -484,17 +493,21 @@ func (s *Span) toEvent() *Event { | |||
| s.dynamicSamplingContext = DynamicSamplingContextFromTransaction(s) | |||
| } | |||
|
|
|||
| contexts := map[string]Context{} | |||
| for k, v := range s.contexts { | |||
| contexts[k] = v | |||
There was a problem hiding this comment.
Maybe worth adding a test here as well.
|
Might be worth adding a new field to |
Let's do this in a follow up PR? |
ref #596
This PR adds the
SetContextstruct method to the span struct.This allows us to remove the usage of setting context on the scope in the OpenTelemetry span processor