feat: Allow configuring pino to log with different keys #1867
Conversation
| } | ||
|
|
||
| before(() => { | ||
| setup(); |
There was a problem hiding this comment.
If I understand this correct this causes that all tests below use the new keys, not just the new injects span context to records with custom keys test.
There was a problem hiding this comment.
That's right. I was having trouble configuring just one test independently of the others with the current setup. I can spend a bit more time on it
There was a problem hiding this comment.
Cleaned them up so that the logKeys config only applies to a single test but the tests are interfering with each other causing the "disabled instrumentation" block of tests to fail. If I comment out all the other describe blocks then they pass. I'm stumped at the moment but will revisit this in the next couple days
There was a problem hiding this comment.
Seems like the pino tests were written in a way to instrument just once and then run all tests.
Don't know if there was a reason. Theoretically .disable() should cause to de-instrument and therefore should allow side effect free tests.
But there might be cases where de-instrument doesn't work, e.g. if pino caches some stuff internal.
There was a problem hiding this comment.
I definitely don't have a perfect understanding of this patching, but I tried creating a new instrumentation + explicitly calling enable() on it as well as clearing the require cache via delete require.cache[require.resolve('pino')] but it made no difference. Would love to hear alternatives to try as welll
There was a problem hiding this comment.
I broke the enabled + disabled tests out into their own modules to skirt the problem. Let me know what you think
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1867 +/- ##
=======================================
Coverage 91.45% 91.45%
=======================================
Files 145 145
Lines 7410 7412 +2
Branches 1483 1484 +1
=======================================
+ Hits 6777 6779 +2
Misses 633 633
|
|
@seemk Could you as owner please take a look? |
| return record; | ||
| } | ||
|
|
||
| export function init(importType: 'global' | 'default' | 'pino' = 'global') { |
There was a problem hiding this comment.
Perhaps instead of using global this should return something like a test context object containing the currently global fields?
|
Thank you @blumamir! Could someone with access merge this whenever you get a chance, thanks! |
Which problem is this PR solving?
#1664
Short description of the changes
Allow configuring the keys that the pino instrumentation adds to logs