Update k8s.io/klog to v2.2.0 #92554
Update k8s.io/klog to v2.2.0 #92554
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
cncf-cla: yes
|
please update go-logr/logr to 0.2.0 and use logr.Logger instead of logr.InfoLogger |
Should update json format here, @serathius if deadline allowed, i can work on it. |
k8s-ci-robot
added
size/L
|
Hey @yuzhiquan, I had a draft prepared when you asked, so I decided to finish this up. Are you interested in looking into it? |
@serathius |
|
/retest |
|
/test pull-kubernetes-e2e-kind-ipv6 |
|
/assign @liggitt Jordan, this looks ready! |
|
Vendor mechanics lgtm, didn't take a look at the reactionary changes yet. Would like @thockin's ack since he was involved in the interface change (looking at https://github.com/go-logr/logr/commits/master) Since logr will be a shared dependency now, it would be great if we could get the interface stabilized sooner rather than later. |
|
/assign @thockin |
There was a problem hiding this comment.
One question about the way logr is being used. As for logr API stability. I am a bit anxious about that - we haven't gotten a TON of feedback, but what we have gotten indicates the API isn't a problem. There are a few open issues (being able to offset stack frames and k/v pairs being the 2 that I car about most) that could be API impacting.
@DirectXMan12 I wonder if we should put some effort into pinning it down, doing some more examples, and just calling thee things 1.0?
| type infoLogger struct { | ||
| lvl zapcore.Level | ||
| // zapLogger is a logr.Logger that uses Zap to record log. | ||
| type zapLogger struct { |
There was a problem hiding this comment.
I am curious why we are embedding this rather than using https://github.com/go-logr/zapr ?
There was a problem hiding this comment.
There are some small differences configuring zap, e.g. EncoderConfig. As codebase was small we decided that it will be easier and faster to first develop it in k/k and propose changes to zapr later.
|
OK
…On Mon, Jun 29, 2020 at 3:05 PM Marek Siarkowicz ***@***.***> wrote:
@serathius commented on this pull request.
________________________________
In staging/src/k8s.io/component-base/logs/json/json.go:
> @@ -35,32 +35,23 @@ var (
timeNow = time.Now
)
-type noopInfoLogger struct{}
-
-// Enabled func in noopInfoLogger always return false
-func (l *noopInfoLogger) Enabled() bool {
- return false
-}
-
-func (l *noopInfoLogger) Info(_ string, _ ...interface{}) {}
-
-var disabledInfoLogger = &noopInfoLogger{}
-
-type infoLogger struct {
- lvl zapcore.Level
+// zapLogger is a logr.Logger that uses Zap to record log.
+type zapLogger struct {
There are some small differences configuring zap, e.g. EncoderConfig. As codebase was small we decided that it will be easier and faster to first develop it in k/k and propose changes to zapr later.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Please let me know if I can help. |
|
@DirectXMan12 @thockin Can we please ship this while you all work on |
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
|
I have no objections - logr is pre 1.0, but that isn't different before or after this PR, so ... |
| // Error is equivalent to the global Error function, guarded by the value of v. | ||
| // See the documentation of V for usage. | ||
| func (v Verbose) Error(err error, msg string, args ...interface{}) { |
There was a problem hiding this comment.
should this be ErrorS? the signature doesn't match the global Error function as the godoc claims
There was a problem hiding this comment.
Yes, this looks like a bug in klog. We are upgrading to klog to pick up a bug fix kubernetes/klog#165 We should be ok with this as currently we are not using klog.V(2).Error.
There was a problem hiding this comment.
this looks like a bug in klog
and klog tagged a release with this? that means we can't change the signature until klog/v3, which I hope we never get to
There was a problem hiding this comment.
@dims proposed to add ErrorS and add comment to discourage people from using Error.
I don't think this is blocking. Are we ok to merge?
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: serathius, 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 |
k8s-ci-robot
added
the
approved
|
heh, review collision /hold for question at https://github.com/kubernetes/kubernetes/pull/92554/files#r447829913 |
k8s-ci-robot
added
the
do-not-merge/hold
|
/milestone v1.19 |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
/kind feature
Ref kubernetes/enhancements#1602
/cc @dims