From 2ef8a2ef5898b7a9c2d16e919c70f16981b6d237 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 28 Jul 2021 09:55:30 +0000 Subject: [PATCH 1/6] OpenTracing Bridge: allow more generic carriers Instead of insisting that the carrier is an HTTPHeaders, cast it or adapt it to the interface we need - TextMapCarrier. Signed-off-by: Bryan Boreham --- CHANGELOG.md | 4 +++ bridge/opentracing/bridge.go | 63 +++++++++++++++++++++++++++++++----- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4db1d233028..79334e486df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Added + +- OpenTracing Bridge: allow any OpenTracing propagation carrier. (#2141) + ### Fixed - Delegated instruments are unwrapped before delegating Callbacks. (#2784) diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 3e931b00e29..4007efe59e9 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -620,6 +620,45 @@ func (s fakeSpan) SpanContext() trace.SpanContext { return s.sc } +// adapt OpenTracing TextMapReader or TextMapWriter to OpenTelemetry propagation.TextMapCarrier +type textMapAdapter struct { + r ot.TextMapReader + w ot.TextMapWriter +} + +func (a textMapAdapter) Get(key string) string { + if a.r == nil { + return "" // not implemented on writer + } + var ret string + _ = a.r.ForeachKey(func(k, v string) error { + if key == k { + ret = v + } + return nil + }) + return ret +} + +func (a textMapAdapter) Set(key string, value string) { + if a.w == nil { + return // not implemented on reader + } + a.w.Set(key, value) +} + +func (a textMapAdapter) Keys() []string { + if a.r == nil { + return nil // not implemented on writer + } + var ret []string + _ = a.r.ForeachKey(func(k, v string) error { + ret = append(ret, k) + return nil + }) + return ret +} + // Inject is a part of the implementation of the OpenTracing Tracer // interface. // @@ -635,18 +674,22 @@ func (t *BridgeTracer) Inject(sm ot.SpanContext, format interface{}, carrier int if builtinFormat, ok := format.(ot.BuiltinFormat); !ok || builtinFormat != ot.HTTPHeaders { return ot.ErrUnsupportedFormat } - hhcarrier, ok := carrier.(ot.HTTPHeadersCarrier) + // If carrier implements the required interface directly, use that + tmCarrier, ok := carrier.(propagation.TextMapCarrier) if !ok { - return ot.ErrInvalidCarrier + tmWriter, ok := carrier.(ot.TextMapWriter) // otherwise see if we can wrap it + if !ok { + return ot.ErrInvalidCarrier + } + tmCarrier = textMapAdapter{w: tmWriter} } - header := http.Header(hhcarrier) fs := fakeSpan{ Span: noop.Span, sc: bridgeSC.otelSpanContext, } ctx := trace.ContextWithSpan(context.Background(), fs) ctx = baggage.ContextWithBaggage(ctx, bridgeSC.bag) - t.getPropagator().Inject(ctx, propagation.HeaderCarrier(header)) + t.getPropagator().Inject(ctx, tmCarrier) return nil } @@ -658,12 +701,16 @@ func (t *BridgeTracer) Extract(format interface{}, carrier interface{}) (ot.Span if builtinFormat, ok := format.(ot.BuiltinFormat); !ok || builtinFormat != ot.HTTPHeaders { return nil, ot.ErrUnsupportedFormat } - hhcarrier, ok := carrier.(ot.HTTPHeadersCarrier) + // If carrier implements the required interface directly, use that + tmCarrier, ok := carrier.(propagation.TextMapCarrier) if !ok { - return nil, ot.ErrInvalidCarrier + tmReader, ok := carrier.(ot.TextMapReader) // otherwise see if we can wrap it + if !ok { + return nil, ot.ErrInvalidCarrier + } + tmCarrier = textMapAdapter{r: tmReader} } - header := http.Header(hhcarrier) - ctx := t.getPropagator().Extract(context.Background(), propagation.HeaderCarrier(header)) + ctx := t.getPropagator().Extract(context.Background(), tmCarrier) baggage := baggage.FromContext(ctx) bridgeSC := &bridgeSpanContext{ bag: baggage, From 875cadf2c0545382b7efb889b750138ad1e594e6 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 16 Apr 2022 21:07:29 +0100 Subject: [PATCH 2/6] Improve comment Suggested by Tyler Yahn Signed-off-by: Bryan Boreham --- bridge/opentracing/bridge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 4007efe59e9..c759f68f672 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -620,7 +620,7 @@ func (s fakeSpan) SpanContext() trace.SpanContext { return s.sc } -// adapt OpenTracing TextMapReader or TextMapWriter to OpenTelemetry propagation.TextMapCarrier +// textMapAdapter adapts an OpenTracing TextMapReader or TextMapWriter to an OpenTelemetry propagation.TextMapCarrier. type textMapAdapter struct { r ot.TextMapReader w ot.TextMapWriter From 0e16e36c2e8849058f2496af3afa4e0ff14ce10d Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 18 Apr 2022 11:17:07 +0100 Subject: [PATCH 3/6] Add test for BridgeTracer Inject and Extract Signed-off-by: Bryan Boreham --- bridge/opentracing/bridge_test.go | 73 +++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 bridge/opentracing/bridge_test.go diff --git a/bridge/opentracing/bridge_test.go b/bridge/opentracing/bridge_test.go new file mode 100644 index 00000000000..4688e48a914 --- /dev/null +++ b/bridge/opentracing/bridge_test.go @@ -0,0 +1,73 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package opentracing + +import ( + "reflect" + "testing" + + ot "github.com/opentracing/opentracing-go" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/bridge/opentracing/internal" + "go.opentelemetry.io/otel/propagation" +) + +func TestMapCarrier(t *testing.T) { + carrier := propagation.MapCarrier{} + testBridgeWithCarrier(t, carrier) +} + +func TestHeaderCarrier(t *testing.T) { + carrier := propagation.HeaderCarrier{} + testBridgeWithCarrier(t, carrier) +} + +func TestOTHTTPHeadersCarrier(t *testing.T) { + carrier := ot.HTTPHeadersCarrier{} + testBridgeWithCarrier(t, carrier) +} + +func TestOTTextMapCarrier(t *testing.T) { + carrier := ot.TextMapCarrier{} + testBridgeWithCarrier(t, carrier) +} + +func testBridgeWithCarrier(t *testing.T, carrier interface{}) { + mockOtelTracer := internal.NewMockTracer() + bridgeTracer, _ := NewTracerPair(mockOtelTracer) + otel.SetTextMapPropagator(propagation.TraceContext{}) + + span := bridgeTracer.StartSpan("testSpan1") + defer span.Finish() + + err := bridgeTracer.Inject(span.Context(), ot.HTTPHeaders, carrier) + if err != nil { + t.Errorf("Inject error: %s", err) + } + + extractedSpanContext, err := bridgeTracer.Extract(ot.HTTPHeaders, carrier) + if err != nil { + t.Errorf("Extract error: %s", err) + } + + // Make a copy of the SpanContext with remote set to true + originalSpanContext := span.Context().(*bridgeSpanContext) + copySpanContext := *originalSpanContext + copySpanContext.otelSpanContext = copySpanContext.otelSpanContext.WithRemote(true) + // Now the copy should be equal to the original we passed in. + if !reflect.DeepEqual(©SpanContext, extractedSpanContext) { + t.Errorf("Extracted span context does not match: %#v, %#v", span.Context(), extractedSpanContext) + } +} From fcb9b2a36230684e8b2f6c214d3672ec5af278a8 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 18 Apr 2022 11:18:26 +0100 Subject: [PATCH 4/6] Special-case extract from ot.HTTPHeadersCarrier Values stored in http.Header get title-cased, i.e. `traceparent` will turn into `Traceparent`. Since HTTPHeadersCarrier.ForeachKey does not undo this change, special-case that one type with a different wrapper that will allow the value to be fetched. Signed-off-by: Bryan Boreham --- bridge/opentracing/bridge.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index c759f68f672..a101e711dbf 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -701,14 +701,20 @@ func (t *BridgeTracer) Extract(format interface{}, carrier interface{}) (ot.Span if builtinFormat, ok := format.(ot.BuiltinFormat); !ok || builtinFormat != ot.HTTPHeaders { return nil, ot.ErrUnsupportedFormat } - // If carrier implements the required interface directly, use that - tmCarrier, ok := carrier.(propagation.TextMapCarrier) - if !ok { - tmReader, ok := carrier.(ot.TextMapReader) // otherwise see if we can wrap it - if !ok { - return nil, ot.ErrInvalidCarrier - } - tmCarrier = textMapAdapter{r: tmReader} + var tmCarrier propagation.TextMapCarrier + switch typedCarrier := carrier.(type) { + case propagation.TextMapCarrier: + // If carrier implements the required interface directly, use that. + tmCarrier = typedCarrier + case ot.HTTPHeadersCarrier: + // HTTPHeadersCarrier requires special wrapping, because Set() title-cases the key. + header := http.Header(typedCarrier) + tmCarrier = propagation.HeaderCarrier(header) + case ot.TextMapReader: + // We can wrap anything corresponding to TextMapReader. + tmCarrier = textMapAdapter{r: typedCarrier} + default: + return nil, ot.ErrInvalidCarrier } ctx := t.getPropagator().Extract(context.Background(), tmCarrier) baggage := baggage.FromContext(ctx) From 344c66b09a2d922605b2d2bcd4a87fa8be7d1592 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 18 Apr 2022 11:41:21 +0100 Subject: [PATCH 5/6] Add test for textMapAdapter.Keys This is not currently used in normal functioning of Inject and Extract, but might get used in future so give it some testing now. Test adapted from `open-telemetry/opentelemetry-go/propagation/propagation_test.go` Signed-off-by: Bryan Boreham --- bridge/opentracing/bridge_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/bridge/opentracing/bridge_test.go b/bridge/opentracing/bridge_test.go index 4688e48a914..b74da123454 100644 --- a/bridge/opentracing/bridge_test.go +++ b/bridge/opentracing/bridge_test.go @@ -16,6 +16,7 @@ package opentracing import ( "reflect" + "sort" "testing" ot "github.com/opentracing/opentracing-go" @@ -24,6 +25,25 @@ import ( "go.opentelemetry.io/otel/propagation" ) +func TestTextMapAdapterKeys(t *testing.T) { + carrier := ot.TextMapCarrier{ + "foo": "bar", + "baz": "qux", + } + + keys := textMapAdapter{r: carrier}.Keys() + sort.Strings(keys) + expected := []string{"baz", "foo"} + if !reflect.DeepEqual(expected, keys) { + t.Errorf("Keys do not match: %#v, %#v", expected, keys) + } + // Check what happens if we read from a write-capable adaptor. + keys = textMapAdapter{w: carrier}.Keys() + if keys != nil { + t.Errorf("Keys should be nil: %#v", keys) + } +} + func TestMapCarrier(t *testing.T) { carrier := propagation.MapCarrier{} testBridgeWithCarrier(t, carrier) From 8c487806f3fcad0c494f878d4818f2aa5fdd42a7 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 18 Apr 2022 12:05:43 +0100 Subject: [PATCH 6/6] Lint Signed-off-by: Bryan Boreham --- bridge/opentracing/bridge_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/bridge/opentracing/bridge_test.go b/bridge/opentracing/bridge_test.go index b74da123454..97390162ad1 100644 --- a/bridge/opentracing/bridge_test.go +++ b/bridge/opentracing/bridge_test.go @@ -20,6 +20,7 @@ import ( "testing" ot "github.com/opentracing/opentracing-go" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/bridge/opentracing/internal" "go.opentelemetry.io/otel/propagation"