From 449063cfafa9eb6277714d0325fd0669470ef36b Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 3 Mar 2020 12:02:11 -0600 Subject: [PATCH 1/7] feat: add baggage support to the opentracing shim Signed-off-by: Ruben Vargas --- .../src/shim.ts | 53 +++++++++++++++---- .../test/Shim.test.ts | 15 +++++- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/packages/opentelemetry-shim-opentracing/src/shim.ts b/packages/opentelemetry-shim-opentracing/src/shim.ts index 14a49f411f5..e0fb46cb2d7 100644 --- a/packages/opentelemetry-shim-opentracing/src/shim.ts +++ b/packages/opentelemetry-shim-opentracing/src/shim.ts @@ -22,7 +22,7 @@ import { setActiveSpan, } from '@opentelemetry/core'; import * as opentracing from 'opentracing'; -import { defaultSetter } from '@opentelemetry/api'; +import { CorrelationContext, defaultSetter } from '@opentelemetry/api'; function translateReferences(references: opentracing.Reference[]): api.Link[] { const links: api.Link[] = []; @@ -72,10 +72,15 @@ function getContextWithParent(options: opentracing.SpanOptions) { */ export class SpanContextShim extends opentracing.SpanContext { private readonly _spanContext: api.SpanContext; + private _correlationContext: api.CorrelationContext; - constructor(spanContext: api.SpanContext) { + constructor( + spanContext: api.SpanContext, + correlationContext: CorrelationContext + ) { super(); this._spanContext = spanContext; + this._correlationContext = correlationContext; } /** @@ -85,6 +90,13 @@ export class SpanContextShim extends opentracing.SpanContext { return this._spanContext; } + /** + * Returns the underlying {@link api.CorrelationContext} + */ + getCorrelationContext(): api.CorrelationContext { + return this._correlationContext; + } + /** * Returns the trace ID as a string. */ @@ -98,6 +110,18 @@ export class SpanContextShim extends opentracing.SpanContext { toSpanId(): string { return this._spanContext.spanId; } + + getBaggageItem(key: string): string | undefined { + const entry: api.EntryValue = this._correlationContext[key]; + return entry === undefined ? undefined : entry.value; + } + + setBaggageItem(key: string, value: string) { + this._correlationContext = Object.assign( + { [key]: { value } }, + this._correlationContext + ); + } } /** @@ -125,11 +149,19 @@ export class TracerShim extends opentracing.Tracer { getContextWithParent(options) ); + let correlationContext: CorrelationContext = {}; + if (options.childOf) { + if (options.childOf instanceof SpanShim) { + const shimContext = options.childOf.context() as SpanContextShim; + correlationContext = shimContext.getCorrelationContext(); + } + } + if (options.tags) { span.setAttributes(options.tags); } - return new SpanShim(this, span); + return new SpanShim(this, span, correlationContext); } _inject( @@ -156,7 +188,7 @@ export class TracerShim extends opentracing.Tracer { this._logger.warn( 'OpentracingShim.inject() does not support FORMAT_BINARY' ); - // @todo: Implement binary format + // @todo: Implement binary formats return; } default: @@ -200,10 +232,14 @@ export class SpanShim extends opentracing.Span { private readonly _contextShim: SpanContextShim; private readonly _tracerShim: TracerShim; - constructor(tracerShim: TracerShim, span: api.Span) { + constructor( + tracerShim: TracerShim, + span: api.Span, + correlationalCtx: CorrelationContext + ) { super(); this._span = span; - this._contextShim = new SpanContextShim(span.context()); + this._contextShim = new SpanContextShim(span.context(), correlationalCtx); this._tracerShim = tracerShim; } @@ -295,12 +331,11 @@ export class SpanShim extends opentracing.Span { } getBaggageItem(key: string): string | undefined { - // TODO: should this go into the context? - return undefined; + return this._contextShim.getBaggageItem(key); } setBaggageItem(key: string, value: string): this { - // TODO: should this go into the context? + this._contextShim.setBaggageItem(key, value); return this; } diff --git a/packages/opentelemetry-shim-opentracing/test/Shim.test.ts b/packages/opentelemetry-shim-opentracing/test/Shim.test.ts index 9524045d15a..4061c757264 100644 --- a/packages/opentelemetry-shim-opentracing/test/Shim.test.ts +++ b/packages/opentelemetry-shim-opentracing/test/Shim.test.ts @@ -190,7 +190,20 @@ describe('OpenTracing Shim', () => { it('can set and retrieve baggage', () => { span.setBaggageItem('baggage', 'item'); - // TODO: baggage + const value = span.getBaggageItem('baggage'); + assert.equal('item', value); + + const childSpan = shimTracer.startSpan('child-span1', { + childOf: span, + }); + childSpan.setBaggageItem('key2', 'item2'); + + // child should have parent baggage items. + assert.equal('item', childSpan.getBaggageItem('baggage')); + assert.equal('item2', childSpan.getBaggageItem('key2')); + + // Parent shouldn't have the childr baggage item. + assert.equal(span.getBaggageItem('key2'), undefined); }); }); }); From 82fa8d22ace924b8d35855bb7a47dfd05a5d3e77 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Fri, 17 Jul 2020 14:06:10 -0500 Subject: [PATCH 2/7] feat: propagate correlational context on shim Signed-off-by: Ruben Vargas --- .../src/shim.ts | 27 ++++++++++++------- .../test/Shim.test.ts | 2 +- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/packages/opentelemetry-shim-opentracing/src/shim.ts b/packages/opentelemetry-shim-opentracing/src/shim.ts index e0fb46cb2d7..4f549c3b52f 100644 --- a/packages/opentelemetry-shim-opentracing/src/shim.ts +++ b/packages/opentelemetry-shim-opentracing/src/shim.ts @@ -19,10 +19,12 @@ import { getExtractedSpanContext, NoopLogger, setExtractedSpanContext, + setCorrelationContext, setActiveSpan, + getCorrelationContext, } from '@opentelemetry/core'; import * as opentracing from 'opentracing'; -import { CorrelationContext, defaultSetter } from '@opentelemetry/api'; +import { Context, CorrelationContext, defaultSetter } from '@opentelemetry/api'; function translateReferences(references: opentracing.Reference[]): api.Link[] { const links: api.Link[] = []; @@ -169,7 +171,9 @@ export class TracerShim extends opentracing.Tracer { format: string, carrier: unknown ): void { - const opentelemSpanContext: api.SpanContext = (spanContext as SpanContextShim).getSpanContext(); + const oTelSpanContext: api.SpanContext = (spanContext as SpanContextShim).getSpanContext(); + const oTelSpanCorrelationContext: api.CorrelationContext = (spanContext as SpanContextShim).getCorrelationContext(); + if (!carrier || typeof carrier !== 'object') return; switch (format) { case opentracing.FORMAT_HTTP_HEADERS: @@ -177,9 +181,9 @@ export class TracerShim extends opentracing.Tracer { api.propagation.inject( carrier, defaultSetter, - setExtractedSpanContext( - api.Context.ROOT_CONTEXT, - opentelemSpanContext + setCorrelationContext( + setExtractedSpanContext(api.Context.ROOT_CONTEXT, oTelSpanContext), + oTelSpanCorrelationContext ) ); return; @@ -199,13 +203,16 @@ export class TracerShim extends opentracing.Tracer { switch (format) { case opentracing.FORMAT_HTTP_HEADERS: case opentracing.FORMAT_TEXT_MAP: { - const context = getExtractedSpanContext( + const context: Context = api.propagation.extract(carrier); + const spanContext = getExtractedSpanContext(context); + const correlationContext = getCorrelationContext( api.propagation.extract(carrier) ); - if (!context) { + + if (!spanContext) { return null; } - return new SpanContextShim(context); + return new SpanContextShim(spanContext, correlationContext || {}); } case opentracing.FORMAT_BINARY: { // @todo: Implement binary format @@ -223,8 +230,8 @@ export class TracerShim extends opentracing.Tracer { /** * SpanShim wraps an {@link types.Span} and implements the OpenTracing Span API * around it. - * @todo: Out of band baggage propagation is not currently supported. - */ + * + * */ export class SpanShim extends opentracing.Span { // _span is the original OpenTelemetry span that we are wrapping with // an opentracing interface. diff --git a/packages/opentelemetry-shim-opentracing/test/Shim.test.ts b/packages/opentelemetry-shim-opentracing/test/Shim.test.ts index 4061c757264..5f7089616f5 100644 --- a/packages/opentelemetry-shim-opentracing/test/Shim.test.ts +++ b/packages/opentelemetry-shim-opentracing/test/Shim.test.ts @@ -135,7 +135,7 @@ describe('OpenTracing Shim', () => { describe('SpanContextShim', () => { it('returns the correct context', () => { - const shim = new SpanContextShim(INVALID_SPAN_CONTEXT); + const shim = new SpanContextShim(INVALID_SPAN_CONTEXT, {}); assert.strictEqual(shim.getSpanContext(), INVALID_SPAN_CONTEXT); assert.strictEqual(shim.toTraceId(), INVALID_SPAN_CONTEXT.traceId); assert.strictEqual(shim.toSpanId(), INVALID_SPAN_CONTEXT.spanId); From 02b4c4d70fe00d074cbd84e9c103bdac85a522f7 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Fri, 17 Jul 2020 16:53:35 -0500 Subject: [PATCH 3/7] chore: remove redundant api import Signed-off-by: Ruben Vargas --- packages/opentelemetry-shim-opentracing/src/shim.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/opentelemetry-shim-opentracing/src/shim.ts b/packages/opentelemetry-shim-opentracing/src/shim.ts index 4f549c3b52f..f4ce3e5a805 100644 --- a/packages/opentelemetry-shim-opentracing/src/shim.ts +++ b/packages/opentelemetry-shim-opentracing/src/shim.ts @@ -24,7 +24,6 @@ import { getCorrelationContext, } from '@opentelemetry/core'; import * as opentracing from 'opentracing'; -import { Context, CorrelationContext, defaultSetter } from '@opentelemetry/api'; function translateReferences(references: opentracing.Reference[]): api.Link[] { const links: api.Link[] = []; @@ -78,7 +77,7 @@ export class SpanContextShim extends opentracing.SpanContext { constructor( spanContext: api.SpanContext, - correlationContext: CorrelationContext + correlationContext: api.CorrelationContext ) { super(); this._spanContext = spanContext; @@ -151,7 +150,7 @@ export class TracerShim extends opentracing.Tracer { getContextWithParent(options) ); - let correlationContext: CorrelationContext = {}; + let correlationContext: api.CorrelationContext = {}; if (options.childOf) { if (options.childOf instanceof SpanShim) { const shimContext = options.childOf.context() as SpanContextShim; @@ -180,7 +179,7 @@ export class TracerShim extends opentracing.Tracer { case opentracing.FORMAT_TEXT_MAP: { api.propagation.inject( carrier, - defaultSetter, + api.defaultSetter, setCorrelationContext( setExtractedSpanContext(api.Context.ROOT_CONTEXT, oTelSpanContext), oTelSpanCorrelationContext @@ -203,7 +202,7 @@ export class TracerShim extends opentracing.Tracer { switch (format) { case opentracing.FORMAT_HTTP_HEADERS: case opentracing.FORMAT_TEXT_MAP: { - const context: Context = api.propagation.extract(carrier); + const context: api.Context = api.propagation.extract(carrier); const spanContext = getExtractedSpanContext(context); const correlationContext = getCorrelationContext( api.propagation.extract(carrier) @@ -242,7 +241,7 @@ export class SpanShim extends opentracing.Span { constructor( tracerShim: TracerShim, span: api.Span, - correlationalCtx: CorrelationContext + correlationalCtx: api.CorrelationContext ) { super(); this._span = span; From e81c8d03fce664a52382840a8871b553b813fdfb Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 21 Jul 2020 19:05:17 -0500 Subject: [PATCH 4/7] chore: small style changes Signed-off-by: Ruben Vargas --- .../opentelemetry-shim-opentracing/src/shim.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/opentelemetry-shim-opentracing/src/shim.ts b/packages/opentelemetry-shim-opentracing/src/shim.ts index f4ce3e5a805..61089584078 100644 --- a/packages/opentelemetry-shim-opentracing/src/shim.ts +++ b/packages/opentelemetry-shim-opentracing/src/shim.ts @@ -113,8 +113,7 @@ export class SpanContextShim extends opentracing.SpanContext { } getBaggageItem(key: string): string | undefined { - const entry: api.EntryValue = this._correlationContext[key]; - return entry === undefined ? undefined : entry.value; + return this._correlationContext[key]?.value } setBaggageItem(key: string, value: string) { @@ -151,11 +150,9 @@ export class TracerShim extends opentracing.Tracer { ); let correlationContext: api.CorrelationContext = {}; - if (options.childOf) { - if (options.childOf instanceof SpanShim) { - const shimContext = options.childOf.context() as SpanContextShim; - correlationContext = shimContext.getCorrelationContext(); - } + if (options.childOf instanceof SpanShim) { + const shimContext = options.childOf.context() as SpanContextShim; + correlationContext = shimContext.getCorrelationContext(); } if (options.tags) { @@ -170,8 +167,9 @@ export class TracerShim extends opentracing.Tracer { format: string, carrier: unknown ): void { - const oTelSpanContext: api.SpanContext = (spanContext as SpanContextShim).getSpanContext(); - const oTelSpanCorrelationContext: api.CorrelationContext = (spanContext as SpanContextShim).getCorrelationContext(); + const spanContextShim:SpanContextShim = spanContext as SpanContextShim + const oTelSpanContext: api.SpanContext = spanContextShim.getSpanContext(); + const oTelSpanCorrelationContext: api.CorrelationContext = spanContextShim.getCorrelationContext(); if (!carrier || typeof carrier !== 'object') return; switch (format) { @@ -230,7 +228,7 @@ export class TracerShim extends opentracing.Tracer { * SpanShim wraps an {@link types.Span} and implements the OpenTracing Span API * around it. * - * */ + **/ export class SpanShim extends opentracing.Span { // _span is the original OpenTelemetry span that we are wrapping with // an opentracing interface. From 474013d722812f9ddd7a41ea1b449a079c860512 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 21 Jul 2020 20:25:35 -0500 Subject: [PATCH 5/7] fix: handle repeated keys in baggage, added test Signed-off-by: Ruben Vargas --- .../opentelemetry-shim-opentracing/src/shim.ts | 15 +++++++-------- .../test/Shim.test.ts | 18 +++++++++++++++++- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/packages/opentelemetry-shim-opentracing/src/shim.ts b/packages/opentelemetry-shim-opentracing/src/shim.ts index 61089584078..5dd8d3e7a5e 100644 --- a/packages/opentelemetry-shim-opentracing/src/shim.ts +++ b/packages/opentelemetry-shim-opentracing/src/shim.ts @@ -113,14 +113,13 @@ export class SpanContextShim extends opentracing.SpanContext { } getBaggageItem(key: string): string | undefined { - return this._correlationContext[key]?.value + return this._correlationContext[key]?.value; } setBaggageItem(key: string, value: string) { - this._correlationContext = Object.assign( - { [key]: { value } }, - this._correlationContext - ); + this._correlationContext = Object.assign({}, this._correlationContext, { + [key]: { value }, + }); } } @@ -151,8 +150,8 @@ export class TracerShim extends opentracing.Tracer { let correlationContext: api.CorrelationContext = {}; if (options.childOf instanceof SpanShim) { - const shimContext = options.childOf.context() as SpanContextShim; - correlationContext = shimContext.getCorrelationContext(); + const shimContext = options.childOf.context() as SpanContextShim; + correlationContext = shimContext.getCorrelationContext(); } if (options.tags) { @@ -167,7 +166,7 @@ export class TracerShim extends opentracing.Tracer { format: string, carrier: unknown ): void { - const spanContextShim:SpanContextShim = spanContext as SpanContextShim + const spanContextShim: SpanContextShim = spanContext as SpanContextShim; const oTelSpanContext: api.SpanContext = spanContextShim.getSpanContext(); const oTelSpanCorrelationContext: api.CorrelationContext = spanContextShim.getCorrelationContext(); diff --git a/packages/opentelemetry-shim-opentracing/test/Shim.test.ts b/packages/opentelemetry-shim-opentracing/test/Shim.test.ts index 5f7089616f5..fbb033684da 100644 --- a/packages/opentelemetry-shim-opentracing/test/Shim.test.ts +++ b/packages/opentelemetry-shim-opentracing/test/Shim.test.ts @@ -202,8 +202,24 @@ describe('OpenTracing Shim', () => { assert.equal('item', childSpan.getBaggageItem('baggage')); assert.equal('item2', childSpan.getBaggageItem('key2')); - // Parent shouldn't have the childr baggage item. + // Parent shouldn't have the child baggage item. assert.equal(span.getBaggageItem('key2'), undefined); }); + + it('can set and retrieve baggage with same key', () => { + span.setBaggageItem('key1', 'value1'); + const value = span.getBaggageItem('key1'); + assert.equal('value1', value); + + const childSpan = shimTracer.startSpan('child-span1', { + childOf: span, + }); + childSpan.setBaggageItem('key2', 'value2'); + childSpan.setBaggageItem('key1', 'value3'); + + // child should have parent baggage items. + assert.equal('value3', childSpan.getBaggageItem('key1')); + assert.equal('value2', childSpan.getBaggageItem('key2')); + }); }); }); From ad0411b4581a332cedc2f7cf67228370563bee56 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 21 Jul 2020 21:55:28 -0500 Subject: [PATCH 6/7] chore: test baggage propagation Signed-off-by: Ruben Vargas --- .../src/shim.ts | 6 ++--- .../test/Shim.test.ts | 27 ++++++++++++++++++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/packages/opentelemetry-shim-opentracing/src/shim.ts b/packages/opentelemetry-shim-opentracing/src/shim.ts index 5dd8d3e7a5e..49033a6d05f 100644 --- a/packages/opentelemetry-shim-opentracing/src/shim.ts +++ b/packages/opentelemetry-shim-opentracing/src/shim.ts @@ -152,6 +152,8 @@ export class TracerShim extends opentracing.Tracer { if (options.childOf instanceof SpanShim) { const shimContext = options.childOf.context() as SpanContextShim; correlationContext = shimContext.getCorrelationContext(); + } else if (options.childOf instanceof SpanContextShim) { + correlationContext = options.childOf.getCorrelationContext(); } if (options.tags) { @@ -201,9 +203,7 @@ export class TracerShim extends opentracing.Tracer { case opentracing.FORMAT_TEXT_MAP: { const context: api.Context = api.propagation.extract(carrier); const spanContext = getExtractedSpanContext(context); - const correlationContext = getCorrelationContext( - api.propagation.extract(carrier) - ); + const correlationContext = getCorrelationContext(context); if (!spanContext) { return null; diff --git a/packages/opentelemetry-shim-opentracing/test/Shim.test.ts b/packages/opentelemetry-shim-opentracing/test/Shim.test.ts index fbb033684da..51b0bdd890b 100644 --- a/packages/opentelemetry-shim-opentracing/test/Shim.test.ts +++ b/packages/opentelemetry-shim-opentracing/test/Shim.test.ts @@ -22,6 +22,8 @@ import { INVALID_SPAN_CONTEXT, timeInputToHrTime, HttpTraceContext, + CompositePropagator, + HttpCorrelationContext, } from '@opentelemetry/core'; import { propagation } from '@opentelemetry/api'; import { performance } from 'perf_hooks'; @@ -32,7 +34,11 @@ describe('OpenTracing Shim', () => { provider.getTracer('default') ); opentracing.initGlobalTracer(shimTracer); - propagation.setGlobalPropagator(new HttpTraceContext()); + const compositePropagator = new CompositePropagator({ + propagators: [new HttpTraceContext(), new HttpCorrelationContext()], + }); + + propagation.setGlobalPropagator(compositePropagator); describe('TracerShim', () => { let span: opentracing.Span; @@ -86,6 +92,25 @@ describe('OpenTracing Shim', () => { /* const extractedContext = shimTracer.extract(opentracing.FORMAT_BINARY, { buffer: new Uint8Array(carrier)}); */ /* assert.strictEqual(context.toSpanId(), extractedContext.toSpanId()) */ }); + + it('injects/extracts a span with baggage', () => { + const carrier: { [key: string]: unknown } = {}; + span.setBaggageItem('baggage1', 'value1'); + span.setBaggageItem('baggage2', 'value2'); + shimTracer.inject(span, opentracing.FORMAT_HTTP_HEADERS, carrier); + const extractedContext = shimTracer.extract( + opentracing.FORMAT_HTTP_HEADERS, + carrier + ) as SpanContextShim; + const childSpan = shimTracer.startSpan('other-span', { + childOf: extractedContext, + }) as SpanShim; + assert.ok(extractedContext !== null); + assert.strictEqual(context.toTraceId(), extractedContext!.toTraceId()); + assert.strictEqual(context.toSpanId(), extractedContext!.toSpanId()); + assert.strictEqual(childSpan.getBaggageItem('baggage1'), 'value1'); + assert.strictEqual(childSpan.getBaggageItem('baggage2'), 'value2'); + }); }); it('creates parent/child relationship using a span object', () => { From 73b29d5c9663b30a491b50d378b3325aabecbee1 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Thu, 23 Jul 2020 17:50:48 -0500 Subject: [PATCH 7/7] chore: rename some variables Signed-off-by: Ruben Vargas --- packages/opentelemetry-shim-opentracing/src/shim.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-shim-opentracing/src/shim.ts b/packages/opentelemetry-shim-opentracing/src/shim.ts index 49033a6d05f..c5d4417c3a3 100644 --- a/packages/opentelemetry-shim-opentracing/src/shim.ts +++ b/packages/opentelemetry-shim-opentracing/src/shim.ts @@ -238,11 +238,11 @@ export class SpanShim extends opentracing.Span { constructor( tracerShim: TracerShim, span: api.Span, - correlationalCtx: api.CorrelationContext + correlationContext: api.CorrelationContext ) { super(); this._span = span; - this._contextShim = new SpanContextShim(span.context(), correlationalCtx); + this._contextShim = new SpanContextShim(span.context(), correlationContext); this._tracerShim = tracerShim; }