[sdk-trace-*] breaking clean-up of features that do not exist in logs and metrics SDKs

[pichlermarc]

Description

@opentelemetry/sdk-trace packages do include some features that are barely used and not present in other SDKs, such as the ability to override a method to have it create exporters based on environment variables) - a feature that has since been moved to @opentelemetry/sdk-node).

There are also other features that are remnants of times where the trace SDK was the only one that existed (BasicTracerProvider#register(), for instance), this is functionality that can be moved to @opentelemetry/sdk-node instead.

Proposal

We remove these features to avoid duplication of logic our packages, basically everything here:

opentelemetry-js/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts

Lines 204 to 270 in 4179ddb

	/**
	* 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 _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(
	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<TextMapPropagator[]>(
	(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,
	});
	}
	}
	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;
	}

BasicTracerProvider#register() can be removed from BasicTracerProvider and moved to a stand-alone function to provide a migration path for people that cannot use @opentelemetry/sdk-node (this is the proposal from #4672, but when done right can also take care of #2218)

opentelemetry-js/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts

Lines 129 to 149 in 4179ddb

	/**
	* Register this TracerProvider for use with the OpenTelemetry API.
	* Undefined values may be replaced with defaults, and
	* null values will be skipped.
	*
	* @param config Configuration object for SDK registration
	*/
	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);
	}
	}

Then we can make it so that a ContextManager and Propagator gets registered whenever any SDK (including logs, metrics) is registered by @opentelemetry/sdk-node, which takes care of #3862, which is a commonly encountered issue.

This then also lets us remove

opentelemetry-js/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts

Lines 57 to 68 in 4179ddb

	protected static readonly _registeredPropagators = new Map<
	string,
	PROPAGATOR_FACTORY
	>([
	['tracecontext', () => new W3CTraceContextPropagator()],
	['baggage', () => new W3CBaggagePropagator()],
	]);
	protected static readonly _registeredExporters = new Map<
	string,
	EXPORTER_FACTORY
	>();

which has been a source of confusion in the past, and encourages some pretty odd patterns that we previously used in @opentelemetry/sdk-node but that @david-luna fortunately already cleaned up in #5138

That then leads us to a point where there's almost no difference between NodeTracerProvider and WebTracerProvider so we could consolidate them into a single package @opentelemetry/sdk-trace, which will align the trace package name with the other SDK package like @opentelemetry/sdk-logs and @opentelemetry/sdk-metrics. Any remaining functionality exported from @opentelemetry/sdk-trace-web can be changed to be a @opentelemetry/instrumentation-utils-web package or similar, or moved to the actual instrumentation packages that use these utils to eliminate the need for it completely.

In short: doing this will let us remove a lot of code, eliminates differences between the SDKs, and simplifies work on the Trace SDK going forward - especially around #4672 and #5217

[pichlermarc]

Here's an example of how I'd remove the exporter instantiation functionality: #5239.

It's the first step - and I think the one with the least impact for end-users if we decide to go this route.

[dyladan]

I think this is well reasoned and agree with everything here. +1 from me

[pichlermarc]

cc @open-telemetry/javascript-approvers if there are no objections then I'll break this down into implementation issues 🙂 Please let me know if you have objections.

[trentm]

+1

[pichlermarc]

Alright, marking this as accepted 🙂
Broken down into

• [sdk-trace-base] drop exporter instantiation feature #5339
  • has PR feat(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters #5239
• [sdk-trace-base] [sdk-trace-node] remove propagator auto-configuration from package #4672
  • has PR feat(sdk-trace-base)!: drop ability to auto-instantiate propagators beyond defaults #5355
• TBD: issue to drop register() method
• TBD: issue to follow-up on consolidating the three trace packages (base, node, web) into one (stretch goal for 2.0 - if there's no time, we'll need to push it to 3.x)