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

implement a --suppress-logger option to logging to suppress logging for one or multiple loggers #7873

Closed
wants to merge 5 commits into from

Conversation

symonk
Copy link
Member

@symonk symonk commented Oct 7, 2020

Hi everyone, I've been away for a while but I'm hoping to get some contributions going again. Not entirely sure where to go with this one, but I thought I'd make a start on it. I'm not entirely sure on the best way to test this, caplog came to mind as a useful tool.

This PR attempts to add a --suppress-logger=foo --suppress-logger=bar capabilities to the core logging plugin to allow the user to control what can sometimes be considered 'bloating' output from various loggers.

Thanks for your time as always and I look forward to any discussions / direction and feedback.

closes #7431

@symonk symonk requested a review from nicoddemus October 7, 2020 22:12
# disable propagation for the given logger preventing of event passing to ancestor handlers
# adding a NullHandler to prevent logging.LastResort handler from logging warnings to stderr
logger = logging.getLogger(name)
logger.addHandler(logging.NullHandler())
Copy link
Member Author

Choose a reason for hiding this comment

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

unsure about this

Copy link
Member

Choose a reason for hiding this comment

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

Not sure myself, but what happens to other handlers that are installed in the logger? I suppose they will still process messages?

Copy link
Member Author

@symonk symonk Oct 8, 2020

Choose a reason for hiding this comment

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

Interesting one, from a quick check it will still use such handlers, this is outlined below:

>>> import logging
>>> log = logging.getLogger('pytest')
>>> log.setLevel(logging.DEBUG)
>>> # add a stream handler to stderr by default
>>> log.addHandler(logging.StreamHandler())
>>> log.debug('stderr')
stderr
>>> log.addHandler(logging.NullHandler())
>>> log.debug('does this still write to stderr')
does this still write to stderr

two questions:

  • Should we iterate / remove the handlers on the given logger?
  • Should we check the hierarchy of loggers if the logger provided exists, else I assume its going to register a new one by badly typed --suppress-logger names into the hierarchy dict (maybe thats not a big deal)

Copy link
Member Author

@symonk symonk Oct 8, 2020

Choose a reason for hiding this comment

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

#1 check name is in: 
logs = [logging.getLogger(name).name for name in logging.root.manager.loggerDict] ?

#2 empty logger handlers for the given logger, unsure of the implications here
logger.handlers = [logging.NullHandler()]

Copy link
Contributor

Choose a reason for hiding this comment

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

What we do to suppress logs of certain loggers is to simply set the logLevel of those loggers to CRITICAL. Have you thought about doing that?

Copy link
Member Author

@symonk symonk Oct 8, 2020

Choose a reason for hiding this comment

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

No I didn’t think about that as I was thinking about fully suppressing any level, however doing something like that may be better suited as I’d assume even users who wanted this feature would/should actually care about such messages from those libraries etc. Feels like a nice middle ground, thoughts?

@nicoddemus
Copy link
Member

Hi everyone, I've been away for a while but I'm hoping to get some contributions going again.

Sure thing, welcome back @symonk!

@nicoddemus nicoddemus requested a review from twmr October 8, 2020 12:19
@nicoddemus
Copy link
Member

Deferring this review to our logging in-house expert @thisch. 😁

@symonk
Copy link
Member Author

symonk commented Oct 8, 2020

Hi everyone, I've been away for a while but I'm hoping to get some contributions going again.

Sure thing, welcome back @symonk!

Thanks! I hope everyone is doing well. Being so eager to get back, i accidentally did this on the master branch of my fork :D my apologies

@symonk symonk closed this Oct 24, 2020
@nicoddemus
Copy link
Member

Hey @symonk, can you leave a comment explaining why you decided to close this? It helps to have a record with the reasons that led to the decision.

@symonk
Copy link
Member Author

symonk commented Nov 17, 2020

My apologies @nicoddemus I closed this as I (plan) to tackle the issue through incorporating a configurable option as outlined here: #7417 which given such configuration at runtime could tackle this issue in itself, I was unsure how to proceed on this one standalone (given the other pending work). I can re-open it if you think there is value there :)

@nicoddemus
Copy link
Member

No worries, that's fine, thanks!

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.

RFE: allow to selectively disable loggers from command-line
3 participants