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

fix: Update aws-lambda-instrumentation to SDK v0.25.0 #660

Merged
merged 6 commits into from
Sep 22, 2021

Conversation

willarmiros
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Upgrading the opentelemetry-instrumentation-aws-lambda package's core dependencies to 0.25.0, and fixing associated breaking changes.

@willarmiros willarmiros requested a review from a team September 15, 2021 06:14
@willarmiros willarmiros changed the title Lambda bug fix Lambda instrumentation bug with SDK v0.25.0 Sep 15, 2021
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #660 (9fb777e) into main (76e0d0f) will decrease coverage by 0.37%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #660      +/-   ##
==========================================
- Coverage   96.82%   96.44%   -0.38%     
==========================================
  Files           9       14       +5     
  Lines         630     1210     +580     
  Branches      124      171      +47     
==========================================
+ Hits          610     1167     +557     
- Misses         20       43      +23     
Impacted Files Coverage Δ
...ws-lambda/test/integrations/lambda-handler.test.ts 97.97% <0.00%> (ø)
...nstrumentation-aws-lambda/test/lambda-test/sync.js 100.00% <0.00%> (ø)
...st/integrations/lambda-handler.force-flush.test.ts 96.66% <0.00%> (ø)
...-instrumentation-aws-lambda/src/instrumentation.ts 90.84% <0.00%> (ø)
...strumentation-aws-lambda/test/lambda-test/async.js 100.00% <0.00%> (ø)

@willarmiros willarmiros marked this pull request as draft September 15, 2021 07:42
@willarmiros
Copy link
Contributor Author

Converting to draft since this doesn't fix the bug (yet). Just wanted to test it with GH Actions, but am reproducing locally now.

@willarmiros willarmiros changed the title fix Lambda instrumentation bug with SDK v0.25.0 fix: Update aws-lambda-instrumentation to SDK v0.25.0 Sep 21, 2021
@willarmiros willarmiros marked this pull request as ready for review September 21, 2021 21:22
@willarmiros
Copy link
Contributor Author

Now that #661 is merged this is ready for review 👍

@@ -23,8 +23,8 @@ import { AwsLambdaInstrumentation } from '../../src';
import {
BatchSpanProcessor,
InMemorySpanExporter,
} from '@opentelemetry/tracing';
import { NodeTracerProvider } from '@opentelemetry/node';
} from '@opentelemetry/sdk-trace-base';
Copy link
Member

Choose a reason for hiding this comment

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

This dependency is missing in package.json dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

This is in devDependencies. Basically, if the tests passes, then we are good as the dependencies are good.

Copy link
Member

Choose a reason for hiding this comment

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

It was not, but it has been added. It was a transitive dependency of something else which is why the tests were passing but it is good practice to declare all of your dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently, I'm blind. Thanks for sharing your perspective.

@vmarchaud vmarchaud requested a review from dyladan September 22, 2021 09:53
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.

5 participants