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.

Sign up for GitHub

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

Jump to bottom

Update aws lambda spec to remove X-Ray Env propagation #3166

Merged
merged 11 commits into from
Feb 10, 2023
Merged

Update aws lambda spec to remove X-Ray Env propagation #3166

merged 11 commits into from
Feb 10, 2023

Conversation

tylerbenson
Copy link
Member

@tylerbenson tylerbenson commented Feb 1, 2023 •
edited
Loading

Per discussion in the FAAS SIG, we decided that the aws x-ray environment variable should be moved to a span link to avoid interfering with the configured propagators.

We also decided that EventToCarrier should be specified in the spec to improve cross language instrumentation consistency.

I removed EventToCarrier from this PR to focus on the x-ray propagation. We can revisit in a separate PR.

Fixes #3060

@tylerbenson
Update aws lambda spec to remove X-Ray Env propagation
72ed38c 
Per discussion in the FAAS SIG, we decided that the aws x-ray environment variable should be moved to a span link to avoid interfering with the configured propagators.

We also decided that `EventToCarrier` should be specified in the spec to improve cross language instrumentation consistency.
@tylerbenson tylerbenson requested review from a team February 1, 2023 21:19
tylerbenson and others added 2 commits February 1, 2023 17:09
@tylerbenson @tsloughter
Add links to links :-)
f8be2b6 
Co-authored-by: Tristan Sloughter <t@crashfast.com>
@tylerbenson
More review changes
85c0d9d 
Pulled section about EventToCarrier for further discussion.
@tylerbenson
Copy link
Member Author

@Aneurysm9 note: I pulled the section about EventToCarrier as it was causing some confusion. We should discuss further and put in a separate PR for it.

@cartersocha
Copy link

fyi @tedsuo we have a spec reviewer block here

@cartersocha
Copy link

Approved by AWS Lambdas SME's - @Aneurysm9

@cartersocha
Copy link

No GCP or Azure SME input required

tylerbenson and others added 3 commits February 8, 2023 11:08
@tylerbenson @MrAlias
Update specification/trace/semantic_conventions/instrumentation/aws-l…
959dec9 
…ambda.md

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@tylerbenson @MrAlias
Update specification/trace/semantic_conventions/instrumentation/aws-l…
73cfa16 
…ambda.md

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@tylerbenson
Merge branch 'main' into tyler/lambda
a28f747
@tigrannajaryan
Copy link
Member

This has enough approvals but merging is blocked because there are unresolved conversations.

@tylerbenson
Copy link
Member Author

@tigrannajaryan thanks for the call-out. I've resolved the remaining conversation. Should be good to go!

@carlosalberto
Copy link
Contributor

@tyler Forgot to mention we need a CHANGELOG entry as well 😓

@tsloughter tsloughter mentioned this pull request Feb 10, 2023
Merged
11 tasks
@carlosalberto carlosalberto merged commit 7116e72 into open-telemetry:main Feb 10, 2023
@tylerbenson tylerbenson deleted the tyler/lambda branch February 10, 2023 17:33
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
@tsloughter
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent
68ca172 
See open-telemetry/opentelemetry-specification#3166
@tsloughter tsloughter mentioned this pull request Feb 24, 2023
Closed
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
@tsloughter
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent
94b3e70 
See open-telemetry/opentelemetry-specification#3166
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
@tsloughter
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent
4746424 
See open-telemetry/opentelemetry-specification#3166
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
@tsloughter
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent
e95cdf9 
See open-telemetry/opentelemetry-specification#3166
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
@tsloughter
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent
e478429 
See open-telemetry/opentelemetry-specification#3166
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
@tsloughter
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent
6037833 
See open-telemetry/opentelemetry-specification#3166
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
@tsloughter
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent
05b949c 
See open-telemetry/opentelemetry-specification#3166
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 25, 2023
@tsloughter
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent
67d1ded 
See open-telemetry/opentelemetry-specification#3166
tylerbenson added a commit to tylerbenson/opentelemetry-java-instrumentation that referenced this pull request Mar 3, 2023
@tylerbenson
Move X-Ray Env Variable propagation to span link instead of parent
9fdd241 
open-telemetry/opentelemetry-specification#3166

Per discussion in the FAAS SIG, we decided that the AWS X-Ray environment variable should be moved to a span link to avoid interfering with the configured propagators.
@tylerbenson tylerbenson mentioned this pull request Mar 3, 2023
Merged
@MrAlias MrAlias mentioned this pull request Mar 6, 2023
Closed
laurit pushed a commit to open-telemetry/opentelemetry-java-instrumentation that referenced this pull request Mar 16, 2023
@tylerbenson
Move X-Ray Env Variable propagation to span link instead of parent (#…
083acdd 
…7970)

open-telemetry/opentelemetry-specification#3166

Per discussion in the FAAS SIG, we decided that the AWS X-Ray
environment variable should be moved to a span link to avoid interfering
with the configured propagators.

Also related to #5167.
@martinkuba martinkuba mentioned this pull request Mar 23, 2023
Closed
@tylerbenson tylerbenson mentioned this pull request Jul 17, 2023
Closed
@wangzlei
Copy link
Contributor

  1. This spec changes might be against with Span links spec and Messaging system. FYI the ambient case in https://github.com/open-telemetry/oteps/blob/main/text/trace/0220-messaging-semantic-conventions-span-structure.md#user-content-instrumenting-pull-based-scenarios. If user's Lambda function is invoked by AWS Lambda, AWS Lambda should be the parent of user Lambda function.
  2. AWS Lambda is working on the passthrough mode fix and would be released soon. After this fix, if user disables tracing for a root Lambda, the environment does not have trace header with sample flag 0.

May I know the deeper reason why changing parent-child to be links?

@tylerbenson
Copy link
Member Author

@wangzlei This PR is probably not the best place to have this discussion. Can I suggest you open a new issue or join the FAAS SIG meeting?

The quick summary is that the prior behavior would break traces for other vendors when x-ray was enabled because the x-ray internal spans were not available.

@carlosalberto
Copy link
Contributor

@wangzlei As Tyler said, please open an issue in https://github.com/open-telemetry/semantic-conventions (and maybe join the FaaS call? We meet every Thursday at 8am Pacific)

@jsuereth jsuereth mentioned this pull request Aug 21, 2023
Closed
@Aneurysm9 Aneurysm9 mentioned this pull request Aug 21, 2023
Closed
@tylerbenson tylerbenson mentioned this pull request Aug 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@ronyis ronyis ronyis left review comments

@tsloughter tsloughter tsloughter approved these changes

@codeboten codeboten codeboten approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@MrAlias MrAlias MrAlias approved these changes

Assignees

@arminru arminru

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AWS Lambda spec inappropriately prioritizes x-ray propagation