-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
⚠️ Refactor logging structure, avoid mandatory Zap #290
⚠️ Refactor logging structure, avoid mandatory Zap #290
Conversation
@pwittrock do you remember what the idea was behind |
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.
looks good to me. Have one minor comment about default logger.
|
||
// Log is the base logger used by kubebuilder. It delegates | ||
// to another logr.Logger. You *must* call SetLogger to | ||
// get any actual logging. |
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.
wondering if we can set stdout-logger as default ? (or we need to ensure scaffolding in kubebuilder is doing that) because stdout-logging is a sane default.
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.
Also, can we include an example of how to enable stdout logger ?
pkg/log/zap/zap.go
Outdated
} | ||
|
||
// LoggerTo returns a new Logger implementation using Zap which logs | ||
// to the given destination, instead of stderr. It otherise behaves like |
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.
s/otherise/otherwise
This splits up pkg/runtime/log into pkg/log, pkg/internal/log, and pkg/log/zap. - pkg/log contains generic logging helpers, and the generic root logger It *only* depends on logr, and thus won't pull in Zap if you don't need it. - pkg/internal/log contains the internal log handle (with the appropriate name). Nothing outside CR should be logging with this handle anyway, so this enforces it. - pkg/log/zap contains Zap-related setup code, and thus CR only has a dependency on Zap if you pull in this package. - pkg/runtime/log remains as a deprecated package to ease refactoring and compatibility. While practically a few types have been removed from this package, in reality most consumers shouldn't notice.
This switches everything except alias.go over to the new package layout (mainly having internals use pkg/internal/log, and examples use pkg/log).
This switches alias.go over to the new logging layout. In order to avoid alias.go pulling in Zap, we don't put ZapLogger in alias.go any more. Practically, this is probably fine. It means an extra line in main.go, but it's just one, and allows people to avoid extraneous dependencies.
140b281
to
3c5adf4
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12 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 |
add scaffold from controller-tools with backward compatibility
The way we currently have logging structured, you have to pull in Zap, even if you don't use Zap as your logr backend.
In the spirit of avoiding dependency bloat, this separates zap off into its own package, and moves around some of the logging package hierarchy:
pkg/log contains generic logging helpers, and the generic root logger
It only depends on logr, and thus won't pull in Zap if you don't
need it.
pkg/internal/log contains the internal log handle (with the
appropriate name). Nothing outside CR should be logging with this
handle anyway, so this enforces it.
pkg/log/zap contains Zap-related setup code, and thus CR only has a
dependency on Zap if you pull in this package.
pkg/runtime/log remains as a deprecated package to ease refactoring
and compatibility. While practically a few types have been removed
from this package, in reality most consumers shouldn't notice.
This has the practical effect of meaning that while you now have to explicitly import
pkg/log/zap
if you want Zap setup, if you don't pull that in, you don't get Zap listed as a dependency.