search

iamolegga / nestjs-pino

Platform agnostic logger for NestJS based on Pino with REQUEST CONTEXT IN EVERY LOG
MIT License
1.23k stars 98 forks source link

[QUESTION] Performance and async_hooks #322

Closed tfalvo closed 4 years ago

tfalvo commented 4 years ago

Thank you guys for your good job with this library. Pino is a great logger, and nestjs-pino provides a very nice integration for NestJS.

However, I'm interested to know your point of view about performance question we can read about async_hooks which is still in experimental status, and with the main reason of performance impact. In addition to this (or linked with this), async_hooks is not recommended for production deployment.

As you mentioned in nestjs-pino docs, REQUEST scope can have performance issues. It's true and I fully agree. But async_hooks have also some performance impact too.

It will be very friendly if you could share your opinion on this topic. Thank you very much.

iamolegga commented 4 years ago

Hi, I actually don't know what to say. As a short answer I personally didn't reach some limitations, but my projects aren't highload. So if you have some you are free to use something else or make some improvements to this lib 🙂 If you want to see some benchmarks and compares of different implementation (async hooks vs request scope), sorry, but I don't have enough time for them, but they are more then welcome. Main goal of this lib is possibility to track logs from different contexts, and it's obvious it has some overhead, as any other functionality. When creating this lib I've chose async hooks instead of request scope because from my point of view unexpected creation of services depended on logger on every request is a wrong behaviour and could lead to bugs in some situations when you have some side effects on creation.

About not recommended for production deployment: it depends on project's limitation and requirements. This lib is based on cls-hooked which is quite popular, and from that fact I can say that it's performance (and async hooks's one) is satisfying a lot of projects including mine. Moreover when nodejs#12 will be unsupported I'm planning to replace it with AsyncLocalStorage.

i hope this info is helpful to you 🙂

tfalvo commented 4 years ago

👍 Thank you for your answer @iamolegga. I really appreciate. I don't have any experience yet with async_hooks, it's why I was a little worried about performance after some readings. Maybe, I should take in account this performance topic, but without overestimating it.

Thanks again!