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

carlosalberto · 2023-08-16T17:53:26Z

This is done to offer compatibility to users with the previous behavior (which used the context in this X-Ray environment variable as the main parent).

Please review @open-telemetry/lambda-extension-approvers @dyladan @open-telemetry/python-maintainers

Note: I didn't include an actual name for this option, but I'm open to suggestions.

Closely related to open-telemetry/opentelemetry-js-contrib#1411

This is done in order to help users migrating from the previous behavior (which is offered via this new flag) and the new approach (which uses links).

codeboten · 2023-08-16T18:17:56Z

docs/faas/aws-lambda.md

@@ -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`,


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`,

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

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.

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?

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)

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".

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

👍

(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)

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

(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".

docs/faas/aws-lambda.md

dyladan

Looks good. I thought we had agreed on some sort of deadline to come up with a longer term fix. Is that not part of this PR?

CHANGELOG.md

dyladan · 2023-08-16T18:29:19Z

docs/faas/aws-lambda.md

@@ -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`,


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

carlosalberto · 2023-08-17T18:35:55Z

@dyladan I don't think so, no. The longer term may take longer, and we don't want to block progress for now (hopefully we also have a smooth transition). Correct me if I'm wrong @open-telemetry/lambda-extension-approvers

cc @rapphil as you have a PR to address this for Python

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

Co-authored-by: Alex Boten <alex@boten.ca>

Aneurysm9 · 2023-08-18T19:14:07Z

This change does not work for me. I have created #272 as an alternate proposal.

Oberon00 · 2023-08-21T11:54:45Z

docs/faas/aws-lambda.md

@@ -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`,


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.

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.

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

jsuereth · 2023-08-23T19:32:48Z

FYI - TC discussion and opinion on this PR: #277 (comment)

jsuereth · 2023-10-23T17:07:11Z

Closing this in favor of ongoing discussion in #354

carlosalberto added 2 commits August 16, 2023 19:32

[FaaS] Add an option to use the X-Ray env var as main parent.

5f9f1d4

This is done in order to help users migrating from the previous behavior (which is offered via this new flag) and the new approach (which uses links).

Add a blank CHANGELOG entry for now.

33bbe47

carlosalberto requested review from a team August 16, 2023 17:53

github-actions bot assigned jsuereth Aug 16, 2023

codeboten reviewed Aug 16, 2023

View reviewed changes

dyladan reviewed Aug 16, 2023

View reviewed changes

carlosalberto and others added 2 commits August 17, 2023 12:36

Update CHANGELOG.md

ce370c6

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

Update docs/faas/aws-lambda.md

791496e

Co-authored-by: Alex Boten <alex@boten.ca>

Oberon00 reviewed Aug 21, 2023

View reviewed changes

trask mentioned this pull request Aug 21, 2023

FaaS: Change requirements regarding handling of AWS Lambda-provided SpanContext #272

Closed

Aneurysm9 mentioned this pull request Aug 21, 2023

FaaS: Change requirements regarding handling of AWS Lambda-provided SpanContext (take 2) #277

Closed

jsuereth closed this Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

carlosalberto commented Aug 16, 2023

codeboten Aug 16, 2023

jsuereth Aug 16, 2023

Oberon00 Aug 21, 2023

Aneurysm9 Aug 21, 2023

Oberon00 Aug 21, 2023

Aneurysm9 Aug 21, 2023

trask Aug 21, 2023

trask Aug 21, 2023

Aneurysm9 Aug 21, 2023

Aneurysm9 Aug 21, 2023

dyladan left a comment

dyladan Aug 16, 2023

carlosalberto commented Aug 17, 2023

Aneurysm9 commented Aug 18, 2023

Oberon00 Aug 21, 2023 •

edited

Loading

Aneurysm9 Aug 21, 2023

Oberon00 Aug 24, 2023 •

edited

Loading

jsuereth commented Aug 23, 2023

jsuereth commented Oct 23, 2023

	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`,

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

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

Conversation

carlosalberto commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Aug 17, 2023

Aneurysm9 commented Aug 18, 2023

Oberon00 Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

jsuereth commented Aug 23, 2023

jsuereth commented Oct 23, 2023

Oberon00 Aug 21, 2023 •

edited

Loading

Oberon00 Aug 24, 2023 •

edited

Loading