Add option to set logging level #795
Conversation
mingkang111
requested review from
VipulMascarenhas,
darenr,
mayoor,
mrDzurb and
qiuosier
as code owners
oracle-contributor-agreement
bot
added
the
OCA Verified
mingkang111
force-pushed
the
feature/aquav1.0.1
branch
from
4c9c349 to
4a6bb03
Compare
mingkang111
force-pushed
the
feature/set_logging_level
branch
from
4450c8b to
e62cb3c
Compare
mingkang111
force-pushed
the
feature/set_logging_level
branch
from
dfe60f2 to
5117acd
Compare
| return f.read() | ||
| except Exception as e: | ||
| logger.error(f"Failed to read file {file_path}. {e}") | ||
| logger.debug(f"Failed to read file {file_path}. {e}") |
There was a problem hiding this comment.
Why changing it from error to debug?
There was a problem hiding this comment.
Because when read_file failed to find file, it will not break the main functionality, which might lead to numerous unnecessary log entries at the error level. This could fill the console with non-critical error messages, potentially masking more significant issues. This change could help keep the log output more relevant and manageable.
| # TODO: allow the user to setup the logging level? | ||
| logging.basicConfig(stream=sys.stdout, level=logging.INFO) | ||
| logger = logging.getLogger("ODSC_AQUA") | ||
| logger = logging.getLogger("ads.aqua") |
There was a problem hiding this comment.
We can import this logger from ads.aqua?
There was a problem hiding this comment.
Hi QQ, import this logger from ads.aqua causing circular import during testing. So I use getLogger at here instead.
| logger = logging.getLogger(__name__) | ||
| handler = logging.StreamHandler(sys.stdout) | ||
| logger.setLevel(logging.INFO) | ||
| ENV_VAR_LOG_LEVEL = "ADS_AQUA_LOG_LEVEL" |
There was a problem hiding this comment.
Just curious. Do we expect in the future to have a separate env var for every package? Wouldn't it be better to have one ENV var for entire ADS (ADS_LOG_LEVEL), and utilize it everywhere in the future?
There was a problem hiding this comment.
We can do an overhaul separately. I dont want to mislead the users to think that entire ADS respects the env variable. Hence, my suggestion to make it Aqua specific.
| return level | ||
|
|
||
|
|
||
| def configure_aqua_logger(): |
There was a problem hiding this comment.
Can we reuse the logger in ads.init?
| logger = logging.getLogger(__name__) | ||
| handler = logging.StreamHandler(sys.stdout) | ||
| logger.setLevel(logging.INFO) | ||
| ENV_VAR_LOG_LEVEL = "ADS_AQUA_LOG_LEVEL" |
There was a problem hiding this comment.
We can do an overhaul separately. I dont want to mislead the users to think that entire ADS respects the env variable. Hence, my suggestion to make it Aqua specific.
5 participants
Description
This PR aims to provide option to set logging level for AQUA API and CLI.
User experience
CLI: set through flag
--log-level=DEBUGAPI: set through env var
ADS_AQUA_LOG_LEVEL=DEBUGChanged
Testing