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

refactor(sdk-trace-base): remove _registeredSpanProcessors from BasicTracerProvider #5177

Merged
merged 3 commits into from
Nov 21, 2024
Merged

refactor(sdk-trace-base): remove _registeredSpanProcessors from BasicTracerProvider #5177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c84e43
refactor(sdk-trace-base): remove _registeredSpanProcessors from Basic…
david-luna Nov 19, 2024
7b4a5a3
chore: update changelog
david-luna Nov 19, 2024
75b0843
chore: fix markdown lint issue
david-luna Nov 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
1 change: 1 addition & 0 deletions CHANGELOG_NEXT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,6 @@
### :house: (Internal)

* chore: remove checks for unsupported node versions [#4341](https://github.com/open-telemetry/opentelemetry-js/pull/4341) @dyladan
* refactor(sdk-trace-base): remove `BasicTracerProvider._registeredSpanProcessors` private property. [#5134](https://github.com/open-telemetry/opentelemetry-js/pull/5177) @david-luna

### :bug: (Bug Fix)
96 changes: 50 additions & 46 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 @@ -50,6 +50,7 @@ import {
NoopSpanProcessor,
IdGenerator,
AlwaysOffSampler,
SpanProcessor,
} from '@opentelemetry/sdk-trace-base';
import * as assert from 'assert';
import * as semver from 'semver';
Expand Down Expand Up @@ -257,13 +258,25 @@ describe('Node SDK', () => {
);
const apiTracerProvider =
trace.getTracerProvider() as ProxyTracerProvider;
assert.ok(apiTracerProvider.getDelegate() instanceof NodeTracerProvider);
const nodeTracerProvider = apiTracerProvider.getDelegate();

assert.ok(nodeTracerProvider instanceof NodeTracerProvider);

const spanProcessor = nodeTracerProvider.getActiveSpanProcessor() as any;

assert(
spanProcessor.constructor.name === 'MultiSpanProcessor',
'is MultiSpanProcessor'
);

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = spanProcessor[
'_spanProcessors'
] as SpanProcessor[];

assert(sdk['_tracerProvider'] instanceof NodeTracerProvider);
assert(listOfProcessors.length === 3);
assert(
listOfProcessors.length === 3,
'it has the right amount of processors'
);
assert(listOfProcessors[0] instanceof NoopSpanProcessor);
assert(listOfProcessors[1] instanceof SimpleSpanProcessor);
assert(listOfProcessors[2] instanceof BatchSpanProcessor);
Expand Down Expand Up @@ -1099,6 +1112,18 @@ describe('Node SDK', () => {
describe('setup exporter from env', () => {
let stubLoggerError: Sinon.SinonStub;

const getSdkSpanProcessors = (sdk: NodeSDK) => {
const tracerProvider = sdk['_tracerProvider'];

assert(tracerProvider instanceof NodeTracerProvider);

const activeSpanProcessor = tracerProvider.getActiveSpanProcessor();

assert(activeSpanProcessor.constructor.name === 'MultiSpanProcessor');

return (activeSpanProcessor as any)['_spanProcessors'] as SpanProcessor[];
};

beforeEach(() => {
stubLoggerError = Sinon.stub(diag, 'warn');
});
Expand All @@ -1109,8 +1134,7 @@ describe('setup exporter from env', () => {
it('should use default exporter when nor env neither SDK config is given', async () => {
const sdk = new NodeSDK();
sdk.start();
const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1124,8 +1148,7 @@ describe('setup exporter from env', () => {
traceExporter,
});
sdk.start();
const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1140,8 +1163,7 @@ describe('setup exporter from env', () => {
spanProcessor,
});
sdk.start();
const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof SimpleSpanProcessor);
Expand All @@ -1156,8 +1178,7 @@ describe('setup exporter from env', () => {
traceExporter,
});
sdk.start();
const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1172,8 +1193,7 @@ describe('setup exporter from env', () => {
sampler: new AlwaysOffSampler(),
});
sdk.start();
const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert.ok(
sdk['_tracerProvider']!['_config']?.sampler instanceof AlwaysOffSampler
Expand All @@ -1190,9 +1210,7 @@ describe('setup exporter from env', () => {
env.OTEL_EXPORTER_OTLP_TRACES_PROTOCOL = 'grpc';
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1208,9 +1226,7 @@ describe('setup exporter from env', () => {
env.OTEL_EXPORTER_OTLP_TRACES_PROTOCOL = 'grpc';
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1231,12 +1247,10 @@ describe('setup exporter from env', () => {
'OTEL_TRACES_EXPORTER contains "none". SDK will not be initialized.'
);

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const activeProcessor = sdk['_tracerProvider']?.getActiveSpanProcessor();
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 0);
assert(activeProcessor instanceof NoopSpanProcessor);
assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof NoopSpanProcessor);
delete env.OTEL_TRACES_EXPORTER;
await sdk.shutdown();
});
Expand All @@ -1246,8 +1260,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1267,8 +1280,7 @@ describe('setup exporter from env', () => {
'OTEL_TRACES_EXPORTER contains "none" along with other exporters. Using default otlp exporter.'
);

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand Down Expand Up @@ -1303,8 +1315,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1321,8 +1332,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 2);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1341,8 +1351,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1359,8 +1368,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 2);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1379,8 +1387,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 3);
assert(listOfProcessors[0] instanceof BatchSpanProcessor);
Expand All @@ -1400,8 +1407,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 2);
assert(listOfProcessors[0] instanceof SimpleSpanProcessor);
Expand All @@ -1417,8 +1423,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof SimpleSpanProcessor);
Expand All @@ -1433,8 +1438,7 @@ describe('setup exporter from env', () => {
const sdk = new NodeSDK();
sdk.start();

const listOfProcessors =
sdk['_tracerProvider']!['_registeredSpanProcessors']!;
const listOfProcessors = getSdkSpanProcessors(sdk);

assert(listOfProcessors.length === 1);
assert(listOfProcessors[0] instanceof SimpleSpanProcessor);
Expand Down
25 changes: 12 additions & 13 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 @@ -67,10 +67,9 @@ export class BasicTracerProvider implements TracerProvider {
>();

private readonly _config: TracerConfig;
private readonly _registeredSpanProcessors: SpanProcessor[] = [];
private readonly _tracers: Map<string, Tracer> = new Map();

private activeSpanProcessor: SpanProcessor;
private activeSpanProcessor: MultiSpanProcessor;
readonly resource: IResource;

constructor(config: TracerConfig = {}) {
Expand All @@ -89,20 +88,20 @@ export class BasicTracerProvider implements TracerProvider {
resource: this.resource,
});

const spanProcessors: SpanProcessor[] = [];

if (config.spanProcessors?.length) {
this._registeredSpanProcessors = [...config.spanProcessors];
this.activeSpanProcessor = new MultiSpanProcessor(
this._registeredSpanProcessors
);
spanProcessors.push(...config.spanProcessors);
} else {
const defaultExporter = this._buildExporterFromEnv();
if (defaultExporter !== undefined) {
const batchProcessor = new BatchSpanProcessor(defaultExporter);
this.activeSpanProcessor = batchProcessor;
} else {
this.activeSpanProcessor = new NoopSpanProcessor();
}
spanProcessors.push(
defaultExporter
? new BatchSpanProcessor(defaultExporter)
: new NoopSpanProcessor()
);
}

this.activeSpanProcessor = new MultiSpanProcessor(spanProcessors);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to reviewer: forceFlush method was only iterating over the _registeredSpanProcessors. So when using the default exporter and processor (the else case) the BatchSpanProcessor.forceFlush method was never called. I've added a test for it

}

getTracer(
Expand Down Expand Up @@ -154,7 +153,7 @@ export class BasicTracerProvider implements TracerProvider {

forceFlush(): Promise<void> {
const timeout = this._config.forceFlushTimeoutMillis;
const promises = this._registeredSpanProcessors.map(
const promises = this.activeSpanProcessor['_spanProcessors'].map(
(spanProcessor: SpanProcessor) => {
return new Promise(resolve => {
let state: ForceFlushState;
Expand Down
6 changes: 2 additions & 4 deletions packages/opentelemetry-sdk-trace-base/src/Span.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,8 @@ export class SpanImpl implements Span {
private readonly _startTimeProvided: boolean;

/**
* Constructs a new Span instance.
*
* @deprecated calling Span constructor directly is not supported. Please use tracer.startSpan.
Copy link
Contributor Author

@david-luna david-luna Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

note to reviewer: this is a leftover from #5048. The class is now internal and used in Tracer

* */
* Constructs a new SpanImpl instance.
*/
constructor(
parentTracer: Tracer,
context: Context,
Expand Down
Loading
Loading