Provide ability to config pino-pretty

j-dav commented 1 year ago

What problem does this feature solve?

Slack thread

Currently the logger package doesn't allow passing of custom pino-pretty CLI commands i.e colorize, singleLine. It can get rather frustrating to read on multi line and grepping is made difficult by the multiline printout. Being able to grep and search for certain things in the terminal is quite important for finding relevant logs in development process.
It will solve the above point by allowing users to configure their pino-pretty with options outlined by pino-pretty docs.

Ideal solution

Provide ability to pass in a pino-prettyrc configuration file and/or ability to add it to the command line args like "nodemon web-server.js ./node_modules/.bin/pino-pretty --singleLine"

Requires change to the logger package

rowanmanning commented 1 year ago

Hiya 👋 I've had a look at this and it's more complex than it first seems. If you're using the JavaScript API of pino-pretty (which we are) then the logic to include the config file (here and here) isn't used. The options I've considered are:

1. Replicating the pino-pretty CLI logic

We could replicate the pino-pretty CLI logic. This would mean that any .pino-prettyrc file in your project will get loaded and used as a config just like if you were using the pino-pretty CLI manually. There are a couple of reasons I'm not happy with this approach:

We replicate a lot of logic from pino-pretty which is more code we have to maintain
Config file loading isn't something that should be a concern of the logger IMO
It ties us closer to Pino as a logging solution if we use the pino-prettyrc file names and makes it far harder in future to migrate away if we decide to

🙅 I'm pretty sure I don't want to do this.

2. Allowing configuring pino-pretty when creating a logger

We could allow passing pino-pretty options into the logger when it's constructed, e.g.

const { Logger } = require('@dotcom-reliability-kit/logger');
const logger = new Logger({
    prettyOptions: {
        singleLine: true
    }
});

This still ties us to pino-pretty more tightly but it's less of an issue code/maintenance-wise - it'd be a relatively small change. however there's an issue: you'd always have to construct a logger for your application rather than being able to rely on the Reliability Kit default logger:

const { Logger } = require('@dotcom-reliability-kit/logger');
const logger = new Logger({ /* ... */ });

// rather than

const logger = require('@dotcom-reliability-kit/logger');

This makes every app that wants to configure pino-pretty a little more complicated. It also won't work if, for example, the Reliability Kit logger is installed as a transitive dependency and constructed separately within another dependency. E.g. if n-express performs some logging it might not also pass in the singleLine config.

Another issue is that some of the Pino Pretty options will genuinely break local logging, e.g. overriding messageKey could result in the log message not being displayed.

💁 I could be persuaded to do this if you can find neat solutions to the issues above.

3. Allow setting only specific options (with environment variables)

This is similar to option 2, except we'd add only the pino-pretty options that we actually need (e.g. for now probably just singleLine and colorize). These would be set in the same way as option #2 except they'd be limited to the options we decide to support, like:

const { Logger } = require('@dotcom-reliability-kit/logger');
const logger = new Logger({
    prettyOptions: {
        singleLine: true,  // works
        messageKey: 'lol'  // does not work
    }
});

This resolves the second issue in option 2 by preventing important options from being overridden. We can also solve the first issue in option 2 by allowing these options to be set with an environment variable in a similar way to the log level, e.g:

LOG_PRETTIFIER_SINGLE_LINE=true node my-app.js

const logger = require('@dotcom-reliability-kit/logger'); // already configured based on the env var

This still introduces some additional complexity to the logger.

💁 I could be persuaded to solve the issue this way if my final option really doesn't work for you.

4. Allow disabling the prettifier and solving the issue in-app

pino-pretty does have a command line tool which supports all of the configurations that people might need. Our current implementation stops it being possible to use this way (because we check for an install of pino-pretty and hijack the logs in JavaScript land to prettify them). This was preferable in Customer Products where we just want one way of doing things that's consistent across our apps. Otherwise we'd have to ensure that every app adds the following to their start command:

node my-app.js | pino-pretty

We could add an environment variable to disable our internal prettifier so that other teams can use pino-pretty the traditional way as above. This allows you to set any options you want via CLI flags or a config file:

LOG_DISABLE_PRETTIFIER=true node my-app.js | pino-pretty

The small issue with this is that any team using this approach will need to set the messageKey option to message manually themselves if they want the logs to be prettified correctly.

👍 this is my preferred solution at the moment.

I'm interested in other people's thoughts about this. We don't need this right now in Customer Products so I'd like thoughts from people in other groups on the solution and thoughts from CP engineers on any maintenance issues they foresee with the potential solutions.

I'm also interested if you have further possible ways to solve this!

GeoffThorpeFT commented 1 year ago

As I'm reaching the end of my n-logger migration for my Heroku apps I would prefer not to have to change the code again. What I particularly like about the current approach is its almost a search/replace followed by pino-pretty as a devDependency 😄 . I certainly would not want to have to create a logger instance each time. Any more for this PR would not be nice: https://github.com/Financial-Times/biz-ops-admin/pull/1153/files 😢

Is there an option that would allow the default logger that is returned by the simple import/require to automatically apply the values proposed by your environmental variable solution?

rowanmanning commented 1 year ago

Hey Geoff :) nice work on the migration so far! To make it clear, none of the solutions I proposed will require further code changes if you don't need the singleLine logger configuration, and we won't be releasing a breaking change any time soon. It's more an opt-in feature.

The environment variable options would allow the default logger to pick up the values 👍 so options 3 and 4 would not require you to instantiate a new logger if you wanted to set the singleLine pino-pretty option

j-dav commented 1 year ago

The environment variable options would allow the default logger to pick up the values 👍 so options 3 and 4 would not require you to instantiate a new logger if you wanted to set the singleLine pino-pretty option

Thanks for looking into this Rowan, I like ENV implementation. Being able to add CLI args via option 4 by disabling and just using your local globally installed pino-pretty package seems like the most straightforward option and gives the user freedom to do what suits.

rowanmanning commented 1 year ago

@j-dav the prettifier is now able to be disabled as discussed. See the documentation here on how to do this. This should allow you to configure pino-pretty however you like by disabling it and directly using the CLI.

Financial-Times / dotcom-reliability-kit