From 0f7757a5dc8bca0ab35b1120b7ed2fa118b48103 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 25 Feb 2025 08:49:36 +0100 Subject: [PATCH 1/4] sdk/trace: Remove goroutine leak in simpleSpanProcessor.Shutdown --- CHANGELOG.md | 4 ++++ sdk/go.mod | 1 + sdk/go.sum | 2 ++ sdk/trace/batch_span_processor_test.go | 12 ++++++++++-- sdk/trace/main_test.go | 14 ++++++++++++++ sdk/trace/simple_span_processor.go | 2 +- sdk/trace/util_test.go | 8 +++++++- 7 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 sdk/trace/main_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d56b7daafd..f774d590fec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,10 @@ The next release will require at least [Go 1.23]. - Update `github.com/prometheus/common` to v0.62.0., which changes the `NameValidationScheme` to `NoEscaping`. This allows metrics names to keep original delimiters (e.g. `.`), rather than replacing with underscores. This is controlled by the `Content-Type` header, or can be reverted by setting `NameValidationScheme` to `LegacyValidation` in `github.com/prometheus/common/model`. (#6198) +### Fixes + +- Eliminate goroutine leak for the processor returned by `NewSimpleSpanProcessor` when `Shutdown` is called and the passed `ctx` is canceled and `SpanExporter.Shutdown` has not returned. (#TODO) + diff --git a/sdk/go.mod b/sdk/go.mod index 71b75bbe10f..a69019fef56 100644 --- a/sdk/go.mod +++ b/sdk/go.mod @@ -11,6 +11,7 @@ require ( github.com/stretchr/testify v1.10.0 go.opentelemetry.io/otel v1.34.0 go.opentelemetry.io/otel/trace v1.34.0 + go.uber.org/goleak v1.3.0 golang.org/x/sys v0.30.0 ) diff --git a/sdk/go.sum b/sdk/go.sum index 5039c1bdac3..253d7356f91 100644 --- a/sdk/go.sum +++ b/sdk/go.sum @@ -21,6 +21,8 @@ github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOf github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= go.opentelemetry.io/auto/sdk v1.1.0 h1:cH53jehLUN6UFLY71z+NDOiNJqDdPRaXzTel0sJySYA= go.opentelemetry.io/auto/sdk v1.1.0/go.mod h1:3wSPjt5PWp2RhlCcmmOial7AvC4DQqZb7a7wCow3W8A= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= golang.org/x/sys v0.30.0 h1:QjkSwP/36a20jFYWkSue1YwXzLmsV5Gfq7Eiy72C1uc= golang.org/x/sys v0.30.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/sdk/trace/batch_span_processor_test.go b/sdk/trace/batch_span_processor_test.go index 786c102628c..0a3f053534a 100644 --- a/sdk/trace/batch_span_processor_test.go +++ b/sdk/trace/batch_span_processor_test.go @@ -12,8 +12,6 @@ import ( "testing" "time" - ottest "go.opentelemetry.io/otel/sdk/internal/internaltest" - "github.com/go-logr/logr" "github.com/go-logr/logr/funcr" "github.com/stretchr/testify/assert" @@ -21,6 +19,7 @@ import ( "go.opentelemetry.io/otel/internal/global" "go.opentelemetry.io/otel/sdk/internal/env" + ottest "go.opentelemetry.io/otel/sdk/internal/internaltest" sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" "go.opentelemetry.io/otel/trace" @@ -544,6 +543,9 @@ func TestBatchSpanProcessorForceFlushCancellation(t *testing.T) { cancel() bsp := sdktrace.NewBatchSpanProcessor(indefiniteExporter{}) + t.Cleanup(func() { + assert.NoError(t, bsp.Shutdown(context.Background())) + }) if got, want := bsp.ForceFlush(ctx), context.Canceled; !errors.Is(got, want) { t.Errorf("expected %q error, got %v", want, got) } @@ -556,6 +558,9 @@ func TestBatchSpanProcessorForceFlushTimeout(t *testing.T) { <-ctx.Done() bsp := sdktrace.NewBatchSpanProcessor(indefiniteExporter{}) + t.Cleanup(func() { + assert.NoError(t, bsp.Shutdown(context.Background())) + }) if got, want := bsp.ForceFlush(ctx), context.DeadlineExceeded; !errors.Is(got, want) { t.Errorf("expected %q error, got %v", want, got) } @@ -569,6 +574,9 @@ func TestBatchSpanProcessorForceFlushQueuedSpans(t *testing.T) { tp := sdktrace.NewTracerProvider( sdktrace.WithBatcher(exp), ) + t.Cleanup(func() { + assert.NoError(t, tp.Shutdown(context.Background())) + }) tracer := tp.Tracer("tracer") diff --git a/sdk/trace/main_test.go b/sdk/trace/main_test.go new file mode 100644 index 00000000000..ac6d5ff970c --- /dev/null +++ b/sdk/trace/main_test.go @@ -0,0 +1,14 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package trace + +import ( + "testing" + + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} diff --git a/sdk/trace/simple_span_processor.go b/sdk/trace/simple_span_processor.go index 554111bb4a2..664e13e03f0 100644 --- a/sdk/trace/simple_span_processor.go +++ b/sdk/trace/simple_span_processor.go @@ -58,7 +58,7 @@ func (ssp *simpleSpanProcessor) Shutdown(ctx context.Context) error { var err error ssp.stopOnce.Do(func() { stopFunc := func(exp SpanExporter) (<-chan error, func()) { - done := make(chan error) + done := make(chan error, 1) return done, func() { done <- exp.Shutdown(ctx) } } diff --git a/sdk/trace/util_test.go b/sdk/trace/util_test.go index 6ae195977a2..c058a5e8312 100644 --- a/sdk/trace/util_test.go +++ b/sdk/trace/util_test.go @@ -4,12 +4,18 @@ package trace_test import ( + "context" "testing" + "github.com/stretchr/testify/assert" + sdktrace "go.opentelemetry.io/otel/sdk/trace" ) -func basicTracerProvider(_ *testing.T) *sdktrace.TracerProvider { +func basicTracerProvider(t *testing.T) *sdktrace.TracerProvider { tp := sdktrace.NewTracerProvider(sdktrace.WithSampler(sdktrace.AlwaysSample())) + t.Cleanup(func() { + assert.NoError(t, tp.Shutdown(context.Background())) + }) return tp } From e12c7663b34ed4045acbf5ff7c98714aee7a0a5e Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 25 Feb 2025 08:50:52 +0100 Subject: [PATCH 2/4] Update changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f774d590fec..4b4eef8beb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ The next release will require at least [Go 1.23]. ### Fixes -- Eliminate goroutine leak for the processor returned by `NewSimpleSpanProcessor` when `Shutdown` is called and the passed `ctx` is canceled and `SpanExporter.Shutdown` has not returned. (#TODO) +- Eliminate goroutine leak for the processor returned by `NewSimpleSpanProcessor` when `Shutdown` is called and the passed `ctx` is canceled and `SpanExporter.Shutdown` has not returned. (#6368) From c9f3c4a5d04d0616629d725367b7ed7f6a246ca3 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 25 Feb 2025 08:56:24 +0100 Subject: [PATCH 3/4] go mod tidy --- bridge/opencensus/go.sum | 2 ++ bridge/opencensus/test/go.sum | 2 ++ exporters/otlp/otlptrace/go.sum | 2 ++ exporters/otlp/otlptrace/otlptracehttp/go.sum | 2 ++ exporters/stdout/stdouttrace/go.sum | 2 ++ exporters/zipkin/go.sum | 2 ++ 6 files changed, 12 insertions(+) diff --git a/bridge/opencensus/go.sum b/bridge/opencensus/go.sum index 873b22e8d25..cc35b7ae6a4 100644 --- a/bridge/opencensus/go.sum +++ b/bridge/opencensus/go.sum @@ -61,6 +61,8 @@ go.opencensus.io v0.24.0 h1:y73uSU6J157QMP2kn2r30vwW1A2W2WFwSCGnAVxeaD0= go.opencensus.io v0.24.0/go.mod h1:vNK8G9p7aAivkbmorf4v+7Hgx+Zs0yY+0fOtgBfjQKo= go.opentelemetry.io/auto/sdk v1.1.0 h1:cH53jehLUN6UFLY71z+NDOiNJqDdPRaXzTel0sJySYA= go.opentelemetry.io/auto/sdk v1.1.0/go.mod h1:3wSPjt5PWp2RhlCcmmOial7AvC4DQqZb7a7wCow3W8A= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= diff --git a/bridge/opencensus/test/go.sum b/bridge/opencensus/test/go.sum index 2321f1f9f35..919c8ad9d69 100644 --- a/bridge/opencensus/test/go.sum +++ b/bridge/opencensus/test/go.sum @@ -55,6 +55,8 @@ go.opencensus.io v0.24.0 h1:y73uSU6J157QMP2kn2r30vwW1A2W2WFwSCGnAVxeaD0= go.opencensus.io v0.24.0/go.mod h1:vNK8G9p7aAivkbmorf4v+7Hgx+Zs0yY+0fOtgBfjQKo= go.opentelemetry.io/auto/sdk v1.1.0 h1:cH53jehLUN6UFLY71z+NDOiNJqDdPRaXzTel0sJySYA= go.opentelemetry.io/auto/sdk v1.1.0/go.mod h1:3wSPjt5PWp2RhlCcmmOial7AvC4DQqZb7a7wCow3W8A= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= diff --git a/exporters/otlp/otlptrace/go.sum b/exporters/otlp/otlptrace/go.sum index 3734c7e2415..dd6901205ed 100644 --- a/exporters/otlp/otlptrace/go.sum +++ b/exporters/otlp/otlptrace/go.sum @@ -23,6 +23,8 @@ go.opentelemetry.io/auto/sdk v1.1.0 h1:cH53jehLUN6UFLY71z+NDOiNJqDdPRaXzTel0sJyS go.opentelemetry.io/auto/sdk v1.1.0/go.mod h1:3wSPjt5PWp2RhlCcmmOial7AvC4DQqZb7a7wCow3W8A= go.opentelemetry.io/proto/otlp v1.5.0 h1:xJvq7gMzB31/d406fB8U5CBdyQGw4P399D1aQWU/3i4= go.opentelemetry.io/proto/otlp v1.5.0/go.mod h1:keN8WnHxOy8PG0rQZjJJ5A2ebUoafqWp0eVQ4yIXvJ4= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= golang.org/x/sys v0.30.0 h1:QjkSwP/36a20jFYWkSue1YwXzLmsV5Gfq7Eiy72C1uc= golang.org/x/sys v0.30.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= google.golang.org/protobuf v1.36.5 h1:tPhr+woSbjfYvY6/GPufUoYizxw1cF/yFoxJ2fmpwlM= diff --git a/exporters/otlp/otlptrace/otlptracehttp/go.sum b/exporters/otlp/otlptrace/otlptracehttp/go.sum index f9131a9b70d..b48d22528b4 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/go.sum +++ b/exporters/otlp/otlptrace/otlptracehttp/go.sum @@ -31,6 +31,8 @@ go.opentelemetry.io/otel/sdk/metric v1.32.0 h1:rZvFnvmvawYb0alrYkjraqJq0Z4ZUJAiy go.opentelemetry.io/otel/sdk/metric v1.32.0/go.mod h1:PWeZlq0zt9YkYAp3gjKZ0eicRYvOh1Gd+X99x6GHpCQ= go.opentelemetry.io/proto/otlp v1.5.0 h1:xJvq7gMzB31/d406fB8U5CBdyQGw4P399D1aQWU/3i4= go.opentelemetry.io/proto/otlp v1.5.0/go.mod h1:keN8WnHxOy8PG0rQZjJJ5A2ebUoafqWp0eVQ4yIXvJ4= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= golang.org/x/net v0.35.0 h1:T5GQRQb2y08kTAByq9L4/bz8cipCdA8FbRTXewonqY8= golang.org/x/net v0.35.0/go.mod h1:EglIi67kWsHKlRzzVMUD93VMSWGFOMSZgxFjparz1Qk= golang.org/x/sys v0.30.0 h1:QjkSwP/36a20jFYWkSue1YwXzLmsV5Gfq7Eiy72C1uc= diff --git a/exporters/stdout/stdouttrace/go.sum b/exporters/stdout/stdouttrace/go.sum index 5039c1bdac3..253d7356f91 100644 --- a/exporters/stdout/stdouttrace/go.sum +++ b/exporters/stdout/stdouttrace/go.sum @@ -21,6 +21,8 @@ github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOf github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= go.opentelemetry.io/auto/sdk v1.1.0 h1:cH53jehLUN6UFLY71z+NDOiNJqDdPRaXzTel0sJySYA= go.opentelemetry.io/auto/sdk v1.1.0/go.mod h1:3wSPjt5PWp2RhlCcmmOial7AvC4DQqZb7a7wCow3W8A= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= golang.org/x/sys v0.30.0 h1:QjkSwP/36a20jFYWkSue1YwXzLmsV5Gfq7Eiy72C1uc= golang.org/x/sys v0.30.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/exporters/zipkin/go.sum b/exporters/zipkin/go.sum index 74ae9302f31..e2065a1ce37 100644 --- a/exporters/zipkin/go.sum +++ b/exporters/zipkin/go.sum @@ -23,6 +23,8 @@ github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOf github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= go.opentelemetry.io/auto/sdk v1.1.0 h1:cH53jehLUN6UFLY71z+NDOiNJqDdPRaXzTel0sJySYA= go.opentelemetry.io/auto/sdk v1.1.0/go.mod h1:3wSPjt5PWp2RhlCcmmOial7AvC4DQqZb7a7wCow3W8A= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= golang.org/x/sys v0.30.0 h1:QjkSwP/36a20jFYWkSue1YwXzLmsV5Gfq7Eiy72C1uc= golang.org/x/sys v0.30.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 95e266e3a512092aa869c825fa395a00dd6b3ba8 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 25 Feb 2025 09:00:56 +0100 Subject: [PATCH 4/4] Cleanup in benchmarks --- sdk/trace/batch_span_processor_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sdk/trace/batch_span_processor_test.go b/sdk/trace/batch_span_processor_test.go index 0a3f053534a..49575e841fa 100644 --- a/sdk/trace/batch_span_processor_test.go +++ b/sdk/trace/batch_span_processor_test.go @@ -649,6 +649,9 @@ func BenchmarkSpanProcessorOnEnd(b *testing.B) { tracetest.NewNoopExporter(), sdktrace.WithMaxExportBatchSize(bb.batchSize), ) + b.Cleanup(func() { + _ = bsp.Shutdown(context.Background()) + }) snap := tracetest.SpanStub{}.Snapshot() b.ResetTimer() @@ -673,6 +676,9 @@ func BenchmarkSpanProcessorVerboseLogging(b *testing.B) { tracetest.NewNoopExporter(), sdktrace.WithMaxExportBatchSize(10), )) + b.Cleanup(func() { + _ = tp.Shutdown(context.Background()) + }) tracer := tp.Tracer("bench") ctx := context.Background()