Create and use Custom logger, not modifying root looging module #32
Conversation
There was a problem hiding this comment.
Aside from a few style comments, this looks like it'll work, so I'll approve. Getting this change in should remove the duplicate logs that have been bothering us in our server ops review.
The main potential downside is that quisby logging messages will only be in the local ~/.pquisby/pquisby.log file, which means that if we see problems in Opensearch we'll need to log in to the container and hope that the container hasn't been restarted since the problem occurred.
I think I would have preferred that you leave your logger setup as it was, but only on your own getLogger() instance rather than the root logger, so that it wouldn't affect other logging in the process but would still capture Quisby logs (through stderr/conmon) for analysis without having to find your local log file.
| if not os.path.exists(log_location): | ||
| os.makedirs(log_location) | ||
| log_filename = log_location + "pquisby.log" | ||
| log_file_max_bytes = 5 |
There was a problem hiding this comment.
Since you're defining this as a local variable, I don't see how there's any advantage over simply using maxBytes=5 * 1024 * 1024 below, which would also clarify the units you're using ... maxBytes isn't really a good description since it represents "megabytes".
The same goes for log_backup_count. Separating configuration constants out at the top of the file, or in a separate file, can make sense; but in this case when both are local variables anyway I think just using the values in context would be easier to read.
| # Create a stream handler and add it to the custom logger | ||
| # stream_handler = logging.StreamHandler() | ||
| # stream_handler.setLevel(log_level) | ||
| # stream_handler.setFormatter(formatter) | ||
| # custom_logger.addHandler(stream_handler) |
There was a problem hiding this comment.
Do you want to merge the commented code? This is your code, but I'd normally recommend against that because it's just confusing for future readers who don't know why it was commented.
| custom_logger.info("********************** Starting preprocessing of data **********************") | ||
| custom_logger.info("Input Type: " + str(input_type)) | ||
| custom_logger.info("Benchmark Type: " + str(test_name)) | ||
| custom_logger.info("Dataset Name: " + str(dataset_name)) |
There was a problem hiding this comment.
I've noticed these messages, and it makes your log output extremely "noisy". That's less of an issue now that it'll be restricted to your ~/.pquisby/pquisby.log; but do you really need this, with the *** header, for each call?
| @@ -0,0 +1,5 @@ | |||
| from pquisby.lib.logging_configure.custom_logging import configure_custom_logging | |||
|
|
|||
| custom_logger = configure_custom_logging() | |||
There was a problem hiding this comment.
This is still being configured at import time rather than at runtime. It's less important now since your custom_logger won't affect anything but Quisby code; but that still seems odd to me.
No description provided.