-
Notifications
You must be signed in to change notification settings - Fork 892
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
apply serializers to args once before asObject or transmit #1971
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -302,7 +302,7 @@ function createWrap (self, opts, rootLogger, level) { | |
const proto = (Object.getPrototypeOf && Object.getPrototypeOf(this) === _console) ? _console : this | ||
for (var i = 0; i < args.length; i++) args[i] = arguments[i] | ||
|
||
if (opts.serialize && !opts.asObject) { | ||
if (opts.serialize && !opts.transmit) { | ||
applySerializers(args, this._serialize, this.serializers, this._stdErrSerialize) | ||
} | ||
if (opts.asObject || opts.formatters) { | ||
|
@@ -330,17 +330,22 @@ function createWrap (self, opts, rootLogger, level) { | |
|
||
function asObject (logger, level, args, ts, formatters = {}) { | ||
const { | ||
level: levelFormatter = () => logger.levels.values[level], | ||
level: levelFormatter, | ||
log: logObjectFormatter = (obj) => obj | ||
} = formatters | ||
if (logger._serialize) applySerializers(args, logger._serialize, logger.serializers, logger._stdErrSerialize) | ||
const argsCloned = args.slice() | ||
let msg = argsCloned[0] | ||
const logObject = {} | ||
if (ts) { | ||
logObject.time = ts | ||
} | ||
logObject.level = levelFormatter(level, logger.levels.values[level]) | ||
|
||
if (levelFormatter) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. restore previous implementation to fix this bug |
||
const formattedLevel = levelFormatter(level, logger.levels.values[level]) | ||
Object.assign(logObject, formattedLevel) | ||
} else { | ||
logObject.level = logger.levels.values[level] | ||
} | ||
|
||
let lvl = (logger._childLevel | 0) + 1 | ||
if (lvl < 1) lvl = 1 | ||
|
@@ -361,9 +366,9 @@ function applySerializers (args, serialize, serializers, stdErrSerialize) { | |
for (const i in args) { | ||
if (stdErrSerialize && args[i] instanceof Error) { | ||
args[i] = pino.stdSerializers.err(args[i]) | ||
} else if (typeof args[i] === 'object' && !Array.isArray(args[i])) { | ||
} else if (typeof args[i] === 'object' && !Array.isArray(args[i]) && serialize) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to start a for loop, if serialize is not true |
||
for (const k in args[i]) { | ||
if (serialize && serialize.indexOf(k) > -1 && k in serializers) { | ||
if (serialize.indexOf(k) > -1 && k in serializers) { | ||
args[i][k] = serializers[k](args[i][k]) | ||
} | ||
} | ||
|
@@ -385,6 +390,7 @@ function transmit (logger, opts, args) { | |
logger.serializers, | ||
logger._stdErrSerialize === undefined ? true : logger._stdErrSerialize | ||
) | ||
|
||
logger._logEvent.ts = ts | ||
logger._logEvent.messages = args.filter(function (arg) { | ||
// bindings can only be objects, so reference equality check via indexOf is fine | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,7 +144,7 @@ Unlike server pino the serializers apply to every object passed to the logger me | |
if the `asObject` option is `true`, this results in the serializers applying to the | ||
first object (as in server pino). | ||
|
||
For more info on serializers see https://github.com/pinojs/pino/blob/master/docs/api.md#parameters. | ||
For more info on serializers see https://github.com/pinojs/pino/blob/master/docs/api.md#mergingobject. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix docs |
||
|
||
### `transmit` (Object) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -284,7 +284,6 @@ test('children inherit parent serializers', ({ end, is }) => { | |
|
||
test('children serializers get called', ({ end, is }) => { | ||
const parent = pino({ | ||
test: 'this', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not part of the pino config, or log parameters, seems like a mistake |
||
browser: { | ||
serialize: true, | ||
write (o) { | ||
|
@@ -301,7 +300,6 @@ test('children serializers get called', ({ end, is }) => { | |
|
||
test('children serializers get called when inherited from parent', ({ end, is }) => { | ||
const parent = pino({ | ||
test: 'this', | ||
serializers: parentSerializers, | ||
browser: { | ||
serialize: true, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,7 +166,7 @@ test('opts.browser.asObject logs pino-like object to console', ({ end, ok, is }) | |
end() | ||
}) | ||
|
||
test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is }) => { | ||
test('opts.browser.formatters (level) logs pino-like object to console', ({ end, ok, is }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test names were the same, adding context for the different formatters config |
||
const info = console.info | ||
console.info = function (o) { | ||
is(o.level, 30) | ||
|
@@ -189,7 +189,7 @@ test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is | |
end() | ||
}) | ||
|
||
test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is }) => { | ||
test('opts.browser.formatters (log) logs pino-like object to console', ({ end, ok, is }) => { | ||
const info = console.info | ||
console.info = function (o) { | ||
is(o.level, 30) | ||
|
@@ -213,40 +213,66 @@ test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is | |
end() | ||
}) | ||
|
||
test('opts.browser.write func log single string', ({ end, ok, is }) => { | ||
const instance = pino({ | ||
test('opts.browser.serialize and opts.browser.transmit only serializes log data once', ({ end, ok, is }) => { | ||
const instance = require('../browser')({ | ||
serializers: { | ||
extras (data) { | ||
return { serializedExtras: data } | ||
} | ||
}, | ||
browser: { | ||
serialize: ['extras'], | ||
transmit: { | ||
level: 'info', | ||
send (level, o) { | ||
is(o.messages[0].extras.serializedExtras, 'world') | ||
} | ||
} | ||
} | ||
}) | ||
|
||
instance.info({ extras: 'world' }, 'test') | ||
end() | ||
}) | ||
|
||
test('opts.browser.serialize and opts.asObject only serializes log data once', ({ end, ok, is }) => { | ||
const instance = require('../browser')({ | ||
serializers: { | ||
extras (data) { | ||
return { serializedExtras: data } | ||
} | ||
}, | ||
browser: { | ||
serialize: ['extras'], | ||
asObject: true, | ||
write: function (o) { | ||
is(o.level, 30) | ||
is(o.msg, 'test') | ||
ok(o.time) | ||
is(o.extras.serializedExtras, 'world') | ||
} | ||
} | ||
}) | ||
instance.info('test') | ||
|
||
instance.info({ extras: 'world' }, 'test') | ||
end() | ||
}) | ||
|
||
test('opts.browser.write func string joining', ({ end, ok, is }) => { | ||
test('opts.browser.write func log single string', ({ end, ok, is }) => { | ||
const instance = pino({ | ||
browser: { | ||
write: function (o) { | ||
is(o.level, 30) | ||
is(o.msg, 'test test2 test3') | ||
is(o.msg, 'test') | ||
ok(o.time) | ||
} | ||
} | ||
}) | ||
instance.info('test %s %s', 'test2', 'test3') | ||
instance.info('test') | ||
|
||
end() | ||
}) | ||
|
||
test('opts.browser.write func string joining when asObject is true', ({ end, ok, is }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test was repeated, see line 262 below, I deleted this, git somehow is reconciling my changes adding new test using the these lines |
||
test('opts.browser.write func string joining', ({ end, ok, is }) => { | ||
const instance = pino({ | ||
browser: { | ||
asObject: true, | ||
write: function (o) { | ||
is(o.level, 30) | ||
is(o.msg, 'test test2 test3') | ||
|
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 the required change to fix the bug