Repository: go-logr/logr
Pull request #166 (2022-12-03 16:27:26)

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
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 <thockin@google.com>
See https://github.com/go-logr/logr/pull/158#discussion_r1037686093


Patch:
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/funcr/funcr.go b/funcr/funcr.go
index 0baf003..e52f0cd 100644
--- a/funcr/funcr.go
+++ b/funcr/funcr.go
@@ -218,7 +218,7 @@ func newFormatter(opts Options, outfmt outputFormat) Formatter {
 		prefix:       "",
 		values:       nil,
 		depth:        0,
-		opts:         opts,
+		opts:         &opts,
 	}
 	return f
 }
@@ -232,7 +232,7 @@ type Formatter struct {
 	values       []interface{}
 	valuesStr    string
 	depth        int
-	opts         Options
+	opts         *Options
 }
 
 // outputFormat indicates which outputFormat to use.
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))


Test patch:
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_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")
 	}
 }