From 1d2a936cfdf429519b396f3d9c8715d54d79dfaf Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Wed, 24 Aug 2022 21:26:46 -0600 Subject: [PATCH 01/13] Update tracer to guard for a nil ctx --- sdk/trace/tracer.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index f4a1f96f3d6..b0c0e6ef361 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -37,6 +37,11 @@ var _ trace.Tracer = &tracer{} func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanStartOption) (context.Context, trace.Span) { config := trace.NewSpanStartConfig(options...) + // If ctx is nil, set to context.Background() as context.WithValue will panic on a nil value. + if ctx == nil { + ctx = context.Background() + } + // For local spans created by this SDK, track child span count. if p := trace.SpanFromContext(ctx); p != nil { if sdkSpan, ok := p.(*recordingSpan); ok { From 97d898cc8513989baf26bea6b1781405c2140111 Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Wed, 24 Aug 2022 21:37:12 -0600 Subject: [PATCH 02/13] add test --- sdk/trace/trace_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 8d95782115b..7be91632591 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -363,6 +363,21 @@ func TestStartSpanWithParent(t *testing.T) { } } +// Test we get a successful span as a new root if a nil context is sent in, as opposed to a panic. +// See https://github.com/open-telemetry/opentelemetry-go/issues/3109 +func TestStartSpanWithNilContext(t *testing.T) { + alwaysSampleTp := NewTracerProvider() + sampledTr := alwaysSampleTp.Tracer("AlwaysSampled") + + defer func() { + if r := recover(); r != nil { + t.Error("unexpected panic creating span with nil context") + } + }() + + _, _ := sampledTr.Start(nil, "should-not-panic") +} + func TestStartSpanNewRootNotSampled(t *testing.T) { alwaysSampleTp := NewTracerProvider() sampledTr := alwaysSampleTp.Tracer("AlwaysSampled") @@ -385,6 +400,9 @@ func TestStartSpanNewRootNotSampled(t *testing.T) { if s3.SpanContext().IsSampled() { t.Error(fmt.Errorf("got child span is sampled, want child span WithNewRoot() and with sampler: ParentBased(NeverSample()) to not be sampled")) } + + // Test we get a successful span as a new root if a nil context is sent in + _, s4 := neverSampledTr.Start(nil, "span3-new-root", trace.WithNewRoot()) } func TestSetSpanAttributesOnStart(t *testing.T) { From e9d0b880210bf2a83d46fcf801193758a9a919ed Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Wed, 24 Aug 2022 21:41:47 -0600 Subject: [PATCH 03/13] undo first attempt --- sdk/trace/trace_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 7be91632591..3e86fab92de 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -400,9 +400,6 @@ func TestStartSpanNewRootNotSampled(t *testing.T) { if s3.SpanContext().IsSampled() { t.Error(fmt.Errorf("got child span is sampled, want child span WithNewRoot() and with sampler: ParentBased(NeverSample()) to not be sampled")) } - - // Test we get a successful span as a new root if a nil context is sent in - _, s4 := neverSampledTr.Start(nil, "span3-new-root", trace.WithNewRoot()) } func TestSetSpanAttributesOnStart(t *testing.T) { From 80c84b064963cd33a9bf2fa1089d426a95d50ce3 Mon Sep 17 00:00:00 2001 From: Chester Cheung Date: Thu, 25 Aug 2022 11:43:53 +0800 Subject: [PATCH 04/13] Update sdk/trace/trace_test.go --- sdk/trace/trace_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 3e86fab92de..270088d1596 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -375,7 +375,7 @@ func TestStartSpanWithNilContext(t *testing.T) { } }() - _, _ := sampledTr.Start(nil, "should-not-panic") + _, _ = sampledTr.Start(nil, "should-not-panic") } func TestStartSpanNewRootNotSampled(t *testing.T) { From f7471f79e3e50ccb80795f46bc0555ad1f51c7e4 Mon Sep 17 00:00:00 2001 From: Chester Cheung Date: Thu, 25 Aug 2022 11:47:55 +0800 Subject: [PATCH 05/13] Update sdk/trace/trace_test.go --- sdk/trace/trace_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 270088d1596..766290cbc05 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -374,8 +374,7 @@ func TestStartSpanWithNilContext(t *testing.T) { t.Error("unexpected panic creating span with nil context") } }() - - _, _ = sampledTr.Start(nil, "should-not-panic") + _, _ = sampledTr.Start(context.TODO(), "should-not-panic") } func TestStartSpanNewRootNotSampled(t *testing.T) { From 2d82497be86ffd65ac66fee69f50da6ecc21c7f9 Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Wed, 24 Aug 2022 21:56:44 -0600 Subject: [PATCH 06/13] Update trace_test.go --- sdk/trace/trace_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 766290cbc05..5081843a13c 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -363,18 +363,19 @@ func TestStartSpanWithParent(t *testing.T) { } } -// Test we get a successful span as a new root if a nil context is sent in, as opposed to a panic. +// Test we get a successful span as a new root if a nil context is sent in, as opposed to a panic. // See https://github.com/open-telemetry/opentelemetry-go/issues/3109 func TestStartSpanWithNilContext(t *testing.T) { alwaysSampleTp := NewTracerProvider() sampledTr := alwaysSampleTp.Tracer("AlwaysSampled") defer func() { - if r := recover(); r != nil { - t.Error("unexpected panic creating span with nil context") - } - }() - _, _ = sampledTr.Start(context.TODO(), "should-not-panic") + if r := recover(); r != nil { + t.Error("unexpected panic creating span with nil context") + } + }() + //nolint (no nil context, but that's the point of the test) + sampledTr.Start(nil, "should-not-panic") } func TestStartSpanNewRootNotSampled(t *testing.T) { From 29eddb43a3b728884cd078d8f7e5b9ef9f8222d8 Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Wed, 24 Aug 2022 22:01:26 -0600 Subject: [PATCH 07/13] Update comment in nolint directive --- sdk/trace/trace_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 5081843a13c..004f0481ef5 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -374,7 +374,7 @@ func TestStartSpanWithNilContext(t *testing.T) { t.Error("unexpected panic creating span with nil context") } }() - //nolint (no nil context, but that's the point of the test) + //nolint // no nil context, but that's the point of the test sampledTr.Start(nil, "should-not-panic") } From 8e08be97008d1494ddecfb83d8c67fbee64db5fd Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Thu, 25 Aug 2022 10:32:59 -0600 Subject: [PATCH 08/13] Update sdk/trace/trace_test.go Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> --- sdk/trace/trace_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 004f0481ef5..767c9362e49 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -374,7 +374,7 @@ func TestStartSpanWithNilContext(t *testing.T) { t.Error("unexpected panic creating span with nil context") } }() - //nolint // no nil context, but that's the point of the test + //nolint:staticcheck // no nil context, but that's the point of the test sampledTr.Start(nil, "should-not-panic") } From 477c873e90536722002cdef7d0d6a560f5bfe104 Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Thu, 25 Aug 2022 10:35:13 -0600 Subject: [PATCH 09/13] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 508b6b30baa..20ab5756ab5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The exponential histogram mapping functions have been updated with exact upper-inclusive boundary support following the [corresponding specification change](https://github.com/open-telemetry/opentelemetry-specification/pull/2633). (#2982) +- Attempting to start a span with a nil `context` will no longer cause a panic. ## [1.9.0/0.0.3] - 2022-08-01 From 5b14ee4b00d4e4702058d8abf42071fe78510937 Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Thu, 25 Aug 2022 10:38:12 -0600 Subject: [PATCH 10/13] Make var names better --- sdk/trace/trace_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 767c9362e49..4e7cfd13b26 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -366,8 +366,8 @@ func TestStartSpanWithParent(t *testing.T) { // Test we get a successful span as a new root if a nil context is sent in, as opposed to a panic. // See https://github.com/open-telemetry/opentelemetry-go/issues/3109 func TestStartSpanWithNilContext(t *testing.T) { - alwaysSampleTp := NewTracerProvider() - sampledTr := alwaysSampleTp.Tracer("AlwaysSampled") + tp := NewTracerProvider() + tr := tp.Tracer("NoPanic") defer func() { if r := recover(); r != nil { @@ -375,7 +375,7 @@ func TestStartSpanWithNilContext(t *testing.T) { } }() //nolint:staticcheck // no nil context, but that's the point of the test - sampledTr.Start(nil, "should-not-panic") + tr.Start(nil, "should-not-panic") } func TestStartSpanNewRootNotSampled(t *testing.T) { From 272441e1fa4af2ad027c1f571972196700baae74 Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Thu, 25 Aug 2022 10:55:18 -0600 Subject: [PATCH 11/13] add PR ref to changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20ab5756ab5..0a044b0366a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The exponential histogram mapping functions have been updated with exact upper-inclusive boundary support following the [corresponding specification change](https://github.com/open-telemetry/opentelemetry-specification/pull/2633). (#2982) -- Attempting to start a span with a nil `context` will no longer cause a panic. +- Attempting to start a span with a nil `context` will no longer cause a panic. (#3110) ## [1.9.0/0.0.3] - 2022-08-01 From 80dcd384d82936f50f1508cccf89af9aa22c2fcb Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Thu, 25 Aug 2022 14:55:35 -0600 Subject: [PATCH 12/13] Update sdk/trace/trace_test.go Co-authored-by: Tyler Yahn --- sdk/trace/trace_test.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 4e7cfd13b26..c6adbb77818 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -369,13 +369,8 @@ func TestStartSpanWithNilContext(t *testing.T) { tp := NewTracerProvider() tr := tp.Tracer("NoPanic") - defer func() { - if r := recover(); r != nil { - t.Error("unexpected panic creating span with nil context") - } - }() - //nolint:staticcheck // no nil context, but that's the point of the test - tr.Start(nil, "should-not-panic") + // nolint:staticcheck // no nil context, but that's the point of the test. + assert.NotPanics(t, func() { tr.Start(nil, "should-not-panic") }) } func TestStartSpanNewRootNotSampled(t *testing.T) { From c897c6c26ca7935d8b3327eee63c158c00cb06d7 Mon Sep 17 00:00:00 2001 From: Mitch Usher Date: Thu, 25 Aug 2022 14:55:47 -0600 Subject: [PATCH 13/13] Update sdk/trace/tracer.go Co-authored-by: Tyler Yahn --- sdk/trace/tracer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index b0c0e6ef361..7b11fc465c6 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -37,8 +37,8 @@ var _ trace.Tracer = &tracer{} func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanStartOption) (context.Context, trace.Span) { config := trace.NewSpanStartConfig(options...) - // If ctx is nil, set to context.Background() as context.WithValue will panic on a nil value. if ctx == nil { + // Prevent trace.ContextWithSpan from panicking. ctx = context.Background() }