Add tests and fix opentracing bridge defer warning (#3029)

dmathieu · MrAlias · web-flow · commit 05c5509915fc · 2022-07-22T11:46:06.000-07:00
* add tests and fix opentracing bridge defer warning

* add changelog entry

* Update CHANGELOG.md

Co-authored-by: Tyler Yahn &lt;MrAlias@users.noreply.github.com&gt;

* Update bridge/opentracing/bridge_test.go

Co-authored-by: Tyler Yahn &lt;MrAlias@users.noreply.github.com&gt;

Co-authored-by: Tyler Yahn &lt;MrAlias@users.noreply.github.com&gt;
diff --git 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
diff --git 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
@@ -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)
+		})
+	}
+}
