From 9712bfa1ba17fd2de6a4eb4275f510f15727422e Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Tue, 19 Jul 2022 16:07:13 +0200 Subject: [PATCH 1/4] add tests and fix opentracing bridge defer warning --- bridge/opentracing/bridge.go | 5 ++- bridge/opentracing/bridge_test.go | 65 +++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 2927e86535a..2922b9869d5 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -328,7 +328,8 @@ var _ ot.TracerContextWithSpanExtension = &BridgeTracer{} func NewBridgeTracer() *BridgeTracer { return &BridgeTracer{ setTracer: bridgeSetTracer{ - otelTracer: noopTracer, + warningHandler: func(msg string) {}, + otelTracer: noopTracer, }, warningHandler: func(msg string) {}, propagator: nil, @@ -434,7 +435,7 @@ func (t *BridgeTracer) StartSpan(operationName string, opts ...ot.StartSpanOptio trace.WithLinks(links...), trace.WithSpanKind(kind), ) - if checkCtx != checkCtx2 { + if ot.SpanFromContext(checkCtx2) != nil { t.warnOnce.Do(func() { t.warningHandler("SDK should have deferred the context setup, see the documentation of go.opentelemetry.io/otel/bridge/opentracing/migration\n") }) diff --git a/bridge/opentracing/bridge_test.go b/bridge/opentracing/bridge_test.go index eea3091f265..8c39b67a009 100644 --- a/bridge/opentracing/bridge_test.go +++ b/bridge/opentracing/bridge_test.go @@ -24,6 +24,7 @@ import ( ot "github.com/opentracing/opentracing-go" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) @@ -360,3 +361,67 @@ func TestBridgeTracer_ExtractAndInject(t *testing.T) { }) } } + +type nonDeferWrapperTracer struct { + *WrapperTracer +} + +func (t *nonDeferWrapperTracer) Start(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { + // Run start on the parent wrapper with a brand new context + // So `WithDeferredSetup` hasn't been called, and the Opentracing context is injected + return t.WrapperTracer.Start(context.Background(), name, opts...) +} + +func TestBridgeTracer_StartSpan(t *testing.T) { + testCases := []struct { + name string + before func(*testing.T, *BridgeTracer) + expectWarnings []string + }{ + { + name: "with no option set", + expectWarnings: []string{ + "The OpenTelemetry tracer is not set, default no-op tracer is used! Call SetOpenTelemetryTracer to set it up.\n", + }, + }, + { + name: "with wrapper tracer set", + before: func(t *testing.T, bridge *BridgeTracer) { + wTracer := NewWrapperTracer(bridge, otel.Tracer("test")) + bridge.SetOpenTelemetryTracer(wTracer) + }, + expectWarnings: []string(nil), + }, + { + name: "with a non-defered wrapper tracer", + before: func(t *testing.T, bridge *BridgeTracer) { + wTracer := &nonDeferWrapperTracer{ + NewWrapperTracer(bridge, otel.Tracer("test")), + } + bridge.SetOpenTelemetryTracer(wTracer) + }, + expectWarnings: []string{ + "SDK should have deferred the context setup, see the documentation of go.opentelemetry.io/otel/bridge/opentracing/migration\n", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var warningMessages []string + bridge := NewBridgeTracer() + bridge.SetWarningHandler(func(msg string) { + warningMessages = append(warningMessages, msg) + }) + + if tc.before != nil { + tc.before(t, bridge) + } + + span := bridge.StartSpan("test") + assert.NotNil(t, span) + + assert.Equal(t, tc.expectWarnings, warningMessages) + }) + } +} From 40c01a8c22800e0ceb03b12b4a6b3d3964904ebc Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Tue, 19 Jul 2022 16:11:15 +0200 Subject: [PATCH 2/4] add changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21cef2402fb..fc385118765 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm The package contains semantic conventions from the `v1.11.0` version of the OpenTelemetry specification. (#3009) - Add http.method attribute to http server metric. (#3018) +### Fixed + +- Invalid warning for context setup being deferred in Opentracing bridge (#3029). + ## [1.8.0/0.31.0] - 2022-07-08 ### Added From 6efdca9f7c94bd7a3a6c7941519ffe533128a44e Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Fri, 22 Jul 2022 19:22:11 +0200 Subject: [PATCH 3/4] Update CHANGELOG.md Co-authored-by: Tyler Yahn --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc385118765..4bff4ee4bf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed -- Invalid warning for context setup being deferred in Opentracing bridge (#3029). +- Invalid warning for context setup being deferred in OpenTracing bridge (#3029). ## [1.8.0/0.31.0] - 2022-07-08 From 788bf01463adf60af6829077b7b094bcf1d35f29 Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Fri, 22 Jul 2022 19:22:21 +0200 Subject: [PATCH 4/4] Update bridge/opentracing/bridge_test.go Co-authored-by: Tyler Yahn --- bridge/opentracing/bridge_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bridge/opentracing/bridge_test.go b/bridge/opentracing/bridge_test.go index 8c39b67a009..a64d6f73919 100644 --- a/bridge/opentracing/bridge_test.go +++ b/bridge/opentracing/bridge_test.go @@ -368,7 +368,7 @@ type nonDeferWrapperTracer struct { func (t *nonDeferWrapperTracer) Start(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { // Run start on the parent wrapper with a brand new context - // So `WithDeferredSetup` hasn't been called, and the Opentracing context is injected + // so `WithDeferredSetup` hasn't been called, and the OpenTracing context is injected. return t.WrapperTracer.Start(context.Background(), name, opts...) }