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

Improve Logging #45

Open
tomschr opened this issue Jul 21, 2018 · 9 comments
Open

Improve Logging #45

tomschr opened this issue Jul 21, 2018 · 9 comments
Labels
enhancement New feature or request
Milestone

Comments

@tomschr
Copy link
Contributor

tomschr commented Jul 21, 2018

Problem

In most modules there is this line:

import logging
# ...
LOG = logging.getLogger(__name__)

However, we have already a logging module: rng2doc/log.py.

Solution

Use the "right" logging module. 😉

@tomschr tomschr added the enhancement New feature or request label Jul 21, 2018
@tomschr tomschr added this to the 0.2.4 milestone Jul 21, 2018
@jloehel
Copy link
Collaborator

jloehel commented Jul 21, 2021

Hi Tom,

can you please explain this. Usually I use module-level logger like it is described here: https://docs.python.org/3/howto/logging.html#advanced-logging-tutorial. What logger should I use instead? Should I use a single package logger?

@tomschr
Copy link
Contributor Author

tomschr commented Jul 21, 2021

hi Jürgen, 👋

well, I guess you can initialize the logger instance in rng2doc/log.py. Under Configuring the logger you can see an example. To adapt that to our code, I could imagine something like this:

# log.py


# create logger
log = logging.getLogger("rng2doc")
log.setLevel(logging.DEBUG)  # Not sure if this is needed

# create console handler and set level to debug
ch = logging.StreamHandler()
ch.setLevel(logging.DEBUG)  # This should be set in cli.py

# create formatter
formatter = CustomConsoleFormatter('')

# add formatter to ch
ch.setFormatter(formatter)

# add ch to logger
log.addHandler(ch)

Whenever you need this logger, import it as from log import log and use it like log.debug("Hi").

I haven't tested it, but that would be the idea. If we don't need the CustomConsoleFormatter class anymore, the whole setup would be a bit simpler.

@jloehel
Copy link
Collaborator

jloehel commented Jul 21, 2021

Thanks for the explanation. I thought logging.getLogger("rng2doc")/logging.getLogger(__package__) will return always the same object. What advantage do we get importing it from log.py? Draft: https://github.com/openSUSE/rng2doc/pull/61/files

@tomschr
Copy link
Contributor Author

tomschr commented Jul 21, 2021

By the way: Great to see you!

Thanks for the explanation. I thought logging.getLogger("rng2doc")/logging.getLogger(__package__) will return always the same object.

Not always, it depends where you call it. See this StackOverflow article:

"So, for a module located in foo/bar/baz.py, __name__ is set to foo.bar.baz, and __package__ is set to foo.bar, while foo/bar/__init__.py will have foo.bar for both the __name__ and __package__ attributes."

In other words, __package__ can be used to create hierarchical loggers, whereas "rng2doc" will always return the parent logger instance, regardless of the module level.

Hierarchical loggers can be used if you want to distinguish between different modules inside your project. For example, you can configure a transformation and an analyze logger, but with different log levels. One could log into a file, the other prints the message to the console. Just a matter of configuration. 😉

What advantage do we get importing it from log.py?

The same you get when you split something into a new file: separation. 🙂

It's probably a matter of taste where you put it. As you need to access your logger in different places of the project, I found it convenient to add it to a dedicated log.py file and import it wherever I need it. That way I can bundle all the necessary setup steps in one file and only import the logger instance.

Additionally, if I want to switch from Python native setup to a INI configuration file setup, I think it's easier to add everything into this file. However, that's my taste and how I prefer to do it. It doesn't mean it's a standard (although I've found it in several projects).

@tomschr
Copy link
Contributor Author

tomschr commented Jul 21, 2021

My tomschr/example-cli-argparse.py gist contains an example of setting up the logger with a dictionary object.

@jloehel
Copy link
Collaborator

jloehel commented Jul 21, 2021

By the way: Great to see you!

Thanks :-)

I added another commit which bundles the setup and the logger in log.py again.

@tomschr
Copy link
Contributor Author

tomschr commented Jul 22, 2021

I added another commit which bundles the setup and the logger in log.py again.

Looking at your draft, I think the function setup_logging() is unnecessary. Why? Because it's enough to just import the logger instance. I envision something like this:

import logging

logger = logging.getLogger("rng2doc")
_handler = logging.StreamHandler()
_handler.setFormatter(logging.Formatter('[%(levelname)s] %(message)s'))
logger.addHandler(_handler)

As the logger.setLevel(level) call is done after CLI parsing, you don't need that in the above code.

@jloehel
Copy link
Collaborator

jloehel commented Jul 22, 2021

I have added the function because I wanted to prevent this:

 ipython
Python 3.6.12 (default, Dec 02 2020, 09:44:23) [GCC]
Type 'copyright', 'credits' or 'license' for more information
IPython 7.16.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import logging

In [2]: logger = logging.getLogger("rng2doc")

In [3]: logger.handlers
Out[3]: []

In [4]: from rng2doc import rng

In [5]: logger.handlers
Out[5]: [<StreamHandler <stderr> (NOTSET)>]

I can add a NullHandler instead.

@tomschr
Copy link
Contributor Author

tomschr commented Jul 23, 2021

I can add a NullHandler instead.

That would be a good idea. 👍 It's also recommended in the Hitchhiker's Guide to Python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants