From 4aa4d5bf50df88341566da26dbcb6e2c1018afcf Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Mon, 13 Jun 2022 15:08:25 +0300 Subject: [PATCH 01/12] chore: remove removeEvent since it's not used anywhere --- .../test/common/BasicTracerProvider.test.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts index e2b17bb5614..f67eb82d169 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -46,8 +46,6 @@ import { } from '../../src'; describe('BasicTracerProvider', () => { - let removeEvent: (() => void) | undefined; - let envSource: Record; if (typeof process === 'undefined') { envSource = (globalThis as unknown) as Record; @@ -61,10 +59,6 @@ describe('BasicTracerProvider', () => { afterEach(() => { sinon.restore(); - if (removeEvent) { - removeEvent(); - removeEvent = undefined; - } }); describe('constructor', () => { From fc995ef7c44e9a4033a4774dacb66fd707a8a3f0 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Mon, 13 Jun 2022 17:01:20 +0300 Subject: [PATCH 02/12] test: use sinon assertions and always restore stubs and spies --- .../test/common/BasicTracerProvider.test.ts | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts index f67eb82d169..c808319d8f6 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -267,7 +267,7 @@ describe('BasicTracerProvider', () => { }); afterEach(() => { - setGlobalPropagatorStub.restore(); + sinon.restore(); // otherwise we may assign 'undefined' (a string) if (originalPropagators !== undefined) { @@ -282,10 +282,10 @@ describe('BasicTracerProvider', () => { provider.register({ propagator: new DummyPropagator(), }); - assert.ok( - setGlobalPropagatorStub.calledOnceWithExactly( - sinon.match.instanceOf(DummyPropagator) - ) + + sinon.assert.calledOnceWithExactly( + setGlobalPropagatorStub, + sinon.match.instanceOf(DummyPropagator) ); }); @@ -293,10 +293,9 @@ describe('BasicTracerProvider', () => { const provider = new BasicTracerProvider(); provider.register(); - assert.ok( - setGlobalPropagatorStub.calledOnceWithExactly( - sinon.match.instanceOf(CompositePropagator) - ) + sinon.assert.calledOnceWithExactly( + setGlobalPropagatorStub, + sinon.match.instanceOf(CompositePropagator) ); assert.deepStrictEqual(setGlobalPropagatorStub.args[0][0].fields(), [ 'traceparent', @@ -312,13 +311,10 @@ describe('BasicTracerProvider', () => { const provider = new BasicTracerProvider({}); provider.register(); - assert.ok( - warnStub.calledOnceWithExactly( - 'Propagator "missing-propagator" requested through environment variable is unavailable.' - ) + sinon.assert.calledOnceWithExactly( + warnStub, + 'Propagator "missing-propagator" requested through environment variable is unavailable.' ); - - warnStub.restore(); }); }); From fd0ec876e1ea71be38a57b1aa5a92bdba6365cec Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Mon, 13 Jun 2022 17:09:32 +0300 Subject: [PATCH 03/12] test: test the common inheritance models for BasicTracerProvider --- .../test/common/BasicTracerProvider.test.ts | 136 +++++++++++++++--- 1 file changed, 113 insertions(+), 23 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts index c808319d8f6..d495ceae476 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -45,8 +45,35 @@ import { BatchSpanProcessor, } from '../../src'; +class DummyPropagator implements TextMapPropagator { + inject( + context: Context, + carrier: any, + setter: TextMapSetter + ): void { + throw new Error('Method not implemented.'); + } + extract( + context: Context, + carrier: any, + getter: TextMapGetter + ): Context { + throw new Error('Method not implemented.'); + } + fields(): string[] { + throw new Error('Method not implemented.'); + } +} + +class DummyExporter extends InMemorySpanExporter {} + describe('BasicTracerProvider', () => { let envSource: Record; + let setGlobalPropagatorStub: sinon.SinonSpy< + [TextMapPropagator], + boolean + >; + if (typeof process === 'undefined') { envSource = (globalThis as unknown) as Record; } else { @@ -54,6 +81,10 @@ describe('BasicTracerProvider', () => { } beforeEach(() => { + // to avoid actually registering the TraceProvider and leaking env to other tests + sinon.stub(trace, 'setGlobalTracerProvider'); + setGlobalPropagatorStub = sinon.spy(propagation, 'setGlobalPropagator'); + context.disable(); }); @@ -234,35 +265,94 @@ describe('BasicTracerProvider', () => { }); }); - describe('.register()', () => { - describe('propagator', () => { - class DummyPropagator implements TextMapPropagator { - inject( - context: Context, - carrier: any, - setter: TextMapSetter - ): void { - throw new Error('Method not implemented.'); - } - extract( - context: Context, - carrier: any, - getter: TextMapGetter - ): Context { - throw new Error('Method not implemented.'); + describe('Custom TracerProvider through inheritance', () => { + beforeEach(() => { + envSource.OTEL_TRACES_EXPORTER = 'custom-exporter'; + envSource.OTEL_PROPAGATORS = 'custom-propagator'; + }); + + afterEach(() => { + delete envSource.OTEL_TRACES_EXPORTER; + delete envSource.OTEL_PROPAGATORS; + sinon.restore(); + }); + + it('can be extended by overriding registered components', () => { + class CustomTracerProvider extends BasicTracerProvider { + protected static override readonly _registeredPropagators = new Map< + string, + () => TextMapPropagator + >([ + ['custom-propagator', () => new DummyPropagator()], + ]); + + protected static override readonly _registeredExporters = new Map< + string, + () => SpanExporter + >([ + ['custom-exporter', () => new DummyExporter()], + ]); + } + + const provider = new CustomTracerProvider({}); + provider.register(); + const processor = provider.getActiveSpanProcessor(); + assert(processor instanceof BatchSpanProcessor); + // @ts-expect-error access configured to verify its the correct one + const exporter = processor._exporter; + assert(exporter instanceof DummyExporter); + + sinon.assert.calledOnceWithExactly(setGlobalPropagatorStub, sinon.match.instanceOf(DummyPropagator)); + }); + + it('the old way of extending still works', () => { + // this is an anti-pattern, but we test that for backwards compatibility + class CustomTracerProvider extends BasicTracerProvider { + protected static override readonly _registeredPropagators = new Map< + string, + () => TextMapPropagator + >([ + ['custom-propagator', () => new DummyPropagator()], + ]); + + protected static override readonly _registeredExporters = new Map< + string, + () => SpanExporter + >([ + ['custom-exporter', () => new DummyExporter()], + ]); + + protected override _getPropagator(name: string): TextMapPropagator | undefined { + return ( + super._getPropagator(name) || + CustomTracerProvider._registeredPropagators.get(name)?.() + ); } - fields(): string[] { - throw new Error('Method not implemented.'); + + protected override _getSpanExporter(name: string): SpanExporter | undefined { + return ( + super._getSpanExporter(name) || + CustomTracerProvider._registeredExporters.get(name)?.() + ); } } - let setGlobalPropagatorStub: sinon.SinonSpy< - [TextMapPropagator], - boolean - >; + const provider = new CustomTracerProvider({}); + provider.register(); + const processor = provider.getActiveSpanProcessor(); + assert(processor instanceof BatchSpanProcessor); + // @ts-expect-error access configured to verify its the correct one + const exporter = processor._exporter; + assert(exporter instanceof DummyExporter); + + sinon.assert.calledOnceWithExactly(setGlobalPropagatorStub, sinon.match.instanceOf(DummyPropagator)); + }); + }); + + describe('.register()', () => { + describe('propagator', () => { let originalPropagators: string | number | undefined | string[]; beforeEach(() => { - setGlobalPropagatorStub = sinon.spy(propagation, 'setGlobalPropagator'); originalPropagators = envSource.OTEL_PROPAGATORS; }); From 8c769b8b36bb1e3a09d718b92e9f03af8990a1b6 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Mon, 13 Jun 2022 21:35:08 +0300 Subject: [PATCH 04/12] test: remove clearing module cache - not required anymore --- .../test/NodeTracerProvider.test.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts index 0d53e25eb6e..d62113b4542 100644 --- a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts @@ -34,18 +34,9 @@ const sleep = (time: number) => return setTimeout(resolve, time); }); -const INSTALLED_PLUGINS_PATH = path.join( - __dirname, - 'instrumentation', - 'node_modules' -); - describe('NodeTracerProvider', () => { let provider: NodeTracerProvider; let contextManager: ContextManager; - before(() => { - module.paths.push(INSTALLED_PLUGINS_PATH); - }); beforeEach(() => { contextManager = new AsyncHooksContextManager(); @@ -53,8 +44,6 @@ describe('NodeTracerProvider', () => { }); afterEach(() => { - // clear require cache - Object.keys(require.cache).forEach(key => delete require.cache[key]); contextManager.disable(); context.disable(); }); From 4a9ec4d51d86a9786fa88ccc5856d038ad7b9737 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Mon, 13 Jun 2022 21:52:12 +0300 Subject: [PATCH 05/12] test: implement tests for NodeTracerProvider --- .../test/NodeTracerProvider.test.ts | 139 +++++++++++++++++- 1 file changed, 134 insertions(+), 5 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts index d62113b4542..d95392a1b29 100644 --- a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts @@ -14,19 +14,32 @@ * limitations under the License. */ +import * as sinon from 'sinon'; +import * as assert from 'assert'; + import { context, + Context, + ContextManager, + propagation, + ROOT_CONTEXT, + TextMapGetter, + TextMapPropagator, + TextMapSetter, + trace, TraceFlags, - propagation, trace, } from '@opentelemetry/api'; import { AlwaysOnSampler, AlwaysOffSampler } from '@opentelemetry/core'; import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; -import { Span } from '@opentelemetry/sdk-trace-base'; +import { + BatchSpanProcessor, + InMemorySpanExporter, + Span, + SpanExporter, +} from '@opentelemetry/sdk-trace-base'; import { Resource } from '@opentelemetry/resources'; import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; -import * as assert from 'assert'; -import * as path from 'path'; -import { ContextManager, ROOT_CONTEXT } from '@opentelemetry/api'; + import { NodeTracerProvider } from '../src/NodeTracerProvider'; const sleep = (time: number) => @@ -232,4 +245,120 @@ describe('NodeTracerProvider', () => { ]); }); }); + + + describe('Custom TracerProvider through inheritance', () => { + class DummyPropagator implements TextMapPropagator { + inject( + context: Context, + carrier: any, + setter: TextMapSetter + ): void { + throw new Error('Method not implemented.'); + } + extract( + context: Context, + carrier: any, + getter: TextMapGetter + ): Context { + throw new Error('Method not implemented.'); + } + fields(): string[] { + throw new Error('Method not implemented.'); + } + } + + class DummyExporter extends InMemorySpanExporter {} + + let setGlobalPropagatorStub: sinon.SinonSpy< + [TextMapPropagator], + boolean + >; + + beforeEach(() => { + // to avoid actually registering the TraceProvider and leaking env to other tests + sinon.stub(trace, 'setGlobalTracerProvider'); + setGlobalPropagatorStub = sinon.spy(propagation, 'setGlobalPropagator'); + + process.env.OTEL_TRACES_EXPORTER = 'custom-exporter'; + process.env.OTEL_PROPAGATORS = 'custom-propagator'; + }); + + afterEach(() => { + delete process.env.OTEL_TRACES_EXPORTER; + delete process.env.OTEL_PROPAGATORS; + sinon.restore(); + }); + + it('can be extended by overriding registered components', () => { + class CustomTracerProvider extends NodeTracerProvider { + protected static override readonly _registeredPropagators = new Map< + string, + () => TextMapPropagator + >([ + ['custom-propagator', () => new DummyPropagator()], + ]); + + protected static override readonly _registeredExporters = new Map< + string, + () => SpanExporter + >([ + ['custom-exporter', () => new DummyExporter()], + ]); + } + + const provider = new CustomTracerProvider({}); + provider.register(); + const processor = provider.getActiveSpanProcessor(); + assert(processor instanceof BatchSpanProcessor); + // @ts-expect-error access configured to verify its the correct one + const exporter = processor._exporter; + assert(exporter instanceof DummyExporter); + + sinon.assert.calledOnceWithExactly(setGlobalPropagatorStub, sinon.match.instanceOf(DummyPropagator)); + }); + + it('the old way of extending still works', () => { + // this is an anti-pattern, but we test that for backwards compatibility + class CustomTracerProvider extends NodeTracerProvider { + protected static override readonly _registeredPropagators = new Map< + string, + () => TextMapPropagator + >([ + ['custom-propagator', () => new DummyPropagator()], + ]); + + protected static override readonly _registeredExporters = new Map< + string, + () => SpanExporter + >([ + ['custom-exporter', () => new DummyExporter()], + ]); + + protected override _getPropagator(name: string): TextMapPropagator | undefined { + return ( + super._getPropagator(name) || + CustomTracerProvider._registeredPropagators.get(name)?.() + ); + } + + protected override _getSpanExporter(name: string): SpanExporter | undefined { + return ( + super._getSpanExporter(name) || + CustomTracerProvider._registeredExporters.get(name)?.() + ); + } + } + + const provider = new CustomTracerProvider({}); + provider.register(); + const processor = provider.getActiveSpanProcessor(); + assert(processor instanceof BatchSpanProcessor); + // @ts-expect-error access configured to verify its the correct one + const exporter = processor._exporter; + assert(exporter instanceof DummyExporter); + + sinon.assert.calledOnceWithExactly(setGlobalPropagatorStub, sinon.match.instanceOf(DummyPropagator)); + }); + }); }); From b7c5ea262c343f098d7e0a2263a04c0b287b6991 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Tue, 7 Jun 2022 15:40:56 +0300 Subject: [PATCH 06/12] feat: allow for extending BasicTracerProvider with custom registered components --- .../src/BasicTracerProvider.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts index a964de5840f..b0560fc3f15 100644 --- a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts @@ -202,12 +202,23 @@ export class BasicTracerProvider implements TracerProvider { return this.activeSpanProcessor.shutdown(); } + /** + * TS cannot yet infer the type of this.constructor: + * https://github.com/Microsoft/TypeScript/issues/3841#issuecomment-337560146 + * There is no need to override either of the getters in your child class. + * The type of the registered component maps should be the same across all + * classes in the inheritance tree. + */ protected _getPropagator(name: string): TextMapPropagator | undefined { - return BasicTracerProvider._registeredPropagators.get(name)?.(); + return ( + (this.constructor as typeof BasicTracerProvider)._registeredPropagators + ).get(name)?.(); } protected _getSpanExporter(name: string): SpanExporter | undefined { - return BasicTracerProvider._registeredExporters.get(name)?.(); + return ( + (this.constructor as typeof BasicTracerProvider)._registeredExporters + ).get(name)?.(); } protected _buildPropagatorFromEnv(): TextMapPropagator | undefined { From 6c9b998428a9910de02f88a459d78de0eee015cb Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Wed, 8 Jun 2022 14:51:49 +0300 Subject: [PATCH 07/12] fix: add propagators from `BasicTracerProvider` to `NodeTracerProvider` --- .../src/NodeTracerProvider.ts | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts index 8e4575a390c..e7e791611ec 100644 --- a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts @@ -23,6 +23,7 @@ import { BasicTracerProvider, PROPAGATOR_FACTORY, SDKRegistrationConfig, + SpanExporter, } from '@opentelemetry/sdk-trace-base'; import * as semver from 'semver'; import { NodeTracerConfig } from './config'; @@ -40,6 +41,7 @@ export class NodeTracerProvider extends BasicTracerProvider { string, PROPAGATOR_FACTORY >([ + ...BasicTracerProvider._registeredPropagators, [ 'b3', () => @@ -68,10 +70,22 @@ export class NodeTracerProvider extends BasicTracerProvider { super.register(config); } - protected override _getPropagator(name: string): TextMapPropagator | undefined { + /** + * TS cannot yet infer the type of this.constructor: + * https://github.com/Microsoft/TypeScript/issues/3841#issuecomment-337560146 + * Do not override either of the getters in your child class unless you really need to. + * We have to do it here because the previous version of BasicTracerProvider's getters + * had BasicTracerProvider's static method hardcoded and we want do undo that behavior. + */ + protected override _getPropagator(name: string): TextMapPropagator | undefined { return ( - super._getPropagator(name) || - NodeTracerProvider._registeredPropagators.get(name)?.() - ); + (this.constructor as typeof BasicTracerProvider)._registeredPropagators + ).get(name)?.(); + } + + protected override _getSpanExporter(name: string): SpanExporter | undefined { + return ( + (this.constructor as typeof BasicTracerProvider)._registeredExporters + ).get(name)?.(); } } From 1c6422adf575cf0f487f3e67e57d939439eca63c Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Tue, 14 Jun 2022 17:12:53 +0300 Subject: [PATCH 08/12] test: refactor instanceof asserts to make it easier to debug --- .../test/registration.test.ts | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-node/test/registration.test.ts b/packages/opentelemetry-sdk-trace-node/test/registration.test.ts index 4db2d334b4b..09cc24b327a 100644 --- a/packages/opentelemetry-sdk-trace-node/test/registration.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/registration.test.ts @@ -14,6 +14,9 @@ * limitations under the License. */ +import * as assert from 'assert'; +import { inspect } from 'util'; + import { context, propagation, @@ -25,10 +28,16 @@ import { AsyncLocalStorageContextManager, } from '@opentelemetry/context-async-hooks'; import { CompositePropagator } from '@opentelemetry/core'; -import * as assert from 'assert'; import { NodeTracerProvider } from '../src'; import * as semver from 'semver'; +const assertInstanceOf = (actual: object, ExpectedInstance: Function) => { + assert.ok( + actual instanceof ExpectedInstance, + `Expected ${inspect(actual)} to be instance of ${ExpectedInstance.name}` + ); +}; + const DefaultContextManager = semver.gte(process.version, '14.8.0') ? AsyncLocalStorageContextManager : AsyncHooksContextManager; @@ -44,9 +53,9 @@ describe('API registration', () => { const tracerProvider = new NodeTracerProvider(); tracerProvider.register(); - assert.ok(context['_getContextManager']() instanceof DefaultContextManager); - assert.ok( - propagation['_getGlobalPropagator']() instanceof CompositePropagator + assertInstanceOf(context['_getContextManager'](), DefaultContextManager); + assertInstanceOf( + propagation['_getGlobalPropagator'](), CompositePropagator ); const apiTracerProvider = trace.getTracerProvider() as ProxyTracerProvider; @@ -80,8 +89,8 @@ describe('API registration', () => { assert.strictEqual(context['_getContextManager'](), ctxManager, 'context manager should not change'); - assert.ok( - propagation['_getGlobalPropagator']() instanceof CompositePropagator + assertInstanceOf( + propagation['_getGlobalPropagator'](), CompositePropagator ); const apiTracerProvider = trace.getTracerProvider() as ProxyTracerProvider; @@ -98,7 +107,7 @@ describe('API registration', () => { assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); - assert.ok(context['_getContextManager']() instanceof DefaultContextManager); + assertInstanceOf(context['_getContextManager'](), DefaultContextManager); const apiTracerProvider = trace.getTracerProvider() as ProxyTracerProvider; assert.ok(apiTracerProvider.getDelegate() === tracerProvider); From 9a343f6be6e415841e489c584dc4cbdd135353c6 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Tue, 14 Jun 2022 17:23:37 +0300 Subject: [PATCH 09/12] test: make tests work with linked packages --- .../test/NodeTracerProvider.test.ts | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts index d95392a1b29..24ba20f4b03 100644 --- a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts @@ -270,33 +270,33 @@ describe('NodeTracerProvider', () => { class DummyExporter extends InMemorySpanExporter {} - let setGlobalPropagatorStub: sinon.SinonSpy< - [TextMapPropagator], - boolean - >; - beforeEach(() => { - // to avoid actually registering the TraceProvider and leaking env to other tests - sinon.stub(trace, 'setGlobalTracerProvider'); - setGlobalPropagatorStub = sinon.spy(propagation, 'setGlobalPropagator'); - process.env.OTEL_TRACES_EXPORTER = 'custom-exporter'; process.env.OTEL_PROPAGATORS = 'custom-propagator'; + + propagation.disable(); + trace.disable(); }); afterEach(() => { delete process.env.OTEL_TRACES_EXPORTER; delete process.env.OTEL_PROPAGATORS; + + propagation.disable(); + trace.disable(); + sinon.restore(); }); it('can be extended by overriding registered components', () => { + const propagator = new DummyPropagator(); + class CustomTracerProvider extends NodeTracerProvider { protected static override readonly _registeredPropagators = new Map< string, () => TextMapPropagator >([ - ['custom-propagator', () => new DummyPropagator()], + ['custom-propagator', () => propagator], ]); protected static override readonly _registeredExporters = new Map< @@ -315,17 +315,19 @@ describe('NodeTracerProvider', () => { const exporter = processor._exporter; assert(exporter instanceof DummyExporter); - sinon.assert.calledOnceWithExactly(setGlobalPropagatorStub, sinon.match.instanceOf(DummyPropagator)); + assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); }); it('the old way of extending still works', () => { + const propagator = new DummyPropagator(); + // this is an anti-pattern, but we test that for backwards compatibility class CustomTracerProvider extends NodeTracerProvider { protected static override readonly _registeredPropagators = new Map< string, () => TextMapPropagator >([ - ['custom-propagator', () => new DummyPropagator()], + ['custom-propagator', () => propagator], ]); protected static override readonly _registeredExporters = new Map< @@ -358,7 +360,7 @@ describe('NodeTracerProvider', () => { const exporter = processor._exporter; assert(exporter instanceof DummyExporter); - sinon.assert.calledOnceWithExactly(setGlobalPropagatorStub, sinon.match.instanceOf(DummyPropagator)); + assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); }); }); }); From 15980842367ed991e298dd9107452c349f615b33 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Tue, 12 Jul 2022 09:00:42 +0300 Subject: [PATCH 10/12] fix: remove overrides from NodeTracerProvider --- .../src/NodeTracerProvider.ts | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts index e7e791611ec..4072851ce89 100644 --- a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts @@ -69,23 +69,4 @@ export class NodeTracerProvider extends BasicTracerProvider { super.register(config); } - - /** - * TS cannot yet infer the type of this.constructor: - * https://github.com/Microsoft/TypeScript/issues/3841#issuecomment-337560146 - * Do not override either of the getters in your child class unless you really need to. - * We have to do it here because the previous version of BasicTracerProvider's getters - * had BasicTracerProvider's static method hardcoded and we want do undo that behavior. - */ - protected override _getPropagator(name: string): TextMapPropagator | undefined { - return ( - (this.constructor as typeof BasicTracerProvider)._registeredPropagators - ).get(name)?.(); - } - - protected override _getSpanExporter(name: string): SpanExporter | undefined { - return ( - (this.constructor as typeof BasicTracerProvider)._registeredExporters - ).get(name)?.(); - } } From c9ff69321aba5a19d5de21e29b6103ec9a65f684 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Tue, 12 Jul 2022 10:32:33 +0300 Subject: [PATCH 11/12] feat: test the getters for components from parent class --- .../test/common/BasicTracerProvider.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts index d495ceae476..8575b044f67 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -29,9 +29,10 @@ import { } from '@opentelemetry/api'; import { CompositePropagator } from '@opentelemetry/core'; import { - AlwaysOnSampler, AlwaysOffSampler, + AlwaysOnSampler, TraceState, + W3CTraceContextPropagator, } from '@opentelemetry/core'; import { Resource } from '@opentelemetry/resources'; import * as assert from 'assert'; @@ -283,6 +284,7 @@ describe('BasicTracerProvider', () => { string, () => TextMapPropagator >([ + ...BasicTracerProvider._registeredPropagators, ['custom-propagator', () => new DummyPropagator()], ]); @@ -290,11 +292,15 @@ describe('BasicTracerProvider', () => { string, () => SpanExporter >([ + ...BasicTracerProvider._registeredExporters, ['custom-exporter', () => new DummyExporter()], ]); } const provider = new CustomTracerProvider({}); + assert(provider['_getPropagator']('tracecontext') instanceof W3CTraceContextPropagator); + /* BasicTracerProvider has no exporters by default, so skipping testing the exporter getter */ + provider.register(); const processor = provider.getActiveSpanProcessor(); assert(processor instanceof BatchSpanProcessor); From 61d43d56a88a8562741f6e6cd1ca7a01593b8d1c Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Tue, 12 Jul 2022 11:27:46 +0300 Subject: [PATCH 12/12] style: fix lint --- packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts index 4072851ce89..9d552162f64 100644 --- a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts @@ -13,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { TextMapPropagator } from '@opentelemetry/api'; import { AsyncHooksContextManager, AsyncLocalStorageContextManager, @@ -23,7 +22,6 @@ import { BasicTracerProvider, PROPAGATOR_FACTORY, SDKRegistrationConfig, - SpanExporter, } from '@opentelemetry/sdk-trace-base'; import * as semver from 'semver'; import { NodeTracerConfig } from './config';