Remove unnecessary FILE log level #741
Conversation
Codecov ReportBase: 92.71% // Head: 92.78% // Increases project coverage by
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
Flags with carried forward coverage won't be shown. Click here to find out more.
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. |
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.
bocekm
force-pushed
the
remove-file-logger
branch
from
46b8d79
to
fc5cfea
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| """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
There was a problem hiding this comment.
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)
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| 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) |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
Ideally I'd want to set this from within toolopts somehow no?
|
@bocekm How do you want to tackle this PR? |
|
Hi @SpyTec, thanks for the reminder. I need to find the time to rework it based on the comments. |
|
@bocekm want to delegate this to someone else? |
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
[RHELC-]is part of the PR titleRelease Pending