Refactors Logging to use the zapr implementation of go-logr/logr #126
Conversation
openshift-ci-robot
added
the
size/XXL
There was a problem hiding this comment.
This is pretty cool, few things:
- Please split the dependency changes (
vendor/,Gopkg.*) into a separate commit - Is it okay to use spaces in key names, like
zone id? Any benefit in starting some convention around names there (e.g. alpha, -, _ only)? - I don't see where you've introduced a controller-runtime SetLogger call, which is needed to get the framework logs flowing
|
@ironcladlou thank you for the feedback. I'll split the dep changes to a separate commit.
The interface doc states nothing about the use of spaces in key names. The zapr example uses a space in the key name. I also found the controller-runtime uses a space in the following key name:
I'll update the PR with the controller-runtime |
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
pkg/log/log.go
Outdated
|
|
||
| // Set a concrete logging implementation for all | ||
| // controller-runtime deferred Loggers. | ||
| log.SetLogger(Logger) |
There was a problem hiding this comment.
Kind of a nit, but init() can be difficult to reason about. Alternative is to extract SetLogger() out to the entrypoint in main.go... as it stands, controller-runtime logging will only be wired if you happen to import our internal logging package, and if you wanted to reconfigure controller-runtime logging in a different context (e.g. tests) you would have to be careful about package loading order relative to your own SetLogger call which will compete with package init.
There was a problem hiding this comment.
Thanks for your feedback. I'm working on refactoring using a slightly different approach that you suggestion. I look forward to seeing what you have to say.
danehans
force-pushed
the
zapr
branch
2 times, most recently
from
b53b322 to
3c6a52d
Compare
| @@ -38,6 +37,13 @@ const ( | |||
| ClusterIngressFinalizer = "ingress.openshift.io/default-cluster-ingress" | |||
| ) | |||
|
|
|||
| var log = logf.Logger.WithName("cluster-ingress-controller") | |||
|
|
|||
| func init() { | |||
There was a problem hiding this comment.
This seems to shift the problem around. Use case: we decide to add a new controller in a sibling package (we actually want to do this for status calc). Either we duplicate the init boilerplate in the new controller package (unnecessary and conflicting calls to SetRuntimeLogger), or contexts in which it would make sense to use just one of the controllers (a test) would have to import this other controller's package to enable logging (confusing).
If logging is wired at a layer above the controller-runtime consumers (e.g. controllers), say at the entrypoint, or maybe the operator package, then we can freely add new controller-runtime consumers without having to deal with package-scoped wiring.
There was a problem hiding this comment.
Ah... that makes sense. I thought I would have to call SetLogger in the controller pkg too, but I was wrong.
|
/test e2e-aws-operator |
|
/retest |
|
Thanks for making such quick work of this! Can tweak in followups (e.g. the weird filename logging quirk). /approve |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, ironcladlou 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 |
openshift-ci-robot
added
the
approved
|
@ironcladlou here are the zapr logs with the calling function's file name and line number removed from messages. |
|
/retest |
The current logging implementation (Logrus), does not implement the logr interface used by controller-runtime. This PR refactors logging to use the zapr implementation of logr so cluster-ingress-operator logging is compatible with controller-runtime logging.