From 9fc3e8bd05cc7e0929131c2f66099ff2eb3190fc Mon Sep 17 00:00:00 2001 From: Olaf Mandel Date: Sun, 6 Oct 2024 17:52:35 +0200 Subject: [PATCH 1/2] configuration: add logging section Allow to configure both the logging-levels and targets of various loggers by adding a new section `logging` to the configuration file. Each entry in that section configures one logger / channel. An example config is: ```yaml logging: - name root level: INFO - name: foomodule.barcollector: level: WARNING target: /path/to/my/collector/logfile.log ``` --- CHANGELOG.md | 1 + README.md | 12 ++++ p3.yml | 3 + p3exporter/__init__.py | 30 ++++++++ tests/test_cases/logging_config.py | 106 +++++++++++++++++++++++++++++ 5 files changed, 152 insertions(+) create mode 100644 tests/test_cases/logging_config.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c3b113e..a976060 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ This project adheres to [Semantic Versioning](http://semver.org/) and [Keep a Ch ### New * Fix \#47 - add environment variables for fundamental parameters +* configuration: add logging section ### Changes diff --git a/README.md b/README.md index 03a6ea4..bcef0b1 100644 --- a/README.md +++ b/README.md @@ -113,8 +113,20 @@ collector_opts: blacklist: - docker0 - lo +logging: + - name: root + level: INFO + - name: foomodule.barcollector + level: WARNING + target: /path/to/my/collector/logfile.log ``` +Logging can optionally be configured for any logger. The entries must +specify the name of the logger and can optionally specify a +logging-level (default: stay at whatever the default logging-level for +that logger is) and/or can specify a file to write the log to (default: +log to stderr). + ### Start-up Configuration You can define two fundamental Parameters on program start-up. The following table summarized you options: diff --git a/p3.yml b/p3.yml index 3a19363..9f6a56f 100644 --- a/p3.yml +++ b/p3.yml @@ -9,3 +9,6 @@ collector_opts: blacklist: - docker0 - lo +logging: + - name: root + level: INFO diff --git a/p3exporter/__init__.py b/p3exporter/__init__.py index f2f5df7..670e80d 100644 --- a/p3exporter/__init__.py +++ b/p3exporter/__init__.py @@ -13,6 +13,35 @@ from p3exporter.web import create_app +def setup_logging(cfg: dict): + """Set up logging as configured. + + The configuration may optionally contain an entry `logging`, + if it does not or if that entry is not an array then does nothing. + Each array element must be a dict that contains at least a key + `name` that refers to the logger to configure. It may also contain + the optional keys `level` and `target` that configure the + logging-level and a file-target, respectively if present. + + :param cfg: Configuration as read from config-file. + :type cfg: dict + """ + if not isinstance(cfg.get('logging'), list): + return + for c in cfg['logging']: + if not isinstance(c, dict): + return + if not isinstance(c.get('name'), str): + return + logger = logging.getLogger(c["name"]) + level = c.get('level') + if level is not None: + logger.setLevel(level) + target = c.get('target') + if target is not None: + logger.addHandler(logging.FileHandler(target)) + + def shutdown(): """Shutdown the app in a clean way.""" logging.info('Shutting down, see you next time!') @@ -41,6 +70,7 @@ def main(): with open(args.config, 'r') as config_file: cfg = yaml.load(config_file, Loader=yaml.SafeLoader) collector_config = CollectorConfig(**cfg) + setup_logging(cfg) Collector(collector_config) diff --git a/tests/test_cases/logging_config.py b/tests/test_cases/logging_config.py new file mode 100644 index 0000000..9f83262 --- /dev/null +++ b/tests/test_cases/logging_config.py @@ -0,0 +1,106 @@ +from p3exporter import setup_logging +import logging +import os.path +import pytest + + +loggers = ["", "foo", "bar"] +files = ["file1.log", "file2.log"] + + +def setup_function(fn): + """Start with a clean slate of default logging-levels and no handlers.""" + for name in loggers: + logger = logging.getLogger(name) + level = logging.WARNING if name == "" else logging.NOTSET + logger.setLevel(level) + for handler in logger.handlers: + logger.removeHandler(handler) + + +def teardown_function(fn): + """Remove any files we may have created.""" + for file in files: + if os.path.exists(file): + os.remove(file) + + +data_logging_levels = [ + pytest.param(None, + [logging.WARNING, logging.NOTSET, logging.NOTSET], + [None, None, None], + id="no logging-section at all"), + pytest.param("Not an array", + [logging.WARNING, logging.NOTSET, logging.NOTSET], + [None, None, None], + id="logging-section has wrong type"), + pytest.param([{"level": "INFO"}, + {"target": "file1.log"}, + {"level": "DEBUG", "target": "file2.log"}], + [logging.WARNING, logging.NOTSET, logging.NOTSET], + [None, None, None], + id="no names in otherwise valid entries"), + pytest.param([{"name": "", "level": "INFO"}, + {"name": "foo", "level": "DEBUG"}], + [logging.INFO, logging.DEBUG, logging.NOTSET], + [None, None, None], + id="levels only, using empty-string for root"), + pytest.param([{"name": "root", "level": "ERROR"}, + {"name": "bar", "level": "CRITICAL"}], + [logging.ERROR, logging.NOTSET, logging.CRITICAL], + [None, None, None], + id="levels only, using name of root"), + pytest.param([{"name": "foo", "level": 10}, + {"name": "bar", "level": 20}], + [logging.WARNING, logging.DEBUG, logging.INFO], + [None, None, None], + id="levels only, using integers for levels"), + pytest.param([{"name": "root", "target": "file1.log"}, + {"name": "foo", "target": "file2.log"}], + [logging.WARNING, logging.NOTSET, logging.NOTSET], + ["file1.log", "file2.log", None], + id="targets only"), + pytest.param([{"name": "foo", "level": "INFO", "target": "file1.log"}], + [logging.WARNING, logging.INFO, logging.NOTSET], + [None, "file1.log", None], + id="both level and target"), + ] + + +@pytest.mark.parametrize("cfg_logging,levels,targets", data_logging_levels) +def test_logging_levels(cfg_logging, levels, targets): + # pytest adds lots of extra handlers, so remember the starting state + orig_handlers = [] + for name in loggers: + logger = logging.getLogger(name) + orig_handlers.append(logger.handlers.copy()) + + # GIVEN an input config-dictionary + cfg = { + "exporter_name": "Test only", + "collectors": [], + "collector_opts": {}, + } + if cfg_logging is not None: + cfg["logging"] = cfg_logging + + # WHEN calling setup_logging() + setup_logging(cfg) + + # THEN the logging-levels should get changed to the expected + for i, name in enumerate(loggers): + logger = logging.getLogger(name) + assert logger.level == levels[i] + + # AND the expected file-handlers should get added + for i, name in enumerate(loggers): + logger = logging.getLogger(name) + added_handlers = [h for h in logger.handlers + if h not in orig_handlers[i]] + if targets[i] is None: + assert len(added_handlers) == 0 + else: + assert len(added_handlers) == 1 + handler = added_handlers[0] + assert isinstance(handler, logging.FileHandler) + assert handler.baseFilename == os.path.abspath(targets[i]) From 0ea72f636302300371ceee54046c6711c8fac640 Mon Sep 17 00:00:00 2001 From: Olaf Mandel Date: Sun, 6 Oct 2024 18:06:09 +0200 Subject: [PATCH 2/2] configuration: add log_level to configuration_opts Add `CollectorBase.setLoggers(logger_names)` as a means for collectors derived from `CollectorBase` to easily configure logging-levels. The new method uses the `collector_opts.logging_level` value (if set in the configuration) to set the logging-level of any loggers given to it. --- CHANGELOG.md | 1 + README.md | 5 +++ p3exporter/collector/__init__.py | 19 +++++++++++ tests/test_cases/logging_config.py | 51 ++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a976060..fc49c5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ This project adheres to [Semantic Versioning](http://semver.org/) and [Keep a Ch ### New * Fix \#47 - add environment variables for fundamental parameters * configuration: add logging section +* configuration: add log_level to configuration_opts ### Changes diff --git a/README.md b/README.md index bcef0b1..2bfaa3c 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,7 @@ collectors: - netdev collector_opts: netdev: + log_level: DEBUG whitelist: blacklist: - docker0 @@ -121,6 +122,10 @@ logging: target: /path/to/my/collector/logfile.log ``` +The `collector_opts` can optionally contain a `log_level` entry which +will configure the logging-level for that specific collector. Note that +support for this must be implemented by each individual collector. + Logging can optionally be configured for any logger. The entries must specify the name of the logger and can optionally specify a logging-level (default: stay at whatever the default logging-level for diff --git a/p3exporter/collector/__init__.py b/p3exporter/collector/__init__.py index d2541a7..fafb558 100644 --- a/p3exporter/collector/__init__.py +++ b/p3exporter/collector/__init__.py @@ -58,6 +58,25 @@ def collector_name_from_class(self): return '_'.join(class_name_parts) + def setLoggers(self, logger_names: list | str): + """Configure the provided logger(s) according to the CollectorConfig. + + It is recommended to call this method from the constructor of any + deriving class. Either bump the required p3exporter version or check + dynamically if the method is supported. + + :param logger_names: Name or names of loggers to configure. + :type logger_names: list or str + """ + if not isinstance(logger_names, list): + logger_names = [logger_names] + if "log_level" not in self.opts: + return + level = self.opts["log_level"] + for name in logger_names: + logger = logging.getLogger(name) + logger.setLevel(level) + class Collector(object): """Base class to load collectors. diff --git a/tests/test_cases/logging_config.py b/tests/test_cases/logging_config.py index 9f83262..a5b9975 100644 --- a/tests/test_cases/logging_config.py +++ b/tests/test_cases/logging_config.py @@ -1,4 +1,5 @@ from p3exporter import setup_logging +from p3exporter.collector import CollectorBase, CollectorConfig import logging import os.path import pytest @@ -104,3 +105,53 @@ def test_logging_levels(cfg_logging, levels, targets): handler = added_handlers[0] assert isinstance(handler, logging.FileHandler) assert handler.baseFilename == os.path.abspath(targets[i]) + + +class FooCollector(CollectorBase): + pass + + +data_collectorbase_setloggers = [ + pytest.param(None, + ["foo", "bar"], + [logging.WARNING, logging.NOTSET, logging.NOTSET], + id="no log_level setting"), + pytest.param("CRITICAL", + "foo", + [logging.WARNING, logging.CRITICAL, logging.NOTSET], + id="single logger-name"), + pytest.param("ERROR", + ["foo", "bar"], + [logging.WARNING, logging.ERROR, logging.ERROR], + id="list of loggers"), + pytest.param(20, + ["", "foo"], + [logging.INFO, logging.INFO, logging.NOTSET], + id="numeric log_level"), + ] + + +@pytest.mark.parametrize("cfg_log_level,logger_names,expected", + data_collectorbase_setloggers) +def test_collectorbase_setloggers(cfg_log_level, logger_names, expected): + # GIVEN an input config-dictionary + cfg = { + "exporter_name": "Test only", + "collectors": ["foo"], + "collector_opts": { + "foo": {} + }, + } + if cfg_log_level is not None: + cfg["collector_opts"]["foo"]["log_level"] = cfg_log_level + + # AND a collector-base using this config + collector = FooCollector(CollectorConfig(**cfg)) + + # WHEN the setLoggers() method is called + collector.setLoggers(logger_names) + + # THEN the logging-levels should get changed to the expected + for i, name in enumerate(loggers): + logger = logging.getLogger(name) + assert logger.level == expected[i]