python-zk / kazoo

Kazoo is a high-level Python library that makes it easier to use Apache Zookeeper.
https://kazoo.readthedocs.io
Apache License 2.0
1.3k stars 386 forks source link

fix(log): add logger for kazoo.handlers.threading #725

Closed zqfan closed 4 months ago

zqfan commented 1 year ago

Fixes #724

No handlers could be found for logger "kazoo.handlers.threading"

for better logging control, just like kazoo.client does

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.65%. Comparing base (92bd0c2) to head (0302623). Report is 16 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #725 +/- ## ========================================== - Coverage 96.73% 96.65% -0.09% ========================================== Files 27 27 Lines 3556 3560 +4 ========================================== + Hits 3440 3441 +1 - Misses 116 119 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ceache commented 6 months ago

Hi @zqfan. Thank you for the PR but, I am sorry but I am not clear on the goals of this PR. What is gained by making the handler reuse the client's logger? Do you have a clear example?

zqfan commented 6 months ago

Hi @zqfan. Thank you for the PR but, I am sorry but I am not clear on the goals of this PR. What is gained by making the handler reuse the client's logger? Do you have a clear example?

https://github.com/python-zk/kazoo/blob/b00d88ff1485f1cb175565c49955bf5d7ce96fef/kazoo/handlers/threading.py#L121 This line needs log to be set, i.e., logging.basicConfig(), otherwise, it might print an error message on stderr: "No handlers could be found for logger "kazoo.handlers.threading"" . As issue https://github.com/python-zk/kazoo/issues/724 pointed out, it leads to some information unnoticed, and cause a lot of time to dig out.

But it might be better if wen can specify a logger handler, which allows us to print log info into a specific file. Upper level application can only deal with kazoo.client, and it is not a good idea to expose kazoo/handlers/threading.py detail to upper level just for a logger handler, so I let it inherit the logger setting in kazoo.client.

this pr keeps same behavior as before, if user don't specify a dedicated logger handler

StephenSorriaux commented 6 months ago

Thank you for the PR.

Just like @ceache said, I am also not sure about the goal of this PR.

I understand the issue about the No handlers could be found for {logger} (which is explained in the kazoo documentation) that, I think, can be solved using a default NullHandler() for our top logger (as per the official documentation).

However, I don't think passing a logger should be the way to go.

zqfan commented 5 months ago

Thank you for the PR.

Just like @ceache said, I am also not sure about the goal of this PR.

I understand the issue about the No handlers could be found for {logger} (which is explained in the kazoo documentation) that, I think, can be solved using a default NullHandler() for our top logger (as per the official documentation).

However, I don't think passing a logger should be the way to go.

My project doesn't configure a logging.basicConfig for everything, it adjust and control major module's behaviour separately. For now, to avoid too much message printed to my log file, I have to do something like this:

logger = logging.getLogger("kazoo.client")
logger.setLevel(logging.ERROR)  // for other module, it is INFO
for h in mymodule.logger.handlers:  // a different file for other module
    logger.addHandler(h)
self.client = KazooClient(hosts=ZK_HOSTS,logger=logger)

If kazoo.handlers.threading could not inherit logger settings from kazoo client, then I have to do it twice. I think end user should treat kazoo as a integrated module, settings should be done only once.

StephenSorriaux commented 5 months ago

If kazoo.handlers.threading could not inherit logger settings from kazoo client, then I have to do it twice. I think end user should treat kazoo as a integrated module, settings should be done only once.

That can already be done: kazoo.handlers.threading does not inherit from kazoo.client but both inherit from kazoo, ie.

import logging
logging.basicConfig(level=logging.INFO)
...
logger = logging.getLogger("kazoo")
logger.setLevel(logging.ERROR)

@ceache I think the possible use case for this, which is not explained here or in the initial issue, is the same as the reason why we have a logger configuration in the client (see https://github.com/python-zk/kazoo/pull/85): passing a logger around to possibly discriminate between multiple instances of kazoo running on the same host or whatever. I must say I am... intrigued: isn't that something the developer must take care during the development (via the formatter, etc.)? boto3 or requests, to take some "famous" libs, do not let developers pass around their logger and I can easily understand why since Python already provides everything for that... Anyway, I would love your thoughts on this.

jeffwidman commented 4 months ago

I understand what you're trying to achieve, but as @StephenSorriaux points out above, I think it's already solvable if you change the logger you're tuning to kazoo rather than kazoo.client.

May I recommend @brandon-rhodes excellent logging_tree library? It's been a massive time saver for me over the years when I'm debugging unfamiliar code.

Closing, but if for some reason you can't achieve this with existing tooling please comment and we can re-evaluate.