-
Notifications
You must be signed in to change notification settings - Fork 201
Upgrade to otel sdk v2 #1789
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
base: main
Are you sure you want to change the base?
Upgrade to otel sdk v2 #1789
Conversation
…ble is no longer exported in version 2.0.0
…feat/nodejs/upgrade-otel-sdk
…recated addSpanProcessor method, remove support for configureTracerProvider global method
@@ -1 +0,0 @@ | |||
console.log(require('@opentelemetry/core').VERSION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is VERSION
no longer exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, see upgrade guide: https://github.com/open-telemetry/opentelemetry-js/blob/main/doc/upgrade-to-2.x.md?plain=1#L253
"@opentelemetry/exporter-logs-otlp-http": "^0.200.0", | ||
"@opentelemetry/exporter-metrics-otlp-http": "^0.200.0", | ||
"@opentelemetry/exporter-trace-otlp-http": "^0.200.0", | ||
"@opentelemetry/exporter-zipkin": "^2.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need exporter-zipkin
dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this because since the 2.0 upgrade, the tracing sdk does not support creation of exporters through env vars anymore. I then assumed that previously the zipkin exporter was supported by default because of the way it was listed here: https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
But now upon further inspection it does seem that this wasn't supported before this functionality was removed from the tracing sdk: https://github.com/open-telemetry/opentelemetry-js/pull/5239/files
So I will remove this, as we don't need to support it by default.
|
||
const stringToExporter = new Map<string, () => SpanExporter>([ | ||
['otlp', () => new OTLPTraceExporter()], | ||
['zipkin', () => new ZipkinExporter()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to have and support Zipkin exporter by default?
if (typeof configureTracer === 'function') { | ||
config = configureTracer(config); | ||
} else if (process.env.OTEL_TRACES_EXPORTER) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, instead of else if (process.env.OTEL_TRACES_EXPORTER)
, we can try to get exporters from env if no span processor is configured by the configureTracer
function before.
Because, even though configureTracer
function is specified, user might not have interest to configure span processor (left its configuration to the Lambda layer here), but configure other properties of the TracerConfig
.
And also, I think, OTEL_TRACES_EXPORTER
env var should override the exporters set by the configureTracer
function (for example, user is playing with different exporters without need to change the code and redeploy by just setting the env var.)
Therefore, I think, the following logic might be better:
- In the
getExportersFromEnv
, if there is noOTEL_TRACES_EXPORTER
env var, return empty array. - In the
getExportersFromEnv
, ifOTEL_TRACES_EXPORTER
is specified asnone
, returnnull
. - And here:
// Try to get exporters from env var
const exporters = getExportersFromEnv();
if (!exporters) {
// `null` exporters means tracing exporter is disabled, so skip trace provider initialization
return;
}
// If there is specified `configureTracer` function by the user, use it to setup the config
if (typeof configureTracer === 'function') {
config = configureTracer(config);
}
if (exporters.length) {
// If there are exporters configured by env var, clear the ones configured before.
// Because, the exporters configured by env var should override the ones configured by code.
config.spanProcessors = [];
exporters.map(exporter => {
if (exporter instanceof ConsoleSpanExporter) {
config.spanProcessors.push(new SimpleSpanProcessor(exporter));
} else {
config.spanProcessors.push(new BatchSpanProcessor(exporter));
}
});
}
config.spanProcessors = config.spanProcessors || [];
// If no exporter configured, use the default one
if (config.spanProcessors.length === 0) {
// Default
config.spanProcessors.push(
new BatchSpanProcessor(new OTLPTraceExporter()),
);
}
// Logging for debug
if (logLevel === DiagLogLevel.DEBUG) {
config.spanProcessors.push(
new SimpleSpanProcessor(new ConsoleSpanExporter()),
);
}
const tracerProvider = new NodeTracerProvider(config);
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me, will fix
Still work in progress. Most of it should be covered by this PR, but still doing some smoke-testing on my own lambdas