-
Notifications
You must be signed in to change notification settings - Fork 621
fix(instrumentation-pino): Propagate all args to mixin
#2983
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
Conversation
We noticed that our user-defined `mixin` was breaking because it could not access the third `logger` argument. https://getpino.io/#/docs/api?id=mixin-function
| return Object.assign( | ||
| otelMixin(ctx, level), | ||
| origMixin(ctx, level) | ||
| origMixin(ctx, level, ...rest) |
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.
We could also explore an approach similar to https://github.com/DataDog/dd-trace-js/blob/213d56fb37bc7b929ee9cea99d21bb00eeafc412/packages/datadog-instrumentations/src/pino.js#L37-L51
Interesting. What is your environment? I'm just curious. Are you running some "in-browser dev environment"? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2983 +/- ##
=======================================
Coverage 89.83% 89.83%
=======================================
Files 188 188
Lines 9294 9295 +1
Branches 1907 1907
=======================================
+ Hits 8349 8350 +1
Misses 945 945
🚀 New features to boost your workflow:
|
|
Test failures. I'm guessing they are due to the test being dependent on a particular newer version of pino. The feature where the logger is passed into the mixin as the 3rd arg came in pinojs/pino#1709 which was included in v8.14.0. and the current installed pino is Still, we'll need to write the test to work with older versions for the "test-all-versions" tests. |
… to latest pino@8 Not updating to pino@9 because I think that drops support for some versions of Node.js we support here and might require using scripts/skip-test-if.js which I don't want to do yet.
trentm
left a comment
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.
This is great. Thanks for the fix.
|
Thanks for getting the tests going @trentm! Turns out this was PEBKAC; I recently set up a new laptop and mistakenly had Node.js version defaulted to v24 in the absence of a |
Which problem is this PR solving?
We noticed that our user-defined
mixinwas breaking because it could not access the thirdloggerargument. Excerpt from https://getpino.io/#/docs/api?id=mixin-function:[Update(by trentm): the 3rd arg to a mixin was added in [email protected].]
Short description of the changes
Try to propagate
mixinargs.Unfortunately I couldn't get this running on my local;
npm run compilejust hangs on my machine. So this is just some quick guesswork, hopefully it's enough to understand the problem though!