Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added 'LOGGER_TRANSPORT' env key #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

finkinfridom
Copy link

@finkinfridom finkinfridom commented Dec 20, 2023

PR for issue #28
Added a new LOGGER_TRANSPORT env variable to easily setup pino loggerTransport property

fyi: it seems the tests are failing (and it happens into master too)

...(process.env.LOGGER_TRANSPORT
? {
transport: {
target: process.env.LOGGER_TRANSPORT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to have a default transport to pino-pretty if LOGGER_TRANSPORT is not defined

Copy link
Author

@finkinfridom finkinfridom Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not wrong, by default Pino uses stdout as the default transport (as per right now).
We are using a different transport in our source code because we are ingesting logs through OpenTelemetry (which could be considered a standard) as it prefers JSON format (similarly to datadog and other products).
I could change the code tomorrow to provide JSON as the default if you want.

Pino by default uses JSON as standard transport (if nothing is specified as in the changes I made).
In your current source code, you're forcing pino-pretty which is cool for console output but not useful when ingesting data through DataDog (or similar products).
I think providing a fallback value would make it impossible to "reset" the behavior to the default one.

As a mid-term improvements, there could be the possibility to specify loggerOptions (similarly to how payload does) on an init level

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants