Skip to content

Avoid using global logger #15

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

Merged
merged 2 commits into from
May 2, 2025
Merged

Avoid using global logger #15

merged 2 commits into from
May 2, 2025

Conversation

catap
Copy link
Contributor

@catap catap commented Apr 30, 2025

Somehow it fixes mitmproxy/mitmproxy#7680

@notypecheck
Copy link
Owner

Thanks a lot, I will merge and make a release in a couple of days.

Copy link

@mhils mhils left a comment

Choose a reason for hiding this comment

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

FWIW a very common pattern here is to have

logger = logging.getLogger(__name__)

at the beginning of the file and then use logger in the rest of the file. :)

@catap
Copy link
Contributor Author

catap commented Apr 30, 2025

The only things I don't understand, yet is why this line:

    # register handler
    _handlers[name] = handler
    logging.debug("registered %r handler: %r", name, handler)

brokes mitmproxy, and rewrite it into:

    logging.getLogger(__name__).debug("registered %r handler: %r", name, handler)

or

    logging.getLogger().debug("registered %r handler: %r", name, handler)

doesn't broke it.

@catap
Copy link
Contributor Author

catap commented Apr 30, 2025

Aha!

The call logging.debug (or similar one) make implicit configuration of the logger, as documented here: https://docs.python.org/3/library/logging.html#logging.debug and it dismiss the second configuration from user application.

@catap
Copy link
Contributor Author

catap commented Apr 30, 2025

FWIW a very common pattern here is to have

logger = logging.getLogger(__name__)

at the beginning of the file and then use logger in the rest of the file. :)

The commit in question had removed it :)

650121d#diff-fb2e5a842fd78374e92b5bd2034fcfd0a8a1b25488f23c906abcd4326d2ae410

@mhils
Copy link

mhils commented Apr 30, 2025

https://docs.python.org/3/library/logging.html and https://docs.python.org/3/howto/logging.html both recommend logging.getLogger(__name__), so I think it'd worth to reconsider that approach when using the logging module. :)

@notypecheck
Copy link
Owner

I wasn't aware that logging.debug could cause side effects 👀, I'll just introduce a global logger how other libraries usually do that.

@catap
Copy link
Contributor Author

catap commented May 1, 2025

@ThirVondukr now it should pass lint

@notypecheck notypecheck merged commit b619e82 into notypecheck:main May 2, 2025
9 checks passed
@catap catap deleted the patch-1 branch May 2, 2025 14:32
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.

Logging brokes mitmproxy as CLI
3 participants