From 6a4674cac178241b5af0a586bebd3f60ff119886 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 5 Jan 2021 10:35:27 -0800 Subject: [PATCH 1/5] Add missing header comments --- discard.go | 16 ++++++++++++++++ discard_test.go | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/discard.go b/discard.go index 267c796..b9d8419 100644 --- a/discard.go +++ b/discard.go @@ -1,3 +1,19 @@ +/* +Copyright 2020 The logr Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package logr // Discard returns a valid Logger that discards all messages logged to it. diff --git a/discard_test.go b/discard_test.go index 2ba2769..c14c37c 100644 --- a/discard_test.go +++ b/discard_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2020 The logr Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package logr import ( From 4eabc2e5e1a4f11de442201955a26a70e4e291e1 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 5 Jan 2021 10:45:47 -0800 Subject: [PATCH 2/5] Move comments arount for cleaner godoc --- logr.go | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/logr.go b/logr.go index e86896c..0916ad9 100644 --- a/logr.go +++ b/logr.go @@ -14,18 +14,15 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package logr defines abstract interfaces for logging. Packages can depend on -// these interfaces and callers can implement logging in whatever way is -// appropriate. -// // 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. Until there is a significant 2nd implementation, // I don't really know how it will change. -// -// The logging specifically makes it non-trivial to use format strings, to encourage -// attaching structured information instead of unstructured format strings. + +// Package logr defines abstract interfaces for logging. Packages can depend on +// these interfaces and callers can implement logging in whatever way is +// appropriate. // // Usage // @@ -114,14 +111,14 @@ limitations under the License. // generally best to avoid using the following keys, as they're frequently used // by implementations: // -// - `"caller"`: the calling information (file/line) of a particular log line. -// - `"error"`: the underlying error value in the `Error` method. -// - `"level"`: the log level. -// - `"logger"`: the name of the associated logger. -// - `"msg"`: the log message. -// - `"stacktrace"`: the stack trace associated with a particular log line or -// error (often from the `Error` message). -// - `"ts"`: the timestamp for a log line. +// * `"caller"`: the calling information (file/line) of a particular log line. +// * `"error"`: the underlying error value in the `Error` method. +// * `"level"`: the log level. +// * `"logger"`: the name of the associated logger. +// * `"msg"`: the log message. +// * `"stacktrace"`: the stack trace associated with a particular log line or +// error (often from the `Error` message). +// * `"ts"`: the timestamp for a log line. // // Implementations are encouraged to make use of these keys to represent the // above concepts, when necessary (for example, in a pure-JSON output form, it @@ -190,7 +187,9 @@ type Logger interface { WithName(name string) Logger } -// InfoLogger provides compatibility with code that relies on the v0.1.0 interface +// InfoLogger provides compatibility with code that relies on the v0.1.0 +// interface. +// // Deprecated: use Logger instead. This will be removed in a future release. type InfoLogger = Logger From 7efc46107329f0a7f95ca138a24a2a4c14aa3ad7 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 5 Jan 2021 11:14:13 -0800 Subject: [PATCH 3/5] Add tests for context funcs --- logr_test.go | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 logr_test.go diff --git a/logr_test.go b/logr_test.go new file mode 100644 index 0000000..4d50317 --- /dev/null +++ b/logr_test.go @@ -0,0 +1,76 @@ +/* +Copyright 2021 The logr Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package logr + +import ( + "context" + "testing" +) + +// testLogger is a Logger just for testing that does nothing. +type testLogger struct{} + +func (l *testLogger) Enabled() bool { + return false +} + +func (l *testLogger) Info(msg string, keysAndValues ...interface{}) { +} + +func (l *testLogger) Error(err error, msg string, keysAndValues ...interface{}) { +} + +func (l *testLogger) V(level int) Logger { + return l +} + +func (l *testLogger) WithValues(keysAndValues ...interface{}) Logger { + return l +} + +func (l *testLogger) WithName(name string) Logger { + return l +} + +// Verify that it actually implements the interface +var _ Logger = &testLogger{} + +func TestContext(t *testing.T) { + ctx := context.TODO() + + if out := FromContext(ctx); out != nil { + t.Errorf("expected nil logger, got %#v", out) + } + if out := FromContextOrDiscard(ctx); out == nil { + t.Errorf("expected non-nil logger") + } else if _, ok := out.(discardLogger); !ok { + t.Errorf("expected a discardLogger, got %#v", out) + } + + logger := &testLogger{} + lctx := NewContext(ctx, logger) + if out := FromContext(lctx); out == nil { + t.Errorf("expected non-nil logger") + } else if out.(*testLogger) != logger { + t.Errorf("expected output to be the same as input: got in=%p, out=%p", logger, out) + } + if out := FromContextOrDiscard(lctx); out == nil { + t.Errorf("expected non-nil logger") + } else if out.(*testLogger) != logger { + t.Errorf("expected output to be the same as input: got in=%p, out=%p", logger, out) + } +} From 85b0f8df8c2f96c6ba50d550908be62babe852e6 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 5 Jan 2021 11:36:26 -0800 Subject: [PATCH 4/5] Effectively EOL testing.NullLogger It remains as a type, but is just an alias to DiscardLogger. --- discard.go | 20 ++++++++++---------- discard_test.go | 2 +- logr.go | 6 ++++-- logr_test.go | 4 ++-- testing/null.go | 32 +++++--------------------------- 5 files changed, 22 insertions(+), 42 deletions(-) diff --git a/discard.go b/discard.go index b9d8419..2bafb13 100644 --- a/discard.go +++ b/discard.go @@ -19,33 +19,33 @@ package logr // Discard returns a valid Logger that discards all messages logged to it. // It can be used whenever the caller is not interested in the logs. func Discard() Logger { - return discardLogger{} + return DiscardLogger{} } -// discardLogger is a Logger that discards all messages. -type discardLogger struct{} +// DiscardLogger is a Logger that discards all messages. +type DiscardLogger struct{} -func (l discardLogger) Enabled() bool { +func (l DiscardLogger) Enabled() bool { return false } -func (l discardLogger) Info(msg string, keysAndValues ...interface{}) { +func (l DiscardLogger) Info(msg string, keysAndValues ...interface{}) { } -func (l discardLogger) Error(err error, msg string, keysAndValues ...interface{}) { +func (l DiscardLogger) Error(err error, msg string, keysAndValues ...interface{}) { } -func (l discardLogger) V(level int) Logger { +func (l DiscardLogger) V(level int) Logger { return l } -func (l discardLogger) WithValues(keysAndValues ...interface{}) Logger { +func (l DiscardLogger) WithValues(keysAndValues ...interface{}) Logger { return l } -func (l discardLogger) WithName(name string) Logger { +func (l DiscardLogger) WithName(name string) Logger { return l } // Verify that it actually implements the interface -var _ Logger = discardLogger{} +var _ Logger = DiscardLogger{} diff --git a/discard_test.go b/discard_test.go index c14c37c..e2a01cb 100644 --- a/discard_test.go +++ b/discard_test.go @@ -23,7 +23,7 @@ import ( func TestDiscard(t *testing.T) { l := Discard() - if _, ok := l.(discardLogger); !ok { + if _, ok := l.(DiscardLogger); !ok { t.Error("did not return the expected underlying type") } // Verify that none of the methods panic, there is not more we can test. diff --git a/logr.go b/logr.go index 0916ad9..0fbcd91 100644 --- a/logr.go +++ b/logr.go @@ -190,7 +190,9 @@ type Logger interface { // InfoLogger provides compatibility with code that relies on the v0.1.0 // interface. // -// Deprecated: use Logger instead. This will be removed in a future release. +// Deprecated: InfoLogger is an artifact of early versions of this API. New +// users should never use it and existing users should use Logger instead. This +// will be removed in a future release. type InfoLogger = Logger type contextKey struct{} @@ -212,7 +214,7 @@ func FromContextOrDiscard(ctx context.Context) Logger { return v } - return discardLogger{} + return Discard() } // NewContext returns a new context derived from ctx that embeds the Logger. diff --git a/logr_test.go b/logr_test.go index 4d50317..703c7fd 100644 --- a/logr_test.go +++ b/logr_test.go @@ -57,8 +57,8 @@ func TestContext(t *testing.T) { } if out := FromContextOrDiscard(ctx); out == nil { t.Errorf("expected non-nil logger") - } else if _, ok := out.(discardLogger); !ok { - t.Errorf("expected a discardLogger, got %#v", out) + } else if _, ok := out.(DiscardLogger); !ok { + t.Errorf("expected a DiscardLogger, got %#v", out) } logger := &testLogger{} diff --git a/testing/null.go b/testing/null.go index e3a5351..f5444b1 100644 --- a/testing/null.go +++ b/testing/null.go @@ -19,30 +19,8 @@ package testing import "github.com/go-logr/logr" // NullLogger is a logr.Logger that does nothing. -type NullLogger struct{} - -var _ logr.Logger = NullLogger{} - -func (_ NullLogger) Info(_ string, _ ...interface{}) { - // Do nothing. -} - -func (_ NullLogger) Enabled() bool { - return false -} - -func (_ NullLogger) Error(_ error, _ string, _ ...interface{}) { - // Do nothing. -} - -func (log NullLogger) V(_ int) logr.Logger { - return log -} - -func (log NullLogger) WithName(_ string) logr.Logger { - return log -} - -func (log NullLogger) WithValues(_ ...interface{}) logr.Logger { - return log -} +// +// Deprecated: NullLogger is idenitcal to logr.DiscardLogger. It is retained +// for backwards compatibility, but new users should use logr.DiscardLogger +// instead. +type NullLogger = logr.DiscardLogger From 52a060803ca53ec6ed3e772dd16ae2f6fc62e7fc Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 5 Jan 2021 10:49:39 -0800 Subject: [PATCH 5/5] Add optional CallDepth support This adds an optional extension interface which log implementations may implement, plus a helper function to use this interface if it is supported. Discussed in https://github.com/go-logr/logr/pull/31 and specifically https://github.com/go-logr/logr/pull/31#issuecomment-754180027 --- logr.go | 45 ++++++++++++++++++++++++++++++++++++++++++++- logr_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/logr.go b/logr.go index 0916ad9..8705b78 100644 --- a/logr.go +++ b/logr.go @@ -142,7 +142,7 @@ import ( // TODO: consider adding back in format strings if they're really needed // TODO: consider other bits of zap/zapcore functionality like ObjectMarshaller (for arbitrary objects) -// TODO: consider other bits of glog functionality like Flush, InfoDepth, OutputStats +// TODO: consider other bits of glog functionality like Flush, OutputStats // Logger represents the ability to log messages, both errors and not. type Logger interface { @@ -219,3 +219,46 @@ func FromContextOrDiscard(ctx context.Context) Logger { func NewContext(ctx context.Context, l Logger) context.Context { return context.WithValue(ctx, contextKey{}, l) } + +// CallDepthLogger represents a Logger that knows how to climb the call stack +// to identify the original call site and can offset the depth by a specified +// number of frames. This is useful for users who have helper functions +// between the "real" call site and the actual calls to Logger methods. +// Implementations that log information about the call site (such as file, +// function, or line) would otherwise log information about the intermediate +// helper functions. +// +// This is an optional interface and implementations are not required to +// support it. +type CallDepthLogger interface { + Logger + + // WithCallDepth returns a Logger that will offset the call stack by the + // specified number of frames when logging call site information. If depth + // is 0 the attribution should be to the direct caller of this method. If + // depth is 1 the attribution should skip 1 call frame, and so on. + // Successive calls to this are additive. + WithCallDepth(depth int) Logger +} + +// WithCallDepth returns a Logger that will offset the call stack by the +// specified number of frames when logging call site information, if possible. +// This is useful for users who have helper functions between the "real" call +// site and the actual calls to Logger methods. If depth is 0 the attribution +// should be to the direct caller of this function. If depth is 1 the +// attribution should skip 1 call frame, and so on. Successive calls to this +// are additive. +// +// If the underlying log implementation supports the CallDepthLogger interface, +// the WithCallDepth method will be called and the result returned. If the +// implementation does not support CallDepthLogger, the original Logger will be +// returned. +// +// Callers which care about whether this was supported or not should test for +// CallDepthLogger support themselves. +func WithCallDepth(logger Logger, depth int) Logger { + if decorator, ok := logger.(CallDepthLogger); ok { + return decorator.WithCallDepth(depth) + } + return logger +} diff --git a/logr_test.go b/logr_test.go index 4d50317..f45c29f 100644 --- a/logr_test.go +++ b/logr_test.go @@ -74,3 +74,39 @@ func TestContext(t *testing.T) { t.Errorf("expected output to be the same as input: got in=%p, out=%p", logger, out) } } + +// testCallDepthLogger is a Logger just for testing that does nothing. +type testCallDepthLogger struct { + *testLogger + depth int +} + +func (l *testCallDepthLogger) WithCallDepth(depth int) Logger { + return &testCallDepthLogger{l.testLogger, l.depth + depth} +} + +// Verify that it actually implements the interface +var _ CallDepthLogger = &testCallDepthLogger{} + +func TestWithCallDepth(t *testing.T) { + // Test an impl that does not support it. + t.Run("not supported", func(t *testing.T) { + in := &testLogger{} + out := WithCallDepth(in, 42) + if out.(*testLogger) != in { + t.Errorf("expected output to be the same as input: got in=%p, out=%p", in, out) + } + }) + + // Test an impl that does support it. + t.Run("supported", func(t *testing.T) { + in := &testCallDepthLogger{&testLogger{}, 0} + out := WithCallDepth(in, 42) + if out.(*testCallDepthLogger) == in { + t.Errorf("expected output to be different than input: got in=out=%p", in) + } + if cdl := out.(*testCallDepthLogger); cdl.depth != 42 { + t.Errorf("expected depth=42, got %d", cdl.depth) + } + }) +}