-
Notifications
You must be signed in to change notification settings - Fork 571
fix(aws-lambda): BasicTracerProvider not force flushing #661
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(aws-lambda): BasicTracerProvider not force flushing #661
Conversation
When the tracer provider is set, the type is a ProxyTracerProvider that proxies to the BasicTracerProvider. This unwraps and gets the delegate to attempt to force flush.
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #661 +/- ##
==========================================
- 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
|
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.
@longility these comments should fix the unit tests (assuming no typos!)
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: William Armiros <[email protected]>
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.
Looks good to me. One minor suggestion up to you
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
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.
Thanks @longility! It looks like there are some lint errors to fix quickly.
This will unblock #660. @Flarna please take a look when you get a chance.
I’m not at my desk so maybe at least two hours before I can get to it |
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-aws-lambda/test/integrations/lambda-handler.force-flush.test.ts
Show resolved
Hide resolved
@Flarna is this ready for merge? |
@willarmiros I have approved yes. Not sure about the general guidelines. At least in the past one or two of the otel-js maintainers had to approve but maybe this has changed in contrib repo. |
Ah I didn't see that @dyladan had been re-requested for a review nvm - my bad. |
I figured I made enough changes for him to re-review to spot check. Sorry for the delay. |
When the tracer provider is set, the type is a ProxyTracerProvider that proxies to the BasicTracerProvider. This unwraps and gets the delegate to attempt to force flush.
fixes #647