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.

Sign up for GitHub

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(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters #5239

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters #5239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a87a54
feat(sdk-trace): remove ability to statically register exporters to B…
pichlermarc Dec 6, 2024
2eae41a
fixup! feat(sdk-trace): remove ability to statically register exporte…
pichlermarc Dec 6, 2024
a84b54d
Merge branch 'next' into feat/remove-tracerprovider-exporter-extensib…
pichlermarc Dec 20, 2024
c342661
Merge branch 'main' into feat/remove-tracerprovider-exporter-extensib…
pichlermarc Dec 20, 2024
3e9a07a
chore: add changelog entry
pichlermarc Dec 20, 2024
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,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

### :rocket: (Enhancement)

Expand Down
5 changes: 2 additions & 3 deletions experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1470,7 +1470,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 @@ -1483,8 +1483,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
Open in desktop
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 @@ -92,13 +85,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 @@ -214,12 +200,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 @@ -256,16 +236,4 @@ export class BasicTracerProvider implements TracerProvider {
});
}
}

protected _buildExporterFromEnv(): SpanExporter | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so IIUC it means now BasicTraceProvider wont' look up to env to resolve the exporter and it is mandatory to pass it into the constructor right?

This means NodeSDK or any other component creating the provider is responsible to resolve the exporter from env if applies right?

Copy link
Member Author

@pichlermarc pichlermarc Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so IIUC it means now BasicTraceProvider wont' look up to env to resolve the exporter and it is mandatory to pass it into the constructor right?

This means NodeSDK or any other component creating the provider is responsible to resolve the exporter from env if applies right?

Yep - that's the solution I'd propose. 🙂

This PR is just a proposal, so we don't have to do it, but I feel like this is a cleaner approach as we'd eliminate an additional (confusing) way to accomplish the same thing.

Neither the LoggerProvider nor MeterProvider have equivalent functionality and I'm not aware of any requests that I remember that asked for adding it. 🤔 Removing it also has the bonus of aligning functionality between the three signals. I'd like to align the three signals as much as possible to give a more consistent experience to end-users.

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
Open in desktop
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
Loading