Reduce debug noise for issue #336 #369
Conversation
schema/check_generated_data.py
Outdated
| @@ -15,20 +15,20 @@ | |||
| import schema_validator | |||
| from schema_files import ALL_TEST_TYPES | |||
|
|
|||
| logger = logging.Logger("Checking Test Data vs. Schemas LOGGER") | |||
| logger.setLevel(logging.INFO) | |||
There was a problem hiding this comment.
Suggestion: often these libraries have a way to set the logging level from an environment variable or from the CLI, so that you can do something like DEBUG=* ./genData or ./genData -vv
There was a problem hiding this comment.
Yes, that's possible. I'll take a look.
Also, I've removed lots of excessive logging in the latest commits.
One more thing: when executing in parallel, the individual threads cannot have a logger in their objects because a logger "cannot be pickled". That's a small complication / limit to logging.
There was a problem hiding this comment.
I think don't pickle the logger; just make a new one for each thread
| @@ -301,6 +297,8 @@ def create_summary_reports(self): | |||
|
|
|||
| # The following gets information from all the tests | |||
| summary_report = SummaryReport(self.file_base) | |||
| summary_report.logger = logger | |||
There was a problem hiding this comment.
Why are you assigning the logger into a field of a SummaryReport instance? So far, we've made loggers as class/module-level global objects because they're just about capturing and printing information from noteworthy events in your code. They're not data that has inherent semantic meaning that needs to be stored, represented, passed around, etc. Also, the SummaryReport class doesn't have a logger field.
| @@ -130,10 +131,12 @@ def run_plan(self): | |||
|
|
|||
| if self.options.run_limit: | |||
| self.run_limit = int(self.options.run_limit) | |||
| logging.debug('!!! RUN LIMIT SET: %d', self.run_limit) | |||
| if self.debug: | |||
There was a problem hiding this comment.
To preface, I'm okay with using self.debug to conditionally run code because you regularly want to test and debug things differently locally vs. run things efficiently during a proper production run.
But using that debug field for conditional printing is an anti-pattern that we already replaced by using a logger, setting the logging level for each logging statement, and configuring the log levels appropriately for both the console output & what gets stored to a file.
There are a lot of instances throughout the PR with the pattern "if debug then print (via logging)". We should get rid of the "if debug" part in those places. That's not a loss because viewing the full debug level log output can be done via the file. It's easiest to view the log file in VS Code, which we've seen updates it's view of a file's contents in real time and nicely colorizes log file lines based on the log level of the line.
| @@ -449,7 +447,7 @@ def main(args): | |||
| base_folders, test_types, result_folders = process_args(args) | |||
|
|
|||
| logger = logging.Logger("TEST_GENERATE LOGGER") | |||
| logger.setLevel(logging.INFO) | |||
| logger.setLevel(logging.WARNING) | |||
There was a problem hiding this comment.
By hard-coding the logger level for this file to be WARNING, and given that most of the logging statements here are either DEBUG or INFO, you're effectively turning off all of the logging happening in this module. Is that your intention?
| @@ -18,11 +18,12 @@ | |||
|
|
|||
| from testplan import TestPlan | |||
|
|
|||
| logger = logging.Logger("TEST DRIVER LOGGER") | |||
| logger.setLevel(logging.WARNING) | |||
There was a problem hiding this comment.
Unless there's a good reason, we should not manually (hardcode) the logger level in code. Instead, we should allow the logger level to be set by our existing config file. So here & other places in the PR, where you see logger.setLevel(...), we should remove those statements.
This removes excessive debug output from conformance test logs.