Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add xray propagators that prioritizes xray environment variable #1032
Add xray propagators that prioritizes xray environment variable #1032
Changes from all commits
bb8954c
bc9b8ab
4a323a2
026bd4e
2fafee5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering 2 things:
We don't know how Lambda support w3c, but one possible is using new env variables for w3c trace context and baggage. We can think about rename AwsXrayLambdaPropagator to be more generic
AwsLambdaPropagator
.The current AWSXrayLambdaPropagator combines both Lambda propagator and XRayPropagator, can we think about only handle Lambda environment variable or system property? Then let customer set "propagator=awslambda, xray" if needed. The AwsLambdaPropagator inject() can be a No-Op method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AwsW3CTraceContextLambdaPropagator
/AwsW3CBaggageLambdaPropagator
.xray-lambda
propagator definition semantic-conventions#778 for more detail.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your great help for Lambda support. Would you help me understand the
problem when implementing.
?Your original plan sounds reasonable that if user prefer Lambda env variable, uses propagator order "lambda, w3c, xray". If user prefer http header, uses propagator in order "w3c, xray, lambda". It means "lambda" propagator is for AWS Lambda specific environment and no need handle injection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial POC implemented the original spec which separated the propagators as you suggested. After adding the clause mentioned in that PR, the spec became broken which I didn't discover until I had time to implement the final spec.
If you need more detail of how it was broken, please review the description in open-telemetry/semantic-conventions#778 and let me know if that's unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @tylerbenson
If customer's application is: client-> API GW -> Lambda -> S3, both API GW and Lambda are Active tracing enabled. Could you help draw the expected trace graph? I am trying to understand if the Lambda spec make sense to customer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning on removing that line since the span linking is no longer required by the spec. Are you expecting something different?
I don't understand how OTEL_SDK_DISABLED is relevant here. Are you suggesting this propagator interferes with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS services would like to support the similar functionality as OTEL_SDK_DISABLED does. Take the example of Lambda, if user does not enable Active tracing, the environment variable would have the same trace context with from API GW request. With AWS w3c support, it means OTel in Lambda only need xxxx-Lambda propagator in the future.
Currently we know 2 issues AWS can consider to enhance:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you/we want this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the existing behavior, would be update to be same as OTEL_SDK_DISABLED does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wangzlei FYI: open-telemetry/opentelemetry-java-instrumentation#10930