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

Faas: Add an option to use the X-Ray env var as main parent. #263

Closed
wants to merge 4 commits into from
Closed

Faas: Add an option to use the X-Ray env var as main parent. #263

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f9f1d4
[FaaS] Add an option to use the X-Ray env var as main parent.
carlosalberto Aug 16, 2023
33bbe47
Add a blank CHANGELOG entry for now.
carlosalberto Aug 16, 2023
ce370c6
Update CHANGELOG.md
carlosalberto Aug 17, 2023
791496e
Update docs/faas/aws-lambda.md
carlosalberto Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ release.
- Clarify that `http/dup` has higher precedence than `http` in case both values are present
in `OTEL_SEMCONV_STABILITY_OPT_IN`
([#249](https://github.com/open-telemetry/semantic-conventions/pull/249))
- FaaS: Add an option to use the X-Ray env var as main parent.
([]())
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

## v1.21.0 (2023-07-13)

Expand Down
4 changes: 4 additions & 0 deletions docs/faas/aws-lambda.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ contain an incomplete trace context which indicates X-Ray isn’t enabled. The e
`Context` will be valid and sampled only if AWS X-Ray has been enabled for the Lambda function. A user can
disable AWS X-Ray for the function if the X-Ray Span Link is not desired.

Instrumentation SHOULD include an explicit option to use `Context` in `_X_AMZN_TRACE_ID` as parent insted of `Link`,
Copy link

Choose a reason for hiding this comment

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

Suggested change
Instrumentation SHOULD include an explicit option to use `Context` in `_X_AMZN_TRACE_ID` as parent insted of `Link`,
Instrumentation SHOULD include an explicit option to use `Context` in `_X_AMZN_TRACE_ID` as parent instead of `Link`,

Copy link
Contributor

Choose a reason for hiding this comment

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

It's my understanding this is a temporary stop-gap to help existing user migrate.

We should use similar language/terminology as was done in http semconv about migration period here. See: https://github.com/open-telemetry/semantic-conventions/tree/main/docs/http

Copy link
Member

Choose a reason for hiding this comment

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

Really? I think having an option to use the X-Ray context as parent seems OK also long-term. Though I would change the behavior slightly to always include the link in addition.

Copy link
Member

Choose a reason for hiding this comment

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

More than just "OK", it is imperative that X-Ray users have the ability to utilize the X-Ray span context that is provided to them by the Lambda service. We cannot have a system that makes that impossible and should not have one that leaves it to the instrumentation author to decide whether that category of users should be supported.

What benefit would be gained from having the same span context both as parent and as a link?

Copy link
Member

Choose a reason for hiding this comment

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

What benefit would be gained from having the same span context both as parent and as a link?

The benefit would be that the link would always be there independent of any instrumentation configuration.

(It could also be compared against the parent to determine if the source was X-Ray, but if that is a common use case we would be better of with a "parent_source" span attribute mirroring the link "source" attribute)

Copy link
Member

Choose a reason for hiding this comment

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

I think some of the flexibility across the semantic conventions is intentional in order to cover a broader range of (future) third-party instrumentations. I personally expect all instrumentations published under the OpenTelemetry org to follow all "Recommendeds" and SHOULDs, though I don't think we've had this discussion as a community (I've added this question to next week's semconv meeting).

I generally agree that SHOULD means "there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course." I do not agree that there can be no other course taken as SHOULD is not MUST. I expect that maintaining compatibility while experimental specification language is being worked on to be a good reason for "choosing a different course".

Copy link
Member

Choose a reason for hiding this comment

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

I expect that maintaining compatibility while experimental specification language is being worked on to be a good reason for "choosing a different course".

👍

Copy link
Member

Choose a reason for hiding this comment

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

(at the same time, adopting the latest semconv in instrumentations is the best way to get real user feedback which can be cycled back into semconv before it gets marked as stable)

Copy link
Member

Choose a reason for hiding this comment

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

@jsuereth please see #277 for an attempt at language that would govern how instrumentation constructs a Carrier to be used with a user-specified Propagator. I believe this should address all of your concerns expressed here.

Copy link
Member

Choose a reason for hiding this comment

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

(at the same time, adopting the latest semconv in instrumentations is the best way to get real user feedback which can be cycled back into semconv before it gets marked as stable)

I think that's what happened here. The real user feedback was "this broke every existing usage of this instrumentation with no way to make it work again".

Copy link
Member

Choose a reason for hiding this comment

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

Does the above paragraph also need to be modified to be "aware" of this configuration?

Copy link
Member

@Oberon00 Oberon00 Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is it expected that instrumentations do anything special about X-Ray parents with unset sampled flag? If not, maybe a note could still be added that this may lead to all spans being unsampled unless X-Ray is enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that the case with any parent span context with an unset sampled flag? If so, that's something that should probably be addressed more broadly by the spec.

Copy link
Member

@Oberon00 Oberon00 Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it is, but X-Ray is unusual in that you will get a valid unsampled span context if X-Ray is disabled.

as long as it is a valid `Context`, as described above. Else, it will follow the standard behavior of using the extracted value
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
from the user's configured propagators applied to the HTTP headers.

**Note**: When instrumenting a Java AWS Lambda, instrumentation SHOULD first try to parse an OpenTelemetry `Context` out of the system property `com.amazonaws.xray.traceHeader` using the [AWS X-Ray Propagator](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md) before checking and attempting to parse the environment variable above.

[Span Link]: https://opentelemetry.io/docs/concepts/signals/traces/#span-links
Expand Down
Loading