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

feat: implement lambda context parenting #431

Merged
merged 16 commits into from
May 20, 2021

Conversation

anuraaga
Copy link
Contributor

Which problem is this PR solving?

  • Lambda spans are not connected to remote parent

Short description of the changes

  • Implement specification for lambda remote propagation.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/instrumentation/aws-lambda.md#determining-the-parent-of-a-span

Note, this contains #424, happy to rebase this after that's merged

gregoryfranklin and others added 3 commits April 15, 2021 13:55
Without this, spans created in the lambda function are not part of the
same trace as the handler span.
@anuraaga anuraaga requested a review from a team April 19, 2021 05:04
@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #431 (f680aa6) into main (b7e0bdd) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
+ Coverage   95.26%   95.29%   +0.03%     
==========================================
  Files         140      140              
  Lines        8399     8519     +120     
  Branches      815      824       +9     
==========================================
+ Hits         8001     8118     +117     
- Misses        398      401       +3     
Impacted Files Coverage Δ
...-instrumentation-aws-lambda/src/instrumentation.ts 93.75% <0.00%> (-0.37%) ⬇️
...ws-lambda/test/integrations/lambda-handler.test.ts 97.15% <0.00%> (+1.07%) ⬆️

@dyladan
Copy link
Member

dyladan commented Apr 19, 2021

Using the propagator directly in instrumentations is typically not the recommended approach. You should use api.propagation.extract and the user can register any propagator they want such as w3c, b3, xray, or some combination. Is there some reason this pattern doesn't work for your use case?

@anuraaga
Copy link
Contributor Author

anuraaga commented Apr 20, 2021

@dyladan Yeah the spec specifically says to use the X-Ray propagator

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/instrumentation/aws-lambda.md#determining-the-parent-of-a-span

This is because the X-Ray propagator (better name would be AWS trace format) is an implementation detail of AWS right now. Eventually we'll support more formats but until then, it's the only format that Lambda supports. So if you imagine a user that has their entire stack set to W3C, they could have

A (EKS) --w3c--> B(EKS) --x-amzn--> S3 --x-amzn--> F (Lambda) --w3c--> C (EKS)

AWS instrumentation (call to S3) must use the x-amzn header, regardless of what the user set, or propagation will be lost. Similarly, while we could expect the Lambda function to define w3c and x-amzn as a composite propagator, it's wasteful and unintuitive - there's no need to send x-amzn to C, we only want it to parse the environment variable set by Lambda. So, until AWS supports more formats (a while away), forcing the propagator is the best user experience on AWS.

I realized the note I added for SQS would be helpful for the parenting section too

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/instrumentation/aws-lambda.md#sqs-message

The key point is the format is not directly related to X-Ray and is the correct format for non-X-Ray users too, any AWS user must be using the X-Amzn-Trace-Id header to get propagation within the cloud.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Generally LGTM

Comment on lines 27 to 28
Context as OtelContext,
context as otelContext,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the two different contexts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's context, an object with methods exposing the context API of type ContextAPI. Then there's Context, which is the type of an actual passed around context. It would be nice I think if context could be renamed contextAPI or similar.

@anuraaga
Copy link
Contributor Author

Thanks - rebased

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, just 2 nit picks

kind: SpanKind.SERVER,
attributes: {
[SemanticAttributes.FAAS_EXECUTION]: context.awsRequestId,
'faas.id': context.invokedFunctionArn,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: 'faas.id' - if there isn't a semantic attributes maybe create some const for that ?

Copy link
Member

Choose a reason for hiding this comment

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

@anuraaga
Copy link
Contributor Author

So sorry for the huge delay on this, updated

@anuraaga
Copy link
Contributor Author

Merged master to fix build. Think this is good for another look

@vmarchaud vmarchaud merged commit b9063ba into open-telemetry:main May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants