-
Notifications
You must be signed in to change notification settings - Fork 5.3k
logging: clarify error logging #8686
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
Open
pohly
wants to merge
1
commit into
kubernetes:master
Choose a base branch
from
pohly:error-logging
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+23
−2
Open
Changes from all commits
Commits
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 hidden or 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 |
|---|---|---|
|
|
@@ -60,8 +60,29 @@ Good practices when writing log messages are: | |
|
|
||
| ### What method to use? | ||
|
|
||
| * `klog.ErrorS` - Errors should be used to indicate unexpected behaviours in code, like unexpected errors returned by subroutine function calls. | ||
| Logs generated by `ErrorS` command may be enhanced with additional debug information (by logging library). Calling `ErrorS` with `nil` as error may be acceptable if there is error condition that deserves a stack trace at this origin point. | ||
| * `klog.ErrorS`, [`logr.Logger.Error`](https://pkg.go.dev/github.com/go-logr/logr#Logger.Error): | ||
| Use error logs to inform admins that they might have to do something to fix a problem. | ||
| If there is no `error` instance, pass nil and use the message string and key/value pairs to describe the problem instead of fabricating an error with `fmt.Errorf`. | ||
| If the dependency is allowed, use `runtime.HandleErrorWithContext` or `runtime.HandleErrorWithLogger` instead. | ||
|
|
||
| Don't emit an error log before returning an error because | ||
| it is usually uncertain if and how the caller is going to handle the returned error. | ||
| It might handle it gracefully, in which case alerting an administrator with an error log would be wrong. | ||
| Instead, use `fmt.Errorf` with `%w` to return a more informative error with a trail for where the error came from. | ||
| Avoid "failed to" as prefix when wrapping errors, it quickly becomes repetitive when added multiple times. | ||
|
|
||
| Sometimes it may be useful to log an error for debugging purposes before returning it. Use an info log with at least | ||
| `V(4)` and `err` as key for the key/value pair. | ||
|
|
||
| * [`runtime.HandleErrorWithContext`](https://pkg.go.dev/k8s.io/[email protected]/pkg/util/runtime#HandleErrorWithContext), | ||
| [`runtime.HandleErrorWithLogger`](https://pkg.go.dev/k8s.io/[email protected]/pkg/util/runtime#HandleErrorWithLogger): | ||
| Whenever possible, use this instead of normal error logging at the top of a call chain, i.e. the place where code | ||
| cannot return an error up to its caller, if there is no other way of handling the error (like logging it | ||
| and then exiting the process). | ||
|
|
||
| The `runtime` package supports additional error handling mechanisms that downstream consumers of Kubernetes may add | ||
| (see [ErrorHandlers](https://pkg.go.dev/k8s.io/[email protected]/pkg/util/runtime#pkg-variables)). | ||
| Some [downstream consumers](https://grep.app/search?q=runtime.ErrorHandlers) even suppress logging of errors completely. | ||
|
|
||
| * `klog.InfoS` - Structured logs to the INFO log. `InfoS` should be used for routine logging. It can also be used to log warnings for expected errors (errors that can happen during routine operations). | ||
| Depending on log severity it's important to pick a proper verbosity level to ensure that consumer is neither under nor overwhelmed by log volume: | ||
|
|
||
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.
Great advice, thanks.
lgtm. I'll leave it open for others to comment, but if it's still open in a week or two remind me and I'll tag for merge.
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.
It's been a week and @dashpole also approves of it.
@dashpole, @deads2k, can one of your LGTM?