Skip to content
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

Enabled: always must skip two levels #216

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions logr.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,15 @@ type Logger struct {
// Enabled tests whether this Logger is enabled. For example, commandline
// flags might be used to set the logging verbosity and disable some info logs.
func (l Logger) Enabled() bool {
return l.sink != nil && l.sink.Enabled(l.level)
return l.sink != nil && l.enabled()
}

// enabled ensures that Enabled and Info both call LogSink.Enabled with one
// intermediate stack frame. This wouldn't be necessary if Info had called
// LogSink.Enabled directly in logr v1.0.0, but it didn't and now that cannot
// be changed anymore because implementations depend on this behavior.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understsand the "implementations depend on this behavior" point. It seems like we could either do

(Info, Error, Enabled) -> sink.Enabled

or

(Info, Error, Enabled) -> enabled -> sink.Enabled

And it would be correct, as long as it is consistent - right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understsand the "implementations depend on this behavior" point.

I mean this code:

https://github.com/kubernetes/klog/blob/6632ba5cc9a5331c853a2c6dbc0fbb125e6e7b94/klogr.go#L55-L56

That code works with logr v1.2.0 for logger.Info.

If we switch to (Info, Error, Enabled) -> sink.Enabled, logger.Info will not work correctly in combination with -vmodule anymore when someone uses klog <= v2.100.1 and updates to a future logr release where we made that change.

We can change klog in v2.200.0 and make it depend on such a new logr release, but we cannot prevent the combination above.

And it would be correct, as long as it is consistent - right?

Both are "correct", but (Info, Error, Enabled) -> enabled -> sink.Enabled will cause less pain for downstream users.

Copy link
Contributor

@thockin thockin Sep 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we are left with this weird document that Enabled() needs one more frame. I hate it. If it's already buggy, how bad is it really to just fix the bug?

func (l Logger) enabled() bool {
return l.sink.Enabled(l.level)
}

// Info logs a non-error message with the given key/value pairs as context.
Expand All @@ -271,7 +279,7 @@ func (l Logger) Info(msg string, keysAndValues ...any) {
if l.sink == nil {
return
}
if l.Enabled() {
if l.enabled() {
if withHelper, ok := l.sink.(CallStackHelperLogSink); ok {
withHelper.GetCallStackHelper()()
}
Expand Down Expand Up @@ -447,9 +455,13 @@ func NewContext(ctx context.Context, logger Logger) context.Context {
// LogSinks might want to know.
type RuntimeInfo struct {
// CallDepth is the number of call frames the logr library adds between the
// end-user and the LogSink. LogSink implementations which choose to print
// end-user and the LogSink. LogSink implementations which choose to print
// the original logging site (e.g. file & line) should climb this many
// additional frames to find it.
//
// The CallDepth is given for LogSink.Info and LogSink.Error such that 1 means
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this now fixed (Info -> enabled parallel to Enabled -> enabled)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

I have a test for this in kubernetes/klog#383. That test fails with current logr and passes with this PR.

We can merge it now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we remove this part of the comment?

// "skip one intermediate level". For historic reasons, LogSink.Enabled must
// skip one additional level compared to those two calls.
CallDepth int
}

Expand Down