-
Notifications
You must be signed in to change notification settings - Fork 552
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(instrumentation-aws-lambda): soften "unable to init" message and demote to diag.debug #1836
fix(instrumentation-aws-lambda): soften "unable to init" message and demote to diag.debug #1836
Conversation
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.
LGTM.
It is reasonable to have any given instrumentation enabled, even if it ends up not being relevant for the app; and is commonly the case given the current advice to use auto-instrumentations-node
.
If those envvars (defined at https://docs.aws.amazon.com/lambda/latest/dg/configuration-envvars.html#configuration-envvars-runtime) are not set, then the app isn't in a Lambda env. In that case, it is weird to log an error about it. I'd even consider softening the language of the debug message.
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
…rumentation.ts Co-authored-by: Trent Mick <trentm@gmail.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1836 +/- ##
=======================================
Coverage 96.06% 96.06%
=======================================
Files 14 14
Lines 914 914
Branches 199 199
=======================================
Hits 878 878
Misses 36 36 |
Hey @trentm looks like CI is failing due to code coverage (0% of new lines are covered by tests, the horror). Is it cool to merge this as-is? |
Heh. @beggers I think it is good to go, yes. However, I'm a fairly new approver on this repo, so I was going to wait a little while to give others a chance to approve or not. @carolabadeer You are the component owner, do you have an opinion on this PR? Thanks. |
Which problem is this PR solving?
When not in a Lambda, the AWS Lambda instrumentation spits out an
error
-level message saying it can't initialize. This pollutes logs for all Node applications using autoinstrumentation.Short description of the changes
debug
-level message since this is an expected case.