Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow extending BasicTracerProvider with custom registered components #3023

Merged
merged 16 commits into from
Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,26 +45,51 @@ import {
BatchSpanProcessor,
} from '../../src';

describe('BasicTracerProvider', () => {
let removeEvent: (() => void) | undefined;
class DummyPropagator implements TextMapPropagator {
inject(
context: Context,
carrier: any,
setter: TextMapSetter<any>
): void {
throw new Error('Method not implemented.');
}
extract(
context: Context,
carrier: any,
getter: TextMapGetter<any>
): Context {
throw new Error('Method not implemented.');
}
fields(): string[] {
throw new Error('Method not implemented.');
}
}

class DummyExporter extends InMemorySpanExporter {}

describe('BasicTracerProvider', () => {
let envSource: Record<string, any>;
let setGlobalPropagatorStub: sinon.SinonSpy<
[TextMapPropagator],
boolean
>;

if (typeof process === 'undefined') {
envSource = (globalThis as unknown) as Record<string, any>;
} else {
envSource = process.env as Record<string, any>;
}

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', () => {
Expand Down Expand Up @@ -240,40 +265,99 @@ describe('BasicTracerProvider', () => {
});
});

describe('.register()', () => {
describe('propagator', () => {
class DummyPropagator implements TextMapPropagator {
inject(
context: Context,
carrier: any,
setter: TextMapSetter<any>
): void {
throw new Error('Method not implemented.');
}
extract(
context: Context,
carrier: any,
getter: TextMapGetter<any>
): 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()],
]);
}
dyladan marked this conversation as resolved.
Show resolved Hide resolved

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', () => {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
// 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) {
Expand All @@ -288,21 +372,20 @@ describe('BasicTracerProvider', () => {
provider.register({
propagator: new DummyPropagator(),
});
assert.ok(
setGlobalPropagatorStub.calledOnceWithExactly(
sinon.match.instanceOf(DummyPropagator)
)

sinon.assert.calledOnceWithExactly(
setGlobalPropagatorStub,
sinon.match.instanceOf(DummyPropagator)
);
});

it('should be composite if 2 or more propagators provided in an environment variable', () => {
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',
Expand All @@ -318,13 +401,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();
});
});

Expand Down
22 changes: 18 additions & 4 deletions packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
BasicTracerProvider,
PROPAGATOR_FACTORY,
SDKRegistrationConfig,
SpanExporter,
} from '@opentelemetry/sdk-trace-base';
import * as semver from 'semver';
import { NodeTracerConfig } from './config';
Expand All @@ -40,6 +41,7 @@ export class NodeTracerProvider extends BasicTracerProvider {
string,
PROPAGATOR_FACTORY
>([
...BasicTracerProvider._registeredPropagators,
[
'b3',
() =>
Expand Down Expand Up @@ -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)?.();
dyladan marked this conversation as resolved.
Show resolved Hide resolved
}
dyladan marked this conversation as resolved.
Show resolved Hide resolved
}
Loading