feat: replace Bunyan logger with Pino#3214
Conversation
rpl
left a comment
There was a problem hiding this comment.
@fregante this PR looks pretty good already (thanks for taking care of replacing buyan with pino while keeping the change 100% contained inside the logger module and its unit test!!!), there is only a small change I'd like us to consider before proceeding to merge this PR (getting rid of the check from the ConsoleStream write method to account for both js object vs json string passed as its first parameter).
src/util/logger.js
Outdated
| write(packet, { localProcess = process } = {}) { | ||
| const thisLevel = this.verbose ? bunyan.TRACE : bunyan.INFO; | ||
| write(json, { localProcess = process } = {}) { | ||
| const packet = typeof json === 'string' ? JSON.parse(json) : json; |
There was a problem hiding this comment.
Thought: I was wondering about this part conditioned on typeof json === 'string' and why it was needed, but I see now that pino is actually going to call this always with a string as the first parameter while the only case where it is a js object is when this class is exercised by the related unit test test.logger.js. I feel like I'd prefer to adjust the test to match how this method will be called by the actual pino logger and remove this special handling from here (and rename the parameter from json to jsonString or something like that), so that the unit test doesn't do something different from what the actual pino logger would be doing.
It looks like we just need to do a couple more tweaks to the test.logger.js unit test to get rid of this:
diff --git a/tests/unit/test-util/test.logger.js b/tests/unit/test-util/test.logger.js
index edb8991..bf738e4 100644
--- a/tests/unit/test-util/test.logger.js
+++ b/tests/unit/test-util/test.logger.js
@@ -23,12 +23,12 @@ describe('logger', () => {
describe('ConsoleStream', () => {
function packet(overrides) {
- return {
+ return JSON.stringify({
name: 'some name',
msg: 'some messge',
level: logLevels.values.info,
...overrides,
- };
+ });
}
function fakeProcess() {
@@ -52,13 +52,11 @@ describe('logger', () => {
it('logs names in verbose mode', () => {
const log = new ConsoleStream({ verbose: true });
assert.equal(
- log.format(
- packet({
- name: 'foo',
- msg: 'some message',
- level: logLevels.values.debug,
- }),
- ),
+ log.format({
+ name: 'foo',
+ msg: 'some message',
+ level: logLevels.values.debug,
+ }),
'[foo][debug] some message\n',
);
});
@@ -66,7 +64,7 @@ describe('logger', () => {
it('does not log names in non-verbose mode', () => {
const log = new ConsoleStream({ verbose: false });
assert.equal(
- log.format(packet({ name: 'foo', msg: 'some message' })),
+ log.format({ name: 'foo', msg: 'some message' }),
'some message\n',
);
});@fregante how that sounds? any reasons for preferring to keep it as in this version of this patch that I may have missed to notice?
There was a problem hiding this comment.
any reasons
No, it was just a bad fix on my part. You're right it should accept just a string like it will in production.
I merged your patch and it appears to work
Co-Authored-By: Luca Greco <11484+rpl@users.noreply.github.com>
|
The logs have been handled 🤝
|
bunyan#2456