pinojs / pino-http

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

quietResLogger clarifications #350

Open whitlaaa opened 2 months ago

whitlaaa commented 2 months ago

Big fan of pino and was looking at leveraging pino-http to replace some outdated custom setup in our apps. I was looking at the recently added quietResLogger option and was hoping you could clarify its behavior (and perhaps its intended usage based on #235/#339?).

According to the README, setting quietResLogger to true will include the request id under reqId and "only contain req" (I assume that last bit is just a typo that is supposed to say "only contain res").

However, reqId is only included under res.log if quietReqLogger is also set. Is the intended usage to set both options as the unit tests seem to indicate?

Reading #235, I assumed the use case was either:

function handle (req, res) {
  logger(req, res)
  req.log.info('') // just log the request info
  res.log.info('doing some work') // quiet logs with only reqId
  ...
  res.log.inf('finished doing some work')
  res.end('hello world')
} 

However, I don't think either of the above approaches works if quietReqLogger is set since it suppresses req from both customReceivedMessage and req.log. Obviously I could log that req info in other ways even in quiet mode, but pino's consistent format is preferred.

Thanks and hopefully this made sense. Up way later than I should have been while reading docs and writing this. 😆

mcollina commented 1 month ago

You likely know more about that option that me know. I really regret merging that PR at this point :).

How would you recommend fixing this?

randompixel commented 2 weeks ago

I've also been very confused by these flags. Running Express, the default INFO (12345): [HTTP] request completed message doesn't behave how I'd expect?

From my understanding of the README.md, it sounds like if you set quietReqLogger it replaces the req object with reqId. If you set quietResLogger it removes the res object but leaves the req settings in place?

I'd expect the two flags to work independently of each other, such as: both false quietReqLogger:true / quietResLogger:false quietReqLogger:false / quietResLogger:true both true
Include reqId
Trims Request
Trims Response
--- --- --- --- ---
Expected Full reqId + res Only req Only reqId
But from a very basic app with no other configuration or logging modifications, as far as I can see the current behaviour works like this? both false quietReqLogger:true / quietResLogger:false quietReqLogger:false / quietResLogger:true both true
Actual Full reqId + res + req res reqId + res

So

This doesn't make any sense!

jsumners commented 1 week ago

You likely know more about that option that me know. I really regret merging that PR at this point :).

How would you recommend fixing this?

Let's rip it out and release a new major.