feat(sdk-trace-node): support xray propagator

[anuraags]

Which problem is this PR solving?

Currently the NodeTracerProvider in the opentelemetry-sdk-trace-node package doesn't provide support for an AWS xray propagator.

If I try to use the node sdk and set the OTEL_PROPAGATORS environment variable to xray, it complains that:

Propagator "xray" requested through environment variable is unavailable.

The reason for this is that the NodeTracerProvider doesn't have the AWS XRAY propagator as a registered propagator.

Short description of the changes

This PR simply adds the AWSXRAYPropagator as a registere propagator in the NodeTracerProvider class. It also adds the AWSXrayPropagator package dependency to the opentelemetry-sdk-trace-node package.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I updated the environment variable in the NodeTracerProvider test in packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts to include xray and updated the resulting propagation fields.

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: anuraags / name: Anuraag Sridhar (febfecb, 18a5fd4, cc05237, 00a579b, b8a0a12, 1f8f3a3, c54869b)
• ✅ login: pichlermarc / name: Marc Pichler (602da42, d2db133, 714362b)

[pichlermarc]

This is a feature, not a bugfix.
We don't depend on contrib packages from the core repo, so this depends on #4603 being done first.

Please convert this PR to draft until it's ready for review.

[Flarna]

Is it really needed to depend on xray propagator and require it unconditionally?

There are a lot workloads which will not use xray propagator therefore I think we should not force people to install it.

If we stick on this depends on design other propagators (and maybe other components like exporters, context managers, span processors,...) will follow and SDK will grow for no reason without any chance to opt out.

[anuraags]

Is it really needed to depend on xray propagator and require it unconditionally?

There are a lot workloads which will not use xray propagator therefore I think we should not force people to install it.

If we stick on this depends on design other propagators (and maybe other components like exporters, context managers, span processors,...) will follow and SDK will grow for no reason without any chance to opt out.

My argument to that would be that there is already a jaeger propagator that is baked into the tracer provider at the moment. So what makes jaeger special and not xray?

Also, I thought the the open telemetry way was to opt in to everything through environment variables, so it's not technically unconditional?

[anuraags]

This is a feature, not a bugfix.
We don't depend on contrib packages from the core repo, so this depends on #4603 being done first.

Please convert this PR to draft until it's ready for review.

Will do

[martinkuba]

For tracking purposes, I have this listed as a subtask in #4494

[anuraags]

Thank you @martinkuba !

[Flarna]

My argument to that would be that there is already a jaeger propagator that is baked into the tracer provider at the moment. So what makes jaeger special and not xray?

Also, I thought the the open telemetry way was to opt in to everything through environment variables, so it's not technically unconditional?

You are right, it's not xray specific.
After commenting here I saw #4494 and added also a similar comment there. Let's continue discussion there as it fits better.

[anuraags]

@pichlermarc @martinkuba I have updated this PR now to incorporate the changes in #4603
Are we happy to move this out of draft?

[martinkuba]

@pichlermarc @martinkuba I have updated this PR now to incorporate the changes in #4603 Are we happy to move this out of draft?

Based on this #4494 (comment), I think we want to move the propagators from trace-base to sdk-node. @pichlermarc, can we do this now or does that need to be part of 2.0? And if that's the case, should we add the xray propagators to trace-base now and move all at the same time in 2.0?

[pichlermarc]

@pichlermarc @martinkuba I have updated this PR now to incorporate the changes in #4603 Are we happy to move this out of draft?

Based on this #4494 (comment), I think we want to move the propagators from trace-base to sdk-node. @pichlermarc, can we do this now or does that need to be part of 2.0? And if that's the case, should we add the xray propagators to trace-base now and move all at the same time in 2.0?

IMO we should leave the ones that are in trace-base as-is (not adding any new ones for now) any add any new spec-required ones to sdk-node only.

In SDK 2.0 we remove the propagators from trace-base.

[anuraags]

Okay....I'll take this out of draft status then

[martinkuba]

@pichlermarc Per #4494 (comment), I think you want to have this in the node-sdk package. But it seems to me like it would be fine in sdk-trace-node? Not as a long-term solution, but just for the xray propagators in 1.x. My concern is that documentation for the lambda instrumentation currently shows using NodeTracerProvider directly (instead of NodeSDK).

Could we add it here for 1.x and then move everything to NodeSDK in 2.0?

[martinkuba]

This was discussed in SIG meeting on 4/24. In the future (likely 2.0), we want to have this configuration in one place, so it should reside in the sdk-node package. However, since there are already multiple places right now, we agreed that it is ok to add this to sdk-trace-node for now.

[pichlermarc]

Approving as discussed in the SIG meeting. I took the liberty to update the changelog and rebase the PR.

NOTE: In SDK 2.0 we will drop auto-configuration of propagators from this package entirely, which will include this propagator. #4672