From: Bruce Richardson <bruce.richardson@intel.com>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
Cc: <thomas@monjalon.net>, <david.marchand@redhat.com>,
<ronan.randles@intel.com>, <Honnappa.Nagarahalli@arm.com>,
<ohilyard@iol.unh.edu>, <lijuan.tu@intel.com>, <dev@dpdk.org>
Subject: Re: [PATCH v4 3/9] dts: add basic logging facility
Date: Thu, 8 Sep 2022 09:31:07 +0100 [thread overview]
Message-ID: <YxmoS/YmTpFx8Lzh@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20220729105550.1382664-4-juraj.linkes@pantheon.tech>
On Fri, Jul 29, 2022 at 10:55:44AM +0000, Juraj Linkeš wrote:
> The logging module provides loggers distinguished by two attributes,
> a custom format and a verbosity switch. The loggers log to both console
> and more verbosely to files.
>
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Few small comments inline below.
Thanks,
/Bruce
> ---
> dts/framework/__init__.py | 3 +
> dts/framework/logger.py | 124 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 127 insertions(+)
> create mode 100644 dts/framework/__init__.py
> create mode 100644 dts/framework/logger.py
>
> diff --git a/dts/framework/__init__.py b/dts/framework/__init__.py
> new file mode 100644
> index 0000000000..3c30bccf43
> --- /dev/null
> +++ b/dts/framework/__init__.py
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2022 PANTHEON.tech s.r.o.
> +#
> diff --git a/dts/framework/logger.py b/dts/framework/logger.py
> new file mode 100644
> index 0000000000..920ce0fb15
> --- /dev/null
> +++ b/dts/framework/logger.py
> @@ -0,0 +1,124 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2010-2014 Intel Corporation
> +# Copyright(c) 2022 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022 University of New Hampshire
> +#
> +
> +import logging
> +import os.path
> +from typing import TypedDict
> +
> +"""
> +DTS logger module with several log level. DTS framework and TestSuite log
> +will saved into different log files.
> +"""
> +verbose = False
> +date_fmt = "%d/%m/%Y %H:%M:%S"
Please use Year-month-day ordering for dates, since it's unambiguous - as
well as being an ISO standard date format. (ISO 8601)
> +stream_fmt = "%(asctime)s - %(name)s - %(levelname)s - %(message)s"
> +
> +
> +class LoggerDictType(TypedDict):
> + logger: "DTSLOG"
> + name: str
> + node: str
> +
> +
> +# List for saving all using loggers
> +global Loggers
> +Loggers: list[LoggerDictType] = []
> +
> +
> +def set_verbose() -> None:
> + global verbose
> + verbose = True
> +
Is there a need for a clear_verbose() or "set_not_verbose()" API?
> +
> +class DTSLOG(logging.LoggerAdapter):
> + """
> + DTS log class for framework and testsuite.
> + """
> +
> + node: str
> + logger: logging.Logger
> + sh: logging.StreamHandler
> + fh: logging.FileHandler
> + verbose_handler: logging.FileHandler
> +
> + def __init__(self, logger: logging.Logger, node: str = "suite"):
> + global log_dir
> +
> + self.logger = logger
> + self.logger.setLevel(1) # 1 means log everything
> +
> + self.node = node
> +
> + # add handler to emit to stdout
> + sh = logging.StreamHandler()
> + sh.setFormatter(logging.Formatter(stream_fmt, date_fmt))
> +
> + sh.setLevel(logging.DEBUG) # file handler default level
> + global verbose
> + if verbose is True:
> + sh.setLevel(logging.DEBUG)
> + else:
> + sh.setLevel(logging.INFO) # console handler defaultlevel
The global should be defined at the top of the function.
Looks like some of the setlevel calls are unnecessary; two should be enough
rather than three. For example:
sh.setLevel(logging.INFO)
if verbose:
sh.setLevel(logging.DEGUG)
> +
> + self.logger.addHandler(sh)
> + self.sh = sh
> +
> + if not os.path.exists("output"):
> + os.mkdir("output")
> +
> + fh = logging.FileHandler(f"output/{node}.log")
> + fh.setFormatter(
> + logging.Formatter(
> + fmt="%(asctime)s - %(name)s - %(levelname)s - %(message)s",
> + datefmt=date_fmt,
> + )
> + )
> +
> + fh.setLevel(1) # We want all the logs we can get in the file
> + self.logger.addHandler(fh)
> + self.fh = fh
> +
> + # This outputs EVERYTHING, intended for post-mortem debugging
> + # Also optimized for processing via AWK (awk -F '|' ...)
> + verbose_handler = logging.FileHandler(f"output/{node}.verbose.log")
> + verbose_handler.setFormatter(
> + logging.Formatter(
> + fmt="%(asctime)s|%(name)s|%(levelname)s|%(pathname)s|%(lineno)d|%(funcName)s|"
> + "%(process)d|%(thread)d|%(threadName)s|%(message)s",
> + datefmt=date_fmt,
> + )
> + )
> +
> + verbose_handler.setLevel(1) # We want all the logs we can get in the file
> + self.logger.addHandler(verbose_handler)
> + self.verbose_handler = verbose_handler
> +
> + super(DTSLOG, self).__init__(self.logger, dict(node=self.node))
> +
> + def logger_exit(self) -> None:
> + """
> + Remove stream handler and logfile handler.
> + """
> + for handler in (self.sh, self.fh, self.verbose_handler):
> + handler.flush()
> + self.logger.removeHandler(handler)
> +
> +
> +def getLogger(name: str, node: str = "suite") -> DTSLOG:
> + """
> + Get logger handler and if there's no handler for specified Node will create one.
> + """
> + global Loggers
> + # return saved logger
> + logger: LoggerDictType
> + for logger in Loggers:
> + if logger["name"] == name and logger["node"] == node:
> + return logger["logger"]
> +
> + # return new logger
> + dts_logger: DTSLOG = DTSLOG(logging.getLogger(name), node)
> + Loggers.append({"logger": dts_logger, "name": name, "node": node})
> + return dts_logger
Looking through this patch alone, I see the "verbose" global only seems to
be used to set the log-level in the logger init function. If this is the
only use of it across all the patches in the set, it may be more readable
to change the variable from a "verbose" flag, to instead being a log-level
one. That way your global define is:
log_level = logging.INFO
and set_verbose() becomes:
global log_level
log_level = logging.DEBUG
thereby removing the branch from you logging init fn.
NOTE: I have not yet had the chance to review the rest of the series, so if
verbose is used elsewhere, please ignore this comment.
> --
> 2.30.2
>
next prev parent reply other threads:[~2022-09-08 8:31 UTC|newest]
Thread overview: 105+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-22 12:14 [PATCH v1 0/8] dts: ssh connection to a node Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 1/8] dts: add ssh pexpect library Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 2/8] dts: add locks for parallel node connections Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 3/8] dts: add ssh connection extension Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 4/8] dts: add basic logging facility Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 5/8] dts: add Node base class Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 6/8] dts: add config parser module Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 7/8] dts: add dts runtime workflow module Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 8/8] dts: add main script for running dts Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 0/8] ssh connection to a node Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 1/8] dts: add basic logging facility Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 2/8] dts: add ssh pexpect library Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 3/8] dts: add locks for parallel node connections Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 4/8] dts: add ssh connection extension Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 5/8] dts: add config parser module Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 6/8] dts: add Node base class Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 7/8] dts: add dts workflow module Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 8/8] dts: add dts executable script Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 0/9] dts: ssh connection to a node Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 1/9] dts: add project tools config Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 2/9] dts: add developer tools Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 3/9] dts: add basic logging facility Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 4/9] dts: add ssh pexpect library Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 5/9] dts: add ssh connection extension Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 6/9] dts: add config parser module Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 7/9] dts: add Node base class Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 8/9] dts: add dts workflow module Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 9/9] dts: add dts executable script Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 0/9] dts: ssh connection to a node Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 1/9] dts: add project tools config Juraj Linkeš
2022-08-10 6:30 ` Tu, Lijuan
2022-09-07 16:16 ` Bruce Richardson
2022-09-09 13:38 ` Juraj Linkeš
2022-09-09 13:52 ` Bruce Richardson
2022-09-09 14:13 ` Juraj Linkeš
2022-09-12 14:06 ` Owen Hilyard
2022-09-12 15:15 ` Bruce Richardson
2022-09-13 12:08 ` Juraj Linkeš
2022-09-13 14:18 ` Bruce Richardson
2022-09-13 19:03 ` Honnappa Nagarahalli
2022-09-13 19:19 ` Honnappa Nagarahalli
2022-09-14 9:37 ` Thomas Monjalon
2022-09-14 12:55 ` Juraj Linkeš
2022-09-14 13:11 ` Bruce Richardson
2022-09-14 14:28 ` Thomas Monjalon
2022-09-21 10:49 ` Juraj Linkeš
2022-09-13 19:11 ` Honnappa Nagarahalli
2022-07-29 10:55 ` [PATCH v4 2/9] dts: add developer tools Juraj Linkeš
2022-08-10 6:30 ` Tu, Lijuan
2022-09-07 16:37 ` Bruce Richardson
2022-09-13 12:38 ` Juraj Linkeš
2022-09-13 20:38 ` Honnappa Nagarahalli
2022-09-14 7:37 ` Bruce Richardson
2022-09-14 12:45 ` Juraj Linkeš
2022-09-14 13:13 ` Bruce Richardson
2022-09-14 14:26 ` Thomas Monjalon
2022-09-14 19:08 ` Honnappa Nagarahalli
2022-09-20 12:14 ` Juraj Linkeš
2022-09-20 12:22 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 3/9] dts: add basic logging facility Juraj Linkeš
2022-08-10 6:31 ` Tu, Lijuan
2022-09-08 8:31 ` Bruce Richardson [this message]
2022-09-13 12:52 ` Juraj Linkeš
2022-09-13 23:31 ` Honnappa Nagarahalli
2022-09-14 12:51 ` Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 4/9] dts: add ssh pexpect library Juraj Linkeš
2022-08-10 6:31 ` Tu, Lijuan
2022-09-08 9:53 ` Bruce Richardson
2022-09-13 13:36 ` Juraj Linkeš
2022-09-13 14:23 ` Bruce Richardson
2022-09-13 14:59 ` Stanislaw Kardach
2022-09-13 17:23 ` Owen Hilyard
2022-09-14 0:03 ` Honnappa Nagarahalli
2022-09-14 7:42 ` Bruce Richardson
2022-09-14 7:58 ` Stanislaw Kardach
2022-09-14 19:57 ` Honnappa Nagarahalli
2022-09-19 14:21 ` Owen Hilyard
2022-09-20 17:54 ` Honnappa Nagarahalli
2022-09-21 1:01 ` Tu, Lijuan
2022-09-21 5:37 ` Jerin Jacob
2022-09-22 9:03 ` Juraj Linkeš
2022-09-14 9:42 ` Stanislaw Kardach
2022-09-22 9:41 ` Juraj Linkeš
2022-09-22 14:32 ` Stanislaw Kardach
2022-09-23 7:22 ` Juraj Linkeš
2022-09-23 8:15 ` Bruce Richardson
2022-09-23 10:18 ` Stanislaw Kardach
2022-07-29 10:55 ` [PATCH v4 5/9] dts: add ssh connection extension Juraj Linkeš
2022-08-10 6:32 ` Tu, Lijuan
2022-09-13 17:04 ` Bruce Richardson
2022-09-13 17:32 ` Owen Hilyard
2022-09-14 7:46 ` Bruce Richardson
2022-09-14 12:02 ` Owen Hilyard
2022-09-14 13:15 ` Bruce Richardson
2022-07-29 10:55 ` [PATCH v4 6/9] dts: add config parser module Juraj Linkeš
2022-08-10 6:33 ` Tu, Lijuan
2022-09-13 17:19 ` Bruce Richardson
2022-09-13 17:47 ` Owen Hilyard
2022-09-14 7:48 ` Bruce Richardson
2022-07-29 10:55 ` [PATCH v4 7/9] dts: add Node base class Juraj Linkeš
2022-08-10 6:33 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 8/9] dts: add dts workflow module Juraj Linkeš
2022-08-10 6:34 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 9/9] dts: add dts executable script Juraj Linkeš
2022-08-10 6:35 ` Tu, Lijuan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YxmoS/YmTpFx8Lzh@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=juraj.linkes@pantheon.tech \
--cc=lijuan.tu@intel.com \
--cc=ohilyard@iol.unh.edu \
--cc=ronan.randles@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).