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

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

Merged
merged 11 commits into from
Feb 10, 2023

Conversation

tylerbenson
Copy link
Member

@tylerbenson tylerbenson commented Feb 1, 2023

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

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
Co-authored-by: Tristan Sloughter <[email protected]>
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

@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 😓

@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 added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 24, 2023
tsloughter added a commit to tsloughter/opentelemetry-go-contrib that referenced this pull request Feb 25, 2023
tylerbenson added a commit to tylerbenson/opentelemetry-java-instrumentation that referenced this pull request Mar 3, 2023
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.
laurit pushed a commit to open-telemetry/opentelemetry-java-instrumentation that referenced this pull request Mar 16, 2023
…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.
@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)

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…y#3166)

* Update aws lambda spec to remove X-Ray Env propagation

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS Lambda spec inappropriately prioritizes x-ray propagation