forked from golang/glog
-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
logcheck: contextual logging + enhanced checks #297
Merged
k8s-ci-robot
merged 11 commits into
kubernetes:main
from
pohly:logcheck-contextual-logging
Mar 21, 2022
+2,615
−216
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
93980ca
logcheck: add test cases for parameter check
pohly 0c36cf7
logcheck: move into package
pohly 3f08024
logcheck: also check for format specifier in calls like klog.Info
pohly df531eb
logcheck: support running as golangci-lint plugin
pohly c4c35ff
logcheck: support import renaming and improve klog.V(2) handling
pohly 567c037
logcheck: support logr.Logger
pohly 5b4c326
logcheck: warn about functions with both Logger and Context
pohly 41995aa
logcheck: detect if V().Enabled()
pohly 317fe52
logcheck: optionally warn about using direct klog logging
pohly 216a02e
logcheck: add check for With* and NewContext
pohly 1876dee
logcheck: harmonize report messages
pohly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,82 @@ | ||
This directory contains tool for checking use of unstructured logs in a package. It is created to prevent regression after packages have been migrated to use structured logs. | ||
This directory contains a linter for checking log calls. It was originally | ||
created to detect when unstructured logging calls like `klog.Infof` get added | ||
to files that should only use structured logging calls like `klog.InfoS` | ||
and now also supports other checks. | ||
|
||
**Installation:**`go install k8s.io/klog/hack/tools/logcheck` | ||
**Usage:** `$logcheck.go <package_name>` | ||
# Installation | ||
|
||
`go install k8s.io/klog/hack/tools/logcheck` | ||
|
||
# Usage | ||
|
||
`$logcheck.go <package_name>` | ||
`e.g $logcheck ./pkg/kubelet/lifecycle/` | ||
|
||
# Configuration | ||
|
||
Checks can be enabled or disabled globally via command line flags and env | ||
variables. In addition, the global setting for a check can be modified per file | ||
via a configuration file. That file contains lines in this format: | ||
|
||
``` | ||
<checks> <regular expression> | ||
``` | ||
|
||
`<checks>` is a comma-separated list of the names of checks that get enabled or | ||
disabled when a file name matches the regular expression. A check gets disabled | ||
when its name has `-` as prefix and enabled when there is no prefix or `+` as | ||
prefix. Only checks that are mentioned explicitly are modified. All regular | ||
expressions are checked in order, so later lines can override the previous | ||
ones. | ||
|
||
In this example, checking for klog calls is enabled for all files under | ||
`pkg/scheduler` in the Kubernetes repo except for `scheduler.go` | ||
itself. Parameter checking is disabled everywhere. | ||
|
||
``` | ||
klog,-parameters k8s.io/kubernetes/pkg/scheduler/.* | ||
-klog k8s.io/kubernetes/pkg/scheduler/scheduler.go | ||
``` | ||
pohly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
The names of all supported checks are the ones used as sub-section titles in | ||
the next section. | ||
|
||
# Checks | ||
|
||
## structured (enabled by default) | ||
|
||
Unstructured klog logging calls are flagged as error. | ||
|
||
## klog (disabled by default) | ||
pohly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
None of the klog logging methods may be used. This is even stricter than | ||
`unstructured`. Instead, code should retrieve a logr.Logger from klog and log | ||
through that. | ||
|
||
## parameters (enabled by default) | ||
|
||
This ensures that if certain logging functions are allowed and are used, those | ||
functions are passed correct parameters. | ||
|
||
### all calls | ||
|
||
Format strings are not allowed where plain strings are expected. | ||
|
||
### structured logging calls | ||
|
||
Key/value parameters for logging calls are checked: | ||
- For each key there must be a value. | ||
- Keys must be constant strings. | ||
|
||
This also warns about code that is valid, for example code that collects | ||
key/value pairs in an `[]interface` variable before passing that on to a log | ||
call. Such valid code can use `nolint:logcheck` to disable the warning (when | ||
invoking logcheck through golangci-lint) or the `parameters` check can be | ||
disabled for the file. | ||
|
||
## with-helpers (disabled by default) | ||
|
||
`logr.Logger.WithName`, `logr.Logger.WithValues` and `logr.NewContext` must not | ||
be used. The corresponding helper calls from `k8s.io/klogr` should be used | ||
instead. This is relevant when support contextual logging is disabled at | ||
runtime in klog. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't find this configuration format better than
hack/verify-structured-logging.sh
. Have we first looked if it is possible to simplify the script before writing our own custom configuration format?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.
Did you look at the commit message? It cites several other advantages of integrating with golangci-lint:
with other linters
Moving the logic of what to check in which file out of the script and into the checker is required for that integration.
This new configuration format also scales better: instead of maintaining different lists of files where unstructured calls are banned and another list where all klog logging calls are banned we only need to maintain one config file.
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.
Regarding the script: others are welcome to try and simplify it, but I consider that a dead end and won't spend time on it. The issue about fixing it has been open for months without progress, too.
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.
Not sure if the advantages of golangci-lint you listed impact k8s enough to motivate this change. Personally I would really want to avoid introducing custom configuration as it generates much more work long term. For maintainability of kubernetes we need to ensure that we not only consider cost of implementation, but also how much work other contributors will spend time to understand, debug, fix and extend this format.
I would prefer for introduction of contextual logging to logcheck not to include this. However I don't have enough experience in maintaining validation scripts for K8s, so maybe I'm wrong. I think best would be to get some feedback from
./hack/
directory owners as they will be de final approvers on the configuration./cc bentheelder cblecker deads2k fejta lavalamp liggitt pwittrock SataQiu spiffxp sttts mikedanese
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.
/cc @BenTheElder @cblecker @deads2k @fejta @lavalamp @liggitt @pwittrock @SataQiu @spiffxp @sttts @mikedanese
Guys, can anyone comment on this question: do you prefer to integrate log checking into the common golangci-lint invocation (drawback: a more complex configuration file is needed) or maintaining several different scripts under hack, one for structured logging and another for contextual logging (drawbacks: current script is complex and broken, slower overall, no support for nolint:logcheck)?
The support for integration with golangci-lint is in this PR. The prototype integration into Kubernetes is currently in kubernetes/kubernetes@master...pohly:logcheck
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.
Let me add that I had asked about this before going in this direction: https://kubernetes.slack.com/archives/C09QZ4DQB/p1643049767036400
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.
And here's a PR with the Kubernetes changes, updated right now with the latest code from this PR: kubernetes/kubernetes#108159