contrib: fix span start option races

contrib/gin-gonic/gin/gintrace.go

@@ -44,12 +44,18 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 		if cfg.ignoreRequest(c) {
 			return
 		}
-		opts := append(spanOpts, tracer.ResourceName(cfg.resourceNamer(c)))
+
+		// Make sure opts is a copy of the span options, scoped to this request,
+		// to avoid races and leaks to spanOpts or cfg.spanOpts
+		opts := append([]tracer.StartSpanOption{
+			tracer.ResourceName(cfg.resourceNamer(c)),
+			tracer.Tag(ext.HTTPRoute, c.FullPath()),
+			httptrace.HeaderTagsFromRequest(c.Request, cfg.headerTags),
+		}, spanOpts...)
+
 		if !math.IsNaN(cfg.analyticsRate) {
 			opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 		}
-		opts = append(opts, tracer.Tag(ext.HTTPRoute, c.FullPath()))
-		opts = append(opts, httptrace.HeaderTagsFromRequest(c.Request, cfg.headerTags))
 		span, ctx := httptrace.StartRequestSpan(c.Request, opts...)
 		defer func() {
 			httptrace.FinishRequestSpan(span, c.Writer.Status())

contrib/go-chi/chi.v5/chi.go

@@ -12,6 +12,7 @@ import (
 	"net/http"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec"
@@ -37,20 +38,30 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 		fn(cfg)
 	}
 	log.Debug("contrib/go-chi/chi.v5: Configuring Middleware: %#v", cfg)
-	spanOpts := append(cfg.spanOpts, tracer.ServiceName(cfg.serviceName),
+
+	spanOpts := append([]ddtrace.StartSpanOption{
+		tracer.ServiceName(cfg.serviceName),
 		tracer.Tag(ext.Component, componentName),
-		tracer.Tag(ext.SpanKind, ext.SpanKindServer))
+		tracer.Tag(ext.SpanKind, ext.SpanKindServer),
+	}, cfg.spanOpts...) // copy cfg.spanOpts and avoid modifying it (append doesn't always copy)
+
+	if !math.IsNaN(cfg.analyticsRate) {
+		spanOpts = append(spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
+	}
+
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			if cfg.ignoreRequest(r) {
 				next.ServeHTTP(w, r)
 				return
 			}
-			opts := spanOpts
-			if !math.IsNaN(cfg.analyticsRate) {
-				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
-			}
-			opts = append(opts, httptrace.HeaderTagsFromRequest(r, cfg.headerTags))
+
+			// Make sure opts is a copy of the span options, scoped to this request,
+			// to avoid races and leaks to spanOpts or cfg.spanOpts
+			opts := append([]ddtrace.StartSpanOption{
+				httptrace.HeaderTagsFromRequest(r, cfg.headerTags),
+			}, spanOpts...)
+
 			span, ctx := httptrace.StartRequestSpan(r, opts...)
 			ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
 			defer func() {

contrib/go-chi/chi.v5/chi_test.go

@@ -12,6 +12,7 @@ import (
 	"net/http/httptest"
 	"strconv"
 	"strings"
+	"sync"
 	"testing"
 
 	pappsec "gopkg.in/DataDog/dd-trace-go.v1/appsec"
@@ -609,3 +610,56 @@ func TestUnknownResourceName(t *testing.T) {
 	require.Equal(t, "service-name", spans[0].Tag(ext.ServiceName))
 	require.Equal(t, "GET unknown", spans[0].Tag(ext.ResourceName))
 }
+
+// Highly concurrent test running many goroutines to try to uncover concurrency
+// issues such as deadlocks, data races, etc.
+func TestConcurrency(t *testing.T) {
+	mt := mocktracer.Start()
+	defer mt.Stop()
+
+	expectedCap := 10
+	opts := make([]Option, 0, expectedCap)
+	opts = append(opts, []Option{
+		WithServiceName("foobar"),
+		WithSpanOptions(tracer.Tag("tag1", "value1")),
+	}...)
+	expectedLen := 2
+
+	router := chi.NewRouter()
+	require.Len(t, opts, expectedLen)
+	require.True(t, cap(opts) == expectedCap)
+
+	router.Use(Middleware(opts...))
+	router.Get("/user/{id}", func(w http.ResponseWriter, r *http.Request) {
+		_, ok := tracer.SpanFromContext(r.Context())
+		require.True(t, ok)
+	})
+
+	// Create a bunch of goroutines that will all try to use the same router using our middleware
+	nbReqGoroutines := 1000
+	var startBarrier, finishBarrier sync.WaitGroup
+	startBarrier.Add(1)
+	finishBarrier.Add(nbReqGoroutines)
+
+	for n := 0; n < nbReqGoroutines; n++ {
+		go func() {
+			startBarrier.Wait()
+			defer finishBarrier.Done()
+
+			for i := 0; i < 100; i++ {
+				r := httptest.NewRequest("GET", "/user/123", nil)
+				w := httptest.NewRecorder()
+				router.ServeHTTP(w, r)
+			}
+		}()
+	}
+
+	startBarrier.Done()
+	finishBarrier.Wait()
+
+	// Side effects on opts is not the main purpose of this test, but it's worth checking just in case.
+	require.Len(t, opts, expectedLen)
+	require.True(t, cap(opts) == expectedCap)
+	// All the others config data are internal to the closures in Middleware and cannot be tested.
+	// Running this test with -race is the best chance to find a concurrency issue.
+}

contrib/go-chi/chi/chi.go

@@ -8,6 +8,7 @@ package chi // import "gopkg.in/DataDog/dd-trace-go.v1/contrib/go-chi/chi"
 
 import (
 	"fmt"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
 	"math"
 	"net/http"
 
@@ -46,10 +47,13 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 				next.ServeHTTP(w, r)
 				return
 			}
-			opts := spanOpts
+			opts := make([]ddtrace.StartSpanOption, len(spanOpts), len(spanOpts)+2)
+			copy(opts, cfg.spanOpts)
+
 			if !math.IsNaN(cfg.analyticsRate) {
 				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 			}
+
 			opts = append(opts, httptrace.HeaderTagsFromRequest(r, cfg.headerTags))
 			span, ctx := httptrace.StartRequestSpan(r, opts...)
 			ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)

contrib/google.golang.org/grpc/stats_client.go

@@ -11,6 +11,7 @@ import (
 
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
+
 	"google.golang.org/grpc/stats"
 )
 

contrib/julienschmidt/httprouter/httprouter.go

@@ -13,6 +13,7 @@ import (
 
 	httptraceinternal "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	httptrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/net/http"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/log"
@@ -41,13 +42,16 @@ func New(opts ...RouterOption) *Router {
 	for _, fn := range opts {
 		fn(cfg)
 	}
+
+	cfg.spanOpts = append(cfg.spanOpts, []ddtrace.StartSpanOption{
+		tracer.Tag(ext.Component, componentName),
+		tracer.Tag(ext.SpanKind, ext.SpanKindServer),
+	}...) // copy cfg.spanOpts and avoid modifying it (append doesn't always copy)
+
 	if !math.IsNaN(cfg.analyticsRate) {
 		cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 	}
 
-	cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.SpanKind, ext.SpanKindServer))
-	cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.Component, componentName))
-
 	log.Debug("contrib/julienschmidt/httprouter: Configuring Router: %#v", cfg)
 	return &Router{httprouter.New(), cfg}
 }
@@ -61,12 +65,17 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		route = strings.Replace(route, param.Value, ":"+param.Key, 1)
 	}
 	resource := req.Method + " " + route
-	spanOpts := append(r.config.spanOpts, httptraceinternal.HeaderTagsFromRequest(req, r.config.headerTags))
+
+	// Make sure opts is a copy of the span options, scoped to this request,
+	// to avoid races and leaks to r.config.spanOpts
+	opts := append([]ddtrace.StartSpanOption{
+		httptraceinternal.HeaderTagsFromRequest(req, r.config.headerTags),
+	}, r.config.spanOpts...)
 
 	httptrace.TraceAndServe(r.Router, w, req, &httptrace.ServeConfig{
 		Service:  r.config.serviceName,
 		Resource: resource,
-		SpanOpts: spanOpts,
+		SpanOpts: opts,
 		Route:    route,
 	})
 }

contrib/k8s.io/client-go/kubernetes/kubernetes.go

@@ -33,7 +33,7 @@ const (
 )
 
 // WrapRoundTripperFunc creates a new WrapTransport function using the given set of
-// RountripperOption. It is useful when desiring to enable Trace Analytics or setting
+// RoundTripperOption. It is useful when desiring to enable Trace Analytics or setting
 // up a RoundTripperAfterFunc.
 func WrapRoundTripperFunc(opts ...httptrace.RoundTripperOption) func(http.RoundTripper) http.RoundTripper {
 	return func(rt http.RoundTripper) http.RoundTripper {
@@ -48,19 +48,22 @@ func WrapRoundTripper(rt http.RoundTripper) http.RoundTripper {
 }
 
 func wrapRoundTripperWithOptions(rt http.RoundTripper, opts ...httptrace.RoundTripperOption) http.RoundTripper {
-	opts = append(opts, httptrace.WithBefore(func(req *http.Request, span ddtrace.Span) {
-		span.SetTag(ext.ResourceName, RequestToResource(req.Method, req.URL.Path))
-		span.SetTag(ext.Component, componentName)
-		span.SetTag(ext.SpanKind, ext.SpanKindClient)
-		traceID := span.Context().TraceID()
-		if traceID == 0 {
-			// tracer is not running
-			return
-		}
-		kubeAuditID := strconv.FormatUint(traceID, 10)
-		req.Header.Set("Audit-Id", kubeAuditID)
-		span.SetTag("kubernetes.audit_id", kubeAuditID)
-	}))
+	// Make sure opts is a copy of the span options, scoped to this request,
+	// to avoid races and leaks.
+	opts = append([]httptrace.RoundTripperOption{
+		httptrace.WithBefore(func(req *http.Request, span ddtrace.Span) {
+			span.SetTag(ext.ResourceName, RequestToResource(req.Method, req.URL.Path))
+			span.SetTag(ext.Component, componentName)
+			span.SetTag(ext.SpanKind, ext.SpanKindClient)
+			traceID := span.Context().TraceID()
+			if traceID == 0 {
+				// tracer is not running
+				return
+			}
+			kubeAuditID := strconv.FormatUint(traceID, 10)
+			req.Header.Set("Audit-Id", kubeAuditID)
+			span.SetTag("kubernetes.audit_id", kubeAuditID)
+		})}, opts...)
 	log.Debug("contrib/k8s.io/client-go/kubernetes: Wrapping RoundTripper.")
 	return httptrace.WrapRoundTripper(rt, opts...)
 }

contrib/labstack/echo.v4/echotrace.go

@@ -38,15 +38,20 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
 		fn(cfg)
 	}
 	log.Debug("contrib/labstack/echo.v4: Configuring Middleware: %#v", cfg)
+
 	spanOpts := make([]ddtrace.StartSpanOption, 0, 3+len(cfg.tags))
-	spanOpts = append(spanOpts, tracer.ServiceName(cfg.serviceName))
 	for k, v := range cfg.tags {
 		spanOpts = append(spanOpts, tracer.Tag(k, v))
 	}
 	spanOpts = append(spanOpts,
+		tracer.ServiceName(cfg.serviceName),
 		tracer.Tag(ext.Component, componentName),
 		tracer.Tag(ext.SpanKind, ext.SpanKindServer),
 	)
+	if !math.IsNaN(cfg.analyticsRate) {
+		spanOpts = append(spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
+	}
+
 	return func(next echo.HandlerFunc) echo.HandlerFunc {
 		return func(c echo.Context) error {
 			// If we have an ignoreRequestFunc, use it to see if we proceed with tracing
@@ -61,12 +66,14 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
 			request := c.Request()
 			route := c.Path()
 			resource := request.Method + " " + route
-			opts := append(spanOpts, tracer.ResourceName(resource), tracer.Tag(ext.HTTPRoute, route))
+			// Make sure opts is a copy of the span options, scoped to this request,
+			// to avoid races and leaks to spanOpts
+			opts := append([]ddtrace.StartSpanOption{
+				tracer.ResourceName(resource),
+				tracer.Tag(ext.HTTPRoute, route),
+				httptrace.HeaderTagsFromRequest(request, cfg.headerTags),
+			}, spanOpts...)
 
-			if !math.IsNaN(cfg.analyticsRate) {
-				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
-			}
-			opts = append(opts, httptrace.HeaderTagsFromRequest(request, cfg.headerTags))
 			var finishOpts []tracer.FinishOption
 			if cfg.noDebugStack {
 				finishOpts = []tracer.FinishOption{tracer.NoDebugStack()}

contrib/labstack/echo/echotrace.go

@@ -41,29 +41,34 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
 		fn(cfg)
 	}
 	log.Debug("contrib/labstack/echo: Configuring Middleware: %#v", cfg)
+
 	spanOpts := []ddtrace.StartSpanOption{
 		tracer.ServiceName(cfg.serviceName),
 		tracer.Tag(ext.Component, componentName),
 		tracer.Tag(ext.SpanKind, ext.SpanKindServer),
 	}
+	if !math.IsNaN(cfg.analyticsRate) {
+		spanOpts = append(spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
+	}
+
 	return func(next echo.HandlerFunc) echo.HandlerFunc {
 		return func(c echo.Context) error {
 			request := c.Request()
 			resource := request.Method + " " + c.Path()
-			opts := append(spanOpts, tracer.ResourceName(resource))
+			// Make sure opts is a copy of the span options, scoped to this request,
+			// to avoid races and leaks to spanOpts
+			opts := append([]ddtrace.StartSpanOption{
+				tracer.ResourceName(resource),
+				httptrace.HeaderTagsFromRequest(request, cfg.headerTags),
+			}, spanOpts...)
 
-			if !math.IsNaN(cfg.analyticsRate) {
-				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
-			}
-			opts = append(opts, httptrace.HeaderTagsFromRequest(request, cfg.headerTags))
 			var finishOpts []tracer.FinishOption
 			if cfg.noDebugStack {
 				finishOpts = []tracer.FinishOption{tracer.NoDebugStack()}
 			}
 
 			span, ctx := httptrace.StartRequestSpan(request, opts...)
 			defer func() {
-				//httptrace.FinishRequestSpan(span, c.Response().Status, finishOpts...)
 				span.Finish(finishOpts...)
 			}()
 

contrib/urfave/negroni/negroni.go

@@ -11,6 +11,8 @@ import (
 	"math"
 	"net/http"
 
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
+
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
@@ -33,12 +35,13 @@ type DatadogMiddleware struct {
 }
 
 func (m *DatadogMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
-	opts := append(
-		m.cfg.spanOpts,
+	// Make sure opts is a copy of the span options, scoped to this request,
+	// to avoid races and leaks to m.cfg.spanOpts
+	opts := append([]ddtrace.StartSpanOption{
 		tracer.ServiceName(m.cfg.serviceName),
 		tracer.ResourceName(m.cfg.resourceNamer(r)),
 		httptrace.HeaderTagsFromRequest(r, m.cfg.headerTags),
-	)
+	}, m.cfg.spanOpts...)
 	if !math.IsNaN(m.cfg.analyticsRate) {
 		opts = append(opts, tracer.Tag(ext.EventSampleRate, m.cfg.analyticsRate))
 	}