poc: use TryLock to check lock invariants at runtime

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

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 {

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 "<nil>", just as Sprintf does.
 					// Other panics should not be handled.
-					s.setMeta(key, "<nil>")
+					s.setMeta(key, "<nil>", 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,

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])
 	})
 }

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
 		}
 	}

ddtrace/tracer/tracer.go

@@ -722,20 +722,20 @@ 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)
 		}
 
 	}
 	span.context = newSpanContext(span, context)
 	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