Skip to content
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

feature: quietReqLogger option for a quieter child logger available on the request object #156

Merged
merged 2 commits into from
Sep 21, 2021

Conversation

darrenmce
Copy link
Contributor

@darrenmce darrenmce commented Sep 16, 2021

Hi, I've recently started using this for our logging as we migrated from bunyan to pino.

A behaviour we used to have in the bunyan-land was that our request logging middleware would similarly provide a request-context aware child logger (available at req.log) however it would only contain the request id that you could use to filter logs on and see logs for the entire request transaction (including the one message with the full request serialized).

Instead of just requesting this feature, I took a stab at a PR instead - please let me know if this is something you'd consider.

Some things that I'm on the fence/unsure about:

  1. the name of the option quietReqLogger 🤷‍♂️
  2. the top-level request id being only populated when the quietReqLogger is enabled with no granular control
  3. res.log and req.log are diverging with this option enabled, is there any downstream issues with this?
  4. typescript definitions - not sure who manages these

  - when enabled, will set request ids on log message with new overridable property 'reqId' (customAttributeKeys.reqId)
  - when enabled, will not bind request/customBindings on the child logger attached to the request object
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants