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

anuraags · 2024-04-03T00:40:18Z

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

Signed-off-by: Anu Sridhar <[email protected]>

linux-foundation-easycla · 2024-04-03T00:40:24Z

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)

Revert newline change

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 · 2024-04-03T08:32:51Z

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 · 2024-04-03T21:23:27Z

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 · 2024-04-03T21:24:43Z

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 · 2024-04-03T22:26:06Z

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

anuraags · 2024-04-03T23:07:30Z

Thank you @martinkuba !

Flarna · 2024-04-04T08:10:30Z

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.

…emetry-js into xray-propagator

anuraags · 2024-04-18T01:02:14Z

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

martinkuba · 2024-04-18T04:25:23Z

@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 · 2024-04-18T11:44:45Z

@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 · 2024-04-22T00:35:33Z

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

martinkuba · 2024-04-23T00:08:15Z

@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

…#4602)" This reverts commit 75d88f7.

pichlermarc · 2024-05-22T15:34:16Z

@anuraags FYI we've discovered that this is not spec-compliant as we MUST NOT have the AWS propagator as part of the core repository (and therefore we cannot distribute the propgator be part of core components like the trace SDK - unless the spec changes). Therefore we'll have to roll back this change (see #4727).

As a workaround I plan to introduce a getPropagator() function in the @opentelemetry/auto-instrumentations-node metapackage which will include all third-party propagators. open-telemetry/opentelemetry-js-contrib#2232

* Revert "feat(sdk-trace-node): support xray propagator (#4602)" This reverts commit 75d88f7. * chore: sync package-lock.json

* fix(opentelemetry-sdk-trace-node): support xray propagator Signed-off-by: Anu Sridhar <[email protected]> * linter fix Signed-off-by: Anu Sridhar <[email protected]> * Build trigger * Update package.json Revert newline change * chore: add changelog entry * fix: lint --------- Signed-off-by: Anu Sridhar <[email protected]> Co-authored-by: Marc Pichler <[email protected]>

…#4602)" (open-telemetry#4727) * Revert "feat(sdk-trace-node): support xray propagator (open-telemetry#4602)" This reverts commit 75d88f7. * chore: sync package-lock.json

anuraags added 2 commits April 3, 2024 10:58

fix(opentelemetry-sdk-trace-node): support xray propagator

1f8f3a3

Signed-off-by: Anu Sridhar <[email protected]>

linter fix

c54869b

Signed-off-by: Anu Sridhar <[email protected]>

anuraags requested a review from a team April 3, 2024 00:40

Build trigger

cc05237

anuraags changed the title ~~Xray propagator~~ fix(opentelemetry-sdk-trace-node): support xray propagator Apr 3, 2024

Update package.json

b8a0a12

Revert newline change

pichlermarc requested changes Apr 3, 2024

View reviewed changes

anuraags changed the title ~~fix(opentelemetry-sdk-trace-node): support xray propagator~~ feat(opentelemetry-sdk-trace-node): support xray propagator Apr 3, 2024

anuraags marked this pull request as draft April 3, 2024 23:05

anuraags added 2 commits April 18, 2024 10:59

Merge remote-tracking branch 'upstream/main' into xray-propagator

00a579b

Merge branch 'xray-propagator' of https://github.com/anuraags/opentel…

febfecb

…emetry-js into xray-propagator

Merge branch 'main' into xray-propagator

18a5fd4

anuraags marked this pull request as ready for review April 22, 2024 00:35

anuraags requested a review from pichlermarc April 22, 2024 00:35

martinkuba approved these changes Apr 25, 2024

View reviewed changes

martinkuba mentioned this pull request Apr 30, 2024

Implement xray-lambda propagator via the OTEL_PROPAGATORS env variable #4494

Open

8 tasks

pichlermarc changed the title ~~feat(opentelemetry-sdk-trace-node): support xray propagator~~ feat(sdk-trace-node): support xray propagator May 3, 2024

Merge branch 'main' into xray-propagator

602da42

chore: add changelog entry

714362b

pichlermarc approved these changes May 3, 2024

View reviewed changes

fix: lint

d2db133

pichlermarc merged commit 75d88f7 into open-telemetry:main May 3, 2024
19 checks passed

pichlermarc mentioned this pull request May 3, 2024

chore: prepare release API 1.9.0/Core 1.25.0/Experimental 0.52.0 #4677

Merged

martinkuba mentioned this pull request May 8, 2024

feat(sdk-trace-node): add env mapping for xray-lambda propagator #4685

Closed

pichlermarc added a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request May 22, 2024

Revert "feat(sdk-trace-node): support xray propagator (open-telemetry…

a0f9841

…#4602)" This reverts commit 75d88f7.

pichlermarc mentioned this pull request May 22, 2024

Revert "feat(sdk-trace-node): support xray propagator (#4602)" #4727

Merged

pichlermarc added a commit that referenced this pull request May 23, 2024

Revert "feat(sdk-trace-node): support xray propagator (#4602)" (#4727)

c97f21f

* Revert "feat(sdk-trace-node): support xray propagator (#4602)" This reverts commit 75d88f7. * chore: sync package-lock.json

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anuraags commented Apr 3, 2024

linux-foundation-easycla bot commented Apr 3, 2024 •

edited

Loading

pichlermarc left a comment •

edited

Loading

Flarna commented Apr 3, 2024

anuraags commented Apr 3, 2024

anuraags commented Apr 3, 2024

martinkuba commented Apr 3, 2024

anuraags commented Apr 3, 2024

Flarna commented Apr 4, 2024

anuraags commented Apr 18, 2024

martinkuba commented Apr 18, 2024 •

edited

Loading

pichlermarc commented Apr 18, 2024

anuraags commented Apr 22, 2024

martinkuba commented Apr 23, 2024

martinkuba left a comment

pichlermarc left a comment •

edited

Loading

pichlermarc commented May 22, 2024

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

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

Conversation

anuraags commented Apr 3, 2024

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Apr 3, 2024 • edited Loading

pichlermarc left a comment • edited Loading

Choose a reason for hiding this comment

Flarna commented Apr 3, 2024

anuraags commented Apr 3, 2024

anuraags commented Apr 3, 2024

martinkuba commented Apr 3, 2024

anuraags commented Apr 3, 2024

Flarna commented Apr 4, 2024

anuraags commented Apr 18, 2024

martinkuba commented Apr 18, 2024 • edited Loading

pichlermarc commented Apr 18, 2024

anuraags commented Apr 22, 2024

martinkuba commented Apr 23, 2024

martinkuba left a comment

Choose a reason for hiding this comment

pichlermarc left a comment • edited Loading

Choose a reason for hiding this comment

pichlermarc commented May 22, 2024

linux-foundation-easycla bot commented Apr 3, 2024 •

edited

Loading

pichlermarc left a comment •

edited

Loading

martinkuba commented Apr 18, 2024 •

edited

Loading

pichlermarc left a comment •

edited

Loading