Check for logging calls from root logger

[tmct]

Description

Hi - would you be interested in creating a check, perhaps optional, that steers users towards using namespaced loggers instead of the root logger please? I would like to encourage users in some libraries I work on to follow the __name__ pattern instead of making log calls whose origin is harder to tracke back to.

(Maybe this is not such a good idea - in which case I'd be very happy to hear your thoughts on why! Thanks)

Bad:

import logging

logging.info(...)

Good

import logging

logger = logging.getLogger(__name__)

logger.info(...)

[adamchainz]

Yeah, this is a good idea. I think we’d want to make this an opt-in rule. I think Flake8 can do that, but I couldn't find the docs after a quick look.

WOuld you be able to work on a PR?

[tmct]

Thanks - I think this could save people a lot of the time searching mysterious log lines from my company's internal libraries, if I were to opt-in to this in shared configs. I think such a check would work really nicely in conjunction with LOG002.

I'll try and find some time to work on a PR.

[Zac-HD]

This recently bit me at work - I'd strongly prefer it to be enabled by default, and suggest using logging.getLogger(name=None) if you do in fact want the root logger.

[tmct]

Or indeed logging.root :)

[jakkdl]

I think we’d want to make this an opt-in rule. I think Flake8 can do that, but I couldn't find the docs after a quick look.

The way to do this is with OptionManager.extend_default_ignore inside an add_options function
https://github.com/PyCQA/flake8/blob/5c52d752e64f210369b37654e7b26ce25e2266d6/src/flake8/options/manager.py#L292
example usage in flake8-bugbear:
https://github.com/PyCQA/flake8-bugbear/blob/1855fae573137def68ec1c751b726a0fbb1a1c90/bugbear.py#L147

I can whip up a PR, should be quite straightforward regardless of if it's optional or not.

[adamchainz]

Okay given the consensus I think we can make it a non-opt-in rule. Users can always opt out.

Thanks for the reference though @jakkdl . Would you like to make a PR for the new rule? Including docs and changelog note please. 🙏

Or indeed logging.root

This doesn't seem to be documented.j

[adamchainz]

Sorry just saw you made a PR.

[adamchainz]

Done in #96, released in 1.6.0. Thanks all!

[tmct]

Thanks very much all, this looks great - it catches the root logger uses on my example code!

I've realised that there is a less common form of the same problem, that I've occasionally seen, sorry for not remembering earlier - a user might also use the root logger by neglecting to specify a name, usually by accident:

import logging

logger = logging.getLogger()  # oops, I forgot __name__! It's going to be a while before I notice

logger.info("I think I'm doing good logging - but am I?")

I'd like to see this in too please - should I raise another issue (and/or PR) to discuss? (If we did cover it, could this be covered as another case of LOG015 or would it need to be a new code?)

Many thanks

[adamchainz]

That's a good idea. Let's make it a new error code with a more specific message and description. Just open a new PR, no need for issue.

[jakkdl]

Another option would be to add it to LOG002

[tmct]

Thanks. I only have time to write the tests right now, does this line up with what you would expect?

main...tmct:flake8-logging:main

In particular - the message for LOG016 feels like it should be exactly the same as for LOG015, are you sure we should not combine these?

[tmct]

Missed @jakkdl's comment sorry, let me take a look

[tmct]

Yes potentially this should fall under LOG002. My only concern is that sometimes users really should be accessing the root logger. Ideally to configure handlers rather than to log specific infos, errors. etc. So we have to be careful in what we disallow

[Zac-HD]

I think that disallowing the no-argument form makes sense; if the root logger is desired users can call logging.getLogger(None). I think putting this in LOG002 makes sense.

[adamchainz]

Let’s make a new rule, rather than merging in LOG002. That way there’s space for an explanation about whether to use None or __name__, and more fine-grained rule control.

[adamchainz]

Thanks. I only have time to write the tests right now, does this line up with what you would expect?

main...tmct:flake8-logging:main

The calls to info() etc. can be dropped - the error should be on the getLogger() line.

In particular - the message for LOG016 feels like it should be exactly the same as for LOG015, are you sure we should not combine these?

Let’s use a different message, like “avoid implicitly getting the root logger”.

[tmct]

I have a concern about the details what we are suggesting, let me try to lay it out:

I think that this should be allowed without lint errors in one's "main" method:

import logging

if __name__ == "__main__":
    root_logger = logging.getLogger()
    root_logger.addHandler(...)

so long as root_logger is not used for any "info", "warning" etc.

Do you not think?

[tmct]

Because of the above - I was wondering whether we should instead only complain about getLogger() if that logger is later used to log at all, rather than complaining on initialisation. I haven't managed to work out yet how to achieve that in ast visits, though it sounds similar to some of the other logic around setting logger names.

We could still allow getLogger(None) for its explicitness, but alternatively users wishing to log from root logger could just ignore LOG016 on that line.

[adamchainz]

Do you not think?

Maybe... I have never seen that pattern though. And most of the time the entry point has a better name, like Django uses the django logger. Anyway if we allow getLogger(None) then there’s an easy escape hatch.

Because of the above - I was wondering whether we should instead only complain about getLogger() if that logger is later used to log at all, rather than complaining on initialisation. I haven't managed to work out yet how to achieve that in ast visits, though it sounds similar to some of the other logic around setting logger names.

It’s possible but I don’t see the advantage. If the logger is being created, it could still be imported into another file. #15 is an issue for detecting unused loggers, which would cover that anyway.

[tmct]

Apologies Adam, missed your reply here!

I tend to work with Python users who do more simple console scripts than run frameworks and server, so if we need it we have to fetch the root logger without a nice helper like that django one. It's true that 99% of Python users should use logging.basicConfig(...), as is encouraged as idiomatic in the logging cookbook, and have no need to touch the root logger directly. But for when that function can't satisfy one's needs (e.g. I don't like this, but some people like to send warnings to stderr and info to stdout), they should just include some custom code that sets their handlers on the root logger.

But I think the escape hatch you suggest might be a good solution to allow a way to do this: I wasn't sure initially about getLogger(None), but actually maybe this is a nice simple way of achieving the objective of stopping accidental logging to the root logger. Would be very happy to see this in flake8-logging.

[tmct]

Also, I like the check in #15, we've all seen those accidental logger lines sat there doing nothing :)

[Zac-HD]

Let's reopen this issue, since it's still seeing open discussion? Or if @tmct wants to open a PR, or a new issue, that would work too 🙂

[adamchainz]

Sure, I’m waiting for a PR. I won’t have time to work on this.