⚠️ Remove logs from internal controller #1096
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
k8s-ci-robot
added
approved
vincepri
changed the title
|
lgtm, I don't have any particular preference related to which version to target for this change. |
| @@ -234,7 +234,9 @@ func (c *Controller) reconcileHandler(obj interface{}) bool { | |||
| // resource to be synced. | |||
| if result, err := c.Do.Reconcile(req); err != nil { | |||
| c.Queue.AddRateLimited(req) | |||
| log.Error(err, "Reconciler error") | |||
| if log.V(3).Enabled() { | |||
There was a problem hiding this comment.
For successes I understand the change but for errors?
There was a problem hiding this comment.
There was some discussion on the other PR, to have the error only logged on higher verbosity, chances are that folks using loggers are already logging errors on their own.
There was a problem hiding this comment.
hances are that folks using loggers are already logging errors on their own.
Maybe? We don't know that and only the ppl that do use their own logger will speak up about this being too noisy. Also, the general direction we are going is to get ppl to use the integrated loggers or not?
IMHO this is pretty much the definition of an error so increasing the verbosity seems wrong to me.
There was a problem hiding this comment.
Another potential option would be to extend the current use of context w/ logger so that a calling controller can more easily modify the context and have the errors logging here use the logger from the context.
Then we could avoid doing the error logging in the called code (which we currently prefer for the more detailed information it provides), and have the logging here be done with the additional context that is provided by the called reconciler.
That would save having to remember to log errors everywhere on the error path for reconcilers, avoid losing context related to the logging.
Main downside is that it would still potentially log the wrong filename/line number for the error. I'm not sure if there is a better way to handle that generically, though.
There was a problem hiding this comment.
I guess my point is that we pretty much don't know if and how ppl use these today and I think if we require debug logging to be enabled to see them, then we can also remove those log statements altogether (but probably for 0.7).
Then we could avoid doing the error logging in the called code (which we currently prefer for the more detailed information it provides), and have the logging here be done with the additional context that is provided by the called reconciler.
So basically tell ppl to put the logger back into the ctx if they add additional fields and then re-use that logger here to get these fields?
Main downside is that it would still potentially log the wrong filename/line number for the error. I'm not sure if there is a better way to handle that generically, though.
Do you have that today? For most (all?) controllers I have worked on there is a function call in the very beginning of the reconciler precicely to be able to log all errors. That log line doesn't really have a useful line number associated.
There was a problem hiding this comment.
My desire/goal/requirement is that when an error is logged, you have the file name and line number as close to where the error was created as possible. As things currently stand, this log line emits a file and line number that is about as far away from the error's source as possible. I find little to no value in this log message in its current form.
If there is some way for this log invocation to include the error's provenance, then I'm all for keeping it. Otherwise, my vote is to remove it.
There was a problem hiding this comment.
Removed it, and increased the info log to 5
vincepri
force-pushed
the
increase-verbosity
branch
from
b820e3a to
e4e8977
Compare
vincepri
changed the title
|
/milestone v0.7.x |
|
@alvaroaleman This should be good to go |
|
@vincepri I think we should either remove both of those or keep both, not logging on error but logging on success does not really make sense |
k8s-ci-robot
added
the
needs-rebase
|
I'm fine removing both |
Signed-off-by: Vince Prignano <[email protected]>
vincepri
force-pushed
the
increase-verbosity
branch
from
e4e8977 to
d10e451
Compare
vincepri
changed the title
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
the
lgtm
|
/test pull-controller-runtime-test-master |
|
/retest |
|
For posterity, this was re-introduced in #1245 |
Reconciliation errors are going to be logged only if V >= 3,
while success reconciliations are set for V >= 5.
Signed-off-by: Vince Prignano [email protected]
/assign @alvaroaleman @detiber⚠️ , but we can probably merge in v0.6.2 with a release note for the impact in case we want to.
Set this as a