• 
      

    Standardize logging, and add loggers for tools.

    Review Request #11568 — Created April 1, 2021 and submitted

    Information

    ReviewBot
    release-3.0.x

    Reviewers

    Review Bot's logging was all over the place. Some code was using the
    base logging module, some was calling logging.getLogger(), and some
    was (rightfully) using Celery's get_logger() or get_task_logger()
    functions. The Celery versions are pretty important, given how much
    Celery controls the environment that our code runs in, so we actually
    had some logging that didn't really work before.

    This change aims to standardize logging, future-proofing it at the same
    time. We now have our own reviewbot.utils.log.get_logger() function
    that wraps Celery's get_logger()/get_task_logger(). It defaults to
    creating task loggers, since most operations will run within the context
    of a task.

    BaseTool provides a new logger property for logging. It will
    dynamically construct/retrieve a logger as needed, determining whether
    that logger will be a task logger or a standard logger. This simplifies
    tools even further, taking the guess work out of each implementation.

    The log message formats have been altered to be a little bit nicer and
    more consistent with typical log messages, and to include the logger
    name (which Celery's default message format omits).

    Some log levels have also been adjusted here and there, to cut down on
    unwanted noise.

    Unit tests pass for Python 2.7 and 3.x.

    Verified that the logs on startup (for dependency checks and configuration)
    were using standard loggers without task information, but that all the log
    messages going through during a task run were showing task information.

    Summary ID
    Standardize logging, and add loggers for tools.
    Review Bot's logging was all over the place. Some code was using the base `logging` module, some was calling `logging.getLogger()`, and some was (rightfully) using Celery's `get_logger()` or `get_task_logger()` functions. The Celery versions are pretty important, given how much Celery controls the environment that our code runs in, so we actually had some logging that didn't really work before. This change aims to standardize logging, future-proofing it at the same time. We now have our own `reviewbot.utils.log.get_logger()` function that wraps Celery's `get_logger()`/`get_task_logger()`. It defaults to creating task loggers, since most operations will run within the context of a task. `BaseTool` provides a enw `logger` property for logging. It will dynamically construct/retrieve a logger as needed, determining whether that logger will be a task logger or a standard logger. This simplifies tools even further, taking the guess work out of each implementation. The log message formats have been altered to be a little bit nicer and more consistent with typical log messages, and to include the logger name (which Celery's default message format omits). Some log levels have also been adjusted here and there, to cut down on unwanted noise.
    8bea4a0440ff763d25d1d1e700ff6e3a6f086397
    Description From Last Updated

    I think it's more useful to have this in the method definition rather than the module. Or perhaps both.

    daviddavid
    david
    1. 
        
    2. bot/reviewbot/utils/log.py (Diff revision 1)
       
       
       
      Show all issues

      I think it's more useful to have this in the method definition rather than the module. Or perhaps both.

    3. 
        
    chipx86
    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (c02f81e)