make Discard logger equal to null logger

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
#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 <thockin@google.com>
See #158 (comment)

Author: pohly

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)
 }

discard_test.go

@@ -24,26 +24,31 @@ 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.
 	l.WithName("discard").WithValues("z", 5).Info("Hello world")
 	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")
 	}
 }

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))

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")
 	}
 }