Conversation
4264ac5
to
ee2ed44
Compare
#96 is merged, rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just one suggestion.
.PHONY: start | ||
start: | ||
@GO111MODULE=on go run \ | ||
-mod=vendor \ | ||
-ldflags $(LD_FLAGS) \ | ||
./cmd/gardener-resource-manager \ | ||
--zap-devel=$(ZAP_DEVEL) \ | ||
--zap-log-level=$(ZAP_LOG_LEVEL) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that this isn't your implementation but I somehow don't like the inconsistency between named log levels here and int
levels here.
- The user doesn't know which log levels exist (could be part of the cmd help)
- As a developer using
log.V(1).Info
orlog.Info
it's unclear at which log level which entry is printed.
Do you think it makes sense to either (1) mask the --zap-log-level
and use --v
with an Integer which maps back to a zap-log-level log level or (2) use a constant instead of Integers in the code, e.g. log.V(1).Info --> log.V(Debug).Info
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed this with @timuthy on the phone:
We both find the current implementation to be a bit confusing, especially the stuff with the level inversion that is going on in the zap options in c-r and in zapr.
We think it makes sense to check, how other logging implementations are handling the log levels, e.g. the new klog implementation.
It probably makes sense to add own flags (like --v
) to a) be flexible to change the underlying logging library one day without breaking compatibility and b) make it more aligned with kubernetes components.
In general, it might also make sense to do similar refactorings in the extensions and also gardener itself and have everything consistent.
So let's look into this a bit, so we can have a bit of a guideline for this later on, before we end up with a zoo of different ways to handle logging in our components :D
/author-action
/needs changes
@timebertt The pull request was assigned to you under |
I evaluated our options wrt choosing a logging library together with @timuthy again. Although, logging implementations in zapr, klog, klogr, controller-runtime and all the plumbing in between are kind of confusing, we reached to the following conclusion: This brings several advantages and solves some challenges:
The problem with this currently is, that the new structured logging options in component-base and the other k8s libraries are only available from v1.19 on, which means c-r would also need to be upgraded to v0.7.0, which is not done in g/g yet (ref gardener/gardener#3109). I would propose to close this PR for now and revisit it, once gardener/gardener#3109 is done. /close |
How to categorize this PR?
/area logging usability
/kind enhancement
/priority normal
What this PR does / why we need it:
This PR adds the following flags:
--zap-log-level
(defaults toinfo
): configures the log level--zap-devel
(defaults tofalse
meaning json encoder,info
level): if set to true, will turn on development/console encoding anddebug
log levelBoth options can also be set via make variables during
make start
.Additionally, this PR cleans up logging across the whole codebase in order to make full use of structured logging and the newly configurable log level.
Which issue(s) this PR fixes:
Fixes #66
Special notes for your reviewer:
Release note: