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(lambda): add OTEL_LAMBDA_DISABLE_AWS_CONTEXT_PROPAGATION environment variable #1227

Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Oct 13, 2022

Adds an option to configure the disableAwsContextPropagation by environment variable

@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Merging #1227 (c05e3cf) into main (8499b16) will decrease coverage by 0.98%.
The diff coverage is 93.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1227      +/-   ##
==========================================
- Coverage   96.13%   95.15%   -0.98%     
==========================================
  Files          14       17       +3     
  Lines         906     1177     +271     
  Branches      197      249      +52     
==========================================
+ Hits          871     1120     +249     
- Misses         35       57      +22     
Impacted Files Coverage Δ
...strumentation-document-load/src/instrumentation.ts 98.13% <88.88%> (-0.86%) ⬇️
...-instrumentation-aws-lambda/src/instrumentation.ts 94.18% <100.00%> (ø)

... and 2 files with indirect coverage changes

@@ -77,6 +78,18 @@ export class AwsLambdaInstrumentation extends InstrumentationBase {

constructor(protected override _config: AwsLambdaInstrumentationConfig = {}) {
super('@opentelemetry/instrumentation-aws-lambda', VERSION, _config);
Copy link
Member

@blumamir blumamir Oct 16, 2022

Choose a reason for hiding this comment

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

WDYT about modifying the config object before using it as a paramater in the super call? might be a bit more robust IMO

e.g, apply the env variable logic first, construct an updated config object, and only then call super with this final config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

@dyladan I was about to submit this same PR! It would be great to have this shipped

codeboten pushed a commit to codeboten/opentelemetry-python-contrib that referenced this pull request Dec 19, 2022
…ronment variable

The variable `OTEL_LAMBDA_DISABLE_AWS_CONTEXT_PROPAGATION` can be used to disable aws context propagation. This is similar to the proposed changes in the JS implementation: open-telemetry/opentelemetry-js-contrib#1227

Signed-off-by: Alex Boten <[email protected]>
ocelotl pushed a commit to open-telemetry/opentelemetry-python-contrib that referenced this pull request Dec 19, 2022
…ronment variable (#1507)

* Adds an option to configure `disable_aws_context_propagation` by environment variable

The variable `OTEL_LAMBDA_DISABLE_AWS_CONTEXT_PROPAGATION` can be used to disable aws context propagation. This is similar to the proposed changes in the JS implementation: open-telemetry/opentelemetry-js-contrib#1227

Signed-off-by: Alex Boten <[email protected]>

* update changelog

* Apply suggestions from code review

Co-authored-by: Srikanth Chekuri <[email protected]>

Signed-off-by: Alex Boten <[email protected]>
Co-authored-by: Srikanth Chekuri <[email protected]>
@blumamir
Copy link
Member

blumamir commented Jan 2, 2023

There are still some open comments in this PR which I think are easy to address.
Can you please take a look and fix/resolve the comments, and then rebase so it can be merged?

@blumamir
Copy link
Member

blumamir commented Feb 8, 2023

@dyladan - I cannot rebase this PR.
I think it is ready to be merged. Can you please rebase and merge?

@blumamir
Copy link
Member

blumamir commented Feb 8, 2023

Needs to run lint:fix and check why tests are failing.

Copy link

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

There's been an update toe the spec to change the behaviour and add a span link for the trace available in the environment variable. See open-telemetry/opentelemetry-specification#3166 and here's an example implementation: open-telemetry/opentelemetry-specification#3166. This change can be addressed separately

@github-actions
Copy link
Contributor

This PR 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 Apr 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

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

@github-actions github-actions bot closed this May 8, 2023
@dyladan dyladan reopened this Jun 16, 2023
@github-actions github-actions bot removed the stale label Jun 19, 2023
@dyladan dyladan merged commit 8777cbd into open-telemetry:main Jun 28, 2023
@dyladan dyladan mentioned this pull request Jun 28, 2023
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.

9 participants