Skip to content

Commit

Permalink
feat(sdk-trace)!: remove ability to have BasicTracerProvider instanti…
Browse files Browse the repository at this point in the history 
…ate exporters (#5239)
  • Loading branch information
@pichlermarc
pichlermarc authored Jan 21, 2025
1 parent 6c04a41 commit c00f36e
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 254 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se
* refactor(sdk-trace-base)!: remove `new Span` constructor in favor of `Tracer.startSpan` API [#5048](https://github.com/open-telemetry/opentelemetry-js/pull/5048) @david-luna
* refactor(sdk-trace-base)!: remove `BasicTracerProvider.addSpanProcessor` API in favor of constructor options. [#5134](https://github.com/open-telemetry/opentelemetry-js/pull/5134) @david-luna
* refactor(sdk-trace-base)!: make `resource` property private in `BasicTracerProvider` and remove `getActiveSpanProcessor` API. [#5192](https://github.com/open-telemetry/opentelemetry-js/pull/5192) @david-luna
* feat(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters [#5239](https://github.com/open-telemetry/opentelemetry-js/pull/5239) @pichlermarc
* When extending `BasicTracerProvider`, the class offered multiple methods to facilitate the creation of exporters and auto-pairing with `SpanProcessor`s.
* This functionality has been removed - users may now pass `SpanProcessor`s to the base class constructor when extending
* (user-facing): `_registeredExporters` has been removed
* (user-facing): `_getSpanExporter` has been removed
* (user-facing): `_buildExporterFromEnv` has been removed
* feat(core)!: remove deprecated `IdGenerator` and `RandomIdGenerator` [#5309](https://github.com/open-telemetry/opentelemetry-js/pull/5309) @pichlermarc
* feat(core)!: remove deprecated type `InstrumentationLibrary` [#5308](https://github.com/open-telemetry/opentelemetry-js/pull/5308) @pichlermarc
* (user-facing): please use equivalent type `InstrumentationScope` instead
Expand Down
1 change: 0 additions & 1 deletion experimental/packages/opentelemetry-sdk-node/src/sdk.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,6 @@ export class NodeSDK {
? this._tracerProviderConfig.spanProcessors
: getSpanProcessorsFromEnv();

// If the Provider is configured with Env Exporters, we need to check if the SDK had any manual configurations and set them here
this._tracerProvider = new NodeTracerProvider({
...this._configuration,
resource: this._resource,
Expand Down
5 changes: 2 additions & 3 deletions experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1465,7 +1465,7 @@ describe('setup exporter from env', () => {
await sdk.shutdown();
});

it('should use noop span processor when user sets env exporter to none', async () => {
it('should use empty span processor when user sets env exporter to none', async () => {
env.OTEL_TRACES_EXPORTER = 'none';
const sdk = new NodeSDK();
sdk.start();
Expand All @@ -1478,8 +1478,7 @@ describe('setup exporter from env', () => {

const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof NoopSpanProcessor);
assert.strictEqual(listOfProcessors.length, 0);
delete env.OTEL_TRACES_EXPORTER;
await sdk.shutdown();
});
Expand Down
32 changes: 0 additions & 32 deletions packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,8 @@ import { SpanProcessor } from './SpanProcessor';
import { Tracer } from './Tracer';
import { loadDefaultConfig } from './config';
import { MultiSpanProcessor } from './MultiSpanProcessor';
import { NoopSpanProcessor } from './export/NoopSpanProcessor';
import { SDKRegistrationConfig, TracerConfig } from './types';
import { SpanExporter } from './export/SpanExporter';
import { BatchSpanProcessor } from './platform';
import { reconfigureLimits } from './utility';

export type PROPAGATOR_FACTORY = () => TextMapPropagator;
Expand All @@ -62,11 +60,6 @@ export class BasicTracerProvider implements TracerProvider {
['baggage', () => new W3CBaggagePropagator()],
]);

protected static readonly _registeredExporters = new Map<
string,
EXPORTER_FACTORY
>();

private readonly _config: TracerConfig;
private readonly _tracers: Map<string, Tracer> = new Map();
private readonly _resource: IResource;
Expand All @@ -88,13 +81,6 @@ export class BasicTracerProvider implements TracerProvider {

if (config.spanProcessors?.length) {
spanProcessors.push(...config.spanProcessors);
} else {
const defaultExporter = this._buildExporterFromEnv();
spanProcessors.push(
defaultExporter
? new BatchSpanProcessor(defaultExporter)
: new NoopSpanProcessor()
);
}

this._activeSpanProcessor = new MultiSpanProcessor(spanProcessors);
Expand Down Expand Up @@ -210,12 +196,6 @@ export class BasicTracerProvider implements TracerProvider {
)._registeredPropagators.get(name)?.();
}

protected _getSpanExporter(name: string): SpanExporter | undefined {
return (
this.constructor as typeof BasicTracerProvider
)._registeredExporters.get(name)?.();
}

protected _buildPropagatorFromEnv(): TextMapPropagator | undefined {
// per spec, propagators from env must be deduplicated
const uniquePropagatorNames = Array.from(
Expand Down Expand Up @@ -252,16 +232,4 @@ export class BasicTracerProvider implements TracerProvider {
});
}
}

protected _buildExporterFromEnv(): SpanExporter | undefined {
const exporterName = getEnv().OTEL_TRACES_EXPORTER;
if (exporterName === 'none' || exporterName === '') return;
const exporter = this._getSpanExporter(exporterName);
if (!exporter) {
diag.error(
`Exporter "${exporterName}" requested through environment variable is unavailable.`
);
}
return exporter;
}
}
177 changes: 10 additions & 167 deletions packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ import {
BasicTracerProvider,
NoopSpanProcessor,
Span,
InMemorySpanExporter,
SpanExporter,
BatchSpanProcessor,
AlwaysOnSampler,
AlwaysOffSampler,
ConsoleSpanExporter,
Expand All @@ -59,8 +56,6 @@ class DummyPropagator implements TextMapPropagator {
}
}

class DummyExporter extends InMemorySpanExporter {}

describe('BasicTracerProvider', () => {
let envSource: Record<string, any>;
let setGlobalPropagatorStub: sinon.SinonSpy<[TextMapPropagator], boolean>;
Expand Down Expand Up @@ -90,55 +85,19 @@ describe('BasicTracerProvider', () => {
assert.ok(tracer instanceof BasicTracerProvider);
});

it('should use noop span processor by default', () => {
const tracer = new BasicTracerProvider();
assert.ok(tracer['_activeSpanProcessor'] instanceof MultiSpanProcessor);
assert.ok(
tracer['_activeSpanProcessor']['_spanProcessors'].length === 1
);
assert.ok(
tracer['_activeSpanProcessor']['_spanProcessors'][0] instanceof
NoopSpanProcessor
);
});
it('should use noop span processor by default and no diag error', () => {
it('should use empty span processor by default', () => {
const errorStub = sinon.spy(diag, 'error');
const tracer = new BasicTracerProvider();

assert.ok(tracer['_activeSpanProcessor'] instanceof MultiSpanProcessor);
assert.ok(
tracer['_activeSpanProcessor']['_spanProcessors'].length === 1
);
assert.ok(
tracer['_activeSpanProcessor']['_spanProcessors'][0] instanceof
NoopSpanProcessor
assert.strictEqual(
tracer['_activeSpanProcessor']['_spanProcessors'].length,
0
);
sinon.assert.notCalled(errorStub);
});
});

describe('when user sets unavailable exporter', () => {
it('should use noop span processor by default and show diag error', () => {
envSource.OTEL_TRACES_EXPORTER = 'someExporter';
const errorStub = sinon.spy(diag, 'error');
const tracer = new BasicTracerProvider();

assert.ok(tracer['_activeSpanProcessor'] instanceof MultiSpanProcessor);
assert.ok(
tracer['_activeSpanProcessor']['_spanProcessors'].length === 1
);
assert.ok(
tracer['_activeSpanProcessor']['_spanProcessors'][0] instanceof
NoopSpanProcessor
);
sinon.assert.calledWith(
errorStub,
'Exporter "someExporter" requested through environment variable is unavailable.'
);
delete envSource.OTEL_TRACES_EXPORTER;
});
});

describe('when user sets span processors', () => {
it('should use the span processors defined in the config', () => {
const traceExporter = new ConsoleSpanExporter();
Expand Down Expand Up @@ -448,14 +407,6 @@ describe('BasicTracerProvider', () => {
...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({});
Expand All @@ -465,19 +416,9 @@ describe('BasicTracerProvider', () => {
);
/* BasicTracerProvider has no exporters by default, so skipping testing the exporter getter */
provider.register();

assert.ok(provider['_activeSpanProcessor'] instanceof MultiSpanProcessor);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'].length === 1
);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'][0] instanceof
BatchSpanProcessor
);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'][0][
'_exporter'
] instanceof DummyExporter
assert.strictEqual(
provider['_activeSpanProcessor']['_spanProcessors'].length,
0
);

sinon.assert.calledOnceWithExactly(
Expand All @@ -494,11 +435,6 @@ describe('BasicTracerProvider', () => {
() => 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 {
Expand All @@ -507,32 +443,13 @@ describe('BasicTracerProvider', () => {
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();

assert.ok(provider['_activeSpanProcessor'] instanceof MultiSpanProcessor);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'].length === 1
);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'][0] instanceof
BatchSpanProcessor
);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'][0][
'_exporter'
] instanceof DummyExporter
assert.strictEqual(
provider['_activeSpanProcessor']['_spanProcessors'].length,
0
);

sinon.assert.calledOnceWithExactly(
Expand Down Expand Up @@ -600,57 +517,6 @@ describe('BasicTracerProvider', () => {
);
});
});

describe('exporter', () => {
class CustomTracerProvider extends BasicTracerProvider {
protected override _getSpanExporter(
name: string
): SpanExporter | undefined {
return name === 'memory'
? new InMemorySpanExporter()
: BasicTracerProvider._registeredExporters.get(name)?.();
}
}

afterEach(() => {
delete envSource.OTEL_TRACES_EXPORTER;
});

it('logs error if there is no exporter registered with a given name', () => {
const errorStub = sinon.spy(diag, 'error');

envSource.OTEL_TRACES_EXPORTER = 'missing-exporter';
const provider = new BasicTracerProvider({});
provider.register();
assert.ok(
errorStub.getCall(0).args[0] ===
'Exporter "missing-exporter" requested through environment variable is unavailable.'
);
errorStub.restore();
});

it('registers trace exporter from environment variable', () => {
envSource.OTEL_TRACES_EXPORTER = 'memory';
const provider = new CustomTracerProvider({});
provider.register();

assert.ok(
provider['_activeSpanProcessor'] instanceof MultiSpanProcessor
);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'].length === 1
);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'][0] instanceof
BatchSpanProcessor
);
assert.ok(
provider['_activeSpanProcessor']['_spanProcessors'][0][
'_exporter'
] instanceof InMemorySpanExporter
);
});
});
});

describe('.startSpan()', () => {
Expand Down Expand Up @@ -834,29 +700,6 @@ describe('BasicTracerProvider', () => {
});

describe('.forceFlush()', () => {
it('should call forceFlush with the default processor', done => {
sinon.restore();
const forceFlushStub = sinon.stub(
NoopSpanProcessor.prototype,
'forceFlush'
);
forceFlushStub.resolves();

const tracerProvider = new BasicTracerProvider();

tracerProvider
.forceFlush()
.then(() => {
sinon.restore();
assert(forceFlushStub.calledOnce);
done();
})
.catch(error => {
sinon.restore();
done(error);
});
});

it('should call forceFlush on all registered span processors', done => {
sinon.restore();
const forceFlushStub = sinon.stub(
Expand Down
Loading

0 comments on commit c00f36e

Please sign in to comment.