From 64df1212e1c6c7b5f5dcc9b5a0a922998f68a3e9 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Tue, 21 Jan 2025 17:22:38 +0100 Subject: [PATCH 1/8] feat(sdk-trace-base)!: drop ability to instantiate propagators beyond defaults --- .../opentelemetry-sdk-node/src/sdk.ts | 5 +- .../opentelemetry-sdk-node/src/utils.ts | 74 +++++++++- .../opentelemetry-sdk-node/test/sdk.test.ts | 14 ++ .../opentelemetry-sdk-node/test/utils.test.ts | 103 ++++++++++++++ package-lock.json | 4 - .../src/BasicTracerProvider.ts | 77 ++--------- .../test/common/BasicTracerProvider.test.ts | 106 +------------- .../opentelemetry-sdk-trace-node/package.json | 2 - .../src/NodeTracerProvider.ts | 20 --- .../test/NodeTracerProvider.test.ts | 130 ------------------ .../tsconfig.json | 6 - 11 files changed, 207 insertions(+), 334 deletions(-) create mode 100644 experimental/packages/opentelemetry-sdk-node/test/utils.test.ts diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index e3920bfa395..178722b0d3d 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -74,6 +74,7 @@ import { getResourceDetectorsFromEnv, getSpanProcessorsFromEnv, filterBlanksAndNulls, + getPropgagatorFromEnv, } from './utils'; /** This class represents everything needed to register a fully configured OpenTelemetry Node.js SDK */ @@ -376,7 +377,9 @@ export class NodeSDK { this._tracerProviderConfig?.contextManager ?? // _tracerProviderConfig may be undefined if trace-specific settings are not provided - fall back to raw config this._configuration?.contextManager, - propagator: this._tracerProviderConfig?.textMapPropagator, + propagator: + this._tracerProviderConfig?.textMapPropagator ?? + getPropgagatorFromEnv(), }); } diff --git a/experimental/packages/opentelemetry-sdk-node/src/utils.ts b/experimental/packages/opentelemetry-sdk-node/src/utils.ts index 8b73744f489..12e11de9c75 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/utils.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/utils.ts @@ -14,8 +14,13 @@ * limitations under the License. */ -import { diag } from '@opentelemetry/api'; -import { getEnv, getEnvWithoutDefaults } from '@opentelemetry/core'; +import { diag, TextMapPropagator } from '@opentelemetry/api'; +import { + CompositePropagator, + getEnv, + getEnvWithoutDefaults, + W3CTraceContextPropagator, +} from '@opentelemetry/core'; import { OTLPTraceExporter as OTLPProtoTraceExporter } from '@opentelemetry/exporter-trace-otlp-proto'; import { OTLPTraceExporter as OTLPHttpTraceExporter } from '@opentelemetry/exporter-trace-otlp-http'; import { OTLPTraceExporter as OTLPGrpcTraceExporter } from '@opentelemetry/exporter-trace-otlp-grpc'; @@ -35,6 +40,8 @@ import { SpanExporter, SpanProcessor, } from '@opentelemetry/sdk-trace-base'; +import { B3InjectEncoding, B3Propagator } from '@opentelemetry/propagator-b3'; +import { JaegerPropagator } from '@opentelemetry/propagator-jaeger'; const RESOURCE_DETECTOR_ENVIRONMENT = 'env'; const RESOURCE_DETECTOR_HOST = 'host'; @@ -180,3 +187,66 @@ export function getSpanProcessorsFromEnv(): SpanProcessor[] { return processors; } + +/** + * Get a propagator as defined by environment variables + */ +export function getPropgagatorFromEnv(): TextMapPropagator | null | undefined { + // Empty and undefined MUST be treated equal. + if ( + process.env.OTEL_PROPAGATORS === undefined || + process.env.OTEL_PROPAGATORS?.trim() === '' + ) { + // return undefined to fall back to default + return undefined; + } + + // Implementation note: this only contains specification required propagators that are actually hosted in this repo. + // Any other propagators (like aws, aws-lambda, should go into `@opentelemetry/auto-configuration-propagators` instead). + const propagatorsFactory = new Map TextMapPropagator>([ + ['tracecontext', () => new W3CTraceContextPropagator()], + ['baggage', () => new W3CTraceContextPropagator()], + ['b3', () => new B3Propagator()], + [ + 'b3multi', + () => new B3Propagator({ injectEncoding: B3InjectEncoding.MULTI_HEADER }), + ], + ['jaeger', () => new JaegerPropagator()], + ]); + + // Values MUST be deduplicated in order to register a Propagator only once. + const uniquePropagatorNames = Array.from(new Set(getEnv().OTEL_PROPAGATORS)); + + const propagators = uniquePropagatorNames.map(name => { + const propagator = propagatorsFactory.get(name)?.(); + if (!propagator) { + diag.warn( + `Propagator "${name}" requested through environment variable is unavailable.` + ); + return undefined; + } + + return propagator; + }); + + const validPropagators = propagators.reduce( + (list, item) => { + if (item) { + list.push(item); + } + return list; + }, + [] + ); + + if (validPropagators.length === 0) { + // null to signal that the default should **not** be used in its place. + return null; + } else if (uniquePropagatorNames.length === 1) { + return validPropagators[0]; + } else { + return new CompositePropagator({ + propagators: validPropagators, + }); + } +} diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 52f326d7758..3caf794f072 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -414,6 +414,20 @@ describe('Node SDK', () => { assert.equal(actualContextManager, expectedContextManager); await sdk.shutdown(); }); + + it('should register a propagators as defined in OTEL_PROPAGATORS if trace SDK is configured', async () => { + process.env.OTEL_PROPAGATORS = 'b3'; + const sdk = new NodeSDK({ + traceExporter: new ConsoleSpanExporter(), + autoDetectResources: false, + }); + + sdk.start(); + + assert.deepStrictEqual(propagation.fields(), ['b3']); + + await sdk.shutdown(); + }); }); async function waitForNumberOfMetrics( diff --git a/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts b/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts new file mode 100644 index 00000000000..63861c42519 --- /dev/null +++ b/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts @@ -0,0 +1,103 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { getPropgagatorFromEnv } from '../src/utils'; +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { diag } from '@opentelemetry/api'; + +describe('getPropagatorFromEnv', function () { + afterEach(() => { + delete process.env.OTEL_PROPAGATORS; + sinon.restore(); + }); + + describe('should default to undefined', function () { + it('when not defined', function () { + delete process.env.OTEL_PROPAGATORS; + + const propagator = getPropgagatorFromEnv(); + + assert.deepStrictEqual(propagator, undefined); + }); + + it('on empty string', function () { + (process.env as any).OTEL_PROPAGATORS = ''; + + const propagator = getPropgagatorFromEnv(); + + assert.deepStrictEqual(propagator, undefined); + }); + + it('on space-only string', function () { + (process.env as any).OTEL_PROPAGATORS = ' '; + + const propagator = getPropgagatorFromEnv(); + + assert.deepStrictEqual(propagator, undefined); + }); + }); + + it('should return the selected propagator when one is in the list', () => { + process.env.OTEL_PROPAGATORS = 'tracecontext'; + assert.deepStrictEqual(getPropgagatorFromEnv()?.fields(), [ + 'traceparent', + 'tracestate', + ]); + }); + + it('should return the selected propagators when multiple are in the list', () => { + process.env.OTEL_PROPAGATORS = 'b3,jaeger'; + assert.deepStrictEqual(getPropgagatorFromEnv()?.fields(), [ + 'b3', + 'uber-trace-id', + ]); + }); + + it('should return the selected propagators when multiple are in the list', () => { + process.env.OTEL_PROPAGATORS = 'b3,jaeger'; + assert.deepStrictEqual(getPropgagatorFromEnv()?.fields(), [ + 'b3', + 'uber-trace-id', + ]); + }); + + it('should return null and warn if propagators are unknown', () => { + const warnStub = sinon.stub(diag, 'warn'); + + process.env.OTEL_PROPAGATORS = 'my, unknown, propagators'; + assert.deepStrictEqual(getPropgagatorFromEnv(), null); + sinon.assert.calledWithExactly( + warnStub, + 'Propagator "my" requested through environment variable is unavailable.' + ); + sinon.assert.calledWithExactly( + warnStub, + 'Propagator "unknown" requested through environment variable is unavailable.' + ); + sinon.assert.calledWithExactly( + warnStub, + 'Propagator "propagators" requested through environment variable is unavailable.' + ); + sinon.assert.calledThrice(warnStub); + }); + + it('should return null if only "none" is selected', () => { + process.env.OTEL_PROPAGATORS = 'none'; + + assert.deepStrictEqual(getPropgagatorFromEnv(), null); + }); +}); diff --git a/package-lock.json b/package-lock.json index ece793596b0..d2baab1f465 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29711,8 +29711,6 @@ "dependencies": { "@opentelemetry/context-async-hooks": "1.30.0", "@opentelemetry/core": "1.30.0", - "@opentelemetry/propagator-b3": "1.30.0", - "@opentelemetry/propagator-jaeger": "1.30.0", "@opentelemetry/sdk-trace-base": "1.30.0" }, "devDependencies": { @@ -36732,8 +36730,6 @@ "@opentelemetry/api": ">=1.0.0 <1.10.0", "@opentelemetry/context-async-hooks": "1.30.0", "@opentelemetry/core": "1.30.0", - "@opentelemetry/propagator-b3": "1.30.0", - "@opentelemetry/propagator-jaeger": "1.30.0", "@opentelemetry/resources": "1.30.0", "@opentelemetry/sdk-trace-base": "1.30.0", "@opentelemetry/semantic-conventions": "1.28.0", diff --git a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts index 9bbf86bbb9c..dde3d10e060 100644 --- a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts @@ -16,7 +16,6 @@ import { context, - diag, propagation, TextMapPropagator, trace, @@ -26,7 +25,6 @@ import { CompositePropagator, W3CBaggagePropagator, W3CTraceContextPropagator, - getEnv, merge, } from '@opentelemetry/core'; import { IResource, Resource } from '@opentelemetry/resources'; @@ -48,18 +46,14 @@ export enum ForceFlushState { 'unresolved', } +function getDefaultPropagators(): TextMapPropagator[] { + return [new W3CTraceContextPropagator(), new W3CBaggagePropagator()]; +} + /** * This class represents a basic tracer provider which platform libraries can extend */ export class BasicTracerProvider implements TracerProvider { - protected static readonly _registeredPropagators = new Map< - string, - PROPAGATOR_FACTORY - >([ - ['tracecontext', () => new W3CTraceContextPropagator()], - ['baggage', () => new W3CBaggagePropagator()], - ]); - private readonly _config: TracerConfig; private readonly _tracers: Map = new Map(); private readonly _resource: IResource; @@ -117,16 +111,19 @@ export class BasicTracerProvider implements TracerProvider { */ register(config: SDKRegistrationConfig = {}): void { trace.setGlobalTracerProvider(this); - if (config.propagator === undefined) { - config.propagator = this._buildPropagatorFromEnv(); - } if (config.contextManager) { context.setGlobalContextManager(config.contextManager); } - if (config.propagator) { - propagation.setGlobalPropagator(config.propagator); + // undefined means "unset", null means don't register propagator + if (config.propagator !== null) { + propagation.setGlobalPropagator( + config.propagator ?? + new CompositePropagator({ + propagators: getDefaultPropagators(), + }) + ); } } @@ -182,54 +179,4 @@ export class BasicTracerProvider implements TracerProvider { shutdown(): Promise { 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 ( - this.constructor as typeof BasicTracerProvider - )._registeredPropagators.get(name)?.(); - } - - protected _buildPropagatorFromEnv(): TextMapPropagator | undefined { - // per spec, propagators from env must be deduplicated - const uniquePropagatorNames = Array.from( - new Set(getEnv().OTEL_PROPAGATORS) - ); - - const propagators = uniquePropagatorNames.map(name => { - const propagator = this._getPropagator(name); - if (!propagator) { - diag.warn( - `Propagator "${name}" requested through environment variable is unavailable.` - ); - } - - return propagator; - }); - const validPropagators = propagators.reduce( - (list, item) => { - if (item) { - list.push(item); - } - return list; - }, - [] - ); - - if (validPropagators.length === 0) { - return; - } else if (uniquePropagatorNames.length === 1) { - return validPropagators[0]; - } else { - return new CompositePropagator({ - propagators: validPropagators, - }); - } - } } 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 ddb752fc57c..2ead66bb9c9 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -28,7 +28,7 @@ import { diag, } from '@opentelemetry/api'; import { CompositePropagator } from '@opentelemetry/core'; -import { TraceState, W3CTraceContextPropagator } from '@opentelemetry/core'; +import { TraceState } from '@opentelemetry/core'; import { Resource } from '@opentelemetry/resources'; import * as assert from 'assert'; import * as sinon from 'sinon'; @@ -386,97 +386,8 @@ describe('BasicTracerProvider', () => { }); }); - 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()], - ]); - } - - const provider = new CustomTracerProvider({}); - assert( - provider['_getPropagator']('tracecontext') instanceof - W3CTraceContextPropagator - ); - /* BasicTracerProvider has no exporters by default, so skipping testing the exporter getter */ - provider.register(); - assert.strictEqual( - provider['_activeSpanProcessor']['_spanProcessors'].length, - 0 - ); - - 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 override _getPropagator( - name: string - ): TextMapPropagator | undefined { - return ( - super._getPropagator(name) || - CustomTracerProvider._registeredPropagators.get(name)?.() - ); - } - } - - const provider = new CustomTracerProvider({}); - provider.register(); - assert.strictEqual( - provider['_activeSpanProcessor']['_spanProcessors'].length, - 0 - ); - - sinon.assert.calledOnceWithExactly( - setGlobalPropagatorStub, - sinon.match.instanceOf(DummyPropagator) - ); - }); - }); - describe('.register()', () => { describe('propagator', () => { - let originalPropagators: string | number | undefined | string[]; - beforeEach(() => { - originalPropagators = envSource.OTEL_PROPAGATORS; - }); - - afterEach(() => { - sinon.restore(); - - // otherwise we may assign 'undefined' (a string) - if (originalPropagators !== undefined) { - envSource.OTEL_PROPAGATORS = originalPropagators; - } else { - delete envSource.OTEL_PROPAGATORS; - } - }); - it('should be set to a given value if it it provided', () => { const provider = new BasicTracerProvider(); provider.register({ @@ -489,7 +400,7 @@ describe('BasicTracerProvider', () => { ); }); - it('should be composite if 2 or more propagators provided in an environment variable', () => { + it('should use w3c trace context and baggage propagators by default', () => { const provider = new BasicTracerProvider(); provider.register(); @@ -503,19 +414,6 @@ describe('BasicTracerProvider', () => { 'baggage', ]); }); - - it('warns if there is no propagator registered with a given name', () => { - const warnStub = sinon.spy(diag, 'warn'); - - envSource.OTEL_PROPAGATORS = 'missing-propagator'; - const provider = new BasicTracerProvider({}); - provider.register(); - - sinon.assert.calledOnceWithExactly( - warnStub, - 'Propagator "missing-propagator" requested through environment variable is unavailable.' - ); - }); }); }); diff --git a/packages/opentelemetry-sdk-trace-node/package.json b/packages/opentelemetry-sdk-trace-node/package.json index 22a6b056e23..a0c1afdf928 100644 --- a/packages/opentelemetry-sdk-trace-node/package.json +++ b/packages/opentelemetry-sdk-trace-node/package.json @@ -64,8 +64,6 @@ "dependencies": { "@opentelemetry/context-async-hooks": "1.30.0", "@opentelemetry/core": "1.30.0", - "@opentelemetry/propagator-b3": "1.30.0", - "@opentelemetry/propagator-jaeger": "1.30.0", "@opentelemetry/sdk-trace-base": "1.30.0" }, "homepage": "https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-sdk-trace-node", diff --git a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts index abb25f4db91..ee6c67dbbbc 100644 --- a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts @@ -14,14 +14,11 @@ * limitations under the License. */ import { AsyncLocalStorageContextManager } from '@opentelemetry/context-async-hooks'; -import { B3Propagator, B3InjectEncoding } from '@opentelemetry/propagator-b3'; import { BasicTracerProvider, - PROPAGATOR_FACTORY, SDKRegistrationConfig, } from '@opentelemetry/sdk-trace-base'; import { NodeTracerConfig } from './config'; -import { JaegerPropagator } from '@opentelemetry/propagator-jaeger'; /** * Register this TracerProvider for use with the OpenTelemetry API. @@ -31,23 +28,6 @@ import { JaegerPropagator } from '@opentelemetry/propagator-jaeger'; * @param config Configuration object for SDK registration */ export class NodeTracerProvider extends BasicTracerProvider { - protected static override readonly _registeredPropagators = new Map< - string, - PROPAGATOR_FACTORY - >([ - ...BasicTracerProvider._registeredPropagators, - [ - 'b3', - () => - new B3Propagator({ injectEncoding: B3InjectEncoding.SINGLE_HEADER }), - ], - [ - 'b3multi', - () => new B3Propagator({ injectEncoding: B3InjectEncoding.MULTI_HEADER }), - ], - ['jaeger', () => new JaegerPropagator()], - ]); - constructor(config: NodeTracerConfig = {}) { super(config); } diff --git a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts index e076ad0c71d..6acf1b16b45 100644 --- a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts @@ -14,18 +14,12 @@ * 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, } from '@opentelemetry/api'; @@ -211,128 +205,4 @@ describe('NodeTracerProvider', () => { return patchedFn(); }); }); - - describe('.register()', () => { - let originalPropagators: string | number | undefined | string[]; - beforeEach(() => { - originalPropagators = process.env.OTEL_PROPAGATORS; - }); - - afterEach(() => { - // otherwise we may assign 'undefined' (a string) - if (originalPropagators !== undefined) { - (process.env as any).OTEL_PROPAGATORS = originalPropagators; - } else { - delete (process.env as any).OTEL_PROPAGATORS; - } - }); - - it('should allow propagators as per the specification', () => { - (process.env as any).OTEL_PROPAGATORS = 'b3,b3multi,jaeger'; - - const provider = new NodeTracerProvider(); - provider.register(); - - assert.deepStrictEqual(propagation.fields(), [ - 'b3', - 'x-b3-traceid', - 'x-b3-spanid', - 'x-b3-flags', - 'x-b3-sampled', - 'x-b3-parentspanid', - 'uber-trace-id', - ]); - }); - }); - - 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.'); - } - } - - beforeEach(() => { - process.env.OTEL_PROPAGATORS = 'custom-propagator'; - - propagation.disable(); - trace.disable(); - }); - - afterEach(() => { - 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]]); - } - - const provider = new CustomTracerProvider({}); - provider.register(); - assert.ok( - provider['_activeSpanProcessor'].constructor.name === - 'MultiSpanProcessor' - ); - assert.strictEqual( - provider['_activeSpanProcessor']['_spanProcessors'].length, - 0 - ); - 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 override _getPropagator( - name: string - ): TextMapPropagator | undefined { - return ( - super._getPropagator(name) || - CustomTracerProvider._registeredPropagators.get(name)?.() - ); - } - } - - const provider = new CustomTracerProvider({}); - provider.register(); - assert.ok( - provider['_activeSpanProcessor'].constructor.name === - 'MultiSpanProcessor' - ); - assert.strictEqual( - provider['_activeSpanProcessor']['_spanProcessors'].length, - 0 - ); - - assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); - }); - }); }); diff --git a/packages/opentelemetry-sdk-trace-node/tsconfig.json b/packages/opentelemetry-sdk-trace-node/tsconfig.json index 60f86e96760..a216dba8e4f 100644 --- a/packages/opentelemetry-sdk-trace-node/tsconfig.json +++ b/packages/opentelemetry-sdk-trace-node/tsconfig.json @@ -21,12 +21,6 @@ { "path": "../opentelemetry-core" }, - { - "path": "../opentelemetry-propagator-b3" - }, - { - "path": "../opentelemetry-propagator-jaeger" - }, { "path": "../opentelemetry-resources" }, From c0abc8d3f86fa788d3b88d92225376659207ba66 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Tue, 21 Jan 2025 17:54:03 +0100 Subject: [PATCH 2/8] fixup! feat(sdk-trace-base)!: drop ability to instantiate propagators beyond defaults --- experimental/packages/opentelemetry-sdk-node/package.json | 4 +++- experimental/packages/opentelemetry-sdk-node/tsconfig.json | 6 ++++++ package-lock.json | 4 ++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-sdk-node/package.json b/experimental/packages/opentelemetry-sdk-node/package.json index 4b952a8d3fa..3e1fb29b8d5 100644 --- a/experimental/packages/opentelemetry-sdk-node/package.json +++ b/experimental/packages/opentelemetry-sdk-node/package.json @@ -63,7 +63,9 @@ "@opentelemetry/sdk-metrics": "1.30.0", "@opentelemetry/sdk-trace-base": "1.30.0", "@opentelemetry/sdk-trace-node": "1.30.0", - "@opentelemetry/semantic-conventions": "1.28.0" + "@opentelemetry/semantic-conventions": "1.28.0", + "@opentelemetry/propagator-jaeger": "1.30.0", + "@opentelemetry/propagator-b3": "1.30.0" }, "peerDependencies": { "@opentelemetry/api": ">=1.3.0 <1.10.0" diff --git a/experimental/packages/opentelemetry-sdk-node/tsconfig.json b/experimental/packages/opentelemetry-sdk-node/tsconfig.json index 7d18aac481d..e14ef0d799d 100644 --- a/experimental/packages/opentelemetry-sdk-node/tsconfig.json +++ b/experimental/packages/opentelemetry-sdk-node/tsconfig.json @@ -24,6 +24,12 @@ { "path": "../../../packages/opentelemetry-exporter-zipkin" }, + { + "path": "../../../packages/opentelemetry-propagator-b3" + }, + { + "path": "../../../packages/opentelemetry-propagator-jaeger" + }, { "path": "../../../packages/opentelemetry-resources" }, diff --git a/package-lock.json b/package-lock.json index d2baab1f465..93004bbdf11 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2945,6 +2945,8 @@ "@opentelemetry/exporter-trace-otlp-proto": "0.57.0", "@opentelemetry/exporter-zipkin": "1.30.0", "@opentelemetry/instrumentation": "0.57.0", + "@opentelemetry/propagator-b3": "1.30.0", + "@opentelemetry/propagator-jaeger": "1.30.0", "@opentelemetry/resources": "1.30.0", "@opentelemetry/sdk-logs": "0.57.0", "@opentelemetry/sdk-metrics": "1.30.0", @@ -36548,6 +36550,8 @@ "@opentelemetry/exporter-trace-otlp-proto": "0.57.0", "@opentelemetry/exporter-zipkin": "1.30.0", "@opentelemetry/instrumentation": "0.57.0", + "@opentelemetry/propagator-b3": "1.30.0", + "@opentelemetry/propagator-jaeger": "1.30.0", "@opentelemetry/resources": "1.30.0", "@opentelemetry/sdk-logs": "0.57.0", "@opentelemetry/sdk-metrics": "1.30.0", From ffe6c84aaaf96b64f94a160f3c89695d118ae08d Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Tue, 21 Jan 2025 18:10:28 +0100 Subject: [PATCH 3/8] fixup! feat(sdk-trace-base)!: drop ability to instantiate propagators beyond defaults --- CHANGELOG.md | 13 +++++++++++++ experimental/CHANGELOG.md | 2 ++ 2 files changed, 15 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab253b45b20..de02b9e86e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,19 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se * (user-facing): deprecated `AlwaysOffSampler` has moved to `@opentelemetry/sdk-trace-base` * (user-facing): deprecated `TraceIdRatioSampler` has moved to `@opentelemetry/sdk-trace-base` * (user-facing): deprecated `TraceIdRatioSampler` has moved to `@opentelemetry/sdk-trace-base` +* feat(sdk-trace-base)!: drop ability to instantiate propagators beyond defaults [#5355](https://github.com/open-telemetry/opentelemetry-js/pull/5355) @pichlermarc + * (user-facing): only a non-env-var based default is now used on `BasicTracerProvider#register()`. + * propagators can now not be configured via `OTEL_PROPAGATORS` or `window.OTEL_PROPAGATORS` anymore, please pass the propagator to `NodeTracerProvider#register()` instead. + * if not configured directly via code, `BasicTracerProvider#register()` will now fall back to defaults (`tracecontext` and `baggage`) +* feat(sdk-trace-node)!: drop ability to instantiate propagators beyond defaults [#5355](https://github.com/open-telemetry/opentelemetry-js/pull/5355) @pichlermarc + * (user-facing): only a non-env-var based default is now used on `NodeTracerProvider#register()`. + * propagators can now not be configured via `OTEL_PROPAGATORS` anymore, please pass the propagator to `NodeTracerProvider#register()` instead. + * if not configured via code, `NodeTracerProvider#register()` will now fall back to the defaults (`tracecontext` and `baggage`) + * if autoconfiguration based on enviornment variables is needed, please use `NodeSDK` from `@opentelemetry/sdk-node`. +* feat(sdk-trace-web)!: drop ability to instantiate propagators beyond defaults [#5355](https://github.com/open-telemetry/opentelemetry-js/pull/5355) @pichlermarc + * (user-facing): only a non-env-var based default is now used on `WebTracerProvider#register()`. + * propagators can now not be configured via `window.OTEL_PROPAGATORS` anymore, please pass the propagator to `WebTracerProvider#register()` instead. + * if not configured via code, `WebTracerProvider#register()` will now fall back to defaults (`tracecontext` and `baggage`) ### :rocket: (Enhancement) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index d06a791cc8e..b0e18cd0ca1 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -26,6 +26,8 @@ All notable changes to experimental packages in this project will be documented ### :house: (Internal) +* refactor(sdk-node): move code to auto-instantiate propagators into utils [#5355](https://github.com/open-telemetry/opentelemetry-js/pull/5355) @pichlermarc + ## 0.57.0 ### :rocket: (Enhancement) From fb662af513d8bbbfe45cefd3d628950ec37fbe1f Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Tue, 21 Jan 2025 18:24:19 +0100 Subject: [PATCH 4/8] fixup! feat(sdk-trace-base)!: drop ability to instantiate propagators beyond defaults --- .../opentelemetry-sdk-node/test/utils.test.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts b/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts index 63861c42519..f120392d43c 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts @@ -60,17 +60,16 @@ describe('getPropagatorFromEnv', function () { }); it('should return the selected propagators when multiple are in the list', () => { - process.env.OTEL_PROPAGATORS = 'b3,jaeger'; - assert.deepStrictEqual(getPropgagatorFromEnv()?.fields(), [ - 'b3', - 'uber-trace-id', - ]); - }); - - it('should return the selected propagators when multiple are in the list', () => { - process.env.OTEL_PROPAGATORS = 'b3,jaeger'; + process.env.OTEL_PROPAGATORS = 'tracecontext,baggage,b3,b3multi,jaeger'; assert.deepStrictEqual(getPropgagatorFromEnv()?.fields(), [ + 'traceparent', + 'tracestate', 'b3', + 'x-b3-traceid', + 'x-b3-spanid', + 'x-b3-flags', + 'x-b3-sampled', + 'x-b3-parentspanid', 'uber-trace-id', ]); }); From 4072001d19fe6ab3b61917be2660e0f319de2a9c Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Tue, 21 Jan 2025 18:36:49 +0100 Subject: [PATCH 5/8] fixup! feat(sdk-trace-base)!: drop ability to instantiate propagators beyond defaults --- .../test/common/BasicTracerProvider.test.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) 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 2ead66bb9c9..10d6b6de57b 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -26,6 +26,7 @@ import { TextMapGetter, propagation, diag, + ContextManager, } from '@opentelemetry/api'; import { CompositePropagator } from '@opentelemetry/core'; import { TraceState } from '@opentelemetry/core'; @@ -59,6 +60,7 @@ class DummyPropagator implements TextMapPropagator { describe('BasicTracerProvider', () => { let envSource: Record; let setGlobalPropagatorStub: sinon.SinonSpy<[TextMapPropagator], boolean>; + let setGlobalContextManagerStub: sinon.SinonSpy<[ContextManager], boolean>; if (global.process?.versions?.node === undefined) { envSource = globalThis as unknown as Record; @@ -70,6 +72,7 @@ describe('BasicTracerProvider', () => { // to avoid actually registering the TraceProvider and leaking env to other tests sinon.stub(trace, 'setGlobalTracerProvider'); setGlobalPropagatorStub = sinon.spy(propagation, 'setGlobalPropagator'); + setGlobalContextManagerStub = sinon.spy(context, 'setGlobalContextManager'); context.disable(); }); @@ -415,6 +418,34 @@ describe('BasicTracerProvider', () => { ]); }); }); + describe('contextManager', () => { + it('should not be set if not provided', () => { + const provider = new BasicTracerProvider(); + provider.register(); + + sinon.assert.notCalled(setGlobalContextManagerStub); + }); + + it('should be set if provided', () => { + const provider = new BasicTracerProvider(); + const mockContextManager: ContextManager = { + active: sinon.stub(), + bind: sinon.stub(), + disable: sinon.stub(), + enable: sinon.stub(), + with: sinon.stub(), + }; + + provider.register({ + contextManager: mockContextManager, + }); + + sinon.assert.calledOnceWithExactly( + setGlobalContextManagerStub, + mockContextManager + ); + }); + }); }); describe('.startSpan()', () => { From 6a4a5af9881ef1b0d9866631fb950f2baa6aa52a Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 30 Jan 2025 10:33:46 +0100 Subject: [PATCH 6/8] fixup! feat(sdk-trace-base)!: drop ability to instantiate propagators beyond defaults --- .../packages/opentelemetry-sdk-node/src/sdk.ts | 4 ++-- .../packages/opentelemetry-sdk-node/src/utils.ts | 2 +- .../opentelemetry-sdk-node/test/utils.test.ts | 16 ++++++++-------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index 53f8001cddc..b6796f6368c 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -74,7 +74,7 @@ import { getResourceDetectorsFromEnv, getSpanProcessorsFromEnv, filterBlanksAndNulls, - getPropgagatorFromEnv, + getPropagatorFromEnv, } from './utils'; /** This class represents everything needed to register a fully configured OpenTelemetry Node.js SDK */ @@ -379,7 +379,7 @@ export class NodeSDK { this._configuration?.contextManager, propagator: this._tracerProviderConfig?.textMapPropagator ?? - getPropgagatorFromEnv(), + getPropagatorFromEnv(), }); } diff --git a/experimental/packages/opentelemetry-sdk-node/src/utils.ts b/experimental/packages/opentelemetry-sdk-node/src/utils.ts index fdd39f288f8..a256b0023e3 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/utils.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/utils.ts @@ -191,7 +191,7 @@ export function getSpanProcessorsFromEnv(): SpanProcessor[] { /** * Get a propagator as defined by environment variables */ -export function getPropgagatorFromEnv(): TextMapPropagator | null | undefined { +export function getPropagatorFromEnv(): TextMapPropagator | null | undefined { // Empty and undefined MUST be treated equal. if ( process.env.OTEL_PROPAGATORS === undefined || diff --git a/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts b/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts index f120392d43c..0c7639b4d34 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/utils.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { getPropgagatorFromEnv } from '../src/utils'; +import { getPropagatorFromEnv } from '../src/utils'; import * as assert from 'assert'; import * as sinon from 'sinon'; import { diag } from '@opentelemetry/api'; @@ -29,7 +29,7 @@ describe('getPropagatorFromEnv', function () { it('when not defined', function () { delete process.env.OTEL_PROPAGATORS; - const propagator = getPropgagatorFromEnv(); + const propagator = getPropagatorFromEnv(); assert.deepStrictEqual(propagator, undefined); }); @@ -37,7 +37,7 @@ describe('getPropagatorFromEnv', function () { it('on empty string', function () { (process.env as any).OTEL_PROPAGATORS = ''; - const propagator = getPropgagatorFromEnv(); + const propagator = getPropagatorFromEnv(); assert.deepStrictEqual(propagator, undefined); }); @@ -45,7 +45,7 @@ describe('getPropagatorFromEnv', function () { it('on space-only string', function () { (process.env as any).OTEL_PROPAGATORS = ' '; - const propagator = getPropgagatorFromEnv(); + const propagator = getPropagatorFromEnv(); assert.deepStrictEqual(propagator, undefined); }); @@ -53,7 +53,7 @@ describe('getPropagatorFromEnv', function () { it('should return the selected propagator when one is in the list', () => { process.env.OTEL_PROPAGATORS = 'tracecontext'; - assert.deepStrictEqual(getPropgagatorFromEnv()?.fields(), [ + assert.deepStrictEqual(getPropagatorFromEnv()?.fields(), [ 'traceparent', 'tracestate', ]); @@ -61,7 +61,7 @@ describe('getPropagatorFromEnv', function () { it('should return the selected propagators when multiple are in the list', () => { process.env.OTEL_PROPAGATORS = 'tracecontext,baggage,b3,b3multi,jaeger'; - assert.deepStrictEqual(getPropgagatorFromEnv()?.fields(), [ + assert.deepStrictEqual(getPropagatorFromEnv()?.fields(), [ 'traceparent', 'tracestate', 'b3', @@ -78,7 +78,7 @@ describe('getPropagatorFromEnv', function () { const warnStub = sinon.stub(diag, 'warn'); process.env.OTEL_PROPAGATORS = 'my, unknown, propagators'; - assert.deepStrictEqual(getPropgagatorFromEnv(), null); + assert.deepStrictEqual(getPropagatorFromEnv(), null); sinon.assert.calledWithExactly( warnStub, 'Propagator "my" requested through environment variable is unavailable.' @@ -97,6 +97,6 @@ describe('getPropagatorFromEnv', function () { it('should return null if only "none" is selected', () => { process.env.OTEL_PROPAGATORS = 'none'; - assert.deepStrictEqual(getPropgagatorFromEnv(), null); + assert.deepStrictEqual(getPropagatorFromEnv(), null); }); }); From 58ac3f0d918b175b15fce057e72b2ca3fe3b8a89 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 30 Jan 2025 10:38:25 +0100 Subject: [PATCH 7/8] fixup! feat(sdk-trace-base)!: drop ability to instantiate propagators beyond defaults --- experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 2f873ac241c..4025efe2f1a 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -415,7 +415,7 @@ describe('Node SDK', () => { await sdk.shutdown(); }); - it('should register a propagators as defined in OTEL_PROPAGATORS if trace SDK is configured', async () => { + it('should register propagators as defined in OTEL_PROPAGATORS if trace SDK is configured', async () => { process.env.OTEL_PROPAGATORS = 'b3'; const sdk = new NodeSDK({ traceExporter: new ConsoleSpanExporter(), From eec842113405365a67f0659cbb6ba83402dfcc61 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 30 Jan 2025 10:46:13 +0100 Subject: [PATCH 8/8] Update experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts Co-authored-by: Trent Mick --- experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 4025efe2f1a..dc7ccbe6908 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -427,6 +427,7 @@ describe('Node SDK', () => { assert.deepStrictEqual(propagation.fields(), ['b3']); await sdk.shutdown(); + delete process.env.OTEL_PROPAGATORS; }); });