Skip to content
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

Replace AWS X-Ray Environment Span Link section #354

Merged
merged 20 commits into from
Dec 14, 2023

Conversation

tylerbenson
Copy link
Member

@tylerbenson tylerbenson commented Sep 29, 2023

TC decided that users should be able to give priority (controlling the parent/child relationship) to AWS X-Ray's Active Tracing span context (as propagated via the _X_AMZN_TRACE_ID environment variable) by configuring a different order in the OTEL_PROPAGATORS.

There is a separate effort to allow span contexts from additional propagators to be added as span links instead of just overwriting them. That will enable this previous behavior to be maintained.

Here's a POC implementation in Java: open-telemetry/opentelemetry-java-contrib#1032
Note, this POC includes an additional propagator aws that handles the behavior for both xray and xray-lambda. This can easily be removed but included as a demonstration.

Merge requirement checklist

Fixes open-telemetry/opentelemetry-specification#3605

@tylerbenson
Copy link
Member Author

tylerbenson commented Sep 29, 2023

For reference, here's the TC decision. (This PR only addresses the first bullet point.)

  • AWS x-ray has two propagators available
    • ONE does x-ray propagation via attributes / headers
    • ONE does x-ray propagation via the ENV variable
  • We improve Propagation API such that:
    • There is a guaranteed ordering of propagators and you know which one will "win" as parent
    • Propagators that have viable "parent" spans but are not chosen are placed into a "SpanLink" context and instead linked to a span, preserving parent-connections.
  • We update Tracer
    • There is a new "context" attribute that contains SpanLinks
    • New spans created will add links from this attribute.

This means that users wishing to preserve existing x-ray behavior would specify propagators "baggage, w3c, x-ray, x-ray-env". Users wishing the new behavior would specific propagators "baggage, w3c, x-ray-env, x-ray".

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any ideas for alternative name for xray-env?

"env" refers to an implementation detail, and doesn't help much with remembering which one to use in which use case

docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitty suggestions

docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
@tylerbenson tylerbenson marked this pull request as ready for review October 10, 2023 19:43
@tylerbenson tylerbenson requested review from a team as code owners October 10, 2023 19:43
trask
trask previously approved these changes Oct 10, 2023
tylerbenson and others added 10 commits October 12, 2023 15:47
TC decided that users should be able to give priority (controlling the parent/child relationship) to AWS X-Ray's `Active Tracing` span context (as propagated via the `_X_AMZN_TRACE_ID` environment variable) by configuring a different order in the `OTEL_PROPAGATORS`.

There is a separate effort to allow span contexts from additional propagators to be added as span links instead of just overwriting them. That will enable this previous behavior to be maintained.
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@tylerbenson
Copy link
Member Author

tylerbenson commented Oct 12, 2023

(To resolve the merge conflict, I rebased on main and moved the changelog entry... the rest of the PR contents are the same.)

docs/faas/aws-lambda.md Show resolved Hide resolved
@Oberon00 Oberon00 dismissed their stale review October 25, 2023 20:29

I'm dismissing the "Changes requested". I thought I would point out an obvious defect, but it seems to be a point of much discussion, so I'll assume that I did not fully grasp the problem/proposal.

@trask trask dismissed their stale review October 25, 2023 23:00

I think @Oberon00 raises a good question that I hadn't thought about previously, and would like to better understand the implications, especially if this is merged and implemented with the existing composite propagator behavior.

docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the intent is the update this once the notion of defining intended span links in context is formalized, I approve. Left a few comments to hopefully reduce the context required for readers.

docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
docs/faas/aws-lambda.md Outdated Show resolved Hide resolved
@jsuereth jsuereth merged commit 176db0d into open-telemetry:main Dec 14, 2023
9 checks passed
@tylerbenson tylerbenson deleted the tyler/xray-env branch December 14, 2023 16:36
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Josh Suereth <joshuasuereth@google.com>
tylerbenson added a commit to tylerbenson/opentelemetry-java-instrumentation that referenced this pull request Mar 21, 2024
This aligns with the latest spec change:
open-telemetry/semantic-conventions#354

Providing the Lambda span's parent context should continue to be the responsibility of the global propagator.
tylerbenson added a commit to tylerbenson/opentelemetry-java-instrumentation that referenced this pull request Mar 21, 2024
This aligns with the latest spec change:
open-telemetry/semantic-conventions#354

Providing the Lambda span's parent context should continue to be the responsibility of the global propagator.
tylerbenson added a commit to tylerbenson/opentelemetry-java-instrumentation that referenced this pull request Mar 29, 2024
This aligns with the latest spec change:
open-telemetry/semantic-conventions#354

Providing the Lambda span's parent context should continue to be the responsibility of the global propagator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

AWS X-Ray Environment Variable Propagation
10 participants