Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement and standardize logging configuration via config file #1095

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Implement and standardize logging configuration via config file #1095

merged 1 commit into from
Nov 5, 2024

Conversation

ela-kotulska-frequenz
Copy link
Contributor

@ela-kotulska-frequenz ela-kotulska-frequenz commented Nov 4, 2024 •
edited
Loading

  • Add LoggerConfig and LoggingConfig to standardize logging configuration.
  • Create LoggingConfigUpdater to handle runtime config updates.
  • Support individual log level settings for each module.

@ela-kotulska-frequenz ela-kotulska-frequenz added the part:config Affects the configuration management label Nov 4, 2024
@ela-kotulska-frequenz ela-kotulska-frequenz self-assigned this Nov 4, 2024
@ela-kotulska-frequenz ela-kotulska-frequenz requested a review from a team as a code owner November 4, 2024 10:50
@ela-kotulska-frequenz ela-kotulska-frequenz requested review from Marenz and removed request for a team November 4, 2024 10:50
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Nov 4, 2024
@github-actions github-actions bot added the part:docs Affects the documentation label Nov 4, 2024
@ela-kotulska-frequenz ela-kotulska-frequenz changed the title Add support to configure log level via config file. Implement and standardize logging configuration via config file Nov 4, 2024
@ela-kotulska-frequenz ela-kotulska-frequenz linked an issue Nov 4, 2024 that may be closed by this pull request
Add support for standardized logging #997
Closed
Marenz
Marenz previously approved these changes Nov 4, 2024
View reviewed changes
llucax
llucax reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments optional, except for the test, I think it is important to test the _update_logging() method.

src/frequenz/sdk/config/_logging_config_updater.py Outdated Show resolved Hide resolved
src/frequenz/sdk/config/_logging_config_updater.py Outdated Show resolved Hide resolved
src/frequenz/sdk/config/_logging_config_updater.py Outdated Show resolved Hide resolved
tests/config/test_logging_config_updater.py
# Check if default config has been set
logging_mock.basicConfig.assert_called_once()

update_logging_spy = mocker.spy(actor, "_update_logging")
Copy link
Contributor

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_logging method itself. I would either change this test or add another test and mock logging.getLogger to verify it is being called as expected, or maybe even just instead the real logging (basically l = logging.getLogger(...); asser l.level == expected_level) to verify the config was actually changed.

Copy link
Contributor Author

@ela-kotulska-frequenz ela-kotulska-frequenz Nov 5, 2024 •
edited
Loading

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. :)

  1. python doens't allow to override root level config in unit test (which makes sense because it can mess up other tests).
  2. loggings setings for other loggers are not reset after unit test method ends, I had to write teardown method (pytest.fixture) to reset them manually. Please look at test.

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.

Copy link
Contributor

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.

cwasicki
cwasicki reviewed Nov 4, 2024
View reviewed changes
Copy link
Collaborator

@cwasicki cwasicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, agree with Luca on renaming default to root.

@ela-kotulska-frequenz
Copy link
Contributor Author

ela-kotulska-frequenz commented Nov 5, 2024 •
edited
Loading

Old version had a but: with new config, old loggers were not unset. It is fixed and tested, now.

@llucax
Copy link
Contributor

llucax commented Nov 5, 2024

Old version had a but: with new config, old loggers were not unset. It is fixed and tested, now.

Ah, good catch!

llucax
llucax reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one missing update of default -> root_logger and it is good to go!

src/frequenz/sdk/config/_logging_config_updater.py Outdated Show resolved Hide resolved
@ela-kotulska-frequenz
Implement and standardize logging configuration via config file
ce73140 
* Add LoggerConfig and LoggingConfig to standardize logging configuration.
* Create LoggingConfigUpdater to handle runtime config updates.
* Support individual log level settings for each module.i

Signed-off-by: Elzbieta Kotulska <[email protected]>
@llucax
Copy link
Contributor

llucax commented Nov 5, 2024

It's nice to see the config stuff getting some love and being put in the SDK 🎉

@ela-kotulska-frequenz ela-kotulska-frequenz added this pull request to the merge queue Nov 5, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit ec94e3e Nov 5, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwasicki cwasicki cwasicki left review comments

@llucax llucax llucax approved these changes

@Marenz Marenz Marenz left review comments

@shsms shsms Awaiting requested review from shsms

Assignees

@ela-kotulska-frequenz ela-kotulska-frequenz

Labels
part:config Affects the configuration management part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)
Projects
Python SDK Roadmap
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for standardized logging
4 participants
@ela-kotulska-frequenz @llucax @Marenz @cwasicki