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

AWS Lambda Instrumentation should use presence of context, not env var, to parent to Lambda #565

Closed
anuraaga opened this issue Jul 8, 2021 · 6 comments
Labels
bug Something isn't working stale

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Jul 8, 2021

Currently, AWS Lambda instrumentation checks whether a Lambda-provided parent is sampled or not to determine whether to instead extract from headers.

https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts#L322

Actually Lambda always populates the environment variable, and it will be just a trace ID when env var propagation should be ignored. If we don't make sure to ignore this case, then it is never possible for the Lambda to start a sampled trace. This should be tweaked to not overwrite the parent variable

https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts#L312

@anuraaga anuraaga added the bug Something isn't working label Jul 8, 2021
@anuraaga
Copy link
Contributor Author

anuraaga commented Jul 8, 2021

/cc @wangzlei @willarmiros @NathanielRN

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2021

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Nov 8, 2021
@NathanielRN
Copy link
Contributor

Just a quick question,

then it is never possible for the Lambda to start a sampled trace

Are you saying that Lambda cannot start a sampled trace if the parent was constructed by reading an _X_AMZN_TRACE_ID environment variable without a sampling decision? I that makes sense, because "no sampling decision" == "do not sample" IIRC. Actually it sounds a lot like open-telemetry/opentelemetry-python-contrib#649 (comment)

Which we solved by just updating the documentation to warn users that traces will be "not sampled" unless they explicitly set Sample=1; in the _X_AMZN_TRACE_ID.

@github-actions github-actions bot removed the stale label Nov 15, 2021
@NathanielRN
Copy link
Contributor

I was wondering if this is still an issue? I can see that the current implementation only overwrites the parent value if the _X_AMZN_TRACE_ID is ;Sampled=1. This should only be set if we have "active tracing" enabled on the AWS Lambda configuration, and is ;Sampled=0 if this is turned off (based on my tests on a Python Lambda).

if (
spanContext &&
(spanContext.traceFlags & TraceFlags.SAMPLED) === TraceFlags.SAMPLED
) {

There are also methods to disableAwsContextPropagation and provide a helper to extract a custom context from the Lambda event. So I would say the behavior is what we expect according to the OTel specifications for Determining a parents in Lambda Instrumentation? Please let me know what you think!

@github-actions
Copy link
Contributor

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Feb 21, 2022
@github-actions
Copy link
Contributor

This issue was closed because it has been stale for 14 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale
Projects
None yet
Development

No branches or pull requests

2 participants