-
Notifications
You must be signed in to change notification settings - Fork 879
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
integrate fast-redact v3 #917
integrate fast-redact v3 #917
Conversation
Can you please this? I don't understand the change. |
} else { | ||
const wrappedCensor = typeof censor === 'function' ? (value, path) => { | ||
return censor(value, [k, ...path]) | ||
} : censor |
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.
Why is this needed? I would prefer if we avoid this.
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.
Answering for this and also quoted comment
Can you please this? I don't understand the change.
This PR davidmarkclements/fast-redact#16 added the functionality to pass a path
array to the censor
function.
We had to do this because otherwise the path
array is missing the top level key.
For instance, say we have a logger set up like so:
const pino = require('.')
const logger = pino({redact: {paths: ['a.b.c'], censor: console.log }})
logger.info({a: {b: {c: 'redact me'}}})
this will log out: redact me ['a', 'b', 'c']
without this change this will log out: redact me ['b', 'c']
This is because, fast-redact is being used per key on the logged object, so it is passed a redaction path of 'b.c'
and then works on the object {b: {c: 'redact me'}}
instead of 'a.b.c'
, working on the object {a: {b: {c: 'redact me'}}}
.
bc587c0
to
f498f68
Compare
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
I'm not clear. Is this changing the current user API for Pino? |
@jsumners - No breaking changes to the API but the |
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 - once @jsumners is happy with it we're good to merge
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
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
path
argument incensor
functionlib/redaction.js
so that thepath
argument contains top level key e.g. given a pathreq.headers.cookie
req
would not be part ofpath
without these changes e.g.['headers', 'cookie']
instead of['req', 'headers', 'cookie']
Benchmark for master
Benchmark for this branch