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

Add xray propagators that prioritizes xray environment variable #1032

Merged
merged 5 commits into from
Mar 21, 2024

Conversation

tylerbenson
Copy link
Member

This will allow support for AWS's environment variable with no additional config keys and no changes to instrumentation required.

I included a propagator that did just the env var propagation without the standard xray propagation as well as one that does both but prioritizes xray env propagation first.

@rapphil
Copy link

rapphil commented Sep 11, 2023

This is related to open-telemetry/semantic-conventions#277

No decision was made yet.

tylerbenson and others added 4 commits March 5, 2024 11:55
This will allow support for AWS's environment variable with no additional config keys and no changes to instrumentation required.
I included a propagator that did just the env var propagation without the standard xray propagation as well as one that does both but prioritizes xray env propagation first.
Because of the following statement added to the spec, the xray and xray-lambda propagators must be combined:

> To avoid potential issues when extracting with an active span context, the xray-lambda propagator SHOULD check if the provided context already has an active span context. If found, the propagator SHOULD return the provided context unmodified.

With this condition, if they are separate the xray-lambda propagator will see the context from xray and avoid modifying the context.
By combining, the propagator is able to consider the context before xray has modified it.
@tylerbenson
Copy link
Member Author

This PR has been updated to match the latest spec update and is now ready for review.

@tylerbenson
Copy link
Member Author

@srprash @wangzlei please take a look.

return xrayContext;
}
return xrayPropagator.extract(
xrayContext,
Copy link

Choose a reason for hiding this comment

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

Considering 2 things:

  1. AWS Lambda supports w3c in the future
    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.
  2. decoupling AWS Lambda propagator and AWS xray propagator
    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.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. When you add support for tracecontext/baggage, I suggest following a similar pattern and creating a AwsW3CTraceContextLambdaPropagator/AwsW3CBaggageLambdaPropagator.
  2. This was originally the plan, but I ran into a major problem when implementing. The fix was to combine the propagators. See Fix problem in xray-lambda propagator definition semantic-conventions#778 for more detail.

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

problem when implementing

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.

Copy link

@wangzlei wangzlei Mar 18, 2024

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.

Copy link
Member Author

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?

Copy link

@wangzlei wangzlei Mar 19, 2024

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:

  1. AWS services not support w3c trace context. Update to: AWS services support w3c trace header.
  2. Lambda always insert a new trace context when Active tracing is disabled. If Lambda is root, it adds a trace context with sampled flag 0. Update to propagate trace context from upstream to downstream if user does not use Active tracing..

Copy link
Member Author

Choose a reason for hiding this comment

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

If Lambda is root, it adds a trace context with sampled flag 0.

Why would you/we want this?

Choose a reason for hiding this comment

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

If Lambda is root, it adds a trace context with sampled flag 0.

It is the existing behavior, would be update to be same as OTEL_SDK_DISABLED does.

Copy link
Member Author

Choose a reason for hiding this comment

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

…xray/propagator/AwsXrayLambdaPropagator.java
@tylerbenson
Copy link
Member Author

@wangzlei I don't think anything in our discussion above represents a blocker for merging this PR. Is there anything that I'm missing?

If you'd like to continue discussing how this should all work, we should probably include the rest of the SIG instead of in this repo. Please open an issue in the opentelemetry-lambda repo or come attend the SIG meeting.

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.

5 participants