ovhemert / pino-applicationinsights

A transport for pino that sends messages to Azure Application Insights
MIT License
10 stars 12 forks source link

Swallowing all stdout/stderr terminal output #60

Open devinrhode2 opened 2 years ago

devinrhode2 commented 2 years ago

I've noticed that pino-applicationinsights swallows everything into it's writeStream.

If I try to simulate prod locally, and already have a server running on port 3000, server will start, and then immediately crash, next.js will normally print an error message that port 3000 is already in use, the application insights writeStream was not flushing that to stdout.

Maybe it's desirable from a performance perspective to simple capture all stdout/stderr and not let it print to the users terminal.

On the other hand, if server fails to start, or logs are not being sent correctly, then we should send all the logs we've collected to stdout/stderr.

It seems that pinoms is generally a legacy/compatibility package.

pino-applicationinsights Seems to be the weakest link. I think it was built with pino v6 in mind: CleanShot 2022-03-25 at 03 56 44@2x And the docs there led me to use pino-multi-stream in the first place: https://github.com/ovhemert/pino-applicationinsights/blob/master/docs/API.md

But it turns out, pinoms DOES support pino v7..

If I comment out my whole next-logger.config.js file, then the default logger will print logs to the terminal. So there's something wrong with my Azure ApplicationInsights logger.

This was the fix:

CleanShot 2022-03-28 at 15 44 04@2x

I actually stole the idea from https://github.com/pinojs/pino-elasticsearch docs

We could probably steal the same snippet for docs for this project

devinrhode2 commented 2 years ago

For extra context see: https://github.com/atkinchris/next-logger/issues/12

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.