logger: add logLevel to DSCI devFlags #1307
Conversation
|
/cc @lburgazzoli @zdtsw |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## incubation #1307 +/- ##
=============================================
Coverage ? 17.48%
=============================================
Files ? 30
Lines ? 3362
Branches ? 0
=============================================
Hits ? 588
Misses ? 2711
Partials ? 63 ☔ View full report in Codecov by Sentry. |
ykaliuta
force-pushed
the
logger-dsci-override
branch
from
358269a
to
d98f3e1
Compare
Jira: https://issues.redhat.com/browse/RHOAIENG-14713 This is a squashed commit of the patchset (original ids): 959f84d ("logger: import controller-runtime zap as ctrlzap") b8d9cde ("logger: add logLevel to DSCI devFlags") 2d3efe2 ("components, logger: always use controller's logger") 1ef9266 ("components, logger: use one function for ctx creation") - logger: import controller-runtime zap as ctrlzap To avoid patch polluting with the next changes where uber's zap is imported as `zap`. - logger: add logLevel to DSCI devFlags Allow to override global zap log level from DSCI devFlags. Accepts the same values as to `--zap-log-level` command line switch. Use zap.AtomicLevel type to store loglevel. Locally use atomic to store its value. We must store the structure itsel since command line flags parser (ctrlzap.(*levelFlag).Set()) stores the structure there. In its order ctrlzap.addDefault() stores pointers, newOptions() always sets the level to avoid setting it by addDefaults(). Otherwise SetLevel should handle both pointer and the structure as logLevel atomic.Value. It is ok to store structure of zap.AtomicLevel since it itself contains a pointer to the atomic.Int32 and common level value is changed then. The parsing code is modified version of the one from controller-runtime/pkg/log/zap/flag.go. Deprecate DSCI.DevFlags.logmode. - components, logger: always use controller's logger Since the log level is overridable with its own field of devFlags, do not use logmode anymore. It was used to create own logger with own zap backend in case if devFlags exist. Just add name and value to the existing logger instead. Rename NewLoggerWithOptions back to NewLogger since former NewLogger is removed. Change component logger name. "DSC.Component" is redundant. It was usuful when component's logger was created from scratch, but now when it is always based on the reconciler's one, it's clear that it is a part of DSC. - components, logger: use one function for ctx creation Move both logger and component creation to one function. Just a cleanup. Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
ykaliuta
force-pushed
the
logger-dsci-override
branch
from
d98f3e1
to
a9be965
Compare
| Update DSCI CR with .spec.devFlags.logmode, see example : | ||
| Log level can be changed by DSCI devFlags during runtime by setting | ||
| .spec.devFlags.logLevel. It accepts the same values as `--zap-log-level` | ||
| command line switch. See example : |
There was a problem hiding this comment.
to add the default case is the same as production if not set it at all?
There was a problem hiding this comment.
Default log level is INFO, defined by logmode and #1289 . Or what do you mean?
There was a problem hiding this comment.
with the old logmode the default is set to INFO, but if this PR is to add new loglevel and have logmode deprecated, should it somehow address more what loglevel can be used in README?
if i am a user, i wont build my binanary to add --zap-log-level in the code, isntead i intend to set in DSCI.
there is nothing telling me what i should set except from one example with "debug" can be used .spec.devFlags.logLevel in.
There was a problem hiding this comment.
Ah, ok. Well, at the moment the levels are not different. May be we will add details when revisit the messages?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zdtsw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
1 similar comment
|
/retest-required |
openshift-merge-bot
bot
merged commit 3427b72
into
opendatahub-io:incubation
Jira: https://issues.redhat.com/browse/RHOAIENG-14713
This is a squashed commit of the patchset (original ids):
959f84d ("logger: import controller-runtime zap as ctrlzap")
7b8e988 ("logger: add logLevel to DSCI devFlags")
2d3efe2 ("components, logger: always use controller's logger")
1ef9266 ("components, logger: use one function for ctx creation")
To avoid patch polluting with the next changes where uber's zap is imported as
zap.Allow to override global zap log level from DSCI devFlags. Accepts the same values as to
--zap-log-levelcommand line switch.Use zap.AtomicLevel type to store loglevel. Locally use atomic to store its value. We must store the structure itsel since command line flags parser (ctrlzap.(*levelFlag).Set()) stores the structure there. In its order ctrlzap.addDefault() stores pointers, newOptions() always sets the level to avoid setting it by addDefaults(). Otherwise SetLevel should handle both pointer and the structure as logLevel atomic.Value.
It is ok to store structure of zap.AtomicLevel since it itself contains a pointer to the atomic.Int32 and common level value is changed then.
The parsing code is modified version of the one from controller-runtime/pkg/log/zap/flag.go.
Deprecate DSCI.DevFlags.logmode.
Since the log level is overridable with its own field of devFlags, do not use logmode anymore. It was used to create own logger with own zap backend in case if devFlags exist.
Just add name and value to the existing logger instead.
Rename NewLoggerWithOptions back to NewLogger since former NewLogger is removed.
Change component logger name. "DSC.Component" is redundant. It was usuful when component's logger was created from scratch, but now when it is always based on the reconciler's one, it's clear that it is a part of DSC.
Move both logger and component creation to one function.
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria