feat(otel-node)!: update to SDK 2.0 #663
Conversation
| function restoreEnvironment() { | ||
| Object.keys(envToRestore).forEach((k) => { | ||
| process.env[k] = envToRestore[k]; | ||
| delete envToRestore[k]; |
There was a problem hiding this comment.
note for reviewer: IMO the getters defined below should take values directly from prcess.env once the environment vars are restored.
| const logsExportProtocol = | ||
| process.env.OTEL_EXPORTER_OTLP_LOGS_PROTOCOL || | ||
| getEnvVar('OTEL_EXPORTER_OTLP_PROTOCOL'); | ||
| getEnvString('OTEL_EXPORTER_OTLP_PROTOCOL'); |
There was a problem hiding this comment.
% node --import @elastic/opentelemetry-node simple-http-request.js
{"name":"elastic-otel-node","level":40,"msg":"Logs exporter protocol \"undefined\" unknown. Using default \"http/protobuf\" protocol","time":"2025-03-18T20:10:04.261Z"}
...
I think that log warn is because this doesn't have a default fallback now.
| const metricsExportProtocol = | ||
| process.env.OTEL_EXPORTER_OTLP_METRICS_PROTOCOL || | ||
| getEnvVar('OTEL_EXPORTER_OTLP_PROTOCOL'); | ||
| getEnvString('OTEL_EXPORTER_OTLP_PROTOCOL'); |
There was a problem hiding this comment.
% node --import @elastic/opentelemetry-node simple-http-request.js
...
{"name":"elastic-otel-node","level":40,"msg":"Metrics exporter protocol \"undefined\" unknown. Using default \"http/protobuf\" protocol","time":"2025-03-18T20:10:04.263Z"}
...
I think that log warn is because this doesn't have a default fallback now.
| // @ts-ignore -- T is {keyof OtelEnv} but not sure how to make TS infer that | ||
| return otelEnv[name]; | ||
| } | ||
| function makeEnvVarGetter(getterFn) { |
There was a problem hiding this comment.
Hrm. AFAICT, the only reason for the isStashed handling in here is because in elastic-node-sdk.js we are currently calling setupEnvironment before the call the resolveDetectors that uses the OTEL_NODE_RESOURCE_DETECTORS envvar value.
What if we move setupEnvironment() to be just before the super(configuration) call. I think that was the original intent. The only reason the setupEnvironment / restoreEnvironment things are done is to tweak the env for when the NodeSDK constructor runs. (Possibly also its .start() as well.)
Then if the isStashed stuff isn't needed, we could drop the getEnv* functions and just use the get*FromEnv functions from @opentelemetry/core directly, right?
trentm
changed the title
…Environment tweaking closer to NodeSDK ctor call, so don't need special lookup handling for tweaked envvars
|
@david-luna What do you think of commit 3c28ba4? |
Looks good. I'm going to approve and add the changelog entry. Thanks! :) |
https://github.com/open-telemetry/opentelemetry-js/blob/main/doc/upgrade-to-2.x.md
Note: this is not considered a breaking change for EDOT Node.js because:
node --import @elastic/opentelemetry-node ...mechanism. Programmatic bootstrapping of EDOT Node.js will remain experimental and may be broken in minor releases.