Repository: go-logr/logr
Pull request #45 (2021-05-28 21:01:18)

Beef up docs, comments and tests


Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
new file mode 100644
index 0000000..5d37e29
--- /dev/null
+++ b/CONTRIBUTING.md
@@ -0,0 +1,17 @@
+# Contributing
+
+Logr is open to pull-requests, provided they fit within the intended scope of
+the project.  Specifically, this library aims to be VERY small and minimalist,
+with no external dependencies.
+
+## Compatibility
+
+This project intends to follow [semantic versioning](http://semver.org) and
+is very strict about compatibility.  Any proposed changes MUST follow those
+rules.
+
+## Performance
+
+As a logging library, logr must be as light-weight as possible.  Any proposed
+code change must include results of running the [benchmark](./benchmark)
+before and after the change.
diff --git a/README.md b/README.md
index 304163d..e6339e6 100644
--- a/README.md
+++ b/README.md
@@ -1,23 +1,90 @@
-# A more minimal logging API for Go
+# A minimal logging API for Go
 
-Before you consider this package, please read [this blog post by the
-inimitable Dave Cheney][warning-makes-no-sense].  I really appreciate what
-he has to say, and it largely aligns with my own experiences.  Too many
-choices of levels means inconsistent logs.
+logr offers an(other) opinion on how Go programs and libraries can do logging
+without becoming coupled to a particular logging implementation,  This is not
+an implementation of logging - it is an API.  In fact it is two APIs with twop
+different sets of users.
+
+The `Logger` type is intended for application and library authors.  It provides
+a relatively small API which can be used everywhere you want to emit logs.  It
+defers the actual act of writing logs (to files, to stdout, or whatever) to the
+`LogSink` interface.
+
+The `LogSink` interface is intended for logging library implementors.  It is a
+pure interface which can be implemented by to provide the actual logging
+functionality.
+
+This decoupling allows application and library developers to write code in
+terms of `logr.Logger` (which has very low dependency fan-out) while the
+implementation of logging is managed "up stack" (e.g. in or near `main()`).
+Application developers can then switch out implementations as necessary.
+
+Many people assert that libraries should not be logging, and as such efforts
+like this are pointless.  Those people are welcome to convince the authors of
+the tens-of-thousands of libraries that *DO* write logs that they are all
+wrong.  In the meantime, logr takes a more practical approach.
+
+## Typical usage
+
+Somewhere, early in an application's life, it will make a decision about which
+logging library (implementation) it actually wants to use.  Something like:
+
+```
+    func main() {
+        // ... other setup code ...
+
+        // Create the "root" logger.  We have chosen the "logimpl" implementation,
+        // which takes some initial parameters and returns a logr.Logger.
+        logger := logimpl.New(param1, param2)
+
+        // ... other setup code ...
+```
 
-This package offers a purely abstract interface, based on these ideas but with
-a few twists.  Code can depend on just this interface and have the actual
-logging implementation be injected from callers.  Ideally only `main()` knows
-what logging implementation is being used.
+Most apps will call into other libraries, create stuctures to govern the flow,
+etc.  The `logr.Logger` object can be passed to these other libraries, stored
+in structs, or even used as a package-global variable, if needed.  For example:
+
+```
+        app := createTheAppObject(logger)
+        app.Run()
+```
+
+Outside of this early setup, no other packages need to know about the choice of
+implementation.  They write logs in terms of the `logr.Logger` that they
+received:
+
+```
+    type appObject struct {
+        // ... other fields ...
+        logger logr.Logr
+        // ... other fields ...
+    }
+
+    func (app *appObject) Run() {
+        app.logger.Info("starting up", "timestamp", time.Now())
+
+        // ... app code ...
+```
+
+## Background
+
+If the Go standard library had defined an interface for logging, this project
+probably would not be needed.  Alas, here we are.
+
+### Inspiration
+
+Before you consider this package, please read [this blog post by the
+inimitable Dave Cheney][warning-makes-no-sense].  We really appreciate what
+he has to say, and it largely aligns with our own experiences.
 
-# Differences from Dave's ideas
+### Differences from Dave's ideas
 
 The main differences are:
 
 1) Dave basically proposes doing away with the notion of a logging API in favor
-of `fmt.Printf()`.  I disagree, especially when you consider things like output
-locations, timestamps, file and line decorations, and structured logging.  I
-restrict the API to just 2 types of logs: info and error.
+of `fmt.Printf()`.  We disagree, especially when you consider things like output
+locations, timestamps, file and line decorations, and structured logging.  This
+package restricts the logging API to just 2 types of logs: info and error.
 
 Info logs are things you want to tell the user which are not errors.  Error
 logs are, well, errors.  If your code receives an `error` from a subordinate
@@ -31,7 +98,7 @@ may feel very similar, but the primary difference is the lack of semantics.
 Because verbosity is a numerical value, it's safe to assume that an app running
 with higher verbosity means more (and less important) logs will be generated.
 
-This is a BETA grade API.
+## Implementations (non-exhaustive)
 
 There are implementations for the following logging libraries:
 
@@ -44,11 +111,11 @@ There are implementations for the following logging libraries:
 - **github.com/wojas/genericr**: [genericr](https://github.com/wojas/genericr) (makes it easy to implement your own backend)
 - **logfmt** (Heroku style [logging](https://www.brandur.org/logfmt)): [logfmtr](https://github.com/iand/logfmtr)
 
-# FAQ
+## FAQ
 
-## Conceptual
+### Conceptual
 
-## Why structured logging?
+#### Why structured logging?
 
 - **Structured logs are more easily queriable**: Since you've got
   key-value pairs, it's much easier to query your structured logs for
@@ -73,20 +140,20 @@ There are implementations for the following logging libraries:
   objects).  Structured logs allow you to preserve that structure when
   outputting.
 
-## Why V-levels?
+#### Why V-levels?
 
 **V-levels give operators an easy way to control the chattiness of log
 operations**.  V-levels provide a way for a given package to distinguish
 the relative importance or verbosity of a given log message.  Then, if
 a particular logger or package is logging too many messages, the user
-of the package can simply change the v-levels for that library. 
+of the package can simply change the v-levels for that library.
 
-## Why not more named levels, like Warning?
+#### Why not named levels, like Info/Warning/Error?
 
 Read [Dave Cheney's post][warning-makes-no-sense].  Then read [Differences
 from Dave's ideas](#differences-from-daves-ideas).
 
-## Why not allow format strings, too?
+#### Why not allow format strings, too?
 
 **Format strings negate many of the benefits of structured logs**:
 
@@ -96,7 +163,7 @@ from Dave's ideas](#differences-from-daves-ideas).
 - They don't store structured data well, since contents are flattened into
   a string
 
-- They're not cross-referencable
+- They're not cross-referenceable
 
 - They don't compress easily, since the message is not constant
 
@@ -104,9 +171,9 @@ from Dave's ideas](#differences-from-daves-ideas).
 keys, at which point you've gotten key-value logging with meaningless
 keys)
 
-## Practical
+### Practical
 
-## Why key-value pairs, and not a map?
+#### Why key-value pairs, and not a map?
 
 Key-value pairs are *much* easier to optimize, especially around
 allocations.  Zap (a structured logger that inspired logr's interface) has
@@ -117,16 +184,16 @@ While the interface ends up being a little less obvious, you get
 potentially better performance, plus avoid making users type
 `map[string]string{}` every time they want to log.
 
-## What if my V-levels differ between libraries?
+#### What if my V-levels differ between libraries?
 
 That's fine.  Control your V-levels on a per-logger basis, and use the
-`WithName` function to pass different loggers to different libraries.
+`WithName` method to pass different loggers to different libraries.
 
 Generally, you should take care to ensure that you have relatively
 consistent V-levels within a given logger, however, as this makes deciding
 on what verbosity of logs to request easier.
 
-## But I *really* want to use a format string!
+#### But I really want to use a format string!
 
 That's not actually a question.  Assuming your question is "how do
 I convert my mental model of logging with format strings to logging with
@@ -150,13 +217,13 @@ Kubernetes codebase):
   response when requesting url", "attempt", retries, "after
   seconds", seconds, "url", url)`
 
-If you *really* must use a format string, place it as a key value, and
-call `fmt.Sprintf` yourself -- for instance, `log.Printf("unable to
+If you *really* must use a format string, use it in a key's value, and
+call `fmt.Sprintf` yourself.  For instance: `log.Printf("unable to
 reflect over type %T")` becomes `logger.Info("unable to reflect over
 type", "type", fmt.Sprintf("%T"))`.  In general though, the cases where
 this is necessary should be few and far between.
 
-## How do I choose my V-levels?
+#### How do I choose my V-levels?
 
 This is basically the only hard constraint: increase V-levels to denote
 more verbose or more debug-y logs.
@@ -169,7 +236,7 @@ Then gradually choose levels in between as you need them, working your way
 down from 10 (for debug and trace style logs) and up from 1 (for chattier
 info-type logs).
 
-## How do I choose my keys?
+#### How do I choose my keys?
 
 - make your keys human-readable
 - constant keys are generally a good idea
@@ -180,19 +247,19 @@ While key names are mostly unrestricted (and spaces are acceptable),
 it's generally a good idea to stick to printable ascii characters, or at
 least match the general character set of your log lines.
 
-## Why should keys be constant values?
+#### Why should keys be constant values?
 
 The point of structured logging is to make later log processing easier.  Your
 keys are, effectively, the schema of each log message.  If you use different
 keys across instances of the same log-line, you will make your structured logs
 much harder to use.  `Sprintf()` is for values, not for keys!
 
-## Why is this not a pure interface?
+#### Why is this not a pure interface?
 
 The Logger type is implemented as a struct in order to allow the Go compiler to
-optimize things like high-V Info logs that are not triggered.  Not all of these
-implementations are implemented yet, but this structure was suggested as a way to
-ensure they *can* be implemented.  All of the real work is behind the LogSink
-interface.
+optimize things like high-V `Info` logs that are not triggered.  Not all of
+these implementations are implemented yet, but this structure was suggested as
+a way to ensure they *can* be implemented.  All of the real work is behind the
+`LogSink` interface.
 
 [warning-makes-no-sense]: http://dave.cheney.net/2015/11/05/lets-talk-about-logging
diff --git a/benchmark/README.md b/benchmark/README.md
new file mode 100644
index 0000000..e0ecc51
--- /dev/null
+++ b/benchmark/README.md
@@ -0,0 +1,15 @@
+# Benchmarking logr
+
+Any major changes to the logr library must be benchmarked before and after the
+change.
+
+## Running the benchmark
+
+```
+$ go test -bench='.' ./benchmark/
+```
+
+## Fixing the benchmark
+
+If you think this benchmark can be improved, you are probably correct!  PRs are
+very welcome.
diff --git a/discard.go b/discard.go
index 02e0099..9d92a38 100644
--- a/discard.go
+++ b/discard.go
@@ -22,33 +22,33 @@ package logr
 func Discard() Logger {
 	return Logger{
 		level: 0,
-		sink:  discardLogger{},
+		sink:  discardLogSink{},
 	}
 }
 
-// discardLogger is a LogSink that discards all messages.
-type discardLogger struct{}
+// discardLogSink is a LogSink that discards all messages.
+type discardLogSink struct{}
 
 // Verify that it actually implements the interface
-var _ LogSink = discardLogger{}
+var _ LogSink = discardLogSink{}
 
-func (l discardLogger) Init(RuntimeInfo) {
+func (l discardLogSink) Init(RuntimeInfo) {
 }
 
-func (l discardLogger) Enabled(int) bool {
+func (l discardLogSink) Enabled(int) bool {
 	return false
 }
 
-func (l discardLogger) Info(int, string, ...interface{}) {
+func (l discardLogSink) Info(int, string, ...interface{}) {
 }
 
-func (l discardLogger) Error(error, string, ...interface{}) {
+func (l discardLogSink) Error(error, string, ...interface{}) {
 }
 
-func (l discardLogger) WithValues(...interface{}) LogSink {
+func (l discardLogSink) WithValues(...interface{}) LogSink {
 	return l
 }
 
-func (l discardLogger) WithName(string) LogSink {
+func (l discardLogSink) WithName(string) LogSink {
 	return l
 }
diff --git a/examples/tab_logger.go b/examples/tab_logger.go
index 84892ae..fd4a216 100644
--- a/examples/tab_logger.go
+++ b/examples/tab_logger.go
@@ -81,6 +81,8 @@ func (l tabLogSink) WithValues(kvs ...interface{}) logr.LogSink {
 	}
 }
 
+// NewTabLogger is the main entry-point to this implementation.  App developers
+// call this somewhere near main() and thenceforth only deal with logr.Logger.
 func NewTabLogger() logr.Logger {
 	sink := tabLogSink{
 		writer: tabwriter.NewWriter(os.Stderr, 40, 8, 2, '\t', 0),
diff --git a/funcr/funcr.go b/funcr/funcr.go
index 4262969..93c044f 100644
--- a/funcr/funcr.go
+++ b/funcr/funcr.go
@@ -31,7 +31,7 @@ import (
 	"github.com/go-logr/logr"
 )
 
-// New returns a logr.Logger which is implemented by a function.
+// New returns a logr.Logger which is implemented by an arbitrary function.
 func New(fn func(prefix, args string), opts Options) logr.Logger {
 	fnl := &fnlogger{
 		prefix:    "",
@@ -44,6 +44,7 @@ func New(fn func(prefix, args string), opts Options) logr.Logger {
 	return logr.New(fnl)
 }
 
+// Options carries parameters which influence the way logs are generated.
 type Options struct {
 	// LogCaller tells funcr to add a "caller" key to some or all log lines.
 	// This has some overhead, so some users might not want it.
@@ -54,6 +55,7 @@ type Options struct {
 	Verbosity int
 }
 
+// MessageClass indicates which category or categories of messages to consider.
 type MessageClass int
 
 const (
@@ -72,6 +74,10 @@ type fnlogger struct {
 	verbosity int
 }
 
+// Assert conformance to the interfaces.
+var _ logr.LogSink = &fnlogger{}
+var _ logr.CallDepthLogSink = &fnlogger{}
+
 // Magic string for intermediate frames that we should ignore.
 const autogeneratedFrameName = "<autogenerated>"
 
@@ -340,6 +346,3 @@ func (l *fnlogger) WithCallDepth(depth int) logr.LogSink {
 	l2.depth += depth
 	return l2
 }
-
-var _ logr.LogSink = &fnlogger{}
-var _ logr.CallDepthLogSink = &fnlogger{}
diff --git a/logr.go b/logr.go
index 9ea027b..a2303e6 100644
--- a/logr.go
+++ b/logr.go
@@ -16,8 +16,6 @@ limitations under the License.
 
 // This design derives from Dave Cheney's blog:
 //     http://dave.cheney.net/2015/11/05/lets-talk-about-logging
-//
-// This is a BETA grade API.
 
 // Package logr defines a general-purpose logging API and abstract interfaces
 // to back that API.  Packages in the Go ecosystem can depend on this package,
@@ -220,9 +218,7 @@ func (l Logger) Info(msg string, keysAndValues ...interface{}) {
 // while the err argument should be used to attach the actual error that
 // triggered this log line, if present.
 func (l Logger) Error(err error, msg string, keysAndValues ...interface{}) {
-	if l.Enabled() {
-		l.sink.Error(err, msg, keysAndValues...)
-	}
+	l.sink.Error(err, msg, keysAndValues...)
 }
 
 // V returns a new Logger instance for a specific verbosity level, relative to


Test patch:
diff --git a/discard_test.go b/discard_test.go
index 4541ffc..55aff48 100644
--- a/discard_test.go
+++ b/discard_test.go
@@ -24,7 +24,7 @@ import (
 
 func TestDiscard(t *testing.T) {
 	l := Discard()
-	if _, ok := l.sink.(discardLogger); !ok {
+	if _, ok := l.sink.(discardLogSink); !ok {
 		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,18 @@ 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("discardLogger must always say it is disabled")
+		t.Error("discardLogSink must always say it is disabled")
 	}
 }
 
 func TestComparable(t *testing.T) {
 	a := Discard()
 	if !reflect.TypeOf(a).Comparable() {
-		t.Fatal("discardLogger must be comparable")
+		t.Fatal("discardLogSink must be comparable")
 	}
 
 	b := Discard()
 	if a != b {
-		t.Fatal("any two discardLoggers must be equal")
+		t.Fatal("any two discardLogSink must be equal")
 	}
 }
diff --git a/logr_test.go b/logr_test.go
index cb79a1b..8d27537 100644
--- a/logr_test.go
+++ b/logr_test.go
@@ -18,45 +18,338 @@ package logr
 
 import (
 	"context"
+	"fmt"
+	"reflect"
 	"testing"
 )
 
-// testLogSink is a Logger just for testing that does nothing.
-type testLogSink struct{}
+// testLogSink is a Logger just for testing that calls optional hooks on each method.
+type testLogSink struct {
+	fnInit       func(ri RuntimeInfo)
+	fnEnabled    func(lvl int) bool
+	fnInfo       func(lvl int, msg string, kv ...interface{})
+	fnError      func(err error, msg string, kv ...interface{})
+	fnWithValues func(kv ...interface{})
+	fnWithName   func(name string)
+}
+
+var _ LogSink = &testLogSink{}
 
-func (l *testLogSink) Init(RuntimeInfo) {
+func (l *testLogSink) Init(ri RuntimeInfo) {
+	if l.fnInit != nil {
+		l.fnInit(ri)
+	}
 }
 
-func (l *testLogSink) Enabled(int) bool {
+func (l *testLogSink) Enabled(lvl int) bool {
+	if l.fnEnabled != nil {
+		return l.fnEnabled(lvl)
+	}
 	return false
 }
 
-func (l *testLogSink) Info(level int, msg string, keysAndValues ...interface{}) {
+func (l *testLogSink) Info(lvl int, msg string, kv ...interface{}) {
+	if l.fnInfo != nil {
+		l.fnInfo(lvl, msg, kv...)
+	}
 }
 
-func (l *testLogSink) Error(err error, msg string, keysAndValues ...interface{}) {
+func (l *testLogSink) Error(err error, msg string, kv ...interface{}) {
+	if l.fnError != nil {
+		l.fnError(err, msg, kv...)
+	}
 }
 
-func (l *testLogSink) WithValues(keysAndValues ...interface{}) LogSink {
-	return l
+func (l *testLogSink) WithValues(kv ...interface{}) LogSink {
+	if l.fnWithValues != nil {
+		l.fnWithValues(kv...)
+	}
+	out := *l
+	return &out
 }
 
 func (l *testLogSink) WithName(name string) LogSink {
-	return l
+	if l.fnWithName != nil {
+		l.fnWithName(name)
+	}
+	out := *l
+	return &out
 }
 
-// Verify that it actually implements the interface
-var _ LogSink = &testLogSink{}
+type testCallDepthLogSink struct {
+	testLogSink
+	fnWithCallDepth func(depth int)
+}
+
+var _ CallDepthLogSink = &testCallDepthLogSink{}
+
+func (l *testCallDepthLogSink) WithCallDepth(depth int) LogSink {
+	if l.fnWithCallDepth != nil {
+		l.fnWithCallDepth(depth)
+	}
+	out := *l
+	return &out
+}
+
+func TestNew(t *testing.T) {
+	calledInit := 0
+
+	sink := &testLogSink{}
+	sink.fnInit = func(ri RuntimeInfo) {
+		if ri.CallDepth != 1 {
+			t.Errorf("expected runtimeInfo.CallDepth = 1, got %d", ri.CallDepth)
+		}
+		calledInit++
+	}
+	logger := New(sink)
+
+	if logger.sink == nil {
+		t.Errorf("expected sink to be set, got %v", logger.sink)
+	}
+	if calledInit != 1 {
+		t.Errorf("expected sink.Init() to be called once, got %d", calledInit)
+	}
+	if logger.withCallDepth != nil {
+		t.Errorf("expected withCallDepth to be nil, got %v", logger.withCallDepth)
+	}
+}
+
+func TestNewCachesCallDepthInterface(t *testing.T) {
+	sink := &testCallDepthLogSink{}
+	logger := New(sink)
+
+	if logger.withCallDepth == nil {
+		t.Errorf("expected withCallDepth to be set")
+	}
+}
+
+func TestEnabled(t *testing.T) {
+	calledEnabled := 0
+
+	sink := &testLogSink{}
+	sink.fnEnabled = func(lvl int) bool {
+		calledEnabled++
+		return true
+	}
+	logger := New(sink)
+
+	if en := logger.Enabled(); en != true {
+		t.Errorf("expected true")
+	}
+	if calledEnabled != 1 {
+		t.Errorf("expected sink.Enabled() to be called once, got %d", calledEnabled)
+	}
+}
+
+func TestError(t *testing.T) {
+	calledError := 0
+	errInput := fmt.Errorf("error")
+	msgInput := "msg"
+	kvInput := []interface{}{0, 1, 2}
+
+	sink := &testLogSink{}
+	sink.fnError = func(err error, msg string, kv ...interface{}) {
+		calledError++
+		if err != errInput {
+			t.Errorf("unexpected err input, got %v", err)
+		}
+		if msg != msgInput {
+			t.Errorf("unexpected msg input, got %q", msg)
+		}
+		if !reflect.DeepEqual(kv, kvInput) {
+			t.Errorf("unexpected kv input, got %v", kv)
+		}
+	}
+	logger := New(sink)
+
+	logger.Error(errInput, msgInput, kvInput...)
+	if calledError != 1 {
+		t.Errorf("expected sink.Error() to be called once, got %d", calledError)
+	}
+}
+
+func TestV(t *testing.T) {
+	sink := &testLogSink{}
+	logger := New(sink)
+
+	if l := logger.V(0); l.level != 0 {
+		t.Errorf("expected level 0, got %d", l.level)
+	}
+	if l := logger.V(93); l.level != 93 {
+		t.Errorf("expected level 93, got %d", l.level)
+	}
+	if l := logger.V(70).V(6); l.level != 76 {
+		t.Errorf("expected level 76, got %d", l.level)
+	}
+	if l := logger.V(-1); l.level != 0 {
+		t.Errorf("expected level 0, got %d", l.level)
+	}
+	if l := logger.V(1).V(-1); l.level != 1 {
+		t.Errorf("expected level 1, got %d", l.level)
+	}
+}
+
+func TestInfo(t *testing.T) {
+	calledEnabled := 0
+	calledInfo := 0
+	lvlInput := 0
+	msgInput := "msg"
+	kvInput := []interface{}{0, 1, 2}
+
+	sink := &testLogSink{}
+	sink.fnEnabled = func(lvl int) bool {
+		calledEnabled++
+		return lvl < 100
+	}
+	sink.fnInfo = func(lvl int, msg string, kv ...interface{}) {
+		calledInfo++
+		if lvl != lvlInput {
+			t.Errorf("unexpected lvl input, got %v", lvl)
+		}
+		if msg != msgInput {
+			t.Errorf("unexpected msg input, got %q", msg)
+		}
+		if !reflect.DeepEqual(kv, kvInput) {
+			t.Errorf("unexpected kv input, got %v", kv)
+		}
+	}
+	logger := New(sink)
+
+	calledEnabled = 0
+	calledInfo = 0
+	lvlInput = 0
+	logger.Info(msgInput, kvInput...)
+	if calledEnabled != 1 {
+		t.Errorf("expected sink.Enabled() to be called once, got %d", calledEnabled)
+	}
+	if calledInfo != 1 {
+		t.Errorf("expected sink.Info() to be called once, got %d", calledInfo)
+	}
+
+	calledEnabled = 0
+	calledInfo = 0
+	lvlInput = 0
+	logger.V(0).Info(msgInput, kvInput...)
+	if calledEnabled != 1 {
+		t.Errorf("expected sink.Enabled() to be called once, got %d", calledEnabled)
+	}
+	if calledInfo != 1 {
+		t.Errorf("expected sink.Info() to be called once, got %d", calledInfo)
+	}
+
+	calledEnabled = 0
+	calledInfo = 0
+	lvlInput = 93
+	logger.V(93).Info(msgInput, kvInput...)
+	if calledEnabled != 1 {
+		t.Errorf("expected sink.Enabled() to be called once, got %d", calledEnabled)
+	}
+	if calledInfo != 1 {
+		t.Errorf("expected sink.Info() to be called once, got %d", calledInfo)
+	}
+
+	calledEnabled = 0
+	calledInfo = 0
+	lvlInput = 100
+	logger.V(100).Info(msgInput, kvInput...)
+	if calledEnabled != 1 {
+		t.Errorf("expected sink.Enabled() to be called once, got %d", calledEnabled)
+	}
+	if calledInfo != 0 {
+		t.Errorf("expected sink.Info() to not be called, got %d", calledInfo)
+	}
+}
+
+func TestWithValues(t *testing.T) {
+	calledWithValues := 0
+	kvInput := []interface{}{"zero", 0, "one", 1, "two", 2}
+
+	sink := &testLogSink{}
+	sink.fnWithValues = func(kv ...interface{}) {
+		calledWithValues++
+		if !reflect.DeepEqual(kv, kvInput) {
+			t.Errorf("unexpected kv input, got %v", kv)
+		}
+	}
+	logger := New(sink)
+
+	out := logger.WithValues(kvInput...)
+	if calledWithValues != 1 {
+		t.Errorf("expected sink.WithValues() to be called once, got %d", calledWithValues)
+	}
+	if p := out.sink.(*testLogSink); p == sink {
+		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
+	}
+}
+
+func TestWithName(t *testing.T) {
+	calledWithName := 0
+	nameInput := "name"
+
+	sink := &testLogSink{}
+	sink.fnWithName = func(name string) {
+		calledWithName++
+		if name != nameInput {
+			t.Errorf("unexpected name input, got %q", name)
+		}
+	}
+	logger := New(sink)
+
+	out := logger.WithName(nameInput)
+	if calledWithName != 1 {
+		t.Errorf("expected sink.WithName() to be called once, got %d", calledWithName)
+	}
+	if p := out.sink.(*testLogSink); p == sink {
+		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
+	}
+}
+
+func TestWithCallDepthNotImplemented(t *testing.T) {
+	depthInput := 7
+
+	sink := &testLogSink{}
+	logger := New(sink)
+
+	out := logger.WithCallDepth(depthInput)
+	if p := out.sink.(*testLogSink); p != sink {
+		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
+	}
+}
+
+func TestWithCallDepthImplemented(t *testing.T) {
+	calledWithCallDepth := 0
+	depthInput := 7
+
+	sink := &testCallDepthLogSink{}
+	sink.fnWithCallDepth = func(depth int) {
+		calledWithCallDepth++
+		if depth != depthInput {
+			t.Errorf("unexpected depth input, got %d", depth)
+		}
+	}
+	logger := New(sink)
+
+	out := logger.WithCallDepth(depthInput)
+	if calledWithCallDepth != 1 {
+		t.Errorf("expected sink.WithCallDepth() to be called once, got %d", calledWithCallDepth)
+	}
+	if p := out.sink.(*testCallDepthLogSink); p == sink {
+		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
+	}
+}
 
 func TestContext(t *testing.T) {
 	ctx := context.TODO()
 
 	if out, err := FromContext(ctx); err == nil {
 		t.Errorf("expected error, got %#v", out)
+	} else if _, ok := err.(notFoundError); !ok {
+		t.Errorf("expected a notFoundError, got %#v", err)
 	}
+
 	out := FromContextOrDiscard(ctx)
-	if _, ok := out.sink.(discardLogger); !ok {
-		t.Errorf("expected a discardLogger, got %#v", out)
+	if _, ok := out.sink.(discardLogSink); !ok {
+		t.Errorf("expected a discardLogSink, got %#v", out)
 	}
 
 	sink := &testLogSink{}
@@ -65,48 +358,10 @@ func TestContext(t *testing.T) {
 	if out, err := FromContext(lctx); err != nil {
 		t.Errorf("unexpected error: %v", err)
 	} else if p := out.sink.(*testLogSink); p != sink {
-		t.Errorf("expected output to be the same as input: got in=%p, out=%p", sink, p)
+		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 	out = FromContextOrDiscard(lctx)
 	if p := out.sink.(*testLogSink); p != sink {
-		t.Errorf("expected output to be the same as input: got in=%p, out=%p", sink, p)
+		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 }
-
-// testCallDepthLogSink is a Logger just for testing that does nothing.
-type testCallDepthLogSink struct {
-	*testLogSink
-	depth int
-}
-
-func (l *testCallDepthLogSink) WithCallDepth(depth int) LogSink {
-	return &testCallDepthLogSink{l.testLogSink, l.depth + depth}
-}
-
-// Verify that it actually implements the interface
-var _ CallDepthLogSink = &testCallDepthLogSink{}
-
-func TestWithCallDepth(t *testing.T) {
-	// Test an impl that does not support it.
-	t.Run("not supported", func(t *testing.T) {
-		in := &testLogSink{}
-		l := New(in)
-		out := l.WithCallDepth(42)
-		if p := out.sink.(*testLogSink); p != in {
-			t.Errorf("expected output to be the same as input: got in=%p, out=%p", in, p)
-		}
-	})
-
-	// Test an impl that does support it.
-	t.Run("supported", func(t *testing.T) {
-		in := &testCallDepthLogSink{&testLogSink{}, 0}
-		l := New(in)
-		out := l.WithCallDepth(42)
-		if out.sink.(*testCallDepthLogSink) == in {
-			t.Errorf("expected output to be different than input: got in=out=%p", in)
-		}
-		if cdl := out.sink.(*testCallDepthLogSink); cdl.depth != 42 {
-			t.Errorf("expected depth=42, got %d", cdl.depth)
-		}
-	})
-}