sousinha1997 / Quisby

Process benchmarks results and automatically add to google sheets
GNU General Public License v3.0
3 stars 3 forks source link

Pquisby logging (0.0.27) is problematic #25

Open dbutenhof opened 11 months ago

dbutenhof commented 11 months ago

With Pquisby 0.0.27 we've noticed two changes:

  1. Creating a ~/.pquisby/pquisby.log file (of which we'd been unaware until we got a failure creating it during containerized testing, though this doesn't happen usually).
  2. We direct our logs through rsyslogd to the RDU2 OpenSearch dashboard for review, and this week we're seeing doubled log entries, one with our normal format and one with a simplified format.

Looking at the (installed) 0.0.27 code, I can see that

  1. logging_configure.configure_logging() is called unconditionally when post_processing.py is imported;
  2. The logging configure sets up both a RotatingFileHandler to ${HOME}/.pquisby/pquisby.log and a StreamHandler with default arguments, which will log to stderr, and installs both on the process root logger with logging.basicConfig.

While the file handler isn't a problem on its own, I think you really should be doing this on your own Pquisby logger object, not on the global root logger which affects every (Python) logger in the process. Copying Pbench logs into ~/.pquisby/pquisby.log doesn't help anyone! And also logging to stderr means that, in a containerized environment, every log is being written three times: to syslog, to your rotating log file, and to stderr. And, in particular, this accounts for the doubled logs we're seeing on OpenSearch, which is highly undesirable.

Please don't do this! Pquisby can log its own internal messages to a file and/or to stderr if desired, but don't modify the process root logger, which affects everyone else in the process as well. Ideally, something like logging.getLogger("pquisby") at runtime (rather than import time), and add your handler(s) to that rather than to the root logger.

sousinha97 commented 11 months ago

Okay got it dave, let me make this configurable !

On Sat, Oct 14, 2023 at 6:53 PM David Butenhof @.***> wrote:

With Pquisby 0.0.27 we've noticed two changes:

  1. Creating a ~/.pquisby/pquisby.log file (of which we'd been unaware until we got a failure creating it during containerized testing, though this doesn't happen usually).
  2. We direct our logs through rsyslogd to the RDU2 OpenSearch dashboard for review, and this week we're seeing doubled log entries, one with our normal format and one with a simplified format.

Looking at the (installed) 0.0.27 code, I can see that

  1. logging_configure.configure_logging() is called unconditionally when post_processing.py is imported;
  2. The logging configure sets up both a RotatingFileHandler to ${HOME}/.pquisby/pquisby.log and a StreamHandler with default arguments, which will log to stderr, and installs both on the process root logger with logging.basicConfig.

While the file handler isn't a problem on its own, I think you really should be doing this on your own Pquisby logger object, not on the global root logger which affects every (Python) logger in the process. Copying Pbench logs into ~/.pquisby/pquisby.log doesn't help anyone! And also logging to stderr means that, in a containerized environment, every log is being written three times: to syslog, to your rotating log file, and to stderr. And, in particular, this accounts for the doubled logs we're seeing on OpenSearch, which is highly undesirable.

Please don't do this! Pquisby can log its own internal messages to a file and/or to stderr if desired, but don't modify the process root logger, which affects everyone else in the process as well. Ideally, something like logging.getLogger("pquisby") at runtime (rather than import time), and add your handler(s) to that rather than to the root logger.

— Reply to this email directly, view it on GitHub https://github.com/sousinha1997/Quisby/issues/25, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXBEZZGI4GB4LGPG44LX5ODX7KG6JANCNFSM6AAAAAA6AIKVBM . You are receiving this because you are subscribed to this thread.Message ID: @.***>