-
Notifications
You must be signed in to change notification settings - Fork 116
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add option quietResLogger
to supress response data from res.log entries
#256
base: master
Are you sure you want to change the base?
Conversation
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.
can you add the docs to the README?
Done @mcollina This is marked as a draft because I'm not confident on the changes I've made yet since the tests aren't running :/ Actually, I believe that there is something wrong here. I'm still trying to implement this feature. |
ae7df9e
to
58979fa
Compare
58979fa
to
96bd72a
Compare
What errors are you getting? |
let fullReqLogger = reqLog.child({ [reqKey]: req }) | ||
|
||
const resLog = quietResLogger ? logger.child({ [requestIdKey]: req.id }) : logger | ||
let fullResLogger = resLog.child({ [resKey]: res }) |
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.
What errors are you getting?
@mcollina a bunch of tests stop passing after adding this line
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.
The tests should be updated
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.
could we not do this?
const log = quietReqLogger || quietResLogger ? logger.child({ [requestIdKey]: req.id }) : logger
and leave the rest of the code as is?
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.
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.
I actually really want this feature, I'll pull tomorrow and use some work time on it
actually the misunderstood the ticket, I wanted a way to disable the request complete log. have a PR here #259 |
closes #235
I didn't succeeded on making the
npm t
running with no errors, even in the fresh new cloned repo 馃