-
Couldn't load subscription status.
- Fork 20
Implement and standardize logging configuration via config file #1095
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
Merged
ela-kotulska-frequenz
merged 1 commit into
frequenz-floss:v1.x.x
from
ela-kotulska-frequenz:config
Nov 5, 2024
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,194 @@ | ||
| # License: MIT | ||
| # Copyright © 2024 Frequenz Energy-as-a-Service GmbH | ||
|
|
||
| """Read and update logging severity from config.""" | ||
|
|
||
| import logging | ||
| from collections.abc import Mapping | ||
| from dataclasses import field | ||
| from typing import Annotated, Any, Self, cast | ||
|
|
||
| import marshmallow | ||
| import marshmallow.validate | ||
| from frequenz.channels import Receiver | ||
| from marshmallow import RAISE | ||
| from marshmallow_dataclass import class_schema, dataclass | ||
|
|
||
| from frequenz.sdk.actor import Actor | ||
|
|
||
| _logger = logging.getLogger(__name__) | ||
|
|
||
| LogLevel = Annotated[ | ||
| str, | ||
| marshmallow.fields.String( | ||
| validate=marshmallow.validate.OneOf(choices=logging.getLevelNamesMapping()) | ||
| ), | ||
| ] | ||
|
|
||
|
|
||
| @dataclass | ||
| class LoggerConfig: | ||
| """A configuration for a logger.""" | ||
|
|
||
| level: LogLevel = field( | ||
| default="NOTSET", | ||
| metadata={ | ||
| "metadata": { | ||
| "description": "Log level for the logger. Uses standard logging levels." | ||
| }, | ||
| "required": False, | ||
| }, | ||
| ) | ||
| """The log level for the logger.""" | ||
|
|
||
|
|
||
| @dataclass | ||
| class LoggingConfig: | ||
| """A configuration for the logging system.""" | ||
|
|
||
| root_logger: LoggerConfig = field( | ||
| default_factory=LoggerConfig, | ||
| metadata={ | ||
| "metadata": { | ||
| "description": "Default default configuration for all loggers.", | ||
| }, | ||
| "required": False, | ||
| }, | ||
| ) | ||
| """The default log level.""" | ||
|
|
||
| loggers: dict[str, LoggerConfig] = field( | ||
| default_factory=dict, | ||
| metadata={ | ||
| "metadata": { | ||
| "description": "Configuration for a logger (the key is the logger name)." | ||
| }, | ||
| "required": False, | ||
| }, | ||
| ) | ||
| """The list of loggers configurations.""" | ||
|
|
||
| @classmethod | ||
| def load(cls, configs: Mapping[str, Any]) -> Self: # noqa: DOC502 | ||
| """Load and validate configs from a dictionary. | ||
|
|
||
| Args: | ||
| configs: The configuration to validate. | ||
|
|
||
| Returns: | ||
| The configuration if they are valid. | ||
|
|
||
| Raises: | ||
| ValidationError: if the configuration are invalid. | ||
| """ | ||
| schema = class_schema(cls)() | ||
| return cast(Self, schema.load(configs, unknown=RAISE)) | ||
|
|
||
|
|
||
| class LoggingConfigUpdater(Actor): | ||
| """Actor that listens for logging configuration changes and sets them. | ||
|
|
||
| Example: | ||
| `config.toml` file: | ||
| ```toml | ||
| [logging.root_logger] | ||
| level = "INFO" | ||
|
|
||
| [logging.loggers."frequenz.sdk.actor.power_distributing"] | ||
| level = "DEBUG" | ||
|
|
||
| [logging.loggers."frequenz.channels"] | ||
| level = "DEBUG" | ||
| ``` | ||
|
|
||
| ```python | ||
| import asyncio | ||
| from collections.abc import Mapping | ||
| from typing import Any | ||
|
|
||
| from frequenz.channels import Broadcast | ||
| from frequenz.sdk.config import LoggingConfigUpdater, ConfigManager | ||
| from frequenz.sdk.actor import run as run_actors | ||
|
|
||
| async def run() -> None: | ||
| config_channel = Broadcast[Mapping[str, Any]](name="config", resend_latest=True) | ||
| actors = [ | ||
| ConfigManager(config_paths=["config.toml"], output=config_channel.new_sender()), | ||
| LoggingConfigUpdater( | ||
| config_recv=config_channel.new_receiver(limit=1)).map( | ||
| lambda app_config: app_config.get("logging", {} | ||
| ) | ||
| ), | ||
| ] | ||
| await run_actors(*actors) | ||
|
|
||
| asyncio.run(run()) | ||
| ``` | ||
|
|
||
| Now whenever the `config.toml` file is updated, the logging configuration | ||
| will be updated as well. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| config_recv: Receiver[Mapping[str, Any]], | ||
| log_format: str = "%(asctime)s %(levelname)-8s %(name)s:%(lineno)s: %(message)s", | ||
| log_datefmt: str = "%Y-%m-%dT%H:%M:%S%z", | ||
| ): | ||
| """Initialize this instance. | ||
|
|
||
| Args: | ||
| config_recv: The receiver to listen for configuration changes. | ||
| log_format: Use the specified format string in logs. | ||
| log_datefmt: Use the specified date/time format in logs. | ||
| """ | ||
| super().__init__() | ||
| self._config_recv = config_recv | ||
| self._format = log_format | ||
| self._datefmt = log_datefmt | ||
|
|
||
| # Setup default configuration. | ||
| # This ensures logging is configured even if actor fails to start or | ||
| # if the configuration cannot be loaded. | ||
| self._current_config: LoggingConfig = LoggingConfig() | ||
| self._update_logging(self._current_config) | ||
|
|
||
| async def _run(self) -> None: | ||
| """Listen for configuration changes and update logging.""" | ||
| async for message in self._config_recv: | ||
| try: | ||
| new_config = LoggingConfig.load(message) | ||
| except marshmallow.ValidationError: | ||
| _logger.exception( | ||
| "Invalid logging configuration received. Skipping config update" | ||
| ) | ||
| continue | ||
|
|
||
| if new_config != self._current_config: | ||
| self._update_logging(new_config) | ||
|
|
||
| def _update_logging(self, config: LoggingConfig) -> None: | ||
| """Configure the logging level.""" | ||
| # If the logger is not in the new config, set it to NOTSET | ||
| loggers_to_unset = self._current_config.loggers.keys() - config.loggers.keys() | ||
| for logger_id in loggers_to_unset: | ||
| _logger.debug("Unsetting log level for logger '%s'", logger_id) | ||
| logging.getLogger(logger_id).setLevel(logging.NOTSET) | ||
|
|
||
| self._current_config = config | ||
| logging.basicConfig( | ||
| format=self._format, | ||
| level=self._current_config.root_logger.level, | ||
| datefmt=self._datefmt, | ||
| ) | ||
|
|
||
| # For each logger in the new config, set the log level | ||
| for logger_id, logger_config in self._current_config.loggers.items(): | ||
| _logger.debug( | ||
| "Setting log level for logger '%s' to '%s'", | ||
| logger_id, | ||
| logger_config.level, | ||
| ) | ||
| logging.getLogger(logger_id).setLevel(logger_config.level) | ||
|
|
||
| _logger.info("Logging config changed to: %s", self._current_config) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| # License: MIT | ||
| # Copyright © 2024 Frequenz Energy-as-a-Service GmbH | ||
|
|
||
| """Tests for logging config updater.""" | ||
|
|
||
| import asyncio | ||
| import logging | ||
| from collections.abc import Mapping | ||
| from typing import Any | ||
|
|
||
| import pytest | ||
| from frequenz.channels import Broadcast | ||
| from marshmallow import ValidationError | ||
| from pytest_mock import MockerFixture | ||
|
|
||
| from frequenz.sdk.config import LoggerConfig, LoggingConfig, LoggingConfigUpdater | ||
|
|
||
|
|
||
| def test_logging_config() -> None: | ||
| """Test if logging config is correctly loaded.""" | ||
| config_raw = { | ||
| "root_logger": {"level": "DEBUG"}, | ||
| "loggers": { | ||
| "frequenz.sdk.actor": {"level": "INFO"}, | ||
| "frequenz.sdk.timeseries": {"level": "ERROR"}, | ||
| }, | ||
| } | ||
| config = LoggingConfig( | ||
| root_logger=LoggerConfig(level="DEBUG"), | ||
| loggers={ | ||
| "frequenz.sdk.actor": LoggerConfig(level="INFO"), | ||
| "frequenz.sdk.timeseries": LoggerConfig(level="ERROR"), | ||
| }, | ||
| ) | ||
|
|
||
| assert LoggingConfig.load(config_raw) == config | ||
|
|
||
| config_raw = {} | ||
| config = LoggingConfig() | ||
| assert LoggingConfig.load(config_raw) == config | ||
|
|
||
| config_raw = {"root_logger": {"level": "UNKNOWN"}} | ||
| with pytest.raises(ValidationError): | ||
| LoggingConfig.load(config_raw) | ||
|
|
||
| config_raw = {"unknown": {"frequenz.sdk.actor": {"level": "DEBUG"}}} | ||
| with pytest.raises(ValidationError): | ||
| LoggingConfig.load(config_raw) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def cleanup_logs() -> Any: | ||
| """Reset logging to default. | ||
|
|
||
| Python doesn't cleanup logging configuration after tests, so we need to do it manually. | ||
| """ | ||
| yield | ||
|
|
||
| logging.getLogger("frequenz.sdk.actor").setLevel(logging.NOTSET) | ||
| logging.getLogger("frequenz.sdk.timeseries").setLevel(logging.NOTSET) | ||
|
|
||
|
|
||
| async def test_logging_config_updater_actor( | ||
| mocker: MockerFixture, | ||
| cleanup_logs: Any, | ||
| ) -> None: | ||
| """Test if logging is configured and updated correctly.""" | ||
| # Mock method that sets root level logging. | ||
| # Python doesn't cleanup logging configuration after tests. | ||
| # Overriding logging.basicConfig would mess up other tests, so we mock it. | ||
| # This is just for extra safety because changing root logging level in unit tests | ||
| # is not working anyway - python ignores it. | ||
| mocker.patch("frequenz.sdk.config._logging_config_updater.logging.basicConfig") | ||
|
|
||
| config_channel = Broadcast[Mapping[str, Any]](name="config") | ||
| config_sender = config_channel.new_sender() | ||
| async with LoggingConfigUpdater( | ||
| config_recv=config_channel.new_receiver().map( | ||
| lambda app_config: app_config.get("logging", {}) | ||
| ) | ||
| ) as actor: | ||
| assert logging.getLogger("frequenz.sdk.actor").level == logging.NOTSET | ||
| assert logging.getLogger("frequenz.sdk.timeseries").level == logging.NOTSET | ||
|
|
||
| update_logging_spy = mocker.spy(actor, "_update_logging") | ||
|
|
||
| # Send first config | ||
| await config_sender.send( | ||
| { | ||
| "logging": { | ||
| "root_logger": {"level": "ERROR"}, | ||
| "loggers": { | ||
| "frequenz.sdk.actor": {"level": "DEBUG"}, | ||
| "frequenz.sdk.timeseries": {"level": "ERROR"}, | ||
| }, | ||
| } | ||
| } | ||
| ) | ||
| await asyncio.sleep(0.01) | ||
| update_logging_spy.assert_called_once_with( | ||
| LoggingConfig( | ||
| root_logger=LoggerConfig(level="ERROR"), | ||
| loggers={ | ||
| "frequenz.sdk.actor": LoggerConfig(level="DEBUG"), | ||
| "frequenz.sdk.timeseries": LoggerConfig(level="ERROR"), | ||
| }, | ||
| ) | ||
| ) | ||
| assert logging.getLogger("frequenz.sdk.actor").level == logging.DEBUG | ||
| assert logging.getLogger("frequenz.sdk.timeseries").level == logging.ERROR | ||
| update_logging_spy.reset_mock() | ||
|
|
||
| # Update config | ||
| await config_sender.send( | ||
| { | ||
| "logging": { | ||
| "root_logger": {"level": "WARNING"}, | ||
| "loggers": { | ||
| "frequenz.sdk.actor": {"level": "INFO"}, | ||
| }, | ||
| } | ||
| } | ||
| ) | ||
| await asyncio.sleep(0.01) | ||
| expected_config = LoggingConfig( | ||
| root_logger=LoggerConfig(level="WARNING"), | ||
| loggers={ | ||
| "frequenz.sdk.actor": LoggerConfig(level="INFO"), | ||
| }, | ||
| ) | ||
| update_logging_spy.assert_called_once_with(expected_config) | ||
| assert logging.getLogger("frequenz.sdk.actor").level == logging.INFO | ||
| assert logging.getLogger("frequenz.sdk.timeseries").level == logging.NOTSET | ||
| update_logging_spy.reset_mock() | ||
|
|
||
| # Send invalid config to make sure actor doesn't crash and doesn't setup invalid config. | ||
| await config_sender.send({"logging": {"root_logger": {"level": "UNKNOWN"}}}) | ||
| await asyncio.sleep(0.01) | ||
| update_logging_spy.assert_not_called() | ||
| assert actor._current_config == expected_config | ||
| update_logging_spy.reset_mock() | ||
|
|
||
| # Send empty config to reset logging to default | ||
| await config_sender.send({"other": {"var1": 1}}) | ||
| await asyncio.sleep(0.01) | ||
| update_logging_spy.assert_called_once_with(LoggingConfig()) | ||
| assert logging.getLogger("frequenz.sdk.actor").level == logging.NOTSET | ||
| assert logging.getLogger("frequenz.sdk.timeseries").level == logging.NOTSET | ||
| update_logging_spy.reset_mock() | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I didn't know about
spy. But with this you are not actually testing the_update_loggingmethod itself. I would either change this test or add another test and mocklogging.getLoggerto verify it is being called as expected, or maybe even just instead the reallogging(basicallyl = logging.getLogger(...); asser l.level == expected_level) to verify the config was actually changed.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, however things get more complicated. :)
Also I found bug - with new config, old loggers were not unset. It is fixed and tested.
I am not testing root_logging level, but I think it is ok, because then we start testing external library.
I could check if logging.basicConfig was called with correct arguments, but I am not sure it it valuable check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think for that case you can just mock the
logging.getLogger()function and just trust that if it is called with the right parameters it will work as expected.