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

Logger: Add color support for different log levels #4931

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

Fijxu
Copy link
Contributor

@Fijxu Fijxu commented Sep 20, 2024

Looks like this:

image

As I said in the code, this is useful when debugging things in Invidious. Having to find the type of error just by the name of the log level is not comfortable in my opinion.

Feel free to edit anything.

Copy link
Member

@syeopite syeopite left a comment

Choose a reason for hiding this comment

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

LGTM. I'll await some reviews from everyone else to see if the colors should be changed before merging.

src/invidious/helpers/logger.cr Outdated Show resolved Hide resolved
config/config.example.yml Show resolved Hide resolved
@syeopite syeopite added the need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something label Sep 20, 2024
@Fijxu
Copy link
Contributor Author

Fijxu commented Sep 20, 2024

Done, I also added information about the NO_COLOR env variable.
Color output also works on docker logs which is pretty neat.

image

@unixfox
Copy link
Member

unixfox commented Sep 22, 2024

I don't think it's a good idea to have this as enabled by default.

I can see the color messing up with some existing log parsing system or something like that.

@Fijxu
Copy link
Contributor Author

Fijxu commented Sep 27, 2024

I can see the color messing up with some existing log parsing system or something like that.

Yeah you are right. This could break some automation system that does not support ANSI escape codes. It's better to leave it off by default and it would be great to point it on the wiki that this could be turned on (probably because most people would like to leave this setting on) if you want.

src/invidious/helpers/logger.cr Outdated Show resolved Hide resolved
src/invidious/helpers/logger.cr Outdated Show resolved Hide resolved
@SamantazFox SamantazFox added in-testing This feature has been deployed and is being tested and removed need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Oct 7, 2024
@SamantazFox SamantazFox added ready and removed in-testing This feature has been deployed and is being tested labels Oct 31, 2024
@SamantazFox SamantazFox merged commit a760b69 into iv-org:master Nov 8, 2024
8 checks passed
@SamantazFox
Copy link
Member

I forgot to say thanks for your contribution :)

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

Successfully merging this pull request may close these issues.

4 participants