From 4da5305ff29a64c62f54ad43ebbfcb5e1b015fb2 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 28 Nov 2022 10:56:35 +0100 Subject: [PATCH] make Discard logger equal to null logger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original intention was to no treat the discard log sink in a special way. But it needs special treatment and lacking that in V() led to a bug: Discard() became different from Discard().V(1). Adding special detection of the discard logger also helps with the future logging of context values, because that extra work can be skipped for the discard logger. The distinction between null logger (from https://github.com/go-logr/logr/pull/153) and logr.Discard() was very minor. Instead of fixing the issue above with checks for both cases, Discard() now simply returns the null logger. Performance is a bit better: name old time/op new time/op delta DiscardLogInfoOneArg-36 92.7ns ± 5% 88.2ns ± 3% ~ (p=0.056 n=5+5) DiscardLogInfoSeveralArgs-36 239ns ± 0% 231ns ± 1% -3.40% (p=0.008 n=5+5) DiscardLogInfoWithValues-36 240ns ± 1% 236ns ± 3% ~ (p=0.889 n=5+5) DiscardLogV0Info-36 234ns ± 1% 228ns ± 0% -2.62% (p=0.008 n=5+5) DiscardLogV9Info-36 241ns ± 2% 228ns ± 0% -5.23% (p=0.008 n=5+5) DiscardLogError-36 264ns ± 1% 230ns ± 0% -12.78% (p=0.008 n=5+5) DiscardWithValues-36 116ns ± 1% 110ns ± 1% -5.35% (p=0.008 n=5+5) DiscardWithName-36 2.25ns ± 0% 0.72ns ± 0% -68.12% (p=0.008 n=5+5) FuncrLogInfoOneArg-36 2.13µs ± 2% 2.16µs ± 1% ~ (p=0.222 n=5+5) FuncrJSONLogInfoOneArg-36 2.41µs ± 1% 2.42µs ± 1% ~ (p=0.246 n=5+5) FuncrLogInfoSeveralArgs-36 4.53µs ± 4% 4.40µs ± 4% ~ (p=0.151 n=5+5) FuncrJSONLogInfoSeveralArgs-36 4.71µs ± 8% 4.67µs ± 2% ~ (p=0.310 n=5+5) FuncrLogInfoWithValues-36 4.60µs ± 2% 4.61µs ± 4% ~ (p=0.595 n=5+5) FuncrJSONLogInfoWithValues-36 4.81µs ± 3% 4.84µs ± 3% ~ (p=1.000 n=5+5) FuncrLogV0Info-36 4.45µs ± 3% 4.55µs ± 3% ~ (p=0.222 n=5+5) FuncrJSONLogV0Info-36 4.83µs ± 2% 4.78µs ± 3% ~ (p=0.548 n=5+5) FuncrLogV9Info-36 259ns ± 1% 252ns ± 0% -2.48% (p=0.008 n=5+5) FuncrJSONLogV9Info-36 258ns ± 1% 252ns ± 1% -2.52% (p=0.008 n=5+5) FuncrLogError-36 4.97µs ± 1% 4.78µs ± 3% -3.77% (p=0.032 n=5+5) FuncrJSONLogError-36 5.20µs ± 3% 5.13µs ± 2% ~ (p=0.548 n=5+5) FuncrWithValues-36 1.39µs ± 3% 1.38µs ± 3% ~ (p=0.690 n=5+5) FuncrWithName-36 217ns ± 1% 215ns ± 1% -0.62% (p=0.040 n=5+5) FuncrWithCallDepth-36 206ns ± 1% 210ns ± 1% +1.92% (p=0.008 n=5+5) FuncrJSONLogInfoStringerValue-36 2.59µs ± 2% 2.59µs ± 2% ~ (p=1.000 n=5+5) FuncrJSONLogInfoErrorValue-36 2.61µs ± 2% 2.63µs ± 2% ~ (p=0.310 n=5+5) FuncrJSONLogInfoMarshalerValue-36 2.63µs ± 2% 2.63µs ± 1% ~ (p=0.841 n=5+5) There's no difference in allocations. Co-authored-by: Tim Hockin See https://github.com/go-logr/logr/pull/158#discussion_r1037686093 --- discard.go | 32 +------------------------------- discard_test.go | 13 +++++++++---- logr.go | 19 +++++++++++++++++-- logr_test.go | 11 +++++------ 4 files changed, 32 insertions(+), 43 deletions(-) diff --git a/discard.go b/discard.go index 9d92a38..99fe8be 100644 --- a/discard.go +++ b/discard.go @@ -20,35 +20,5 @@ package logr // used whenever the caller is not interested in the logs. Logger instances // produced by this function always compare as equal. func Discard() Logger { - return Logger{ - level: 0, - sink: discardLogSink{}, - } -} - -// discardLogSink is a LogSink that discards all messages. -type discardLogSink struct{} - -// Verify that it actually implements the interface -var _ LogSink = discardLogSink{} - -func (l discardLogSink) Init(RuntimeInfo) { -} - -func (l discardLogSink) Enabled(int) bool { - return false -} - -func (l discardLogSink) Info(int, string, ...interface{}) { -} - -func (l discardLogSink) Error(error, string, ...interface{}) { -} - -func (l discardLogSink) WithValues(...interface{}) LogSink { - return l -} - -func (l discardLogSink) WithName(string) LogSink { - return l + return New(nil) } diff --git a/discard_test.go b/discard_test.go index c9a02de..b46dcc0 100644 --- a/discard_test.go +++ b/discard_test.go @@ -24,7 +24,7 @@ import ( func TestDiscard(t *testing.T) { l := Discard() - if _, ok := l.GetSink().(discardLogSink); !ok { + if l.GetSink() != nil { t.Error("did not return the expected underlying type") } // Verify that none of the methods panic, there is not more we can test. @@ -32,18 +32,23 @@ func TestDiscard(t *testing.T) { l.Info("Hello world", "x", 1, "y", 2) l.V(1).Error(errors.New("foo"), "a", 123) if l.Enabled() { - t.Error("discardLogSink must always say it is disabled") + t.Error("discard loggers must always be disabled") } } func TestComparable(t *testing.T) { a := Discard() if !reflect.TypeOf(a).Comparable() { - t.Fatal("discardLogSink must be comparable") + t.Fatal("discard loggers must be comparable") } b := Discard() if a != b { - t.Fatal("any two discardLogSink must be equal") + t.Fatal("any two discard Loggers must be equal") + } + + c := Discard().V(2) + if b != c { + t.Fatal("any two discard Loggers must be equal") } } diff --git a/logr.go b/logr.go index 6091b41..e027aea 100644 --- a/logr.go +++ b/logr.go @@ -212,11 +212,14 @@ import ( ) // New returns a new Logger instance. This is primarily used by libraries -// implementing LogSink, rather than end users. +// implementing LogSink, rather than end users. Passing a nil sink will create +// a Logger which discards all log lines. func New(sink LogSink) Logger { logger := Logger{} logger.setSink(sink) - sink.Init(runtimeInfo) + if sink != nil { + sink.Init(runtimeInfo) + } return logger } @@ -265,6 +268,9 @@ func (l Logger) Enabled() bool { // information. The key/value pairs must alternate string keys and arbitrary // values. func (l Logger) Info(msg string, keysAndValues ...interface{}) { + if l.sink == nil { + return + } if l.Enabled() { if withHelper, ok := l.sink.(CallStackHelperLogSink); ok { withHelper.GetCallStackHelper()() @@ -298,6 +304,9 @@ func (l Logger) Error(err error, msg string, keysAndValues ...interface{}) { // level means a log message is less important. Negative V-levels are treated // as 0. func (l Logger) V(level int) Logger { + if l.sink == nil { + return l + } if level < 0 { level = 0 } @@ -344,6 +353,9 @@ func (l Logger) WithName(name string) Logger { // WithCallDepth(1) because it works with implementions that support the // CallDepthLogSink and/or CallStackHelperLogSink interfaces. func (l Logger) WithCallDepth(depth int) Logger { + if l.sink == nil { + return l + } if withCallDepth, ok := l.sink.(CallDepthLogSink); ok { l.setSink(withCallDepth.WithCallDepth(depth)) } @@ -365,6 +377,9 @@ func (l Logger) WithCallDepth(depth int) Logger { // implementation does not support either of these, the original Logger will be // returned. func (l Logger) WithCallStackHelper() (func(), Logger) { + if l.sink == nil { + return func() {}, l + } var helper func() if withCallDepth, ok := l.sink.(CallDepthLogSink); ok { l.setSink(withCallDepth.WithCallDepth(1)) diff --git a/logr_test.go b/logr_test.go index 3ca0c7e..00ecf98 100644 --- a/logr_test.go +++ b/logr_test.go @@ -384,8 +384,8 @@ func TestContext(t *testing.T) { } out := FromContextOrDiscard(ctx) - if _, ok := out.sink.(discardLogSink); !ok { - t.Errorf("expected a discardLogSink, got %#v", out) + if out.sink != nil { + t.Errorf("expected a nil sink, got %#v", out) } sink := &testLogSink{} @@ -412,11 +412,10 @@ func TestIsZero(t *testing.T) { if l.IsZero() { t.Errorf("expected not IsZero") } - // Discard is not considered a zero unset logger, but an intentional choice - // to ignore messages that should not be overridden by a component. + // Discard is the same as a nil sink. l = Discard() - if l.IsZero() { - t.Errorf("expected not IsZero") + if !l.IsZero() { + t.Errorf("expected IsZero") } }