Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 semantic conventions for instrumenting AWS Lambda. #1442
Add semantic conventions for instrumenting AWS Lambda. #1442
Changes from 2 commits
a970192
9a3e392
86586ba
a11446f
438eb49
125feac
b94df8c
c4105fc
bc363eb
c258950
2869623
11b0a9f
812ef2e
1065bb5
fc3f707
d10423c
eef19ac
8ce446e
ed97a08
5c9d575
5b2bb84
d6f7640
ca27d56
77a4edf
e5d5934
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the attributes here are not Lambda specific, can we extend the https://github.com/open-telemetry/opentelemetry-specification/blob/main/semantic_conventions/trace/faas.yaml instead? If not, maybe we should namespace them differently. For example, "aws.lambda.id", "aws.lambda.execution", etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was the intention of @anuraaga. FaaS conventions IMHO sufficiently describe the common lambda attributes (e.g. it tells you that the ARN is the faas.id in the faas' note already). But this document describes in more detail how to apply existing semantic conventions to Lambda. So it is IMHO right to refer to (note that
ref
is used here instead of introducing a new attribute withid
) the existing, fitting attributes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern seems commonly useful enough that special support in the semantic convention generator would be nice as in
fixed_value: "AmazonSQS"
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This document doesn't mention its relationship to the FaaS spec, https://github.com/open-telemetry/opentelemetry-specification/blob/main/semantic_conventions/trace/faas.yaml. Is it extending it or overriding it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the first paragraph clarify this, including an actual link to faas? Let me know if something's unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that the application owner should be able to decide this. Ideally, the X-Ray propagator (or a specialized LambdaXrayPropagator) would be written in such a way that it checks the environment variable itself. The instrumentation should not decide this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user can currently decide by enabling or disabling XRay so I think this reflects that choice well. If we had another option in the instrumentation it's two settings related to XRay which I think is just an extra setting.
It reminds me that in a follow-up to the SDK PR I need to describe using the AWS propagator directly on the HTTP calls as we do in Java. It's the only recognized format for the next years so there is no point for an option at least until that changes it's just an extra option. I think it's similar for Lambda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supported by whom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be possible to use any OpenTelemetry-compatible vendor with SQS without paying for X-Ray. I'd like it more if we say something to the effect that "Instrumentations SHOULD default to using AWSTraceHeader with AWS X-Ray Propagator, but SHOULD be configurable to use any other header and propagator". If that makes sense at all. I.e., I don't think we should indirectly "force" users to pay for X-Ray in our semantic conventions, even if AWS has implemented special features to make it nicer to use than other solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also related: #1442 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant supported by our instrumentation.
AWSTraceHeader
doesn't mention the word X-Ray :) It's how to propagate context within AWS services and by having AWS SDK instrumentation and Lambda instrumentation explicitly use the X-Amzn-Trace-Id format (which we generally incorrectly call x-ray, it's actually not really tied to x-ray), it precisely allows any vendor to use AWS asynchronous services with proper propagation. No payment for X-Ray or anything involved. While it might be possible to implement our own propagation through message attributes, I'm not confident it would be propagated between services (e.g., S3 -> SNS -> SQS is a possible flow) while I am confident thatAWSTraceHeader
will be, regardless of tracing vendor. @kubawach has done incredible testing on these propagation cases, I'm guessing for use with Splunk, not X-Ray :PI am going to try adding some text with some of this information to allay the concerns, which are fair but I wouldn't be worried :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support @anuraaga here.
With the first approach to AWS propagation (for SQS producer - consumer) I used "standard" HTTP-headers mechanism, creating a SQS message attribute (upon produce) and extracting parent from it (upon consume). While this approach worked in SQS - SQS scenario, it would fail with more complex (but generally used around the world) scenarios such as S3 - SQS or S3 - SNS - SQS. Therefore I had to switch back to using AWS feature, guaranteeing that if AWS Trace header is set during AWS SDK request, the value will be maintained and returned at the end of the chain (consume - as SQS system attribute). There was simply no other way to do it (ie without relying on AWS features).
To sum up, approach that came out of discussions and code reviews was:
Frankly at the beginning it didn't feel right to implement propagation enforcing AWS trace format but I realised that it's just relying on how a closed system work, just as we do with instrumentation libraries, in order to support as many use cases as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment addressed @arminru ?