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.

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

Remove unnecessary FILE log level #741

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bocekm
Copy link
Member

@bocekm bocekm commented Feb 17, 2023

Having a custom log level was unnecessarily complex and the routing of debug messages to the stdout was not intuitive.
Using a Filter to decide whether a debug level message should be printed to stdout based on the use of the --debug option is cleaner.

Jira Issue: RHELC-

Checklist

  • PR meets acceptance criteria specified in the Jira issue
  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] is part of the PR title
  • Code and tests are documented properly
  • The commits are squashed to as few commits as possible (without losing data)
  • When merged: Jira issue has been updated to Release Pending

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Base: 92.71% // Head: 92.78% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (fc5cfea) compared to base (ea63b78).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #741      +/-   ##
==========================================
+ Coverage   92.71%   92.78%   +0.07%     
==========================================
  Files          24       24              
  Lines        3282     3273       -9     
  Branches      576      573       -3     
==========================================
- Hits         3043     3037       -6     
+ Misses        172      171       -1     
+ Partials       67       65       -2     
Flag Coverage Δ
centos-linux-7 88.05% <100.00%> (+0.05%) ⬆️
centos-linux-8 89.09% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
convert2rhel/logger.py 89.81% <100.00%> (+0.24%) ⬆️
convert2rhel/utils.py 87.33% <100.00%> (+0.56%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Having a custom log level was unnecessarily complex and the routing of
debug messages to the stdout was not intuitive.
Using a Filter to decide whether a debug level message should be printed
to stdout based on the use of the --debug option is cleaner.
"""Print debug messages to the stdout only when --debug is used."""

def filter(self, record):
from convert2rhel.toolopts import tool_opts

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [convert2rhel.toolopts](1) begins an import cycle.
Copy link
Member

Choose a reason for hiding this comment

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

The circular import would also go away with the restructure to have an early log initialization and later log initialization. (If we put the early log initialization into the initialize.py file and leave the second configuration phase with the import convert2rhel.toolopts in this file)

Comment on lines 107 to 112
stdout_handler.setFormatter(formatter)
debug_flag_filter = DebugFlagFilter()
stdout_handler.addFilter(debug_flag_filter)
# Set the DEBUG level as the lowest allowed level to be printed to stdout.
# Whether a debug message is actually printed or not is decided in DebugFlagFilter.
stdout_handler.setLevel(logging.DEBUG)
Copy link
Member

Choose a reason for hiding this comment

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

Does it not work to set this to logging.DEBUG if --debug is given, otherwise set it to loggin.INFO? (If the problem is a circular dependency [Need toolopts to know if --debug was given but toolopts requires that logging has been setup in case it needs to log its values, the way that I've setup logging in other projects is to have two phases. Phase 1 sets it up with default values. Then parse the command line, read the config file, consult environment variables, etc. Then Phase 2, reconfigure portions of logging that user settings can affect.

That's a bigger change than this, but I think it's better structure. And at the risk of premature optimization, it's also less work for the computer. Adjusting the log level results in less work being done if the log message is at a less important level. Setting a filter results in more work being done for every log message emitted.

Copy link
Member

Choose a reason for hiding this comment

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

It is definitely due to circular dependencies, but another thing IIRC is about the log files we touch. We had some bug a few years ago that caused us to not log correctly or that the tool didn't work. Hence doing it after toolopts is initialized. We can take a look later if we can change this

@Venefilyn Venefilyn added the kind/feature New feature or request label Feb 28, 2023
Comment on lines 107 to 112
stdout_handler.setFormatter(formatter)
debug_flag_filter = DebugFlagFilter()
stdout_handler.addFilter(debug_flag_filter)
# Set the DEBUG level as the lowest allowed level to be printed to stdout.
# Whether a debug message is actually printed or not is decided in DebugFlagFilter.
stdout_handler.setLevel(logging.DEBUG)
Copy link
Member

Choose a reason for hiding this comment

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

It is definitely due to circular dependencies, but another thing IIRC is about the log files we touch. We had some bug a few years ago that caused us to not log correctly or that the tool didn't work. Hence doing it after toolopts is initialized. We can take a look later if we can change this

Comment on lines +145 to +148
if record.levelno == logging.DEBUG and not tool_opts.debug:
# not logging a debug level message if the --debug option hasn't been used
return False
return True
Copy link
Member

Choose a reason for hiding this comment

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

Ideally I'd want to set this from within toolopts somehow no?

@Venefilyn
Copy link
Member

@bocekm How do you want to tackle this PR?

@bocekm
Copy link
Member Author

bocekm commented Jan 8, 2024

Hi @SpyTec, thanks for the reminder. I need to find the time to rework it based on the comments.

@Venefilyn
Copy link
Member

@bocekm want to delegate this to someone else?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants