Enable X-Ray propagator for nodejs-instrumentation #1872
Conversation
|
|
There was a problem hiding this comment.
@ademar-prothon thank you for the contribution, please sign the CLA.
|
@open-telemetry/javascript-approvers can you please advise the best way to make propagators available for use via the |
Sorry for the late reply, right now |
| @@ -35,6 +36,7 @@ function getMetricReader() { | |||
| const sdk = new NodeSDK({ | |||
| autoDetectResources: true, | |||
| instrumentations: [getNodeAutoInstrumentations()], | |||
| textMapPropagator: new AWSXRayPropagator(), | |||
There was a problem hiding this comment.
Wouldn't this change make the default propagator x-ray? I don't think this is what we would want in the OpenTelemetry Operator. Ideally we could configure the propagator through an environment variable.
There was a problem hiding this comment.
Agreed. The javascript SDK must recognize all of these values to meet spec. @pichlermarc do know know how the OTEL_PROPAGATORS env var interacts with NodeSDK? If NodeSDK doesn't honor OTEL_PROPAGATORS that feels like a bug.
There was a problem hiding this comment.
NodeSDK does honor OTEL_PROPAGATORS for the internal values (tracecontext, baggage, b3, b3multi, jaeger), via the NodeTracerProvider that is used in the SDK.. The third party options xray and ottrace are available in the js-contrib repo and currently only added via code, not yet available as an environment variable option.
There was a problem hiding this comment.
@JamieDanielson thanks for the details. @ademar-prothon at this point I don't think there is anything we should change in the operator. Instead it would be good to add this functionality to opentelemetry-js directly so that I can enable the xray propagator via the env variable.
Once that feature is release from opentelemetry-js we can update the operator's dependencies.
Following issue
Added X-Ray propagator in the NodeSDK constructor.
I'm not sure how propragators are configured, is it the
NodeSDKselecting the right propagator based on env variable config or should we implement this logic on our side ?Thanks,