-
Notifications
You must be signed in to change notification settings - Fork 182
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
feat(nodejs): minimize out-of-the-box supported propagators to reduce coldstart delay #1664
feat(nodejs): minimize out-of-the-box supported propagators to reduce coldstart delay #1664
Conversation
process.env.OTEL_PROPAGATORS.trim() === '' | ||
) { | ||
return new CompositePropagator({ | ||
propagators: [ |
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.
Could you clarify why the default propagators will not match the ones listed here? https://github.com/open-telemetry/opentelemetry-lambda/blob/main/nodejs/packages/layer/scripts/otel-handler#L12
Also if the defaults are already set in otel-handler, is there any reason to also set it here? (I actually prefer that to be here instead)
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.
@pragmaticivan Because this is the existing behavior.
Currently, propagators are taken from here in the @opentelemetry/auto-configuration-propagators
package and hence I used the impl/logic there.
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.
But the defaults of this project
are tracecontext,baggage,xray
instead. This env variable will always be overwritten by that file, so the code will never hit that block
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.
- without this PR, default propagators (tracecontext,baggage,xray) are set by the
otel-handler
script - with this PR, still default propagators (tracecontext,baggage,xray) are set by the
otel-handler
script
So, this keeps the existing behavior. No need to put the same default list into the newly introduced getPropagator
method too. otel-handler
script is responsible for setting the defaults.
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 really don't understand the issue here. This PR keeps the existing behavior for propagators by default because it uses the existing impl code. Why it was OK before, and NOT OK now?
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 guess you are trying to replicate the other package behavior here
when you might not need this code block. All good, though. I understand that this was the default from the other package, yet this is a block of code that will never
be able to be reached in this package
, which makes it dead code
out of the gate
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.
No one can guarantee that this is dead code
even though otel-handler
is in place.
Because users might put their own initializers and activate it through the NODE_OPTIONS
env var. And in this case, their initializers will run before our wrapper. So, potentially, they can clear the OTEL_PROPAGATORS
env var in their initializer code, hence wrapper should take care of empty OTEL_PROPAGATORS
env var too.
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.
Makes sense, I guess I see the point of this code, but don't see the point of keeping that code in otel-handler 😂
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.
If you are still interested in removing that code block, you can send another PR for it and we can discuss. But the main point of this PR is dropping b3 and jaeger propagator dependencies without changing existing behavior.
and additionally, this PR
@opentelemetry/sdk-trace-node
to be not dependent to the@opentelemetry/propagator-b3
and@opentelemetry/propagator-jaeger
propagator packages.@opentelemetry/sdk-trace-node
dependency) expands the required types can be passed to theconfigureTracerProvider
andconfigureTracer
functions. Since this is type expansion, it should not break backward compatibility, because lower types (NodeTraceProvider
andNodeTracerConfig
) can still be passed to these functions as is.