pinojs / pino-http

🌲 high-speed HTTP logger for Node.js
MIT License
539 stars 117 forks source link

How best to handle extending req.log? #311

Closed ramblingenzyme closed 10 months ago

ramblingenzyme commented 11 months ago

Context: We want to attach the pino-http middleware first so that req.log is always defined and can be used in all Express middleware / all route handlers. However, we also always want to include information about which tenant/customer a request is for, but our authorization middleware gets invoked later.

image

So we hack it by reassigning req.log. However, this middleware is defined and exported from an internal library.

This has caused some friction for the internal consumers of our library when they've setup AsyncLocalStorage to capture req.log, following this article. https://blog.logrocket.com/logging-with-pino-and-asynclocalstorage-in-node-js/

The issue was that this only captured the original req.log, and not the new child logger from our middleware, and required a less than ideal workaround to lazily dereference req.log.

image

We could extend our internal library to be aware of AsyncLocalStorage or move the AsyncLocalStorage logger into the library itself, but is there a better/sanctioned solution or something in the API I've missed that simplifies this?

mcollina commented 11 months ago

THere is no sanctioned solution for this, however it's something that would generically be useful

ramblingenzyme commented 11 months ago

@mcollina, thanks for the quick response. I understand there's no sanctioned solution for AsyncLocalStorage, but is there a better solution for middleware than reassigning req.log if all downstream middleware should have extra bindings?

I don't think I was very clear in asking that question, but I understand the answer is probably the same.

mcollina commented 11 months ago

The best way is to reassign req.log

ramblingenzyme commented 11 months ago

@mcollina would the team be open to merging something like this in (of course with documentation updates and tests) https://github.com/ramblingenzyme/pino-http/pull/1 to provide an official method which lets you extend the bindings and not reassign req.log/res.log?

mcollina commented 10 months ago

No, not really. Proxy is very slow.

ramblingenzyme commented 7 months ago

@mcollina, I've just noticed that Pino has a setBindings method, which is in the TS types but not in the API docs. Should I actually be using that something like logger.setBindings({ ...logger.bindings(), newBindingProp: "string" })