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.

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

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

Open
pichlermarc opened this issue Dec 20, 2024 · 2 comments

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Dec 20, 2024

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:

/**
* 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)

/**
* 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

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
Copy link
Member Author

pichlermarc commented Dec 20, 2024

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
Copy link
Member

dyladan commented Jan 2, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants