diff --git a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts index a964de5840..b0560fc3f1 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 { 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 e2b17bb561..8575b044f6 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'; @@ -45,10 +46,35 @@ import { BatchSpanProcessor, } from '../../src'; -describe('BasicTracerProvider', () => { - let removeEvent: (() => void) | undefined; +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 { @@ -56,15 +82,15 @@ 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(); }); afterEach(() => { sinon.restore(); - if (removeEvent) { - removeEvent(); - removeEvent = undefined; - } }); describe('constructor', () => { @@ -240,40 +266,104 @@ 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 + >([ + ...BasicTracerProvider._registeredPropagators, + ['custom-propagator', () => new DummyPropagator()], + ]); + + protected static override readonly _registeredExporters = new Map< + 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); + // @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; }); afterEach(() => { - setGlobalPropagatorStub.restore(); + sinon.restore(); // otherwise we may assign 'undefined' (a string) if (originalPropagators !== undefined) { @@ -288,10 +378,10 @@ describe('BasicTracerProvider', () => { provider.register({ propagator: new DummyPropagator(), }); - assert.ok( - setGlobalPropagatorStub.calledOnceWithExactly( - sinon.match.instanceOf(DummyPropagator) - ) + + sinon.assert.calledOnceWithExactly( + setGlobalPropagatorStub, + sinon.match.instanceOf(DummyPropagator) ); }); @@ -299,10 +389,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', @@ -318,13 +407,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(); }); }); diff --git a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts index 8e4575a390..9d552162f6 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, @@ -40,6 +39,7 @@ export class NodeTracerProvider extends BasicTracerProvider { string, PROPAGATOR_FACTORY >([ + ...BasicTracerProvider._registeredPropagators, [ 'b3', () => @@ -67,11 +67,4 @@ export class NodeTracerProvider extends BasicTracerProvider { super.register(config); } - - protected override _getPropagator(name: string): TextMapPropagator | undefined { - return ( - super._getPropagator(name) || - NodeTracerProvider._registeredPropagators.get(name)?.() - ); - } } diff --git a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts index 0d53e25eb6..24ba20f4b0 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) => @@ -34,18 +47,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 +57,6 @@ describe('NodeTracerProvider', () => { }); afterEach(() => { - // clear require cache - Object.keys(require.cache).forEach(key => delete require.cache[key]); contextManager.disable(); context.disable(); }); @@ -243,4 +245,122 @@ 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 {} + + beforeEach(() => { + 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', () => propagator], + ]); + + 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); + + 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', () => propagator], + ]); + + 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); + + assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); + }); + }); }); diff --git a/packages/opentelemetry-sdk-trace-node/test/registration.test.ts b/packages/opentelemetry-sdk-trace-node/test/registration.test.ts index 4db2d334b4..09cc24b327 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);