Skip to content

Commit

Permalink
enhance and fix log calls
Browse files Browse the repository at this point in the history
Some of these changes are cosmetic (repeatedly calling klog.V instead of
reusing the result), others address real issues:

- Logging a message only above a certain verbosity threshold without
  recording that verbosity level (if klog.V().Enabled() { klog.Info... }):
  this matters when using a logging backend which records the verbosity
  level.

- Passing a format string with parameters to a logging function that
  doesn't do string formatting.

All of these locations where found by the enhanced logcheck tool from
kubernetes/klog#297.

In some cases it reports false positives, but those can be suppressed with
source code comments.

Kubernetes-commit: edffc700a43e610f641907290a5152ca593bad79
  • Loading branch information
pohly authored and k8s-publishing-bot committed Feb 16, 2022
1 parent 6708a88 commit ba3b8e9
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 11 deletions.
5 changes: 3 additions & 2 deletions pkg/registry/generic/registry/storage_factory.go
Expand Up @@ -49,8 +49,9 @@ func StorageWithCacher() generic.StorageDecorator {
if err != nil {
return s, d, err
}
if klog.V(5).Enabled() {
klog.V(5).InfoS("Storage caching is enabled", objectTypeToArgs(newFunc())...)
if klogV := klog.V(5); klogV.Enabled() {
//nolint:logcheck // It complains about the key/value pairs because it cannot check them.
klogV.InfoS("Storage caching is enabled", objectTypeToArgs(newFunc())...)
}

cacherConfig := cacherstorage.Config{
Expand Down
4 changes: 4 additions & 0 deletions pkg/server/filters/priority-and-fairness.go
Expand Up @@ -125,6 +125,10 @@ func WithPriorityAndFairness(
workEstimate := workEstimator(r, classification.FlowSchemaName, classification.PriorityLevelName)

fcmetrics.ObserveWorkEstimatedSeats(classification.PriorityLevelName, classification.FlowSchemaName, workEstimate.MaxSeats())
// nolint:logcheck // Not using the result of klog.V
// inside the if branch is okay, we just use it to
// determine whether the additional information should
// be added.
if klog.V(4).Enabled() {
httplog.AddKeyValue(ctx, "apf_iseats", workEstimate.InitialSeats)
httplog.AddKeyValue(ctx, "apf_fseats", workEstimate.FinalSeats)
Expand Down
10 changes: 5 additions & 5 deletions pkg/storage/etcd3/logger.go
Expand Up @@ -32,19 +32,19 @@ type klogWrapper struct{}
const klogWrapperDepth = 4

func (klogWrapper) Info(args ...interface{}) {
if klog.V(5).Enabled() {
klog.V(5).InfoSDepth(klogWrapperDepth, fmt.Sprint(args...))
if klogV := klog.V(5); klogV.Enabled() {
klogV.InfoSDepth(klogWrapperDepth, fmt.Sprint(args...))
}
}

func (klogWrapper) Infoln(args ...interface{}) {
if klog.V(5).Enabled() {
klog.V(5).InfoSDepth(klogWrapperDepth, fmt.Sprintln(args...))
if klogV := klog.V(5); klogV.Enabled() {
klogV.InfoSDepth(klogWrapperDepth, fmt.Sprintln(args...))
}
}

func (klogWrapper) Infof(format string, args ...interface{}) {
if klog.V(5).Enabled() {
if klogV := klog.V(5); klogV.Enabled() {
klog.V(5).InfoSDepth(klogWrapperDepth, fmt.Sprintf(format, args...))
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/util/flowcontrol/apf_controller.go
Expand Up @@ -434,8 +434,8 @@ func (cfgCtlr *configController) digestConfigObjects(newPLs []*flowcontrol.Prior

// if we are going to issue an update, be sure we track every name we update so we know if we update it too often.
currResult.updatedItems.Insert(fsu.flowSchema.Name)
if klog.V(4).Enabled() {
klog.V(4).Infof("%s writing Condition %s to FlowSchema %s, which had ResourceVersion=%s, because its previous value was %s, diff: %s",
if klogV := klog.V(4); klogV.Enabled() {
klogV.Infof("%s writing Condition %s to FlowSchema %s, which had ResourceVersion=%s, because its previous value was %s, diff: %s",
cfgCtlr.name, fsu.condition, fsu.flowSchema.Name, fsu.flowSchema.ResourceVersion, fcfmt.Fmt(fsu.oldValue), cmp.Diff(fsu.oldValue, fsu.condition))
}
fsIfc := cfgCtlr.flowcontrolClient.FlowSchemas()
Expand Down
4 changes: 2 additions & 2 deletions pkg/util/flowcontrol/fairqueuing/queueset/queueset.go
Expand Up @@ -573,9 +573,9 @@ func (qs *queueSet) shuffleShardLocked(hashValue uint64, descr1, descr2 interfac
bestQueueIdx = queueIdx
}
}
if klog.V(6).Enabled() {
if klogV := klog.V(6); klogV.Enabled() {
chosenQueue := qs.queues[bestQueueIdx]
klog.V(6).Infof("QS(%s) at t=%s R=%v: For request %#+v %#+v chose queue %d, with sum: %#v & %d seats in use & nextDispatchR=%v", qs.qCfg.Name, qs.clock.Now().Format(nsTimeFmt), qs.currentR, descr1, descr2, bestQueueIdx, chosenQueue.requests.QueueSum(), chosenQueue.seatsInUse, chosenQueue.nextDispatchR)
klogV.Infof("QS(%s) at t=%s R=%v: For request %#+v %#+v chose queue %d, with sum: %#v & %d seats in use & nextDispatchR=%v", qs.qCfg.Name, qs.clock.Now().Format(nsTimeFmt), qs.currentR, descr1, descr2, bestQueueIdx, chosenQueue.requests.QueueSum(), chosenQueue.seatsInUse, chosenQueue.nextDispatchR)
}
return bestQueueIdx
}
Expand Down

0 comments on commit ba3b8e9

Please sign in to comment.