diff --git a/ddtrace/tracer/rules_sampler.go b/ddtrace/tracer/rules_sampler.go index 65f7f1a187..2fa33a79b1 100644 --- a/ddtrace/tracer/rules_sampler.go +++ b/ddtrace/tracer/rules_sampler.go @@ -496,7 +496,7 @@ func (rs *traceRulesSampler) applyRate(span *Span, rate float64, now time.Time, span.setMetric(keyRulesSamplerAppliedRate, rate) delete(span.metrics, keySamplingPriorityRate) // Set the Knuth sampling rate tag when trace sampling rules are applied - span.setMeta(keyKnuthSamplingRate, formatKnuthSamplingRate(rate)) + span.setMeta(keyKnuthSamplingRate, formatKnuthSamplingRate(rate), true) if !sampledByRate(span.traceID, rate) { span.setSamplingPriorityLocked(ext.PriorityUserReject, sampler) return diff --git a/ddtrace/tracer/sampler_test.go b/ddtrace/tracer/sampler_test.go index 8fc9894623..e3354864ee 100644 --- a/ddtrace/tracer/sampler_test.go +++ b/ddtrace/tracer/sampler_test.go @@ -491,7 +491,7 @@ func TestRuleEnvVars(t *testing.T) { func TestRulesSampler(t *testing.T) { makeSpan := func(op string, svc string) *Span { s := newSpan(op, svc, "res-10", randUint64(), randUint64(), 0) - s.setMeta("hostname", "hn-30") + s.setMeta("hostname", "hn-30", false) return s } makeFinishedSpan := func(op, svc, resource string, tags map[string]interface{}) *Span { diff --git a/ddtrace/tracer/span.go b/ddtrace/tracer/span.go index 2481a5af87..eeb39495dd 100644 --- a/ddtrace/tracer/span.go +++ b/ddtrace/tracer/span.go @@ -193,12 +193,12 @@ func (s *Span) SetTag(key string, value interface{}) { } switch key { case ext.Error: - s.setTagError(value, errorConfig{ + s.setTagErrorLocked(value, errorConfig{ noDebugStack: s.noDebugStack, }) return case ext.ErrorNoStackTrace: - s.setTagError(value, errorConfig{ + s.setTagErrorLocked(value, errorConfig{ noDebugStack: true, }) return @@ -223,7 +223,7 @@ func (s *Span) SetTag(key string, value interface{}) { s.pprofCtxActive = pprof.WithLabels(s.pprofCtxActive, pprof.Labels(traceprof.TraceEndpoint, v)) pprof.SetGoroutineLabels(s.pprofCtxActive) } - s.setMeta(key, v) + s.setMeta(key, v, true) return } if v, ok := sharedinternal.ToFloat64(value); ok { @@ -237,18 +237,18 @@ func (s *Span) SetTag(key string, value interface{}) { // If .String() panics due to a nil receiver, we want to catch this // and replace the string value with "", just as Sprintf does. // Other panics should not be handled. - s.setMeta(key, "") + s.setMeta(key, "", false) return } panic(e) } }() - s.setMeta(key, v.String()) + s.setMeta(key, v.String(), true) return } if v, ok := value.([]byte); ok { - s.setMeta(key, string(v)) + s.setMeta(key, string(v), true) return } @@ -265,7 +265,7 @@ func (s *Span) SetTag(key string, value interface{}) { if num, ok := sharedinternal.ToFloat64(v.Interface()); ok { s.setMetric(key, num) } else { - s.setMeta(key, fmt.Sprintf("%v", v)) + s.setMeta(key, fmt.Sprintf("%v", v), true) } } return @@ -292,7 +292,7 @@ func (s *Span) SetTag(key string, value interface{}) { } // not numeric, not a string, not a fmt.Stringer, not a bool, and not an error - s.setMeta(key, fmt.Sprint(value)) + s.setMeta(key, fmt.Sprint(value), true) } // setSamplingPriority locks the span, then updates the sampling priority. @@ -309,7 +309,7 @@ func (s *Span) setSamplingPriority(priority int, sampler samplernames.SamplerNam func (s *Span) setProcessTags(pTags string) { s.mu.Lock() defer s.mu.Unlock() - s.setMeta(keyProcessTags, pTags) + s.setMeta(keyProcessTags, pTags, true) } // root returns the root span of the span's trace. The return value shouldn't be @@ -380,7 +380,7 @@ func (s *Span) SetUser(id string, opts ...UserMonitoringOption) { for k, v := range usrData { if v != "" { // setMeta is used since the span is already locked - root.setMeta(k, v) + root.setMeta(k, v, true) } } } @@ -422,9 +422,9 @@ func (s *Span) forceSetSamplingPriorityLocked(priority int, sampler samplernames s.context.forceSetSamplingPriority(priority, sampler) } -// setTagError sets the error tag. It accounts for various valid scenarios. +// setTagErrorLocked sets the error tag. It accounts for various valid scenarios. // This method is not safe for concurrent use. -func (s *Span) setTagError(value interface{}, cfg errorConfig) { +func (s *Span) setTagErrorLocked(value interface{}, cfg errorConfig) { setError := func(yes bool) { if yes { if s.error == 0 { @@ -455,25 +455,25 @@ func (s *Span) setTagError(value interface{}, cfg errorConfig) { // and provide all the benefits. // TODO: once Error Tracking fix is resolved, update relevant tags here. See #4095 setError(true) - s.setMeta(ext.ErrorMsg, v.Error()) - s.setMeta(ext.ErrorType, reflect.TypeOf(v).String()) + s.setMeta(ext.ErrorMsg, v.Error(), true) + s.setMeta(ext.ErrorType, reflect.TypeOf(v).String(), true) if cfg.noDebugStack { return } switch err := v.(type) { case xerrors.Formatter: - s.setMeta(ext.ErrorDetails, fmt.Sprintf("%+v", v)) + s.setMeta(ext.ErrorDetails, fmt.Sprintf("%+v", v), true) case fmt.Formatter: // pkg/errors approach - s.setMeta(ext.ErrorDetails, fmt.Sprintf("%+v", v)) + s.setMeta(ext.ErrorDetails, fmt.Sprintf("%+v", v), true) case *errortrace.TracerError: // instrumentation/errortrace approach - s.setMeta(ext.ErrorStack, fmt.Sprintf("%+v", v)) - s.setMeta(ext.ErrorHandlingStack, err.Format()) + s.setMeta(ext.ErrorStack, fmt.Sprintf("%+v", v), true) + s.setMeta(ext.ErrorHandlingStack, err.Format(), true) return } stack := takeStacktrace(cfg.stackFrames, cfg.stackSkip) - s.setMeta(ext.ErrorStack, stack) + s.setMeta(ext.ErrorStack, stack, true) case nil: // no error setError(false) @@ -504,7 +504,14 @@ func takeStacktrace(depth uint, skip uint) string { } // setMeta sets a string tag. This method is not safe for concurrent use. -func (s *Span) setMeta(key, v string) { +func (s *Span) setMeta(key, v string, locked bool) { + if s.mu.TryLock() { + if locked { + panic("setMeta: span was not locked") + } else { + s.mu.Unlock() + } + } if s.meta == nil { s.meta = make(map[string]string, 1) } @@ -549,9 +556,9 @@ func (s *Span) setTagBool(key string, v bool) { } default: if v { - s.setMeta(key, "true") + s.setMeta(key, "true", true) } else { - s.setMeta(key, "false") + s.setMeta(key, "false", true) } } } @@ -655,7 +662,7 @@ func (s *Span) Finish(opts ...FinishOption) { } if cfg.Error != nil { s.mu.Lock() - s.setTagError(cfg.Error, errorConfig{ + s.setTagErrorLocked(cfg.Error, errorConfig{ noDebugStack: cfg.NoDebugStack, stackFrames: cfg.StackFrames, stackSkip: cfg.SkipStackFrames, diff --git a/ddtrace/tracer/span_test.go b/ddtrace/tracer/span_test.go index 1b42b134d6..05e7832068 100644 --- a/ddtrace/tracer/span_test.go +++ b/ddtrace/tracer/span_test.go @@ -533,13 +533,17 @@ func TestSpanSetTagError(t *testing.T) { t.Run("off", func(t *testing.T) { span := newBasicSpan("web.request") - span.setTagError(errors.New("error value with no trace"), errorConfig{noDebugStack: true}) + span.mu.Lock() + defer span.mu.Unlock() + span.setTagErrorLocked(errors.New("error value with no trace"), errorConfig{noDebugStack: true}) assert.Empty(t, span.meta[ext.ErrorStack]) }) t.Run("on", func(t *testing.T) { span := newBasicSpan("web.request") - span.setTagError(errors.New("error value with trace"), errorConfig{noDebugStack: false}) + span.mu.Lock() + defer span.mu.Unlock() + span.setTagErrorLocked(errors.New("error value with trace"), errorConfig{noDebugStack: false}) assert.NotEmpty(t, span.meta[ext.ErrorStack]) }) } diff --git a/ddtrace/tracer/spancontext.go b/ddtrace/tracer/spancontext.go index aeee388b1f..9747e1c8f7 100644 --- a/ddtrace/tracer/spancontext.go +++ b/ddtrace/tracer/spancontext.go @@ -525,18 +525,18 @@ func (t *trace) push(sp *Span) { // setTraceTags sets all "trace level" tags on the provided span // t must already be locked. -func (t *trace) setTraceTags(s *Span) { +func (t *trace) setTraceTags(s *Span, locked bool) { for k, v := range t.tags { - s.setMeta(k, v) + s.setMeta(k, v, locked) } for k, v := range t.propagatingTags { - s.setMeta(k, v) + s.setMeta(k, v, locked) } for k, v := range sharedinternal.GetTracerGitMetadataTags() { - s.setMeta(k, v) + s.setMeta(k, v, locked) } if s.context != nil && s.context.traceID.HasUpper() { - s.setMeta(keyTraceID128, s.context.traceID.UpperHex()) + s.setMeta(keyTraceID128, s.context.traceID.UpperHex(), locked) } } @@ -584,7 +584,7 @@ func (t *trace) finishedOne(s *Span) { // TODO(barbayar): make sure this doesn't happen in vain when switching to // the new wire format. We won't need to set the tags on the first span // in the chunk there. - t.setTraceTags(s) + t.setTraceTags(s, false) } // This is here to support the mocktracer. It would be nice to be able to not do this. @@ -624,7 +624,7 @@ func (t *trace) finishedOne(s *Span) { finishedSpans[0].setMetric(keySamplingPriority, *t.priority) if s != t.spans[0] { // Make sure the first span in the chunk has the trace-level tags - t.setTraceTags(finishedSpans[0]) + t.setTraceTags(finishedSpans[0], false) } if tr, ok := tr.(*tracer); ok { t.finishChunk(tr, &chunk{ @@ -647,13 +647,13 @@ func setPeerService(s *Span, tc TracerConf) { isOutboundRequest := spanKind == ext.SpanKindClient || spanKind == ext.SpanKindProducer if _, ok := s.meta[ext.PeerService]; ok { // peer.service already set on the span - s.setMeta(keyPeerServiceSource, ext.PeerService) + s.setMeta(keyPeerServiceSource, ext.PeerService, true) } else if isServerless(tc) { // Set peerService only in outbound Lambda requests if isOutboundRequest { if ps := deriveAWSPeerService(s.meta); ps != "" { - s.setMeta(ext.PeerService, ps) - s.setMeta(keyPeerServiceSource, ext.PeerService) + s.setMeta(ext.PeerService, ps, true) + s.setMeta(keyPeerServiceSource, ext.PeerService, true) } else { log.Debug("Unable to set peer.service tag for serverless span %q", s.name) } @@ -668,13 +668,13 @@ func setPeerService(s *Span, tc TracerConf) { log.Debug("No source tag value could be found for span %q, peer.service not set", s.name) return } - s.setMeta(keyPeerServiceSource, source) + s.setMeta(keyPeerServiceSource, source, true) } // Overwrite existing peer.service value if remapped by the user ps := s.meta[ext.PeerService] if to, ok := tc.PeerServiceMappings[ps]; ok { - s.setMeta(keyPeerServiceRemappedFrom, ps) - s.setMeta(ext.PeerService, to) + s.setMeta(keyPeerServiceRemappedFrom, ps, true) + s.setMeta(ext.PeerService, to, true) } } @@ -774,7 +774,7 @@ func setPeerServiceFromSource(s *Span) string { } for _, source := range sources { if val, ok := s.meta[source]; ok { - s.setMeta(ext.PeerService, val) + s.setMeta(ext.PeerService, val, true) return source } } diff --git a/ddtrace/tracer/tracer.go b/ddtrace/tracer/tracer.go index 33b0541830..25883c2a26 100644 --- a/ddtrace/tracer/tracer.go +++ b/ddtrace/tracer/tracer.go @@ -722,12 +722,12 @@ func spanStart(operationName string, options ...StartSpanOption) *Span { // remote parent if context.origin != "" { // mark origin - span.setMeta(keyOrigin, context.origin) + span.setMeta(keyOrigin, context.origin, false) } } if context.reparentID != "" { - span.setMeta(keyReparentID, context.reparentID) + span.setMeta(keyReparentID, context.reparentID, false) } } @@ -735,7 +735,7 @@ func spanStart(operationName string, options ...StartSpanOption) *Span { if pprofContext != nil { setLLMObsPropagatingTags(pprofContext, span.context) } - span.setMeta("language", "go") + span.setMeta("language", "go", false) // add tags from options for k, v := range opts.Tags { span.SetTag(k, v) @@ -766,7 +766,7 @@ func (t *tracer) StartSpan(operationName string, options ...StartSpanOption) *Sp } span.noDebugStack = t.config.noDebugStack if t.config.hostname != "" { - span.setMeta(keyHostname, t.config.hostname) + span.setMeta(keyHostname, t.config.hostname, false) } span.supportsEvents = t.config.agent.spanEventsAvailable @@ -781,11 +781,11 @@ func (t *tracer) StartSpan(operationName string, options ...StartSpanOption) *Sp } if t.config.version != "" { if t.config.universalVersion || (!t.config.universalVersion && span.service == t.config.serviceName) { - span.setMeta(ext.Version, t.config.version) + span.setMeta(ext.Version, t.config.version, false) } } if t.config.env != "" { - span.setMeta(ext.Environment, t.config.env) + span.setMeta(ext.Environment, t.config.env, false) } if _, ok := span.context.SamplingPriority(); !ok { // if not already sampled or a brand new trace, sample it