From 384910f77f660e854676330bfa57abe5af8c2879 Mon Sep 17 00:00:00 2001 From: Chen Xu Date: Thu, 13 Apr 2023 13:36:53 -0700 Subject: [PATCH] [opentracing bridge] support setting span kind tag after the creation of a span Signed-off-by: Chen Xu --- CHANGELOG.md | 1 + bridge/opentracing/bridge.go | 8 +++- bridge/opentracing/bridge_test.go | 58 ++++++++++++++++++++++++++++- bridge/opentracing/internal/mock.go | 8 +++- bridge/opentracing/mix_test.go | 4 +- 5 files changed, 73 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 952beab76c23..593f058d9c31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `metric.NewNoopMeterProvider` is replaced with `noop.NewMeterProvider` - Wrap `UploadMetrics` error in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/` to improve error message when encountering generic grpc errors. (#3974) - Move global metric back to `go.opentelemetry.io/otel/metric/global` from `go.opentelemetry.io/otel`. (#3986) +- Allow to set span kind tag after a span is created in OpenTracing bridge. ### Fixed diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 867a2a8c387a..650283070d02 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -158,7 +158,13 @@ func (s *bridgeSpan) SetOperationName(operationName string) ot.Span { func (s *bridgeSpan) SetTag(key string, value interface{}) ot.Span { switch key { case string(otext.SpanKind): - // TODO: Should we ignore it? + type readSpanKindSpan interface { + SpanKind() trace.SpanKind + } + // only add span kind tag when otel span kind is not set (internal) + if span, ok := s.otelSpan.(readSpanKindSpan); ok && span.SpanKind() == trace.SpanKindInternal { + s.otelSpan.SetAttributes(otTagToOTelAttr(key, value)) + } case string(otext.Error): if b, ok := value.(bool); ok && b { s.otelSpan.SetStatus(codes.Error, "") diff --git a/bridge/opentracing/bridge_test.go b/bridge/opentracing/bridge_test.go index 20c21a875650..98e613c9123e 100644 --- a/bridge/opentracing/bridge_test.go +++ b/bridge/opentracing/bridge_test.go @@ -510,7 +510,7 @@ func Test_otTagsToOTelAttributesKindAndError(t *testing.T) { b, _ := NewTracerPair(tracer) s := b.StartSpan(tc.name, tc.opt...) - assert.Equal(t, s.(*bridgeSpan).otelSpan.(*internal.MockSpan).SpanKind, tc.expected) + assert.Equal(t, s.(*bridgeSpan).otelSpan.(*internal.MockSpan).SpanKind(), tc.expected) }) } } @@ -546,3 +546,59 @@ func TestBridge_SpanContext_IsSampled(t *testing.T) { }) } } + +func TestBridgeSpan_SetSpanKindTag(t *testing.T) { + testCases := []struct { + name string + inputSpanKind interface{} + setSpanKind interface{} + expectedAttributes []attribute.KeyValue + }{ + { + name: "set span kind tag to client for OTel span with internal span kind", + inputSpanKind: nil, + setSpanKind: ext.SpanKindRPCClientEnum, + expectedAttributes: []attribute.KeyValue{attribute.String(string(ext.SpanKind), string(ext.SpanKindRPCClientEnum))}, + }, + { + name: "set span kind tag to server for OTel span with internal span kind", + inputSpanKind: nil, + setSpanKind: ext.SpanKindRPCServerEnum, + expectedAttributes: []attribute.KeyValue{attribute.String(string(ext.SpanKind), string(ext.SpanKindRPCServerEnum))}, + }, + { + name: "allow to set span kind tag to non-standard span kind", + inputSpanKind: nil, + setSpanKind: "non-standard span kind", + expectedAttributes: []attribute.KeyValue{attribute.String(string(ext.SpanKind), "non-standard span kind")}, + }, + { + name: "set span kind tag to client for OTel span with non-internal span kind", + inputSpanKind: ext.SpanKindRPCServerEnum, + setSpanKind: ext.SpanKindRPCClientEnum, + expectedAttributes: nil, + }, + { + name: "set span kind tag to server for OTel span with non-internal span kind", + inputSpanKind: ext.SpanKindRPCClientEnum, + setSpanKind: ext.SpanKindRPCServerEnum, + expectedAttributes: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + otelMockTracer := internal.NewMockTracer() + bridgeTracer, _ := NewTracerPair(otelMockTracer) + startOption := ot.Tags{} + if tc.inputSpanKind != nil { + startOption = ot.Tags{string(ext.SpanKind): tc.inputSpanKind} + } + span := bridgeTracer.StartSpan("abc", startOption) + span.SetTag(string(ext.SpanKind), tc.setSpanKind) + mockSpan := span.(*bridgeSpan).otelSpan.(*internal.MockSpan) + + assert.Equal(t, tc.expectedAttributes, mockSpan.Attributes) + }) + } +} diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index bb44e93d1f34..dedd42fdb27f 100644 --- a/bridge/opentracing/internal/mock.go +++ b/bridge/opentracing/internal/mock.go @@ -88,7 +88,7 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...trace.SpanS EndTime: time.Time{}, ParentSpanID: t.getParentSpanID(ctx, &config), Events: nil, - SpanKind: trace.ValidateSpanKind(config.SpanKind()), + spanKind: trace.ValidateSpanKind(config.SpanKind()), } if !migration.SkipContextSetup(ctx) { ctx = trace.ContextWithSpan(ctx, span) @@ -185,7 +185,7 @@ type MockSpan struct { mockTracer *MockTracer officialTracer trace.Tracer spanContext trace.SpanContext - SpanKind trace.SpanKind + spanKind trace.SpanKind recording bool Attributes []attribute.KeyValue @@ -292,3 +292,7 @@ func (s *MockSpan) OverrideTracer(tracer trace.Tracer) { } func (s *MockSpan) TracerProvider() trace.TracerProvider { return trace.NewNoopTracerProvider() } + +func (s *MockSpan) SpanKind() trace.SpanKind { + return s.spanKind +} diff --git a/bridge/opentracing/mix_test.go b/bridge/opentracing/mix_test.go index 0cd5a667ca5c..7eae5681962e 100644 --- a/bridge/opentracing/mix_test.go +++ b/bridge/opentracing/mix_test.go @@ -583,8 +583,8 @@ func checkTraceAndSpans(t *testing.T, tracer *internal.MockTracer, expectedTrace if span.ParentSpanID != expectedParentSpanID { t.Errorf("Expected finished span %d (span ID: %d) to have parent span ID %d, but got %d", idx, sctx.SpanID(), expectedParentSpanID, span.ParentSpanID) } - if span.SpanKind != sks[span.SpanContext().SpanID()] { - t.Errorf("Expected finished span %d (span ID: %d) to have span.kind to be '%v' but was '%v'", idx, sctx.SpanID(), sks[span.SpanContext().SpanID()], span.SpanKind) + if span.SpanKind() != sks[span.SpanContext().SpanID()] { + t.Errorf("Expected finished span %d (span ID: %d) to have span.kind to be '%v' but was '%v'", idx, sctx.SpanID(), sks[span.SpanContext().SpanID()], span.SpanKind()) } } }