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 requirement to use X-Ray propagation for aws-sdk client calls #3212

Merged
merged 24 commits into from
May 9, 2023

Conversation

tylerbenson
Copy link
Member

X-Ray is necessary because it is the only format currently supported by AWS to allow context propagation through AWS managed services, for example SNS -> SQS -> Lambda, or S3 -> Lambda.

X-Ray is necessary because it is the only format currently supported by AWS to allow context propagation through AWS managed services, for example SNS -> SQS -> Lambda, or S3 -> Lambda.
@tylerbenson tylerbenson requested review from a team February 15, 2023 20:49
@carlosalberto
Copy link
Contributor

Changelog entry please ;)

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

X-Ray is necessary because it is the only format currently supported by AWS to allow context propagation through AWS managed services, for example SNS -> SQS -> Lambda

At Dynatrace, we have actually been using W3C-tracecontext-based propagation successfully with this scenario, as SNS message attributes are mapped to SQS message attributes or transported as part of the message body (JSON payload). Our backend relies on additional information in the tracestate field that cannot be transported over the X-Ray header.
So we rely on the fact that many existing implementations of this (e.g. Node.Js, Ruby, Python) use (non-system) message attributes and the configured global propagator for this.

If these were changed to only use X-Ray, it would break these scenarios for Dynatrace and our customers (the scenarios are documented e.g. here: https://www.dynatrace.com/support/help/shortlink/opentel-lambda#python-sqs-sns). For example the Python PR open-telemetry/opentelemetry-python-contrib#1673 that probably results from this Spec PR would be problematic for us (we'll also comment there).

or S3 -> Lambda.

This is indeed only possible with using X-Ray headers (but doesn't invalidate the previous scenario)

I propose that we clarify that additional propagation mechanisms MAY be used where it makes sense (e.g. messaging operations).

Also, I think there is no need to use the strong MUST which basically bans instrumentations from offering an option to disable X-Ray propagation.

tylerbenson and others added 2 commits February 16, 2023 13:12
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Required reformatting... sorry for messing up the diff.
@tsloughter
Copy link
Member

I think for HTTP headers in AWS requests the Xray propagator should always be used (until a time AWS changes this).

For per-service instrumentation like MessageAttributes in SQS there is no need because it is invisible to AWS anyway.

What I'm not sure on is whether SQS instrumentation should be additionally inserting context into MessageAttributes or not.

@Oberon00 I notice dynatrace python guide has the user defining the extraction themselves because python's lambda instrumentation doesn't do it -- something I'd like to change but it would be a Link and not the parent, so not a replacement for what you have here. Would requiring similar for injection not be acceptable?

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Feb 21, 2023
@Oberon00
Copy link
Member

Oberon00 commented Feb 21, 2023

Would requiring similar for injection not be acceptable?

For extraction this is conceptually required for batch processing. For injection, it would be a shame if the user would have to do it manually. That would also mean that they would have to create the span manually (since you can only do manual injection before calling the function in which the span is created by the instrumentation). Basically we would not be able to use the instrumentation at all anymore and instruct users to do everything manually, or create and distribute a fork of the instrumentation code.

# Conflicts:
#	CHANGELOG.md
@carlosalberto
Copy link
Contributor

So trying to summarize the current state after the latest comments, just to be clear (please do correct me if I'm wrong):

  • X-Ray seems to be the only propagated format currently, but we would like to keep the door open for future AWS approved/supported formats?
  • Propagation support through message attributes: although the plan has been to allow instrumentation to fallback to this, is this something that can be totally unnecessary by allowing custom propagation formats? Can tracestate really be propagated this way? Based on the previous comments, I'd say no?

On a slightly related note, there's a (merged) OTEP around Messaging Context Propagation which mentions that per-Message context should be propagated, rather than per-request context (cc @pyohannes in case you have two cents to add here, etc).

@Oberon00
Copy link
Member

Oberon00 commented Feb 24, 2023

@carlosalberto

X-Ray seems to be the only propagated format currently, but we would like to keep the door open for future AWS approved/supported formats?

This is true in the general case. But specific APIs will often provide other ways where you can transport extract/inject key-value-pairs, e.g. message attributes for SNS or SQS.

@trask
Copy link
Member

trask commented Apr 19, 2023

/easycla

@nickmango
Copy link

/easycla

3 similar comments
@trask
Copy link
Member

trask commented Apr 20, 2023

/easycla

@trask
Copy link
Member

trask commented Apr 20, 2023

/easycla

@jarias-lfx
Copy link

/easycla

@carlosalberto
Copy link
Contributor

@jsuereth @Aneurysm9 @Oberon00 please review the updated PR - the content itself remains the same, albeit located now in the non-formal supplementary-guides/ directory.

@Oberon00 Oberon00 requested a review from Aneurysm9 April 25, 2023 14:18
CHANGELOG.md Outdated Show resolved Hide resolved
supplementary-guidelines/compatibility/aws.md Outdated Show resolved Hide resolved
supplementary-guidelines/compatibility/aws.md Outdated Show resolved Hide resolved
supplementary-guidelines/compatibility/aws.md Outdated Show resolved Hide resolved
supplementary-guidelines/compatibility/aws.md Outdated Show resolved Hide resolved
supplementary-guidelines/compatibility/aws.md Outdated Show resolved Hide resolved
@Oberon00
Copy link
Member

I realize I added a lot of suggestions despite approving -- but none of them are blocking, so I would actually be OK-ish with merging this as-is (but please do consider my suggestions nevertheless 🙂)

tylerbenson and others added 3 commits May 3, 2023 11:02
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@reyang reyang removed the request for review from Aneurysm9 May 9, 2023 04:21
@reyang
Copy link
Member

reyang commented May 9, 2023

This PR has been existing for long time and already received multiple approvals, I will merge it once the CI is clear. The change would be released in the new https://github.com/open-telemetry/semantic-conventions (to be created) repository.

Related to #3474 (review).

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

Successfully merging this pull request may close these issues.