diff --git a/modules/caddyhttp/tracing/tracer.go b/modules/caddyhttp/tracing/tracer.go index 5d71059ed..ceeacbf0e 100644 --- a/modules/caddyhttp/tracing/tracer.go +++ b/modules/caddyhttp/tracing/tracer.go @@ -62,17 +62,27 @@ func newOpenTelemetryWrapper( return ot, fmt.Errorf("creating resource error: %w", err) } - traceExporter, err := autoexport.NewSpanExporter(ctx) - if err != nil { - return ot, fmt.Errorf("creating trace exporter error: %w", err) - } - ot.propagators = autoprop.NewTextMapPropagator() - tracerProvider := globalTracerProvider.getTracerProvider( - sdktrace.WithBatcher(traceExporter), - sdktrace.WithResource(res), - ) + // Defer creation of the exporter (and its batch span processor goroutine) + // until we know a new provider is actually needed. When the global provider + // already exists it is reused and these options are discarded; building them + // here unconditionally would leak the exporter and a BatchSpanProcessor + // goroutine on every config reload. + tracerProvider, err := globalTracerProvider.getTracerProvider(func() ([]sdktrace.TracerProviderOption, error) { + traceExporter, err := autoexport.NewSpanExporter(ctx) + if err != nil { + return nil, fmt.Errorf("creating trace exporter error: %w", err) + } + + return []sdktrace.TracerProviderOption{ + sdktrace.WithBatcher(traceExporter), + sdktrace.WithResource(res), + }, nil + }) + if err != nil { + return ot, err + } ot.handler = otelhttp.NewHandler(http.HandlerFunc(ot.serveHTTP), ot.spanName, diff --git a/modules/caddyhttp/tracing/tracerprovider.go b/modules/caddyhttp/tracing/tracerprovider.go index 0a3766974..451202769 100644 --- a/modules/caddyhttp/tracing/tracerprovider.go +++ b/modules/caddyhttp/tracing/tracerprovider.go @@ -19,20 +19,30 @@ type tracerProvider struct { tracerProvidersCounter int } -// getTracerProvider create or return an existing global TracerProvider -func (t *tracerProvider) getTracerProvider(opts ...sdktrace.TracerProviderOption) *sdktrace.TracerProvider { +// getTracerProvider create or return an existing global TracerProvider. +// +// buildOpts is only invoked when a new provider must actually be created. +// This matters because some options (notably sdktrace.WithBatcher) eagerly +// start background goroutines when constructed. Building them unconditionally +// and then discarding them on the reuse path would leak a BatchSpanProcessor +// goroutine on every config reload. +func (t *tracerProvider) getTracerProvider(buildOpts func() ([]sdktrace.TracerProviderOption, error)) (*sdktrace.TracerProvider, error) { t.mu.Lock() defer t.mu.Unlock() - t.tracerProvidersCounter++ - if t.tracerProvider == nil { + opts, err := buildOpts() + if err != nil { + return nil, err + } t.tracerProvider = sdktrace.NewTracerProvider( opts..., ) } - return t.tracerProvider + t.tracerProvidersCounter++ + + return t.tracerProvider, nil } // cleanupTracerProvider gracefully shutdown a TracerProvider diff --git a/modules/caddyhttp/tracing/tracerprovider_test.go b/modules/caddyhttp/tracing/tracerprovider_test.go index 5a5df0a23..80123f261 100644 --- a/modules/caddyhttp/tracing/tracerprovider_test.go +++ b/modules/caddyhttp/tracing/tracerprovider_test.go @@ -3,14 +3,19 @@ package tracing import ( "testing" + sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.uber.org/zap" ) +func noOpts() ([]sdktrace.TracerProviderOption, error) { + return nil, nil +} + func Test_tracersProvider_getTracerProvider(t *testing.T) { tp := tracerProvider{} - tp.getTracerProvider() - tp.getTracerProvider() + _, _ = tp.getTracerProvider(noOpts) + _, _ = tp.getTracerProvider(noOpts) if tp.tracerProvider == nil { t.Errorf("There should be tracer provider") @@ -24,8 +29,8 @@ func Test_tracersProvider_getTracerProvider(t *testing.T) { func Test_tracersProvider_cleanupTracerProvider(t *testing.T) { tp := tracerProvider{} - tp.getTracerProvider() - tp.getTracerProvider() + _, _ = tp.getTracerProvider(noOpts) + _, _ = tp.getTracerProvider(noOpts) err := tp.cleanupTracerProvider(zap.NewNop()) if err != nil { @@ -40,3 +45,32 @@ func Test_tracersProvider_cleanupTracerProvider(t *testing.T) { t.Errorf("Tracer providers counter should equal to 1") } } + +// Test_tracersProvider_buildOptsOnlyOnCreate guards against a goroutine leak on +// config reload: buildOpts must be invoked only when a new provider is actually +// created, never on the reuse path. Some options (e.g. sdktrace.WithBatcher) +// eagerly start a BatchSpanProcessor goroutine when constructed, so calling +// buildOpts on every reload would leak one goroutine per reload. +func Test_tracersProvider_buildOptsOnlyOnCreate(t *testing.T) { + tp := tracerProvider{} + + builds := 0 + build := func() ([]sdktrace.TracerProviderOption, error) { + builds++ + return nil, nil + } + + // First call creates the provider and must build options. + _, _ = tp.getTracerProvider(build) + // Subsequent calls reuse the existing provider and must NOT build options again. + for i := 0; i < 5; i++ { + _, _ = tp.getTracerProvider(build) + } + + if builds != 1 { + t.Errorf("buildOpts should be invoked exactly once (on create), got %d", builds) + } + if tp.tracerProvidersCounter != 6 { + t.Errorf("Tracer providers counter should equal to 6, got %d", tp.tracerProvidersCounter) + } +}