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

AWS X-Ray Environment Variable Propagation #3605

Closed
tylerbenson opened this issue Jul 17, 2023 · 8 comments · Fixed by open-telemetry/semantic-conventions#354
Closed

AWS X-Ray Environment Variable Propagation #3605

tylerbenson opened this issue Jul 17, 2023 · 8 comments · Fixed by open-telemetry/semantic-conventions#354
Assignees
Labels
spec:context Related to the specification/context directory

Comments

@tylerbenson
Copy link
Member

This is an alternative proposal to open-telemetry/semantic-conventions#164.

Putting here, but it actually applies more to the propagators found in various contrib repos (eg https://github.com/open-telemetry/opentelemetry-java-contrib/tree/main/aws-xray-propagator).

Prologue

When enabled, X-Ray creates a span that represents the invocation of a lambda. This span participates in the trace by propagating trace info through an environment variable. Lambda instrumentation is then responsible for extracting the span context and using instead of normal propagation. Historically that was used as the span parent which resulted in a broken trace for other vendors. This was resolved in the spec by assigning that span context to a span link instead. After this change was made and released in the Java Instrumentation, AWS decided this was not an acceptable solution. They feel introducing EventToCarrier as a concept to the Lambda instrumentation will solve the problem.

Proposal

I would instead like to revisit a prior proposal for how to support this functionality. In that issue, I suggested the following:

Create a new propagator that can handle both environment variable and carrier propagation appropriately that would be used for lambda instead of the current x-ray propagator.

(For reference our existing propagators. I propose the name xray-env to distinguish from the regular xray propagation.)

This would allow users that send traces to x-ray that want to include this additional span in their trace to configure their propagator. The default state would not interfere with traces sent to other vendors.

Previously Shared Concerns

Some have expressed concern that a propagator extracting from the environment variable instead of the provided carrier is breaking the contract expected by the propagator interface. While I agree this does have some merit, this proposal is isolated to this specific propagator and the proposed propagator name “xray-env” implies the intended behavior.

I also feel this is a far less invasive option in comparison to the proposed EventToCarrier option which requires a completely new configuration setting and an additional interface to be published specific to the lambda instrumentation.

@tylerbenson tylerbenson added the spec:context Related to the specification/context directory label Jul 17, 2023
@Oberon00
Copy link
Member

Oberon00 commented Jul 18, 2023

I actually don't think the propagator should access the environment, because the propagator might be called in a context where it does not make sense to use the "ambient" X-Ray trace ID. And there should really be no need for that as the Lambda instrumentations have the knowledge to implement the required special handling. So I don't really see what problem this propagator solves that could not be solved just as easily within the calling instrumentation.

(And if you'd want to have generic solution to this special problem, how about some kind of composite carrier instead?)

@tylerbenson
Copy link
Member Author

tylerbenson commented Jul 18, 2023

the propagator might be called in a context where it does not make sense to use the "ambient" X-Ray trace ID

According to the prior specification, if the environment variable was set and valid it would always supersede the carrier. In non-lambda situations that environment variable should never be set, so even if users inadvertently enable xray-env in a non-lambda application it should behave exactly the same as xray.

If you're pushing this logic into the instrumentation/carrier, that logic now needs to live in the instrumentation repos and have a new instrumentation specific setting to enable for xray customers.
I prefer to have this behavior encapsulated in a propagator that can be owned/maintained by AWS and allow users to enable it via an already established configuration setting.

@Oberon00
Copy link
Member

Oberon00 commented Jul 19, 2023

In a Lambda situation you may still call the propagator's Extract in more than one place. For example, maybe your Lambda receives a message not through the event but with a manual receive call and then calls the propagator on it.

@tylerbenson
Copy link
Member Author

Is this hypothetical or something you've seen done?
(This hasn't been done currently in the lambda instrumentation I've seen.)
If you're referring to something custom, they're are also free to select specific propagators. For example, we specify elsewhere that xray propagation should be used for calls using the AWS SDK client.

@Oberon00
Copy link
Member

I'm referring to something custom, I don't think a Lambda instrumentation should call Extract in non-Lambda related places. But usually, is there a way to pass a custom propagator to an instrumentation? If we specify that the Lambda instrumentation should have a user-configurable propagator, an xray-env propagator does make sense to me. I thought it was something you'd configure as global, and that sounded strange.

@ithompson-gp
Copy link

ithompson-gp commented Sep 4, 2023

Hi, has there been any update or decision on this?
We're building out serverless systems underpinned by OTEL and would appreciate clarity in the approach when X-Ray (or any other external constant) is in the mix.
Thanks!

@tylerbenson
Copy link
Member Author

@ithompson-gp This is under active discussion. If you have opinions, please join the SIG and discuss.

@tylerbenson
Copy link
Member Author

See update here: open-telemetry/semantic-conventions#354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:context Related to the specification/context directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants