log setup using RotatingFileHandler and basicConfig

Question

I am new to python, and developing a moderate application for which I need to setup a logging mechanism.

The below program sets up log handling, which gives the user an option to specify whether log rotation is required or not.

The rest is my application. The code is mostly written in terms of functions (not using classes) which are irrelevant to the log setup and mostly contain printing log messages.

But before going ahead with the complete application, I would like to know from experts whether the following approach of using RotatingFileHandler and basicConfig is good enough.

I would like to know the comments for below:

1. In case if I want to filter logs (either to stdout or file) based on log levels.
2. Adding any new handlers
3. Any other unforeseen problems (I am just a fresh learner)
4. In general is this the correct approach for supporting RotatingFileHandler and basicConfig
5. Any other general comments

import logging
import logging.handlers

try:
    import configparser
except ImportError:
    # Python 2.x fallback
    import ConfigParser as configparser

import argparse

LOG_FILENAME = 'debug.log'
LOG_PATH = "C:\\tool_temp\\"
# Set up a specific logger with our desired output level
logger = logging.getLogger('MYAPP')

def log_setup(dir_name, config_dict):

    is_log_enabled = config_dict['logrotate_k']
    if is_log_enabled == "enabled":
        print "logrotation enabled"
        # Add the log message rotate_handler to the logger
        rotate_handler = logging.handlers.RotatingFileHandler(dir_name+LOG_FILENAME, maxBytes=512, backupCount=5)

        #'%b %d %H:%M:%S' is to avoid printing 628 after 2021-05-25 16:30:30,628
        formatter = logging.Formatter('%(asctime)s : %(levelname)s: %(filename)s::%(funcName)s:%(lineno)d %(message)s', '%d %b %d %H:%M:%S')

        rotate_handler.setFormatter(formatter)

        logger.addHandler(rotate_handler)
        logger.setLevel(logging.DEBUG)
    else:
        print "logrotation is disabled"
        date_strftime_format = "%d %b %y %H:%M:%S"
        message_format='%(asctime)s : %(levelname)s %(filename)s:: %(funcName)s:%(lineno)d %(message)s'
        #by default, the logging module logs the messages with a severity level of WARNING or above. so used level=logging.DEBUG
        # to log everything from debug to critical
        logging.basicConfig(filename = dir_name+LOG_FILENAME, format = message_format, datefmt = date_strftime_format,level=logging.DEBUG)

def is_file_exist(config_file_path):
    if os.path.exists(config_file_path):
        return True
    else:
        return False

#Reading config file through sections and keys
def read_config(cfg_path):
    config_dict = {}
    config = configparser.ConfigParser()
    try:
        config.read(cfg_path)
        config_dict['logrotate_k'] = config.get('CONF_PARAMS', 'logrotate')
    except Exception as e:
        print("Error",str(e))
        sys.exit()
    return config_dict

# For parsing the command line arguments
# returns dictionary "config_dict" with the app build parameters
def parse_command_line_args():
    config_dict = {}
    parser = argparse.ArgumentParser(description='My Application')

    parser.add_argument('-config_file', type = str)
    parser.add_argument('-logrotate',type=str,help='logrotation enabled or not')

    input_args = parser.parse_args()

    config_file_path = input_args.config_file

    if config_file_path != None:
        if is_file_exist(config_file_path) == True:
            print "Reading  configs from config file: " + config_file_path
            config_dict = read_config(config_file_path)
        else:
            print config_file_path + " does not exists"
            sys.exit()
    else:
        print "reading  configs from command line"
        config_dict['logrotate_k'] = input_args.logrotate
    return config_dict

if __name__ == "__main__":

    config_dict = {}

    config_dict = parse_command_line_args()
    log_setup(LOG_PATH+LOG_FILENAME,config_dict)
    logger.info('this info is from main')
    logger.error('this is test error message from main')

    print config_dict

Answer 1

• Don't use Python 2. Unless you're in legacy software hell, such as Jython, just don't do it.
• C:\tool_temp is not a good location for log files. Depending on intent, this might be better as C:\ProgramData\my_app\logs for instance.
• Instead of initializing the logger in the global namespace and configuring it after the fact, consider instantiating the logger locally, configuring and then returning it to be set on the global namespace afterwards.
• Your configuration dict mechanism is unnecessary. Read about https://docs.python.org/3.8/library/argparse.html#fromfile-prefix-chars which supports what - in another universe - is called a response file; basically a file with the exact same information that a list of command-line arguments would have.
• You call basicConfig, but only in the case where you don't want to rotate logs. This means that in the case where you do want to rotate logs, you're failing to set up a StreamHandler. That's probably an error.
• %d %b %d is probably an error.
• You're using a non-sortable datetime format, which has grave consequences for some log file handling software. Just use a machine-readable ISO8601-style timestamp instead.
• Consider adding PEP484 type hints.
• Your arguments should follow the Unix convention of having a --double-dashed long form and a -s single-dashed short form.

Example code

import logging
from argparse import ArgumentParser, Namespace
from logging import Formatter, Logger, getLogger, FileHandler, StreamHandler
from logging.handlers import RotatingFileHandler
from pathlib import Path
from pprint import pformat

LOG_FILENAME = 'debug.log'
# Consider /var/log/my_app if in Unix, or something under C:\ProgramData if in Windows
LOG_PATH = Path('.') 


def log_setup(dir_name: Path, filename: str, rotation_enabled: bool) -> Logger:

    # Avoid printing 628 after 2021-05-25 16:30:30,628
    formatter = Formatter(
        fmt='%(asctime)s : %(levelname)s: %(filename)s::%(funcName)s:%(lineno)d %(message)s',
        datefmt="%Y-%m-%d %H:%M:%S",
    )

    stream_handler = StreamHandler()
    stream_handler.setFormatter(formatter)

    if rotation_enabled:
        # Add the log message rotate_handler to the logger
        file_handler = RotatingFileHandler(dir_name / filename, maxBytes=512, backupCount=5)
    else:
        file_handler = FileHandler(dir_name / filename)
    file_handler.setFormatter(formatter)

    logger = getLogger('MYAPP')
    logger.addHandler(stream_handler)
    logger.addHandler(file_handler)
    # by default, the logging module logs the messages with a severity level of WARNING or above.
    # so used level=logging.DEBUG to log everything from debug to critical
    logger.setLevel(logging.DEBUG)

    return logger


def parse_command_line_args() -> Namespace:
    parser = ArgumentParser(
        description='My Application',
        fromfile_prefix_chars='@',
    )
    parser.add_argument(
        '-r', '--log-rotate', action='store_true',
        help='enable log rotation',
    )
    return parser.parse_args()


def test_levels() -> None:
    logger.debug('this debug is from our test')
    logger.info('this info is from our test')
    logger.error('this error is from our test')


if __name__ == "__main__":
    config = parse_command_line_args()
    logger = log_setup(Path(LOG_PATH), LOG_FILENAME, config.log_rotate)
    logger.debug(f'Configuration: {pformat(config.__dict__)}')
    test_levels()

Answer 2

is_file_exist

This function basically follows the pattern

if condition:
    return True
else:
    return False

If type(condition) == bool, this pattern can always be simplified to

return condition

If type(condition) != bool (i.e. you're checking for truthy and falsy values), it's still pretty simple:

return bool(condition)

So in your case:

def is_file_exist(config_file_path):
    return os.path.exists(config_file_path)

As you're now only wrapping os.path.exists, you should consider getting rid of the function as it serves no real purpose.