david-caro / python-foreman

Small low level python wrapper around Foreman API
GNU General Public License v2.0
57 stars 37 forks source link

Allow setting the loglevel for python-foreman's log messages #63

Closed pief closed 8 years ago

pief commented 8 years ago

Yes, sorry, that should have been on top of #64. Only then it makes sense and changes the module-level logger only.

We could also do something like:

def __init__(..., loglevel=None):
  if loglevel:
    logger.setLevel(loglevel)

Then the loglevel would only be set if explicitly requested and otherwise indeed inherit from the root logger.

david-caro commented 8 years ago

Still don't think it's a good idea to change it from the class, it's a module level change that will affect any instance, class or method in the module.

pief commented 8 years ago

Not sure I understand. By "module" we refer to "python-foreman", right? What other classes/methods in the module do you mean?

Also, what would be the alternative? Right now, if I tell my application using python-foreman to log messages at log level "debug", I get debug log messages from python-foreman as well and can't suppress them...

david-caro commented 8 years ago

Module in this case is client.py, the same module where the Foreman class is defined.

So what I propose is to create a function, say set_loglevel, in client.py that does the logger.setLevel() thingie. Then you just call it once from your program to set the log level.

If you do it in the init of the Foreman class, if you instantiate it once with level=info, and then instantiate it again with level=debug, it will change the loglevel for any existing instance, and not only that, it changes the level for any function or any other class from the client.py module (as it's changing logger, that is a singleton)

pief commented 8 years ago

Ah, thanks for the explanation. Good suggestion, I'll implement this next Monday.

david-caro commented 8 years ago

Sure, no hurry, glad to help :smile:

phirince commented 8 years ago

The best solution for this problem is to log to a separate logger. e.g: LOGGER_NAME = "foreman.client" logger = logging.getLogger(LOGGER_NAME) logger.warn("Blah Blah")

So, if I want to silence the logs from foreman.client in my script, I just need to do this:

foreman_logger = logging.getLogger("foreman.client") foreman_logger.setLevel("ERROR")

and I'm done. This will not affect any other logs other than foreman's. If I want to change the entire logging behavior, I can do this: logger = logging.getLogger() logger.setLevel("ERROR")

So, this is the cleanest and the most flexible solution.

If you want, I can submit a pull request with the changes.

-PP

david-caro commented 8 years ago

For some reason I though it was using a logger already. So aside from that, the idea of creating a function to set the level of that logger allows you to not need to know the logger name string, so I think it's still useful

pief commented 8 years ago

One problem I just noticed: with this new version there is no way to instantiate a foreman.client.Foreman object and call f.set_loglevel() without having the default loglevel already take effect as the connection is being established :/

pief commented 8 years ago

Here's an alternative, taking on @phirince's suggestion but with a defined interface to retrieve the module's logger name to address @david-caro's comment.

This would do the job for me, too.

david-caro commented 8 years ago

I was expecting something like:

 form foreman import client as foreman_cli
 foreman_cli.set_loglevel('info')
 my_client = foreman_cli.Foreman(foreman_url)

That should work as you expect right?

pief commented 8 years ago

Ping.

david-caro commented 8 years ago

Sorry for the delay, it's being a quite busy year