search

pinojs / pino

🌲 super fast, all natural json logger
http://getpino.io
MIT License
14.21k stars 875 forks source link

`.flush()` doesn't work when pretty-print is enabled #1015

Open roim opened 3 years ago

roim commented 3 years ago

TSIA

https://github.com/roim/pino-pretty.async-flush

roim commented 3 years ago

Funny enough, when you try to use final you get:

pino.final with prettyPrint does not support flushing

The limitation isn't just for final, yet regular loggers don't have a warning.

mcollina commented 3 years ago

Unfortunately so. We do not think it is needed either as prettyPrint is just something for development - so we did not implement it. If you would like to work on it, I'll be happy to review.

Note that https://github.com/pinojs/pino/pull/1003 will make all sort of things possible.

roim commented 3 years ago

I agree it's not needed (can't think of a scenario where you'd need pretty print and async together). This is more of an usability issue--eg it took me maybe 30 min to realize what was wrong in my pino integration.

Do you guys have a stance on how to mitigate these usability pain points? I'd be happy to either log a warning on .flush calls with async mode enabled (assume the perf hit there is negligible since .flush is already intensive), or to add some doc hints, etc

mcollina commented 3 years ago

I'm ok with both a doc approach and a noisy one. @jsumners?

jsumners commented 3 years ago

I am okay with both.

Also, I do not know what "TSIA" means.

micalevisk commented 3 years ago

I do not know what "TSIA" means.

Title Says It All :smile:

satya-nutella commented 3 years ago

@mcollina @jsumners I'd love to work on this. This is going to be my first contribution to pinojs. Any pointers on what would be a good start?

mcollina commented 3 years ago

Update the doc of pino.final() telling that prettyPrint would not work. Also add it to the docs of prettyPrint.

davorpa commented 3 years ago

Hey @mcollina @roim 👋

I'm trying to do my first contribution with this. 🤗 After reading how final and asynchronous logging works, I think showcase could be already covered.

📑 pino.final() notes: https://github.com/pinojs/pino/blob/01a8633d7ff1b59e9fa47cc47e34421053fea10e/docs/api.md#L1016-L1027

📑 Asynchronous logging notes: https://github.com/pinojs/pino/blob/01a8633d7ff1b59e9fa47cc47e34421053fea10e/docs/asynchronous.md#L74-L108

📑 pino({options.prettyPrint}) notes: https://github.com/pinojs/pino/blob/01a8633d7ff1b59e9fa47cc47e34421053fea10e/docs/api.md#L345-L363 📑 options.suppressFlushSyncWarning notes: https://github.com/pinojs/pino/blob/01a8633d7ff1b59e9fa47cc47e34421053fea10e/docs/pretty.md#L85-L100 ↘ suppressFlushSyncWarning = true: https://github.com/pinojs/pino/blob/01a8633d7ff1b59e9fa47cc47e34421053fea10e/lib/tools.js#L234-L247

Let me know if I'm wrong 😉.

Last note about suppressFlushSyncWarning seems to be the noisy approach 🤔. It's warned only on first flush. Then, how to proceed? 💡Maybe mention in final() and pretty printers to know how to disable it?

BR

mcollina commented 3 years ago

Doc would be good, yes! Would you like to send a PR?

davorpa commented 3 years ago

I'll try @mcollina.

Let me see how I can write according to the rest of the documentation