log message verbosity #106978
log message verbosity #106978
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
sig/cloud-provider
| } | ||
| klog.InfoS("Found related ReplicaSets", "replicaSet", klog.KObj(rs), "relatedReplicaSets", relatedNames) | ||
| } | ||
| klog.V(2).InfoS("Found related ReplicaSets", "replicaSet", klog.KObj(rs), "relatedReplicaSets", klog.KObjs(relatedRSs)) |
There was a problem hiding this comment.
klog.KObjs does not produce exactly the same output but that should be fine because it works better with JSON.
I am worried that it might cause overhead when the log message is not actually emitted (see #106945).
Therefore here the prior "enabled" check might make sense even when using klog.KObjs.
There was a problem hiding this comment.
Seems definitely a valid concern, should we try to prioritize the optimization before we push this out? It definitely could have a large impact in terms of resource consumption
There was a problem hiding this comment.
In other places klog.KObjs was used without outer if check. But I myself also lean towards keeping the check until klog.KObj can be shown to not have a performance overhead when called unconditionally.
|
We need @liggitt for approval. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/OWNERS has a very narrow set of approvers and @thockin's approval wasn't enough. |
|
/skip |
k8s-ci-robot
added
the
approved
pohly
force-pushed
the
log-message-verbosity
branch
from
570fae7
to
24ba370
Compare
k8s-ci-robot
removed
the
lgtm
|
Rebased because of a conflict in pkg/kubelet/pleg/generic.go. |
k8s-ci-robot
added
the
sig/api-machinery
pohly
force-pushed
the
log-message-verbosity
branch
from
24ba370
to
ac455f3
Compare
|
/remove-sig api-machinery |
k8s-ci-robot
added
triage/accepted
In the following code pattern, the log message will get logged with v=0 in JSON
output although conceptually it has a higher verbosity:
if klog.V(5).Enabled() {
klog.Info("hello world")
}
Having the actual verbosity in the JSON output is relevant, for example for
filtering out only the important info messages. The solution is to use
klog.V(5).Info or something similar.
Whether the outer if is necessary at all depends on how complex the parameters
are. The return value of klog.V can be captured in a variable and be used
multiple times to avoid the overhead for that function call and to avoid
repeating the verbosity level.
pohly
force-pushed
the
log-message-verbosity
branch
from
ac455f3
to
9eaa2dc
Compare
k8s-ci-robot
added
the
sig/api-machinery
|
@pohly: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/skip |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, pohly, thockin 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 |
|
looks like the |
|
/lgtm |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What type of PR is this?
/kind bug
What this PR does / why we need it:
In the following code pattern, the log message will get logged with v=0 in JSON
output although conceptually it has a higher verbosity:
Having the actual verbosity in the JSON output is relevant, for example for
filtering out only the important info messages. The solution is to use
klog.V(5).Infoor something similar.Whether the outer if is necessary at all depends on how complex the parameters
are. The return value of
klog.Vcan be captured in a variable and be used multipletimes to avoid the overhead for that function call and to avoid repeating
the verbosity level.
Which issue(s) this PR fixes:
Related-to: #102430
Special notes for your reviewer:
git grep 'klog.V([0-9]*).Enabled' cmd/ pkg/ stagingcan be used to find code locations that may have to be modified.Does this PR introduce a user-facing change?