From 448e3dbf421fbb74901ddccef56eacd1a2f9480e Mon Sep 17 00:00:00 2001 From: treble-snake Date: Sat, 27 Mar 2021 20:51:32 +0200 Subject: [PATCH 1/2] Handle non-extending Span classes in Reference constructor The interface allows passing both Span and SpanContext as an argument for creating References. But the `instanceof` check won't always work in runtime, for example, for cases when Span implementation does not extend the opentracing.Span class, but just implements the interface. The popular `jaeger-client-node` library does exactly that. This produces hard-to-track errors. I added additional checks to distinguish between Span and SpanContext instances. Resolves: #161 --- src/functions.ts | 8 -- src/reference.ts | 22 ++++- src/test/fixtures/ExtendingSpan.ts | 16 ++++ src/test/fixtures/ExtendingSpanContext.ts | 8 ++ src/test/fixtures/NonExtendingSpan.ts | 85 ++++++++++++++++++++ src/test/fixtures/NonExtendingSpanContext.ts | 10 +++ src/test/opentracing_api.ts | 32 ++++++++ 7 files changed, 169 insertions(+), 12 deletions(-) create mode 100644 src/test/fixtures/ExtendingSpan.ts create mode 100644 src/test/fixtures/ExtendingSpanContext.ts create mode 100644 src/test/fixtures/NonExtendingSpan.ts create mode 100644 src/test/fixtures/NonExtendingSpanContext.ts diff --git a/src/functions.ts b/src/functions.ts index 862dd28..04af7fd 100644 --- a/src/functions.ts +++ b/src/functions.ts @@ -11,10 +11,6 @@ import SpanContext from './span_context'; * @return a REFERENCE_CHILD_OF reference pointing to `spanContext` */ export function childOf(spanContext: SpanContext | Span): Reference { - // Allow the user to pass a Span instead of a SpanContext - if (spanContext instanceof Span) { - spanContext = spanContext.context(); - } return new Reference(Constants.REFERENCE_CHILD_OF, spanContext); } @@ -26,9 +22,5 @@ export function childOf(spanContext: SpanContext | Span): Reference { * @return a REFERENCE_FOLLOWS_FROM reference pointing to `spanContext` */ export function followsFrom(spanContext: SpanContext | Span): Reference { - // Allow the user to pass a Span instead of a SpanContext - if (spanContext instanceof Span) { - spanContext = spanContext.context(); - } return new Reference(Constants.REFERENCE_FOLLOWS_FROM, spanContext); } diff --git a/src/reference.ts b/src/reference.ts index cf87e25..474fdf1 100644 --- a/src/reference.ts +++ b/src/reference.ts @@ -1,6 +1,23 @@ import Span from './span'; import SpanContext from './span_context'; +const getContext = (contextOrSpan: SpanContext | Span): SpanContext => { + if (contextOrSpan instanceof SpanContext) { + return contextOrSpan; + } + + // Second check is for cases when a Span implementation does not extend + // opentracing.Span class directly (like Jaeger), just implements the same interface. + // The only false-positive case here is a non-extending SpanContext class, + // which has a method called "context". + // But that's too much of a specification violation to take care of. + if (contextOrSpan instanceof Span || 'context' in contextOrSpan) { + return contextOrSpan.context(); + } + + return contextOrSpan; +}; + /** * Reference pairs a reference type constant (e.g., REFERENCE_CHILD_OF or * REFERENCE_FOLLOWS_FROM) with the SpanContext it points to. @@ -39,9 +56,6 @@ export default class Reference { */ constructor(type: string, referencedContext: SpanContext | Span) { this._type = type; - this._referencedContext = ( - referencedContext instanceof Span ? - referencedContext.context() : - referencedContext); + this._referencedContext = getContext(referencedContext); } } diff --git a/src/test/fixtures/ExtendingSpan.ts b/src/test/fixtures/ExtendingSpan.ts new file mode 100644 index 0000000..94fee72 --- /dev/null +++ b/src/test/fixtures/ExtendingSpan.ts @@ -0,0 +1,16 @@ +import Span from '../../span'; +import SpanContext from '../../span_context'; + +/** + * Span implementation for unit tests. Extends opentracing.Span. + */ +export class ExtendingSpan extends Span { + + constructor(private ctx: SpanContext) { + super(); + } + + context(): SpanContext { + return this.ctx; + } +} diff --git a/src/test/fixtures/ExtendingSpanContext.ts b/src/test/fixtures/ExtendingSpanContext.ts new file mode 100644 index 0000000..5e50979 --- /dev/null +++ b/src/test/fixtures/ExtendingSpanContext.ts @@ -0,0 +1,8 @@ +import SpanContext from '../../span_context'; + +export class ExtendingSpanContext extends SpanContext { + + context(): any { + return null; + } +} diff --git a/src/test/fixtures/NonExtendingSpan.ts b/src/test/fixtures/NonExtendingSpan.ts new file mode 100644 index 0000000..f0e2455 --- /dev/null +++ b/src/test/fixtures/NonExtendingSpan.ts @@ -0,0 +1,85 @@ +import * as noop from '../../noop'; +import SpanContext from '../../span_context'; +import Tracer from '../../tracer'; + +/** + * Span implementation for unit tests. Does not extend opentracing.Span, but + * implements the interface + */ +export class NonExtendingSpan { + + constructor(private ctx: SpanContext) { + } + + context(): SpanContext { + return this.ctx; + } + + tracer(): Tracer { + return this._tracer(); + } + + setOperationName(name: string): this { + this._setOperationName(name); + return this; + } + + setBaggageItem(key: string, value: string): this { + this._setBaggageItem(key, value); + return this; + } + + getBaggageItem(key: string): string | undefined { + return this._getBaggageItem(key); + } + + setTag(key: string, value: any): this { + this._addTags({ [key]: value }); + return this; + } + + addTags(keyValueMap: { [key: string]: any }): this { + this._addTags(keyValueMap); + return this; + } + + log(keyValuePairs: { [key: string]: any }, timestamp?: number): this { + this._log(keyValuePairs, timestamp); + return this; + } + + logEvent(eventName: string, payload: any): void { + return this._log({ event: eventName, payload }); + } + + finish(finishTime?: number): void { + this._finish(finishTime); + } + + protected _context(): SpanContext { + return noop.spanContext!; + } + + protected _tracer(): Tracer { + return noop.tracer!; + } + + protected _setOperationName(name: string): void { + } + + protected _setBaggageItem(key: string, value: string): void { + } + + protected _getBaggageItem(key: string): string | undefined { + return undefined; + } + + protected _addTags(keyValuePairs: { [key: string]: any }): void { + } + + protected _log(keyValuePairs: { [key: string]: any }, timestamp?: number): void { + } + + protected _finish(finishTime?: number): void { + } +} diff --git a/src/test/fixtures/NonExtendingSpanContext.ts b/src/test/fixtures/NonExtendingSpanContext.ts new file mode 100644 index 0000000..35382e9 --- /dev/null +++ b/src/test/fixtures/NonExtendingSpanContext.ts @@ -0,0 +1,10 @@ +export class NonExtendingSpanContext { + + toTraceId(): string { + return ''; + } + + toSpanId(): string { + return ''; + } +} diff --git a/src/test/opentracing_api.ts b/src/test/opentracing_api.ts index ec4892b..7d5cd52 100644 --- a/src/test/opentracing_api.ts +++ b/src/test/opentracing_api.ts @@ -1,6 +1,12 @@ import { expect } from 'chai'; import * as opentracing from '../index'; +import Span from '../span'; +import SpanContext from '../span_context'; +import {ExtendingSpan} from './fixtures/ExtendingSpan'; +import {ExtendingSpanContext} from './fixtures/ExtendingSpanContext'; +import {NonExtendingSpan} from './fixtures/NonExtendingSpan'; +import {NonExtendingSpanContext} from './fixtures/NonExtendingSpanContext'; export function opentracingAPITests(): void { describe('Opentracing API', () => { @@ -85,6 +91,32 @@ export function opentracingAPITests(): void { const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span.context()); expect(ref).to.be.an('object'); }); + + it('gets context from custom non-extending span classes', () => { + const ctx = new SpanContext(); + const span = new NonExtendingSpan(ctx) as unknown as Span; + const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span); + expect(ref.referencedContext()).to.equal(ctx); + }); + + it('gets context from custom extending span classes', () => { + const ctx = new SpanContext(); + const span = new ExtendingSpan(ctx) as unknown as Span; + const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span); + expect(ref.referencedContext()).to.equal(ctx); + }); + + it('uses extending contexts', () => { + const ctx = new ExtendingSpanContext(); + const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, ctx); + expect(ref.referencedContext()).to.equal(ctx); + }); + + it('uses non-extending contexts', () => { + const ctx = new NonExtendingSpanContext(); + const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, ctx); + expect(ref.referencedContext()).to.equal(ctx); + }); }); describe('BinaryCarrier', () => { From e55f26bf497ac533a68dad78acff3276bd33edca Mon Sep 17 00:00:00 2001 From: treble-snake Date: Sun, 28 Mar 2021 13:19:34 +0300 Subject: [PATCH 2/2] Improve tests Remove unnecessary fixtures, use simpler approach. --- src/test/fixtures/ExtendingSpan.ts | 16 ---- src/test/fixtures/ExtendingSpanContext.ts | 8 -- src/test/fixtures/NonExtendingSpan.ts | 85 -------------------- src/test/fixtures/NonExtendingSpanContext.ts | 10 --- src/test/opentracing_api.ts | 33 ++++---- 5 files changed, 18 insertions(+), 134 deletions(-) delete mode 100644 src/test/fixtures/ExtendingSpan.ts delete mode 100644 src/test/fixtures/ExtendingSpanContext.ts delete mode 100644 src/test/fixtures/NonExtendingSpan.ts delete mode 100644 src/test/fixtures/NonExtendingSpanContext.ts diff --git a/src/test/fixtures/ExtendingSpan.ts b/src/test/fixtures/ExtendingSpan.ts deleted file mode 100644 index 94fee72..0000000 --- a/src/test/fixtures/ExtendingSpan.ts +++ /dev/null @@ -1,16 +0,0 @@ -import Span from '../../span'; -import SpanContext from '../../span_context'; - -/** - * Span implementation for unit tests. Extends opentracing.Span. - */ -export class ExtendingSpan extends Span { - - constructor(private ctx: SpanContext) { - super(); - } - - context(): SpanContext { - return this.ctx; - } -} diff --git a/src/test/fixtures/ExtendingSpanContext.ts b/src/test/fixtures/ExtendingSpanContext.ts deleted file mode 100644 index 5e50979..0000000 --- a/src/test/fixtures/ExtendingSpanContext.ts +++ /dev/null @@ -1,8 +0,0 @@ -import SpanContext from '../../span_context'; - -export class ExtendingSpanContext extends SpanContext { - - context(): any { - return null; - } -} diff --git a/src/test/fixtures/NonExtendingSpan.ts b/src/test/fixtures/NonExtendingSpan.ts deleted file mode 100644 index f0e2455..0000000 --- a/src/test/fixtures/NonExtendingSpan.ts +++ /dev/null @@ -1,85 +0,0 @@ -import * as noop from '../../noop'; -import SpanContext from '../../span_context'; -import Tracer from '../../tracer'; - -/** - * Span implementation for unit tests. Does not extend opentracing.Span, but - * implements the interface - */ -export class NonExtendingSpan { - - constructor(private ctx: SpanContext) { - } - - context(): SpanContext { - return this.ctx; - } - - tracer(): Tracer { - return this._tracer(); - } - - setOperationName(name: string): this { - this._setOperationName(name); - return this; - } - - setBaggageItem(key: string, value: string): this { - this._setBaggageItem(key, value); - return this; - } - - getBaggageItem(key: string): string | undefined { - return this._getBaggageItem(key); - } - - setTag(key: string, value: any): this { - this._addTags({ [key]: value }); - return this; - } - - addTags(keyValueMap: { [key: string]: any }): this { - this._addTags(keyValueMap); - return this; - } - - log(keyValuePairs: { [key: string]: any }, timestamp?: number): this { - this._log(keyValuePairs, timestamp); - return this; - } - - logEvent(eventName: string, payload: any): void { - return this._log({ event: eventName, payload }); - } - - finish(finishTime?: number): void { - this._finish(finishTime); - } - - protected _context(): SpanContext { - return noop.spanContext!; - } - - protected _tracer(): Tracer { - return noop.tracer!; - } - - protected _setOperationName(name: string): void { - } - - protected _setBaggageItem(key: string, value: string): void { - } - - protected _getBaggageItem(key: string): string | undefined { - return undefined; - } - - protected _addTags(keyValuePairs: { [key: string]: any }): void { - } - - protected _log(keyValuePairs: { [key: string]: any }, timestamp?: number): void { - } - - protected _finish(finishTime?: number): void { - } -} diff --git a/src/test/fixtures/NonExtendingSpanContext.ts b/src/test/fixtures/NonExtendingSpanContext.ts deleted file mode 100644 index 35382e9..0000000 --- a/src/test/fixtures/NonExtendingSpanContext.ts +++ /dev/null @@ -1,10 +0,0 @@ -export class NonExtendingSpanContext { - - toTraceId(): string { - return ''; - } - - toSpanId(): string { - return ''; - } -} diff --git a/src/test/opentracing_api.ts b/src/test/opentracing_api.ts index 7d5cd52..3c99474 100644 --- a/src/test/opentracing_api.ts +++ b/src/test/opentracing_api.ts @@ -1,12 +1,11 @@ import { expect } from 'chai'; import * as opentracing from '../index'; +import MockContext from '../mock_tracer/mock_context'; +import MockSpan from '../mock_tracer/mock_span'; +import MockTracer from '../mock_tracer/mock_tracer'; import Span from '../span'; import SpanContext from '../span_context'; -import {ExtendingSpan} from './fixtures/ExtendingSpan'; -import {ExtendingSpanContext} from './fixtures/ExtendingSpanContext'; -import {NonExtendingSpan} from './fixtures/NonExtendingSpan'; -import {NonExtendingSpanContext} from './fixtures/NonExtendingSpanContext'; export function opentracingAPITests(): void { describe('Opentracing API', () => { @@ -92,28 +91,32 @@ export function opentracingAPITests(): void { expect(ref).to.be.an('object'); }); - it('gets context from custom non-extending span classes', () => { - const ctx = new SpanContext(); - const span = new NonExtendingSpan(ctx) as unknown as Span; + it('should get context from custom extending span classes', () => { + const span = new MockSpan(new MockTracer()); const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span); - expect(ref.referencedContext()).to.equal(ctx); + expect(ref.referencedContext() instanceof SpanContext).to.equal(true); }); - it('gets context from custom extending span classes', () => { + it('should get context from custom non-extending span classes', () => { const ctx = new SpanContext(); - const span = new ExtendingSpan(ctx) as unknown as Span; - const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span); + const pseudoSpan = { + context: () => ctx + } as Span; + const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, pseudoSpan); expect(ref.referencedContext()).to.equal(ctx); }); - it('uses extending contexts', () => { - const ctx = new ExtendingSpanContext(); + it('should use extending contexts', () => { + const ctx = new MockContext({} as MockSpan); const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, ctx); expect(ref.referencedContext()).to.equal(ctx); }); - it('uses non-extending contexts', () => { - const ctx = new NonExtendingSpanContext(); + it('should use non-extending contexts', () => { + const ctx = { + toTraceId: () => '', + toSpanId: () => '' + }; const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, ctx); expect(ref.referencedContext()).to.equal(ctx); });