contrib: fix span start option races (#2418)

contrib/database/sql/conn.go

@@ -11,6 +11,7 @@ import (
 	"math"
 	"time"
 
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	"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"
@@ -285,7 +286,8 @@ func (tp *traceParams) tryTrace(ctx context.Context, qtype QueryType, query stri
 		return
 	}
 	dbSystem, _ := normalizeDBSystem(tp.driverName)
-	opts := append(spanOpts,
+	opts := options.Copy(spanOpts...)
+	opts = append(opts,
 		tracer.ServiceName(tp.cfg.serviceName),
 		tracer.SpanType(ext.SpanTypeSQL),
 		tracer.StartTime(startTime),

contrib/database/sql/sql.go

@@ -236,8 +236,10 @@ func Open(driverName, dataSourceName string, opts ...Option) (*sql.DB, error) {
 			return nil, err
 		}
 		// since we're not using the dsnConnector, we need to register the dsn manually in the config
-		opts = append(opts, WithDSN(dataSourceName))
-		return OpenDB(connector, opts...), nil
+		optsCopy := make([]Option, len(opts))
+		copy(optsCopy, opts) // avoid modifying the provided opts, so make a copy instead, and use this
+		optsCopy = append(optsCopy, WithDSN(dataSourceName))
+		return OpenDB(connector, optsCopy...), nil
 	}
 	return OpenDB(&dsnConnector{dsn: dataSourceName, driver: d}, opts...), nil
 }

contrib/gin-gonic/gin/gintrace.go

@@ -11,6 +11,7 @@ import (
 	"math"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	"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"
@@ -44,7 +45,8 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 		if cfg.ignoreRequest(c) {
 			return
 		}
-		opts := append(spanOpts, tracer.ResourceName(cfg.resourceNamer(c)))
+		opts := options.Copy(spanOpts...) // opts must be a copy of cfg.spanOpts, locally scoped, to avoid races.
+		opts = append(opts, tracer.ResourceName(cfg.resourceNamer(c)))
 		if !math.IsNaN(cfg.analyticsRate) {
 			opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 		}

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/contrib/internal/options"
 	"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"
@@ -46,7 +47,7 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 				next.ServeHTTP(w, r)
 				return
 			}
-			opts := spanOpts
+			opts := options.Copy(spanOpts...) // opts must be a copy of spanOpts, locally scoped, to avoid races.
 			if !math.IsNaN(cfg.analyticsRate) {
 				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 			}

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

@@ -12,6 +12,7 @@ import (
 	"net/http"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	"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"
@@ -46,7 +47,7 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 				next.ServeHTTP(w, r)
 				return
 			}
-			opts := spanOpts
+			opts := options.Copy(spanOpts...) // opts must be a copy of spanOpts, locally scoped, to avoid races.
 			if !math.IsNaN(cfg.analyticsRate) {
 				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 			}

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

@@ -13,6 +13,7 @@ import (
 	"strings"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/google.golang.org/internal/grpcutil"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	"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"
@@ -72,8 +73,7 @@ func startServerSpanFromContext(ctx context.Context, method string, cfg *interce
 	}
 	// copy opts in case the caller reuses the slice in parallel
 	// we will add the items in extraOpts
-	optsLocal := make([]tracer.StartSpanOption, len(cfg.spanOpts), len(cfg.spanOpts)+len(extraOpts))
-	copy(optsLocal, cfg.spanOpts)
+	optsLocal := options.Copy(cfg.spanOpts...)
 	optsLocal = append(optsLocal, extraOpts...)
 	md, _ := metadata.FromContext(ctx) // nil is ok
 	if sctx, err := tracer.Extract(grpcutil.MDCarrier(md)); err == nil {

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

@@ -9,9 +9,9 @@ import (
 	"context"
 	"net"
 
+	"google.golang.org/grpc/stats"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
-	"google.golang.org/grpc/stats"
 )
 
 // NewClientStatsHandler returns a gRPC client stats.Handler to trace RPC calls.

contrib/gorilla/mux/mux.go

@@ -10,8 +10,8 @@ import (
 	"net/http"
 
 	httptraceinternal "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	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"
@@ -96,18 +96,17 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 	var (
-		match    mux.RouteMatch
-		spanopts []ddtrace.StartSpanOption
-		route    string
+		match mux.RouteMatch
+		route string
 	)
+	spanopts := options.Copy(r.config.spanOpts...)
 	// get the resource associated to this request
 	if r.Match(req, &match) && match.Route != nil {
 		if h, err := match.Route.GetHostTemplate(); err == nil {
 			spanopts = append(spanopts, tracer.Tag("mux.host", h))
 		}
 		route, _ = match.Route.GetPathTemplate()
 	}
-	spanopts = append(spanopts, r.config.spanOpts...)
 	spanopts = append(spanopts, httptraceinternal.HeaderTagsFromRequest(req, r.config.headerTags))
 	resource := r.config.resourceNamer(r, req)
 	httptrace.TraceAndServe(r.Router, w, req, &httptrace.ServeConfig{

contrib/internal/options/options.go

@@ -0,0 +1,17 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2023 Datadog, Inc.
+
+package options
+
+import "gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
+
+// Copy should be used any time existing options are copied into
+// a new locally scoped set of options. This is to avoid data races and
+// accidental side effects.
+func Copy(opts ...ddtrace.StartSpanOption) []ddtrace.StartSpanOption {
+	dup := make([]ddtrace.StartSpanOption, len(opts))
+	copy(dup, opts)
+	return dup
+}

contrib/internal/options/options_test.go

@@ -0,0 +1,37 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2023 Datadog, Inc.
+
+package options
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
+)
+
+func TestStringSliceModify(t *testing.T) {
+	t.Run("modify-original", func(t *testing.T) {
+		opts := []ddtrace.StartSpanOption{tracer.Tag("mytag", "myvalue")}
+		optsCopy := Copy(opts...)
+		opts[0] = tracer.ResourceName("somethingelse")
+		cfg := new(ddtrace.StartSpanConfig)
+		for _, fn := range optsCopy {
+			fn(cfg)
+		}
+		assert.Equal(t, "myvalue", cfg.Tags["mytag"])
+	})
+	t.Run("modify-copy", func(t *testing.T) {
+		opts := []ddtrace.StartSpanOption{tracer.Tag("mytag", "myvalue")}
+		optsCopy := Copy(opts...)
+		optsCopy[0] = tracer.ResourceName("somethingelse")
+		cfg := new(ddtrace.StartSpanConfig)
+		for _, fn := range opts {
+			fn(cfg)
+		}
+		assert.Equal(t, "myvalue", cfg.Tags["mytag"])
+	})
+}

contrib/julienschmidt/httprouter/httprouter.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 
 	httptraceinternal "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	httptrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/net/http"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
@@ -61,7 +62,8 @@ 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))
+	spanOpts := options.Copy(r.config.spanOpts...) // spanOpts must be a copy of r.config.spanOpts, locally scoped, to avoid races.
+	spanOpts = append(spanOpts, httptraceinternal.HeaderTagsFromRequest(req, r.config.headerTags))
 
 	httptrace.TraceAndServe(r.Router, w, req, &httptrace.ServeConfig{
 		Service:  r.config.serviceName,

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,7 +48,9 @@ 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) {
+	localOpts := make([]httptrace.RoundTripperOption, len(opts))
+	copy(localOpts, opts) // make a copy of the opts, to avoid data races and side effects.
+	localOpts = append(localOpts, 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)
@@ -62,7 +64,7 @@ func wrapRoundTripperWithOptions(rt http.RoundTripper, opts ...httptrace.RoundTr
 		span.SetTag("kubernetes.audit_id", kubeAuditID)
 	}))
 	log.Debug("contrib/k8s.io/client-go/kubernetes: Wrapping RoundTripper.")
-	return httptrace.WrapRoundTripper(rt, opts...)
+	return httptrace.WrapRoundTripper(rt, localOpts...)
 }
 
 // RequestToResource parses a Kubernetes request and extracts a resource name from it.

contrib/labstack/echo.v4/echotrace.go

@@ -13,6 +13,7 @@ import (
 	"strconv"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	"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"
@@ -61,12 +62,15 @@ 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))
-
+			opts := options.Copy(spanOpts...) // opts must be a copy of spanOpts, locally scoped, to avoid races.
 			if !math.IsNaN(cfg.analyticsRate) {
 				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 			}
-			opts = append(opts, httptrace.HeaderTagsFromRequest(request, cfg.headerTags))
+			opts = append(opts,
+				tracer.ResourceName(resource),
+				tracer.Tag(ext.HTTPRoute, route),
+				httptrace.HeaderTagsFromRequest(request, cfg.headerTags))
+
 			var finishOpts []tracer.FinishOption
 			if cfg.noDebugStack {
 				finishOpts = []tracer.FinishOption{tracer.NoDebugStack()}

contrib/labstack/echo/echotrace.go

@@ -17,6 +17,7 @@ import (
 	"strconv"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	"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"
@@ -50,20 +51,22 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
 		return func(c echo.Context) error {
 			request := c.Request()
 			resource := request.Method + " " + c.Path()
-			opts := append(spanOpts, tracer.ResourceName(resource))
-
+			opts := options.Copy(spanOpts...) // opts must be a copy of spanOpts, locally scoped, to avoid races.
 			if !math.IsNaN(cfg.analyticsRate) {
 				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 			}
-			opts = append(opts, httptrace.HeaderTagsFromRequest(request, cfg.headerTags))
+			opts = append(opts,
+				tracer.ResourceName(resource),
+				httptrace.HeaderTagsFromRequest(request, cfg.headerTags))
+			// TODO: Should this also have an `http.route` tag like the v4 library does?
+
 			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/net/http/trace.go

@@ -11,6 +11,7 @@ import (
 	"net/http"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
 	"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"
@@ -53,7 +54,7 @@ func TraceAndServe(h http.Handler, w http.ResponseWriter, r *http.Request, cfg *
 	if cfg == nil {
 		cfg = new(ServeConfig)
 	}
-	opts := cfg.SpanOpts
+	opts := options.Copy(cfg.SpanOpts...) // make a copy of cfg.SpanOpts to avoid races
 	if cfg.Service != "" {
 		opts = append(opts, tracer.ServiceName(cfg.Service))
 	}