-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Colorize terminal logs based on log level #428
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
Conversation
Signed-off-by: David Hettinger <[email protected]>
Signed-off-by: David Hettinger <[email protected]>
|
Can we work on the colors, please? They're not contrasty enough. |
| ERROR: '\u001b[31m', | ||
| FATAL: '\u001b[31;1m', // bright red | ||
| TRACE: '\u001b[35m', // magenta | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the colors used, or are we loading them from elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the ANSI colors supported by the terminal. If we want to change the color to be rendered at the end, we may want to change them in the theme of the terminal. I think it could be part of another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 let's create a separate issue for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sure, let's work on it in another ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@feloy I want to file the issue, but I am not sure which repo it should land in. Here or in PD, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thanks @adameska for your contribution and the suggestion. I really do like the idea and think it'll help the experience. |
Signed-off-by: David Hettinger <[email protected]>
Signed-off-by: David Hettinger <[email protected]>
|
|
Thank you! I saw something about prettier and was trying to run that to fix it but that wasn't taking... |
Signed-off-by: David Hettinger <[email protected]>
This file contains a class for colorizing JSON strings with customizable ANSI color schemes for different JSON elements. Signed-off-by: David Hettinger <[email protected]>
Add unit tests for JsonColorizer to validate colorization of various JSON structures, including braces, brackets, numbers, booleans, null values, and strings with custom color schemes. Signed-off-by: David Hettinger <[email protected]>
Updated log level color mappings and enhanced the log level detection regex to support additional formats. Signed-off-by: David Hettinger <[email protected]>
The feature is interesting, but I'm afraid it will be very cpu-consuming if it enabled for all containers. I think it would be interesting for the user to select which format to use for each container. @slemeur WDYT? |
I didn't consider the cpu much. I feel like it should be ok but I could just parse the first 10 rows and if 9 of them have recognizable json then enable it if it seems fast enough when I test it with a 10k long file. I don't like the idea of having users enter their logging format since a lot of our applications use different ones. |
|
hello, maybe bring an option to enable this feature. So it's an option people can turn on or off. The most difficult part would be to choose the default behavior. |
|
In this PR, I would suggest to include the code for the non-JSON colorization only, and keep the JSON-related code for another PR, when it can be activated. You may have an option at the extension level, so the user could choose between text and JSON colorization, for all containers. It is not very realistic, as all containers are not always using the same log format, but that would be a first step to be able to test the JSON one. |
I'm going to push hard against users picking one or the other. It's easy to detect if a file is logged in json/structure logging format so we can detect and process if that's the case. IMO the only thing the user should be able to pick is a color theme potentially and colorizing logs at all on/off. I will put the Json coloring in a separate PR but that will be dependent on this PR |
Removed tests for JSON colorization function. Signed-off-by: David Hettinger <[email protected]>
Removed unused JSON colorizer and related color scheme. Signed-off-by: David Hettinger <[email protected]>
Signed-off-by: David Hettinger <[email protected]>
|
Sorry, there was a problem on the pnpm-lock file. The problem should be fixed on main, You'll need to rebase so I can run the ci again |
feloy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Adameska <[email protected]>
feloy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Great work, thanks!







This makes it easier to spot warnings/errors and identify new log lines