-
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
Add SetLogger function to intercept klog output with logr #20
Conversation
/cc @dims |
glog/glog.go
Outdated
@@ -35,7 +35,7 @@ func Flush() { | |||
|
|||
// V is a shim | |||
func V(level Level) Verbose { | |||
return Verbose(bool(klog.V(klog.Level(int32(level))))) | |||
return Verbose(klog.V(klog.Level(int32(level))).Enabled()) |
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.
so previously Verbose was a bool type, but now it's a struct.
isn't the compiler complaining that we are casting a bool to type Verbose here? 😬
cannot convert Enabled() (type bool) to type Verbose
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.
So Verbose has been changed to a struct in klog itself - this is the glog 'shim' which is provided to make transitioning to klog easier.
I've not changed the API surface of the glog package at all, meaning if someone were to use a V level, the message would actually be logged without using a klog 'Verbose'. This is not caused by this patch, rather it's how the package was originally written!
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.
got it.
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.
couple of comments inline. I think this is an interesting first pass, but ideally, we'd be passing some of the information from glog to logr as structured data instead of just flat-dumping to strings. Some is also probably duplicate to what other logr implementations capture. I think we probably want to assume that the given implementation will take care of stuff like backtraces, depth, etc if it wants it, but I'm open to other interpretations.
(flat-dumping to strings also breaks stuff like Zap's optional log-line collapsing, and it thus not recommended by logr)
klog.go
Outdated
buf, file, line := l.header(s, 0) | ||
fmt.Fprintln(buf, args...) | ||
l.output(s, buf, file, line, false) | ||
if logr != nil { | ||
logr.Info(buf.String()) |
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'm not sure we actually want the header in here -- it should be the job of the logr implementation to attach that kind of information, and logging it is just going to produce noise. If it must be included, I'd leave it as a tag or tags
@munnerz is this change still needed after the merge of: |
@neolit123 the klogr package allows you to use My use case for this, is so that I can grab all output from the k8s apimachinery and pipe it via my own logr interface (so that it can be post-processed accordingly) |
Is there still an interest in something like this in klog? If so, I'll pick this up again. I can address to above comments if it's likely we want to go down this route 😄 |
ebe1294
to
1e884be
Compare
I've added a few additional changes on top of this PR, notably:
Hopefully that addresses the above comments 😄 |
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.
So I can use klog as the backend for a logr or a logr as the backend for klog? Can I make a loop?
Can you show what the output here would look like?
Also, logr already suggests "caller"
s a special key for the calling information (file/line) of a particular log line.
klog.go
Outdated
// and Infof. These methods will write to the Info log if called. | ||
// Thus, one may write either | ||
// if glog.V(2) { glog.Info("log this") } | ||
// if glog.V(2).Enabled() { glog.Info("log this") } |
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'm pretty sure there's code that relies on this being a bool, isn't there?
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.
Yep, there is.
There's not really any way we can do this cleanly without this, as a bool does not contain enough information to determine which V level should be logged at when Info
is called on that V.
I mentioned it in the initial PR body, but the actual changeset needed to make kubernetes/kubernetes compile is:
pkg/cloudprovider/providers/gce/cloud/gen.go | 160 +++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------
pkg/cloudprovider/providers/vsphere/vclib/connection.go | 2 +-
pkg/controller/nodelifecycle/node_lifecycle_controller.go | 4 +--
pkg/controller/volume/attachdetach/reconciler/reconciler.go | 6 ++--
pkg/kubectl/cmd/util/helpers.go | 2 +-
pkg/kubelet/eviction/helpers.go | 4 +--
pkg/proxy/endpoints.go | 2 +-
pkg/scheduler/algorithm/predicates/predicates.go | 8 ++---
pkg/scheduler/algorithm/priorities/interpod_affinity.go | 2 +-
pkg/scheduler/algorithm/priorities/resource_allocation.go | 2 +-
pkg/scheduler/algorithm/priorities/resource_limits.go | 2 +-
pkg/scheduler/algorithm/priorities/selector_spreading.go | 2 +-
pkg/scheduler/core/generic_scheduler.go | 6 ++--
plugin/pkg/auth/authorizer/rbac/rbac.go | 2 +-
staging/src/k8s.io/apiserver/pkg/server/httplog/httplog.go | 4 +--
staging/src/k8s.io/apiserver/pkg/util/trace/trace.go | 4 +--
staging/src/k8s.io/client-go/rest/request.go | 8 ++---
staging/src/k8s.io/client-go/transport/round_trippers.go | 8 ++---
18 files changed, 114 insertions(+), 114 deletions(-)
(as of a couple of months ago).
I would be keen to hear any suggestions on how we can avoid this breaking change, but I fear we don't have many options if we want to preserve V levels 🙈
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.
Indeed, this is tricky. Since we more or less adopted glog->klog, we sort of committed to not breaking it.
What about something like this:
make Verbose implement logr.InfoLogger
and use the bool value to determine whether to do nothing or actually call the glog method, which will call the logr method.
Ahh crud, they both define Info()
with different signatures.
We could add a new Vb()
method, fix our code, and leave V()
users behind with some documented but wrong behavior. It could even panic() if SetLogger was called.
Alternately - are there logr changes we could make to make bridging easier?
Alternatively, fork klog2 ?
Yes, you can create a loop 🙈 I don't really know if there's a good way to handle this case. If you are hijacking klog output and piping via logr, then you need to make sure that logr instance isn't logging back to klog (else 🔥)
Didn't realise this - I'll dig into the logr repo some more to find docs on the format of 'caller' 😄
I'll put a demo case together shortly 😄 |
/assign @DirectXMan12 |
it's already breaking consumers in the day since this merged. are we planning to bump to |
Can we pls fix this issue as per @liggitt suggestion. |
@sunnyanand17 wanna create a PR? |
is it possible in general even possible to reliably use client-go without pinned deps? klog/v2 does mean that we think klog is now stable, but it's still probably nicer to consumers to do |
This breaks |
Sorry for jumping in post-merge on this, but it's not clear to me why we want to enable this. If we want people to be able to plug in arbitrary log backends to kubernetes components, wouldn't it make more sense to switch all of the components to use a logging interface like go-logr first, and wire klogr up in the shipped kubernetes components' main() methods? I didn't expect to try to change a concrete implementation like klog to be a passthrough to a generic interface. Breaking existing users of that concrete implementation in the process makes me doubly question why we're doing this this way. |
It's two sides of things, depending on what you're looking for. If you want klog output, you can use klogr. If you actually want some other backend, but don't want to miss out on all the useful client-go logs, you can use |
Yeah, definitely, but this fixes things in the short term, until we finish discussing a structured logging KEP for a release or two. |
@DirectXMan12 @liggitt - i filed #91 for to start a v2 |
I just got bit too. Can we revert and then in a v2 you can break the interface? |
I agree with this |
@munnerz can we revert this please? we can cut a release then get #91 as suggested in #91 (comment) |
Since this is breaking so many people, I think that's a reasonable plan. We did not anticipate that anyone was actually trying to work against head (which is generally a recipe for disaster, but people seem to be doing it anyway). |
Revert opened in #97, can re-introduce this in a v2 stream if desired |
Revert "Merge pull request #20 from munnerz/logr"
This reverts commit 56be75e.
This reverts commit 56be75e.
Revert "Revert "Merge pull request #20 from munnerz/logr""
In k8s.io/klog versions newer than 1.0.0, `klog.V` returns a `klog.Verbose` type that is now a `bool` instead of a `struct`. ```go k8s.io/client-go/transport go/src/k8s.io/client-go/transport/round_trippers.go:70:11: cannot convert klog.V(9) (type klog.Verbose) to type bool go/src/k8s.io/client-go/transport/round_trippers.go:72:11: cannot convert klog.V(8) (type klog.Verbose) to type bool go/src/k8s.io/client-go/transport/round_trippers.go:74:11: cannot convert klog.V(7) (type klog.Verbose) to type bool go/src/k8s.io/client-go/transport/round_trippers.go:76:11: cannot convert klog.V(6) (type klog.Verbose) to type bool ``` See also: kubernetes/klog#20, kubernetes/klog#98
Currently I'm using logr.Logger interface as my logger output, but I can only deal info and error log, I want to distinguish info and fatalf log. What should i do? |
This is an example of how we can modify klog and the work required in order to allow intercepting logs, including preserving the
V
level and severity.In order to do this, I've had to change the type signature of
Verbose
fromtype Verbose bool
to:Consequently, it requires a corresponding change in k/k in the instances where
if klog.V(x) {
(replacing them withif klog.V(x).Enabled()
).The total changeset in k/k to get
bazel build //...
to pass:We can still preserve the old behaviour in the
k8s.io/klog/glog
package, for those that are using the depreplace
method to implement glog.ref #19