Skip to content

Add logging config and guidelines #1132

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

Closed
lukpueh opened this issue Sep 10, 2020 · 10 comments
Closed

Add logging config and guidelines #1132

lukpueh opened this issue Sep 10, 2020 · 10 comments
Labels
discussion Discussions related to the design, implementation and operation of the project documentation Documentation of the project as well as procedural documentation

Comments

@lukpueh
Copy link
Member

lukpueh commented Sep 10, 2020

Add project-wide configuration for the Python logging module, create guideline on how to use it (when to use which logging level), and use it instead of our own logging wrapper.

@lukpueh lukpueh added the discussion Discussions related to the design, implementation and operation of the project label Sep 10, 2020
@lukpueh
Copy link
Member Author

lukpueh commented Sep 10, 2020

I have spent quite some effort on trying to use the loggingmodule the right way in in-toto over the years. Maybe we can revisit related issues/prs to see if this is applicable for TUF and for a general guideline document. See e.g.
in-toto/in-toto#6 (comment), in-toto/in-toto#183, in-toto/in-toto#240.

@lukpueh
Copy link
Member Author

lukpueh commented Sep 10, 2020

cc @MVrachev and #1104

@joshuagl joshuagl added the documentation Documentation of the project as well as procedural documentation label Sep 10, 2020
@joshuagl
Copy link
Member

Some good advice on when to log in #1145

@jku
Copy link
Member

jku commented Sep 18, 2020

The important rules that I think work are:

  • libraries should only use levels WARNING and above when the internal state of the library is in the respective state: Applications can handle logs as they want but by default WARNING and above are shown to the user (printed to console) and that should be taken into consideration
  • warnings (warnings.warn(), not the log level) should be used to communicate with application developer (e.g. about deprecation). Warnings are not shown to end users by default
  • This leaves INFO and DEBUG for logging "normal operation" of the library
  • logging is usually unnecessary if the same information is in an exception raised immediately after

@jku
Copy link
Member

jku commented Oct 19, 2020

After wrestling with the current log module a few times I'll document my understanding of it's contents:

  1. configuring default log handler
    logger = logging.getLogger('tuf') 
    logger.addHandler(logging.NullHandler())
    I believe this is why tuf.log gets imported "just in case" in several modules. The purpose of this is to silence the library logging if application has not setup it's own logging.
  2. logging configuration based on tuf.settings: None of this should happen IMO (especially not by just importing the module), the application can do the same things more transparently in application code
  3. helper functions: There is nothing tuf specific here: application could do all of this independently

In my opinion the log module is not needed:

  • I think case 1 above is worth doing but we would achieve the same result (pretty much backwards-compatibly) by adding the addhandler line into each module that uses logging. Alternatively there could be a tiny internal module that just does this.
  • Cases 2 & 3 would be better handled by some documentation: "Handling TUF logging in your application".

@lukpueh
Copy link
Member Author

lukpueh commented Oct 19, 2020

I think we should use a log module as a central place to configure a base logger, from which all other module loggers inherit automatically by name:

# In log.py (configure base logger handler, format, default level, etc. eg. based on 'tuf.settings') 
import logging 
logger = logging.getLogger("tuf")
logger.addHandler(...
...
# In __init__.py (initialize base handler before anything else)
# This is the only place where 'tuf.log' needs to be imported
import tuf.log

# Alternatively, we could just configure the base logger here and
# omit the 'log.py' module.
# In any other tuf.<module>.py we inherit just by name
import logging
logger = logging.getLogger(__name__) # __name__ == 'tuf.<module>' and thus inherits from 'tuf' base logger.
...

This also allows application developers to just grab the base handler via logging.getLogger('tuf') and customize all of TUF's logging, e.g. silence or make more verbose (see in-toto.log for how this is done in practice).

@jku
Copy link
Member

jku commented Oct 19, 2020

# In __init__.py (initialize base handler before anything else)
# This is the only place where 'tuf.log' needs to be imported
import tuf.log

# Alternatively, we could just configure the base logger here and
# omit the 'log.py' module.

__init__.py would need a single line logging.getLogger("tuf").addHandler(logging.NullHandler()) so that seems reasonable if tuf modules are expected to do "import tuf" anyway.

@lukpueh
Copy link
Member Author

lukpueh commented Oct 19, 2020

Yes, if we don't need any other customization then it's enough to just add it to __init__.py (IIRC, that's what the tuf fork uptane does). In in-toto we do have a separate log.py-module, because we do quite a bit of logger customization for the in-toto command lines tools, which can probably be seen as in-toto applications. TUF OTOH should be a pure library, so configuring a default NullHandler and having application developers customize logging as suited seems like a good idea.

Regarding

... if tuf modules are expected to do "import tuf" anyway.

AFAIK that isn't even necessary. __init__.py is automatically executed if a module of the package is executed.

@sechkova
Copy link
Contributor

sechkova commented Apr 7, 2021

Another logging-related discussion: #1334

@lukpueh
Copy link
Member Author

lukpueh commented Feb 28, 2022

This issue was created for python-tuf's custom tuf.log module, which was removed in #1790.

The latest python-tuf release uses the Python builtin logging module idiomatically and sparingly. A dedicated guideline in addition to official documentation doesn't seem to be necessary.

Logging related issues (e.g. #1875, #1804, #1120) should be resolved according to these official recommendations

@lukpueh lukpueh closed this as completed Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussions related to the design, implementation and operation of the project documentation Documentation of the project as well as procedural documentation
Projects
None yet
Development

No branches or pull requests

4 participants