2
\$\begingroup\$

I have developed an implementation of a logger in Python. I know that Python already has a built-in logger, my goal with this is to get feedback and suggestions for improvement, particularly about code structure.

Code is here:

from .helpers import write_txt
from enum import Enum
from datetime import datetime


# Define type formats of messages
class LogFormats(Enum):
    TITLE = 'title'
    SUBTITLE = 'subtitle'
    ERROR = 'error'
    TAB = 'tab'
    DEFAULT = 'default'


# Handler functions for output messages with different formats

def format_title(message: str, time: str) -> str:
    to_print = f"\n[{time}] ### ### {message.upper()} ### ###\n"
    return to_print

def format_subtitle(message: str, time: str) -> str:
    to_print = f"[{time}] *** *** {message.upper()} *** ***"
    return to_print

def format_error(message: str, time: str) -> str:
    to_print = f"[{time}]\t  ERROR: {message}"
    return to_print

def format_tab(message: str, time: str) -> str:
    to_print = f"[{time}]\t  {message}"
    return to_print

def format_default(message: str, time: str) -> str:
    to_print = f"[{time}] - {message}"
    return to_print


# Handler functions dispatcher
FORMATTER_DISPATCH = {
    LogFormats.TITLE: format_title,
    LogFormats.SUBTITLE: format_subtitle,
    LogFormats.ERROR: format_error,
    LogFormats.TAB: format_tab,
    LogFormats.DEFAULT: format_default
}


class txtFileLogger():
    """Provides functionality for logging to a txt file"""
    registry: str = ''

    @classmethod
    def register_log(cls, msg: str) -> None:
        cls.registry = cls.registry + '\n' + msg

    @classmethod
    def output_log_to_txt(cls, filename = 'txt_log') -> None:
        write_txt(cls.registry, filename)


# ===================
#     MAIN CLASS
#====================

class Logger():
    """Format log messages and outputs it to the console. Also, provide methods for dumping the log session to a txt file."""
    txt_logger = txtFileLogger
    silenced = False

    @classmethod
    def log(cls,
            message: str,
            format: LogFormats = LogFormats.DEFAULT,
            hidden: bool = False
        ) -> None:
        """
        Logs a log message.

        This function logs a message and prints it to the console. The log type can be specified for different message formats. Also the console output can be toggled.

        Args:
            message (str): The message to be logged.
            type (LogTypes, optional): The log type.
            hidden (bool, optional): If set to True, hides the log in the console for this message. The message will be registered and dumped in txt file regardless of this option.
        """

        current_time = datetime.now().strftime("%H:%M:%S")
        msg = str(message)

        # Format log message according to selected type format
        log_type = LogFormats(format)
        formatter = FORMATTER_DISPATCH[log_type]
        formated_msg = formatter(msg, current_time)

        # Registers log for txt file output
        cls.txt_logger.register_log(formated_msg)

        # Print log to console
        if not hidden and not cls.silenced:
            print(formated_msg)
        
    @classmethod
    def dump_log_to_txt(cls, filename: str) -> None:
        """Writes log to a txt file"""
        cls.txt_logger.output_log_to_txt(filename)

    @classmethod
    def mute(cls) -> None:
        """Disables console output."""
        cls.silenced = True
        print('[Logger] -  *** Output to console is currently silenced ***')

    @classmethod
    def unmute(cls) -> None:
        """Enables console output."""
        cls.silenced = False
        print('[Logger] -  *** Enabling logging to console ***')

I implemented this using the class as a kind of "singleton". Thus, I could call the logger in different files and mantain a single source of truth for the registry. Example of usage:

import Logger

log = Logger.log

log('message 1', format='title')
log('message 2', format='subtitle')
log('error message', format='error')

Logger.dump_log_to_txt('foo_filename')

Some general questions:

  • Is there any way to improve the efficiency of my logger implementation?
  • How can I make my code more readable and well-structured?

I appreciate any feedback and suggestions. Thank you!

\$\endgroup\$
1
  • 3
    \$\begingroup\$ The helpers.write_txt function should also be described in this post to make the module usable. \$\endgroup\$
    – rdesparbes
    Commented Apr 8, 2023 at 14:55

2 Answers 2

3
\$\begingroup\$

document the Public API

The various format_foo handler functions are nice enough. There appears to be a prerequisite on message -- that it not contain a \n newline. Or at least it seems to me Author's Intent is "no newlines".

Consider raising an error if a newline is found. At a minimum, add a comment about what message should look like. If a multi-line message is passed in you might split on newline and log each line separately, to e.g. facilitate grep'ing for a string logged between 13:00 .. 14:00.

Consider relaxing the mypy requirement that message must be of str, given that you call str() on it anyway. Then caller can conveniently log e.g. an integer.


class names are Capitalized

class txtFileLogger():

Pep-8 asks that you spell it TxtFileLogger. That way your collaborators will immediately recognize it is a class; ATM it looks like a function.

Similarly, txt_logger = txtFileLogger should be TxtLogger = TxtFileLogger.

May as well delete the superfluous ( ) parens, as we're not inheriting from anything.

        cls.registry = cls.registry + ...

nit: Prefer cls.registry += ...

Consider renaming register_log to append_message.


the zone offset matters

        current_time = datetime.now().strftime("%H:%M:%S")

Logs get stored, then read back, sometimes months later, by both humans and by parsing programs. A timestamp of Monday at 13:00:00 corresponds to at least 24 instants displayed by wristwatches around the world. Consider displaying a Z (UTC) or numeric zone indicator. Consider using UTC rather than local timezone.

Whatever you do, document it! Timestamped messages is a fundamental part of the value-add that this class brings to the table. Be precise about what a logged timestamp actually means.

Consider calling formatter(str(message), current_time) so you can dispense with msg, as it is a not very helpful temp var.

nit, typo: Use conventional English spelling: formatted_msg


design the Public API with the caller in mind

Using the terms "hidden" and "silenced" for the same concept seems to muddy the Public API which callers must learn. Oh, wait, you apparently intended a private cls._silenced. So we have "hidden" vs "muted". Consider renaming the hidden parameter.

Mixing "error" severity with styles like "title" / "subtitle" is a bit jarring. A prospective caller will read that this is a "logging" package and will naturally expect support for different severities. We do this to decouple the concerns of author and maintainers, who want chatty output, from operation during a production run where verbose output might be the opposite of helpful.


unbounded memory growth

This logger seems to be "hard to use", in that a naive loop

while True:
    log(f"{n}")
    n = find_next_estimate(n)

will unexpectedly consume all RAM. (And replacing log with print(n) would not.)

Consider truncating the log when len(cls.registry) exceeds a threshold.

Definitely truncate once dump_log_to_txt has saved the text to disk. Else repeated dumps cause quadratic file growth.


performance

Consider making registry a list of str, to reduce memcpy() cost when we re-allocate as it grows.

One key to logger performance is quickly discarding silenced messages.

for item in a_million_items:
    log(expensive_function_of(item), hidden=True)
    total += len(item) 

Caller should conditionally compute that:

    if {some "muted" predicate that your library provides}:
        log(expensive_function_of(item), hidden=hidden)

This codebase achieves some of its design goals. It is less well documented than python's standard logging module. For some use cases it may be more convenient to use.

I would be happy to delegate or accept maintenance tasks on this codebase.

\$\endgroup\$
2
\$\begingroup\$

All of your format_ functions can be refactored as format strings with named parameters, as in

"\n[{time}] ### ### {message} ### ###\n"

which can then be formatted in one location via .format(time=x, message=x).

LogFormats is fairly confused. Some of the entries are "severities" (ERROR), some of them are formats, and one of them is a DEFAULT that should be set equal to a different named format like DEFAULT = PLAIN.

txtFileLogger needs to be in TitleCase because it's a class. Also, it should be a non-static class with a member for the filename, and write_txt should be moved into it as a member function. There should be one instance per destination file.

Classes that don't inherit from anything should not receive a () suffix.

Logger should also be a non-static class. It's not helpful to have a logger forced to be a singleton. At the most, make Logger a normal class that can either be instantiated or offer a default global instance for convenience.

hidden is awkward. You've effectively hard-coded a mechanism where a file has a lower minimum severity than the console. Look to how the built-in logger does this; you can configure the console and the file with different severities, and then the client side can be naive to which targets care about which severities, and provide a single severity per log entry.

"efficiency" is a loaded term. Use the built-in logger. If you're attempting to push so much log volume through that the built-in logger is a limiting factor in time or space, then you're probably logging too much and rather than attempting to optimise a home-rolled version, you should limit what you log. There's a ceiling to what can usefully be logged in a non-structured way. (Structured logging is a different topic.) Efficiency concerns here are premature optimisation.

For practice, fine, I guess; but in real life don't do any of this and use the built-in logger.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.