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

🔧 Tweak/adjust the logging verbosity greater-eq to warning level #147

Merged
merged 4 commits into from Dec 3, 2021

Conversation

Ousret
Copy link
Owner

@Ousret Ousret commented Dec 3, 2021

I understand that the latest release unexpectedly generated some noise for some people in specific environments.

The engagement I made with charset-normalizer given its wide deployments* still applies. Therefore, regarding :

With this PR I adjust the impact to a minimal impact while keeping backward compatibility.
Fixes/Adress #145

*: Listening as broadly as possible regarding any side-effects to the community

@Ousret Ousret added enhancement New feature or request bugfix release Minor release for bug fixes flourish Not really needed but nice to have! labels Dec 3, 2021
@Ousret
Copy link
Owner Author

Ousret commented Dec 3, 2021

Note for those who will stumble upon this PR.

You may find your recent discovery unexpected and/or unpleasant but you have to understand that in any context,
using requests or any popular HTTP client that if your server serves any resource without explicit/valid charset it will try to guess it using this library the best we can.

Actually, your discoveries demonstrate that we clearly cannot drop that kind of dependency in a short term.

@Ousret Ousret merged commit d28f8ff into master Dec 3, 2021
@Ousret Ousret deleted the revise-logging-impacts branch December 3, 2021 18:53
@Ousret Ousret mentioned this pull request Dec 3, 2021
@0xallie
Copy link

0xallie commented Dec 17, 2021

That's better, but IMO it should be all DEBUG level. Is there any use case for these log messages other than debugging (in which case DEBUG is perfect)? In normal operation charset_normalizer should generally just stay out of the way and return the the results to the code.

Example of a specific use case: In my program the default log level is INFO, so I set logging.getLogger("charset_normalizer").setLevel(logging.WARNING) to hide the charset_normalizer log messages. But I have a --debug option to enable all log messages, and I also use coloredlogs to give debug messages a less distracting gray color. Since I also leave charset_normalizer logs enabled in this mode, seeing INFO log messages when it really should be DEBUG is slightly distracting and makes me think those messages are more important. I would rather not fork charset_normalizer just to patch this, so if you disagree I'll probably just silence its log messages even when my program is in debug mode.

@Ousret
Copy link
Owner Author

Ousret commented Dec 17, 2021

I will not revise the logging level to debug for everything. Historically and still used for readability purposes. I agreed with lowering WARNs. Until the next major no modifications are planned.

I would rather not fork charset_normalizer just to patch this, so if you disagree I'll probably just silence its log messages even when my program is in debug mode.

You already have a good workaround solution.

@0xallie
Copy link

0xallie commented Dec 17, 2021

Yes, but that means if I want to debug an issue with charset_normalizer then I'll have to go into my code and re-enable logging for it because I'll have it disabled even in debug mode. Oh well.

I don't quite understand what you mean with this sentence: "Historically and still used for readability purposes". I can only see one DEBUG level message emitted by charset_normalizer right now and it's quite short too, so I doubt anyone would complain that they're going to be forced to see 1 short message in addition to the current 4 longer ones.

So far I haven't heard a convincing argument for why it needs to be separated this way, especially with there being more INFO messages than DEBUG, when it's generally the other way around. But of course you don't owe me anything and you're free to disagree, I would just prefer if there was a logical explanation for it.

@Ousret
Copy link
Owner Author

Ousret commented Dec 17, 2021

Yes, but that means if I want to debug an issue with charset_normalizer then I'll have to go into my code and re-enable logging for it because I'll have it disabled even in debug mode.

I understand but I am not willing to make changes right now. You will have to compromise.

I don't quite understand what you mean with this sentence: "Historically and still used for readability purposes".

When one spent as many hours as I spent on this package, I just need the logging to be delivered as is to ease my job as a solo maintainer.

Your issue is marginal and I can't say yes to everyone and dismiss my own sanity (among others) in the process.
What I don't understand is the need for a few to globally enable logging across all packages and expect maintainers to act toward the need of a few. It's the job of the developer to carefully choose what to debug/listen to.

@0xallie
Copy link

0xallie commented Dec 17, 2021

I'm sorry if I came across as demanding, I simply suggested it because to me it seemed like a logical thing and not just a personal preference.

You don't have to, but it would certainly help me understand more if you explained why the current setup benefits your workflow. As an outsider it doesn't seem logical to me but I could very well be missing something, but unfortunately I can't read your mind.

Sure, you could argue that I shouldn't be enabling logs from all libraries, but like I said I only enable them in debug mode, where I specifically want to receive debuh logs from libraries as well (especially urllib3). That doesn't mean I want them all lumped together as an unreadable mess in a single color though. This is definitely not the only library I've encountered that uses INFO for messages I consider useless at that level, but it's in the minority.

Anyway, I've found a better solution to my problem. I monkey-patched logging.Logger to force the level to DEBUG when the logger name is charset_normalizer, so I guess you can consider my problem solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix release Minor release for bug fixes enhancement New feature or request flourish Not really needed but nice to have!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants