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

Add a colorizeObjects option to disable object colorization #403

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

Avaq
Copy link
Contributor

@Avaq Avaq commented Feb 20, 2023

This allows log parsers that can detect JSON at the end of a log line to do so without being thrown off by ANSI colors.

Specifically, we're using Pino + Pino Pretty with Papertrail. Papertrail has this feature where they parse JSON at the end of a line. But their JSON parser fails to do so when that JSON is surrounded by ANSI codes.

I could disable colorization altogether, but Papertrail does nicely present ANSI colored logs in their web-UI, so I don't want to do that.

I've sent the Papertrail devs a message, because I think it'd be nicest if it were solved on their side, but as a plan B, and for the time being, this is the next best solution.

@jsumners
Copy link
Member

I'm confused. Why are you sending the output of pino-pretty to a log collector?

@coveralls
Copy link

coveralls commented Feb 20, 2023

Pull Request Test Coverage Report for Build 4262969590

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 4204181376: 0.0%
Covered Lines: 409
Relevant Lines: 409

💛 - Coveralls

@Avaq
Copy link
Contributor Author

Avaq commented Feb 20, 2023

@jsumners I wish Papertrail would work just as well on JSON logs as it does on formatted logs. But it works much better with formatted logs. As I mentioned, it displays colorized logs output in a web UI. It doesn't have any way to present JSON logs nicely, so you have to send it formatted log output for it to be usable. 😕

I found the { singleLine: true, colorizeObjects: false }-setup to be the perfect middle ground where we're utilizing all of Papertrail's features maximally.

@Avaq
Copy link
Contributor Author

Avaq commented Feb 20, 2023

We don't need to merge this. I can just use the fork for now and see what comes out of the Papertrail support ticket. Also, I understand that this change doesn't really align with the JSON logger philosophy: Disabling the colors on objects is really just muddying the separation of data and presentation (we're using some part of the presentation as data).

@Avaq
Copy link
Contributor Author

Avaq commented Feb 20, 2023

For reference, here's what the 3 situations look like in Papertrail:

  • Colors disabled - JSON gets parsed, but no colors
    latest-screenshot
  • Colors enabled - JSON not parsed, but colors
    latest-screenshot
  • Colors enabled, but disabled for objects - JSON gets parsed, and colors
    latest-screenshot

And when sending it unformatted JSON logs, it'll just show the whole JSON object printed as shown in the screenshots, without any way for the user to control how it's formatted.

I don't know if this issue is worth solving here. I just opened the PR because I had the code ready, and it might be useful to some folks.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

(Note that you shouldn't really do what you are doing for the optimal production setup)

@mcollina
Copy link
Member

@jsumners PTAL

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Full documentation is missing. I would imagine it should go between the current lines 59 and 60 of the readme.

This allows log parsers that can detect JSON at the end of a log line to
do so without being thrown off by ANSI colors.
@Avaq
Copy link
Contributor Author

Avaq commented Feb 24, 2023

@jsumners I pushed a version with updated docs. Sorry I missed this earlier.

@Avaq
Copy link
Contributor Author

Avaq commented Feb 24, 2023

I also heard back from Papertrail. They say it'll take a lot to fix on their end, and they proposed that a change to pino-pretty might be the easiest.

@jsumners
Copy link
Member

I also heard back from Papertrail. They say it'll take a lot to fix on their end, and they proposed that a change to pino-pretty might be the easiest.

Of course they will put the onus on the open source community. They have zero incentive to improve their product.

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.

4 participants