From 059d32ca1923b1517abaf9501398af5092f1e948 Mon Sep 17 00:00:00 2001 From: Balki <189196+balki@users.noreply.github.com> Date: Fri, 17 Jun 2022 21:39:45 +0000 Subject: [PATCH 01/15] Fix syntax in documentation --- logr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/logr.go b/logr.go index c3b56b3..25c2884 100644 --- a/logr.go +++ b/logr.go @@ -169,7 +169,7 @@ limitations under the License. // // Logger grants access to the sink to enable type assertions like this: // func DoSomethingWithImpl(log logr.Logger) { -// if underlier, ok := log.GetSink()(impl.Underlier) { +// if underlier, ok := log.GetSink().(impl.Underlier); ok { // implLogger := underlier.GetUnderlying() // ... // } @@ -181,7 +181,7 @@ limitations under the License. // // new logger with that modified sink. It does nothing for loggers where // // the sink doesn't support that parameter. // func WithFoobar(log logr.Logger, foobar int) logr.Logger { -// if foobarLogSink, ok := log.GetSink()(FoobarSink); ok { +// if foobarLogSink, ok := log.GetSink().(FoobarSink); ok { // log = log.WithSink(foobarLogSink.WithFooBar(foobar)) // } // return log From a326b849c513510051eb8cffd44093f2cc74c529 Mon Sep 17 00:00:00 2001 From: Alexis Jeandeau Date: Wed, 1 Jun 2022 20:19:34 +0900 Subject: [PATCH 02/15] testr: use an interface to make it work with *testing.B and *testing.F in addition to *testing.T --- .github/workflows/tests.yaml | 2 +- testr/testr.go | 99 +++++++++++++++++++++++++++++++----- testr/testr_fuzz_test.go | 27 ++++++++++ testr/testr_test.go | 39 +++++++++++++- 4 files changed, 153 insertions(+), 14 deletions(-) create mode 100644 testr/testr_fuzz_test.go diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index adc999f..2b7f2b5 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -6,7 +6,7 @@ jobs: test: strategy: matrix: - version: [ '1.15', '1.16', '1.17' ] + version: [ '1.15', '1.16', '1.17', '1.18' ] platform: [ ubuntu-latest, macos-latest, windows-latest ] runs-on: ${{ matrix.platform }} steps: diff --git a/testr/testr.go b/testr/testr.go index 6fa2783..aaf7de5 100644 --- a/testr/testr.go +++ b/testr/testr.go @@ -59,6 +59,26 @@ func NewWithOptions(t *testing.T, opts Options) logr.Logger { return logr.New(l) } +// TestingT is an interface wrapper around testing.T, testing.B and testing.F. +type TestingT interface { + Helper() + Log(args ...interface{}) +} + +// NewWithInterface returns a logr.Logger that prints through a +// TestingT object. +// In contrast to the simpler New, output formatting can be configured. +func NewWithInterface(t TestingT, opts Options) logr.Logger { + l := &testloggerInterface{ + Formatter: funcr.NewFormatter(funcr.Options{ + LogTimestamp: opts.LogTimestamp, + Verbosity: opts.Verbosity, + }), + t: t, + } + return logr.New(l) +} + // Underlier exposes access to the underlying testing.T instance. Since // callers only have a logr.Logger, they have to know which // implementation is in use, so this interface is less of an @@ -67,6 +87,34 @@ type Underlier interface { GetUnderlying() *testing.T } +// UnderlierInterface exposes access to the underlying TestingT instance. Since +// callers only have a logr.Logger, they have to know which +// implementation is in use, so this interface is less of an +// abstraction and more of a way to test type conversion. +type UnderlierInterface interface { + GetUnderlying() TestingT +} + +// Info logging implementation shared between testLogger and testLoggerInterface. +func logInfo(t TestingT, formatInfo func(int, string, []interface{}) (string, string), level int, msg string, kvList ...interface{}) { + prefix, args := formatInfo(level, msg, kvList) + t.Helper() + if prefix != "" { + args = prefix + ": " + args + } + t.Log(args) +} + +// Error logging implementation shared between testLogger and testLoggerInterface. +func logError(t TestingT, formatError func(error, string, []interface{}) (string, string), err error, msg string, kvList ...interface{}) { + prefix, args := formatError(err, msg, kvList) + t.Helper() + if prefix != "" { + args = prefix + ": " + args + } + t.Log(args) +} + type testlogger struct { funcr.Formatter t *testing.T @@ -87,30 +135,57 @@ func (l testlogger) GetCallStackHelper() func() { } func (l testlogger) Info(level int, msg string, kvList ...interface{}) { - prefix, args := l.FormatInfo(level, msg, kvList) l.t.Helper() - if prefix != "" { - l.t.Logf("%s: %s", prefix, args) - } else { - l.t.Log(args) - } + logInfo(l.t, l.FormatInfo, level, msg, kvList...) } func (l testlogger) Error(err error, msg string, kvList ...interface{}) { - prefix, args := l.FormatError(err, msg, kvList) l.t.Helper() - if prefix != "" { - l.t.Logf("%s: %s", prefix, args) - } else { - l.t.Log(args) - } + logError(l.t, l.FormatError, err, msg, kvList...) } func (l testlogger) GetUnderlying() *testing.T { return l.t } +type testloggerInterface struct { + funcr.Formatter + t TestingT +} + +func (l testloggerInterface) WithName(name string) logr.LogSink { + l.Formatter.AddName(name) + return &l +} + +func (l testloggerInterface) WithValues(kvList ...interface{}) logr.LogSink { + l.Formatter.AddValues(kvList) + return &l +} + +func (l testloggerInterface) GetCallStackHelper() func() { + return l.t.Helper +} + +func (l testloggerInterface) Info(level int, msg string, kvList ...interface{}) { + l.t.Helper() + logInfo(l.t, l.FormatInfo, level, msg, kvList...) +} + +func (l testloggerInterface) Error(err error, msg string, kvList ...interface{}) { + l.t.Helper() + logError(l.t, l.FormatError, err, msg, kvList...) +} + +func (l testloggerInterface) GetUnderlying() TestingT { + return l.t +} + // Assert conformance to the interfaces. var _ logr.LogSink = &testlogger{} var _ logr.CallStackHelperLogSink = &testlogger{} var _ Underlier = &testlogger{} + +var _ logr.LogSink = &testloggerInterface{} +var _ logr.CallStackHelperLogSink = &testloggerInterface{} +var _ UnderlierInterface = &testloggerInterface{} diff --git a/testr/testr_fuzz_test.go b/testr/testr_fuzz_test.go new file mode 100644 index 0000000..d1def0e --- /dev/null +++ b/testr/testr_fuzz_test.go @@ -0,0 +1,27 @@ +//go:build go1.18 +// +build go1.18 + +/* +Copyright 2022 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 testr + +import "testing" + +func TestLoggerTestingF(t *testing.T) { + f := &testing.F{} + _ = NewWithInterface(f, Options{}) +} diff --git a/testr/testr_test.go b/testr/testr_test.go index 0d71a01..4a737b7 100644 --- a/testr/testr_test.go +++ b/testr/testr_test.go @@ -29,7 +29,8 @@ func TestLogger(t *testing.T) { log.V(0).Info("V(0).info") log.V(1).Info("v(1).info") log.Error(fmt.Errorf("error"), "error") - log.WithName("testing").Info("with prefix") + log.WithName("testing").WithValues("value", "test").Info("with prefix") + log.WithName("testing").Error(fmt.Errorf("error"), "with prefix") Helper(log, "hello world") log = NewWithOptions(t, Options{ @@ -37,6 +38,42 @@ func TestLogger(t *testing.T) { Verbosity: 1, }) log.V(1).Info("v(1).info with options") + + underlier, ok := log.GetSink().(Underlier) + if !ok { + t.Error("couldn't get underlier") + } + if t != underlier.GetUnderlying() { + t.Error("invalid underlier") + } +} + +func TestLoggerInterface(t *testing.T) { + log := NewWithInterface(t, Options{}) + log.Info("info") + log.V(0).Info("V(0).info") + log.V(1).Info("v(1).info") + log.Error(fmt.Errorf("error"), "error") + log.WithName("testing").WithValues("value", "test").Info("with prefix") + log.WithName("testing").Error(fmt.Errorf("error"), "with prefix") + Helper(log, "hello world") + + underlier, ok := log.GetSink().(UnderlierInterface) + if !ok { + t.Fatal("couldn't get underlier") + } + underlierT, ok := underlier.GetUnderlying().(*testing.T) + if !ok { + t.Fatal("couldn't get underlying *testing.T") + } + if t != underlierT { + t.Error("invalid underlier") + } +} + +func TestLoggerTestingB(t *testing.T) { + b := &testing.B{} + _ = NewWithInterface(b, Options{}) } func Helper(log logr.Logger, msg string) { From abbd0bc3fdfb5b70acab6905cddca54c9b9dc209 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 14 Jul 2022 14:57:39 -0700 Subject: [PATCH 03/15] Bump Go version for GH actions --- .github/workflows/apidiff.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/apidiff.yaml b/.github/workflows/apidiff.yaml index 340fffb..a76a30c 100644 --- a/.github/workflows/apidiff.yaml +++ b/.github/workflows/apidiff.yaml @@ -10,7 +10,7 @@ jobs: - name: Install Go uses: actions/setup-go@v2 with: - go-version: 1.16.x + go-version: 1.18.x - name: Add GOBIN to PATH run: echo "PATH=$(go env GOPATH)/bin:$PATH" >>$GITHUB_ENV - name: Install dependencies From 8f2e5573da0cdeacb3d4a69d26ab1fddc7ee8214 Mon Sep 17 00:00:00 2001 From: James Chacon Date: Thu, 10 Nov 2022 10:58:50 -0800 Subject: [PATCH 04/15] If logging as JSON and the type is json.RawMessage log it "as-is" (#147) If we're already producing JSON and a sub message field is json.RawMessage (i.e. []byte) then emit it as-is. Otherwise you get [X,Y,Z,..] style which is pretty useless in log in that form. --- funcr/funcr.go | 15 +++++++++++++++ funcr/funcr_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/funcr/funcr.go b/funcr/funcr.go index 7accdb0..99dfe42 100644 --- a/funcr/funcr.go +++ b/funcr/funcr.go @@ -37,6 +37,7 @@ package funcr import ( "bytes" "encoding" + "encoding/json" "fmt" "path/filepath" "reflect" @@ -500,6 +501,20 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32, depth int) s } return buf.String() case reflect.Slice, reflect.Array: + // If this is outputing as JSON make sure this isn't really a json.RawMessage. + // If so just emit "as-is" and don't pretty it as that will just print + // it as [X,Y,Z,...] which isn't terribly useful vs the string form you really want. + if f.outputFormat == outputJSON { + if rm, ok := value.(json.RawMessage); ok { + // If it's empty make sure we emit an empty value as the array style would below. + if len(rm) > 0 { + buf.Write(rm) + } else { + buf.WriteString("null") + } + return buf.String() + } + } buf.WriteByte('[') for i := 0; i < v.Len(); i++ { if i > 0 { diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go index b888fe4..20b26a5 100644 --- a/funcr/funcr_test.go +++ b/funcr/funcr_test.go @@ -230,6 +230,10 @@ type Tembedjsontags struct { Tinner6 `json:"inner6,omitempty"` } +type Trawjson struct { + Message json.RawMessage `json:"message"` +} + func TestPretty(t *testing.T) { // used below newStr := func(s string) *string { @@ -669,6 +673,15 @@ func makeKV(args ...interface{}) []interface{} { } func TestRender(t *testing.T) { + // used below + raw := &Trawjson{} + marshal := &TjsontagsInt{} + var err error + raw.Message, err = json.Marshal(marshal) + if err != nil { + t.Fatalf("json.Marshal error: %v", err) + } + testCases := []struct { name string builtins []interface{} @@ -738,6 +751,21 @@ func TestRender(t *testing.T) { }{"arg", 789}, "val"), expectKV: `""="val" ""="val" ""="val"`, expectJSON: `{"":"val","":"val","":"val"}`, + }, { + name: "json rendering with json.RawMessage", + args: makeKV("key", raw), + expectKV: `"key"={"message":[123,34,105,110,116,49,34,58,48,44,34,45,34,58,48,44,34,73,110,116,53,34,58,48,125]}`, + expectJSON: `{"key":{"message":{"int1":0,"-":0,"Int5":0}}}`, + }, { + name: "byte array not json.RawMessage", + args: makeKV("key", []byte{1, 2, 3, 4}), + expectKV: `"key"=[1,2,3,4]`, + expectJSON: `{"key":[1,2,3,4]}`, + }, { + name: "json rendering with empty json.RawMessage", + args: makeKV("key", &Trawjson{}), + expectKV: `"key"={"message":[]}`, + expectJSON: `{"key":{"message":null}}`, }} for _, tc := range testCases { From b3dc695bce6838cb3f27ac700c1017947bc3235b Mon Sep 17 00:00:00 2001 From: Konrad Wojas Date: Fri, 28 Oct 2022 13:57:23 +0800 Subject: [PATCH 05/15] Make zero value useful and add .IsZero() To accomodate optional loggers, make the zero value of a logger useful and not trigger panics when its methods are called. --- logr.go | 16 +++++++++++++++- logr_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/logr.go b/logr.go index 25c2884..f720386 100644 --- a/logr.go +++ b/logr.go @@ -244,7 +244,7 @@ type Logger struct { // Enabled tests whether this Logger is enabled. For example, commandline // flags might be used to set the logging verbosity and disable some info logs. func (l Logger) Enabled() bool { - return l.sink.Enabled(l.level) + return l.sink != nil && l.sink.Enabled(l.level) } // Info logs a non-error message with the given key/value pairs as context. @@ -273,6 +273,9 @@ func (l Logger) Info(msg string, keysAndValues ...interface{}) { // triggered this log line, if present. The err parameter is optional // and nil may be passed instead of an error instance. func (l Logger) Error(err error, msg string, keysAndValues ...interface{}) { + if l.sink == nil { + return + } if withHelper, ok := l.sink.(CallStackHelperLogSink); ok { withHelper.GetCallStackHelper()() } @@ -294,6 +297,9 @@ func (l Logger) V(level int) Logger { // WithValues returns a new Logger instance with additional key/value pairs. // See Info for documentation on how key/value pairs work. func (l Logger) WithValues(keysAndValues ...interface{}) Logger { + if l.sink == nil { + return l + } l.setSink(l.sink.WithValues(keysAndValues...)) return l } @@ -304,6 +310,9 @@ func (l Logger) WithValues(keysAndValues ...interface{}) Logger { // contain only letters, digits, and hyphens (see the package documentation for // more information). func (l Logger) WithName(name string) Logger { + if l.sink == nil { + return l + } l.setSink(l.sink.WithName(name)) return l } @@ -357,6 +366,11 @@ func (l Logger) WithCallStackHelper() (func(), Logger) { return helper, l } +// IsZero returns true if this logger is an uninitialized zero value +func (l Logger) IsZero() bool { + return l.sink == nil +} + // contextKey is how we find Loggers in a context.Context. type contextKey struct{} diff --git a/logr_test.go b/logr_test.go index 3f63818..3ca0c7e 100644 --- a/logr_test.go +++ b/logr_test.go @@ -18,6 +18,7 @@ package logr import ( "context" + "errors" "fmt" "reflect" "testing" @@ -400,3 +401,42 @@ func TestContext(t *testing.T) { t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p) } } + +func TestIsZero(t *testing.T) { + var l Logger + if !l.IsZero() { + t.Errorf("expected IsZero") + } + sink := &testLogSink{} + l = New(sink) + 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. + l = Discard() + if l.IsZero() { + t.Errorf("expected not IsZero") + } +} + +func TestZeroValue(t *testing.T) { + // Make sure that the zero value is useful and equivalent to a Discard logger. + var l Logger + if l.Enabled() { + t.Errorf("expected not Enabled") + } + if !l.IsZero() { + t.Errorf("expected IsZero") + } + // Make sure that none of these methods cause a crash + l.Info("foo") + l.Error(errors.New("bar"), "some error") + if l.GetSink() != nil { + t.Errorf("expected nil from GetSink") + } + l2 := l.WithName("some-name").V(2).WithValues("foo", 1).WithCallDepth(1) + l2.Info("foo") + l2.Error(errors.New("bar"), "some error") + _, _ = l.WithCallStackHelper() +} From 772cb5388e7a86197eb52535170ddc1b9e080423 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 10 Nov 2022 13:21:08 -0800 Subject: [PATCH 06/15] Remove deprecated linters --- .golangci.yaml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 94ff801..0cffafa 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -6,7 +6,6 @@ linters: disable-all: true enable: - asciicheck - - deadcode - errcheck - forcetypeassert - gocritic @@ -18,10 +17,8 @@ linters: - misspell - revive - staticcheck - - structcheck - typecheck - unused - - varcheck issues: exclude-use-default: false From 46d9b0546f33848e3c13f661b7bd4b2845d7f441 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 10 Nov 2022 13:25:40 -0800 Subject: [PATCH 07/15] Use newer gofmt --- examples/usage_example.go | 30 ++++----- funcr/funcr.go | 4 +- logr.go | 127 +++++++++++++++++++++----------------- 3 files changed, 86 insertions(+), 75 deletions(-) diff --git a/examples/usage_example.go b/examples/usage_example.go index dc337f3..a826d9c 100644 --- a/examples/usage_example.go +++ b/examples/usage_example.go @@ -35,31 +35,31 @@ import ( var objectMap = map[string]Object{ "obj1": Object{ - Name: "obj1", - Kind: "one", + Name: "obj1", + Kind: "one", Details: 33, }, "obj2": Object{ - Name: "obj2", - Kind: "two", + Name: "obj2", + Kind: "two", Details: "hi", }, "obj3": Object{ - Name: "obj3", - Kind: "one", + Name: "obj3", + Kind: "one", Details: 1, }, } type Object struct { - Name string - Kind string + Name string + Kind string Details interface{} } type Client struct { objects map[string]Object - log logr.Logger + log logr.Logger } func (c *Client) Get(key string) (Object, error) { @@ -82,7 +82,7 @@ func (c *Client) Save(obj Object) error { } func (c *Client) WatchNext() string { - time.Sleep(2*time.Second) + time.Sleep(2 * time.Second) keyInd := rand.Intn(len(c.objects)) @@ -99,9 +99,9 @@ func (c *Client) WatchNext() string { } type Controller struct { - log logr.Logger + log logr.Logger expectedKind string - client *Client + client *Client } func (c *Controller) Run() { @@ -144,13 +144,13 @@ func (c *Controller) Run() { func NewController(log logr.Logger, objectKind string) *Controller { ctrlLogger := log.WithName("controller").WithName(objectKind) client := &Client{ - log: ctrlLogger.WithName("client"), + log: ctrlLogger.WithName("client"), objects: objectMap, } return &Controller{ - log: ctrlLogger, + log: ctrlLogger, expectedKind: objectKind, - client: client, + client: client, } } diff --git a/funcr/funcr.go b/funcr/funcr.go index 99dfe42..e83e408 100644 --- a/funcr/funcr.go +++ b/funcr/funcr.go @@ -21,13 +21,13 @@ limitations under the License. // github.com/go-logr/logr.LogSink with output through an arbitrary // "write" function. See New and NewJSON for details. // -// Custom LogSinks +// # Custom LogSinks // // For users who need more control, a funcr.Formatter can be embedded inside // your own custom LogSink implementation. This is useful when the LogSink // needs to implement additional methods, for example. // -// Formatting +// # Formatting // // This will respect logr.Marshaler, fmt.Stringer, and error interfaces for // values which are being logged. When rendering a struct, funcr will use Go's diff --git a/logr.go b/logr.go index f720386..87886aa 100644 --- a/logr.go +++ b/logr.go @@ -21,7 +21,7 @@ limitations under the License. // to back that API. Packages in the Go ecosystem can depend on this package, // while callers can implement logging with whatever backend is appropriate. // -// Usage +// # Usage // // Logging is done using a Logger instance. Logger is a concrete type with // methods, which defers the actual logging to a LogSink interface. The main @@ -30,16 +30,20 @@ limitations under the License. // "structured logging". // // With Go's standard log package, we might write: -// log.Printf("setting target value %s", targetValue) +// +// log.Printf("setting target value %s", targetValue) // // With logr's structured logging, we'd write: -// logger.Info("setting target", "value", targetValue) +// +// logger.Info("setting target", "value", targetValue) // // Errors are much the same. Instead of: -// log.Printf("failed to open the pod bay door for user %s: %v", user, err) +// +// log.Printf("failed to open the pod bay door for user %s: %v", user, err) // // We'd write: -// logger.Error(err, "failed to open the pod bay door", "user", user) +// +// logger.Error(err, "failed to open the pod bay door", "user", user) // // Info() and Error() are very similar, but they are separate methods so that // LogSink implementations can choose to do things like attach additional @@ -47,7 +51,7 @@ limitations under the License. // always logged, regardless of the current verbosity. If there is no error // instance available, passing nil is valid. // -// Verbosity +// # Verbosity // // Often we want to log information only when the application in "verbose // mode". To write log lines that are more verbose, Logger has a V() method. @@ -58,20 +62,22 @@ limitations under the License. // Error messages do not have a verbosity level and are always logged. // // Where we might have written: -// if flVerbose >= 2 { -// log.Printf("an unusual thing happened") -// } +// +// if flVerbose >= 2 { +// log.Printf("an unusual thing happened") +// } // // We can write: -// logger.V(2).Info("an unusual thing happened") // -// Logger Names +// logger.V(2).Info("an unusual thing happened") +// +// # Logger Names // // Logger instances can have name strings so that all messages logged through // that instance have additional context. For example, you might want to add // a subsystem name: // -// logger.WithName("compactor").Info("started", "time", time.Now()) +// logger.WithName("compactor").Info("started", "time", time.Now()) // // The WithName() method returns a new Logger, which can be passed to // constructors or other functions for further use. Repeated use of WithName() @@ -82,25 +88,27 @@ limitations under the License. // joining operation (e.g. whitespace, commas, periods, slashes, brackets, // quotes, etc). // -// Saved Values +// # Saved Values // // Logger instances can store any number of key/value pairs, which will be // logged alongside all messages logged through that instance. For example, // you might want to create a Logger instance per managed object: // // With the standard log package, we might write: -// log.Printf("decided to set field foo to value %q for object %s/%s", -// targetValue, object.Namespace, object.Name) +// +// log.Printf("decided to set field foo to value %q for object %s/%s", +// targetValue, object.Namespace, object.Name) // // With logr we'd write: -// // Elsewhere: set up the logger to log the object name. -// obj.logger = mainLogger.WithValues( -// "name", obj.name, "namespace", obj.namespace) // -// // later on... -// obj.logger.Info("setting foo", "value", targetValue) +// // Elsewhere: set up the logger to log the object name. +// obj.logger = mainLogger.WithValues( +// "name", obj.name, "namespace", obj.namespace) +// +// // later on... +// obj.logger.Info("setting foo", "value", targetValue) // -// Best Practices +// # Best Practices // // Logger has very few hard rules, with the goal that LogSink implementations // might have a lot of freedom to differentiate. There are, however, some @@ -124,15 +132,15 @@ limitations under the License. // around. For cases where passing a logger is optional, a pointer to Logger // should be used. // -// Key Naming Conventions +// # Key Naming Conventions // // Keys are not strictly required to conform to any specification or regex, but // it is recommended that they: -// * be human-readable and meaningful (not auto-generated or simple ordinals) -// * be constant (not dependent on input data) -// * contain only printable characters -// * not contain whitespace or punctuation -// * use lower case for simple keys and lowerCamelCase for more complex ones +// - be human-readable and meaningful (not auto-generated or simple ordinals) +// - be constant (not dependent on input data) +// - contain only printable characters +// - not contain whitespace or punctuation +// - use lower case for simple keys and lowerCamelCase for more complex ones // // These guidelines help ensure that log data is processed properly regardless // of the log implementation. For example, log implementations will try to @@ -141,51 +149,54 @@ limitations under the License. // While users are generally free to use key names of their choice, it's // 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 // would be necessary to represent at least message and timestamp as ordinary // named values). // -// Break Glass +// # Break Glass // // Implementations may choose to give callers access to the underlying // logging implementation. The recommended pattern for this is: -// // Underlier exposes access to the underlying logging implementation. -// // Since callers only have a logr.Logger, they have to know which -// // implementation is in use, so this interface is less of an abstraction -// // and more of way to test type conversion. -// type Underlier interface { -// GetUnderlying() -// } +// +// // Underlier exposes access to the underlying logging implementation. +// // Since callers only have a logr.Logger, they have to know which +// // implementation is in use, so this interface is less of an abstraction +// // and more of way to test type conversion. +// type Underlier interface { +// GetUnderlying() +// } // // Logger grants access to the sink to enable type assertions like this: -// func DoSomethingWithImpl(log logr.Logger) { -// if underlier, ok := log.GetSink().(impl.Underlier); ok { -// implLogger := underlier.GetUnderlying() -// ... -// } -// } +// +// func DoSomethingWithImpl(log logr.Logger) { +// if underlier, ok := log.GetSink().(impl.Underlier); ok { +// implLogger := underlier.GetUnderlying() +// ... +// } +// } // // Custom `With*` functions can be implemented by copying the complete // Logger struct and replacing the sink in the copy: -// // WithFooBar changes the foobar parameter in the log sink and returns a -// // new logger with that modified sink. It does nothing for loggers where -// // the sink doesn't support that parameter. -// func WithFoobar(log logr.Logger, foobar int) logr.Logger { -// if foobarLogSink, ok := log.GetSink().(FoobarSink); ok { -// log = log.WithSink(foobarLogSink.WithFooBar(foobar)) -// } -// return log -// } +// +// // WithFooBar changes the foobar parameter in the log sink and returns a +// // new logger with that modified sink. It does nothing for loggers where +// // the sink doesn't support that parameter. +// func WithFoobar(log logr.Logger, foobar int) logr.Logger { +// if foobarLogSink, ok := log.GetSink().(FoobarSink); ok { +// log = log.WithSink(foobarLogSink.WithFooBar(foobar)) +// } +// return log +// } // // Don't use New to construct a new Logger with a LogSink retrieved from an // existing Logger. Source code attribution might not work correctly and From 7ea0fe385f0fc9725724b6594b78cae701e0eacf Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 10 Nov 2022 13:36:13 -0800 Subject: [PATCH 08/15] Add pkg doc to example --- funcr/example/main.go | 1 + 1 file changed, 1 insertion(+) diff --git a/funcr/example/main.go b/funcr/example/main.go index 476e171..8f8bbd8 100644 --- a/funcr/example/main.go +++ b/funcr/example/main.go @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ +// Package main is an example of using funcr. package main import ( From a8aea2eb594b0d131750bd78067ba69848e14853 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 18 Nov 2022 15:22:40 -0800 Subject: [PATCH 09/15] funcr: JSON invalid output with 1st field omitted We can't just check for `i > 0` - the leading field or fields could have been omitted. --- funcr/funcr.go | 4 +++- funcr/funcr_test.go | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/funcr/funcr.go b/funcr/funcr.go index e83e408..0baf003 100644 --- a/funcr/funcr.go +++ b/funcr/funcr.go @@ -448,6 +448,7 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32, depth int) s if flags&flagRawStruct == 0 { buf.WriteByte('{') } + printComma := false // testing i>0 is not enough because of JSON omitted fields for i := 0; i < t.NumField(); i++ { fld := t.Field(i) if fld.PkgPath != "" { @@ -479,9 +480,10 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32, depth int) s if omitempty && isEmpty(v.Field(i)) { continue } - if i > 0 { + if printComma { buf.WriteByte(',') } + printComma = true // if we got here, we are rendering a field if fld.Anonymous && fld.Type.Kind() == reflect.Struct && name == "" { buf.WriteString(f.prettyWithFlags(v.Field(i).Interface(), flags|flagRawStruct, depth+1)) continue diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go index 20b26a5..8af3970 100644 --- a/funcr/funcr_test.go +++ b/funcr/funcr_test.go @@ -106,6 +106,7 @@ func (t Terrorpanic) Error() string { } type TjsontagsString struct { + String0 string `json:"-"` // first field ignored String1 string `json:"string1"` // renamed String2 string `json:"-"` // ignored String3 string `json:"-,"` // named "-" @@ -115,6 +116,7 @@ type TjsontagsString struct { } type TjsontagsBool struct { + Bool0 bool `json:"-"` // first field ignored Bool1 bool `json:"bool1"` // renamed Bool2 bool `json:"-"` // ignored Bool3 bool `json:"-,"` // named "-" @@ -124,6 +126,7 @@ type TjsontagsBool struct { } type TjsontagsInt struct { + Int0 int `json:"-"` // first field ignored Int1 int `json:"int1"` // renamed Int2 int `json:"-"` // ignored Int3 int `json:"-,"` // named "-" @@ -133,6 +136,7 @@ type TjsontagsInt struct { } type TjsontagsUint struct { + Uint0 int `json:"-"` // first field ignored Uint1 uint `json:"uint1"` // renamed Uint2 uint `json:"-"` // ignored Uint3 uint `json:"-,"` // named "-" @@ -142,6 +146,7 @@ type TjsontagsUint struct { } type TjsontagsFloat struct { + Float0 float64 `json:"-"` // first field ignored Float1 float64 `json:"float1"` // renamed Float2 float64 `json:"-"` // ignored Float3 float64 `json:"-,"` // named "-" @@ -151,6 +156,7 @@ type TjsontagsFloat struct { } type TjsontagsComplex struct { + Complex0 complex128 `json:"-"` // first field ignored Complex1 complex128 `json:"complex1"` // renamed Complex2 complex128 `json:"-"` // ignored Complex3 complex128 `json:"-,"` // named "-" @@ -160,6 +166,7 @@ type TjsontagsComplex struct { } type TjsontagsPtr struct { + Ptr0 *string `json:"-"` // first field ignored Ptr1 *string `json:"ptr1"` // renamed Ptr2 *string `json:"-"` // ignored Ptr3 *string `json:"-,"` // named "-" @@ -169,6 +176,7 @@ type TjsontagsPtr struct { } type TjsontagsArray struct { + Array0 [2]string `json:"-"` // first field ignored Array1 [2]string `json:"array1"` // renamed Array2 [2]string `json:"-"` // ignored Array3 [2]string `json:"-,"` // named "-" @@ -178,6 +186,7 @@ type TjsontagsArray struct { } type TjsontagsSlice struct { + Slice0 []string `json:"-"` // first field ignored Slice1 []string `json:"slice1"` // renamed Slice2 []string `json:"-"` // ignored Slice3 []string `json:"-,"` // named "-" @@ -187,6 +196,7 @@ type TjsontagsSlice struct { } type TjsontagsMap struct { + Map0 map[string]string `json:"-"` // first field ignored Map1 map[string]string `json:"map1"` // renamed Map2 map[string]string `json:"-"` // ignored Map3 map[string]string `json:"-,"` // named "-" From 41ad1c2ab75f42b47a6c1372aef8f5a758b08882 Mon Sep 17 00:00:00 2001 From: Timon Wong Date: Fri, 12 Aug 2022 08:56:17 +0800 Subject: [PATCH 10/15] testr: merge testLogger and testLoggerInterface Signed-off-by: Timon Wong --- testr/testr.go | 56 +++++++++++++-------------------------------- testr/testr_test.go | 2 +- 2 files changed, 17 insertions(+), 41 deletions(-) diff --git a/testr/testr.go b/testr/testr.go index aaf7de5..2772b49 100644 --- a/testr/testr.go +++ b/testr/testr.go @@ -27,11 +27,7 @@ import ( // New returns a logr.Logger that prints through a testing.T object. // Info logs are only enabled at V(0). func New(t *testing.T) logr.Logger { - l := &testlogger{ - Formatter: funcr.NewFormatter(funcr.Options{}), - t: t, - } - return logr.New(l) + return NewWithOptions(t, Options{}) } // Options carries parameters which influence the way logs are generated. @@ -50,11 +46,7 @@ type Options struct { // In contrast to the simpler New, output formatting can be configured. func NewWithOptions(t *testing.T, opts Options) logr.Logger { l := &testlogger{ - Formatter: funcr.NewFormatter(funcr.Options{ - LogTimestamp: opts.LogTimestamp, - Verbosity: opts.Verbosity, - }), - t: t, + testloggerInterface: newLoggerInterfaceWithOptions(t, opts), } return logr.New(l) } @@ -69,14 +61,18 @@ type TestingT interface { // TestingT object. // In contrast to the simpler New, output formatting can be configured. func NewWithInterface(t TestingT, opts Options) logr.Logger { - l := &testloggerInterface{ + l := newLoggerInterfaceWithOptions(t, opts) + return logr.New(&l) +} + +func newLoggerInterfaceWithOptions(t TestingT, opts Options) testloggerInterface { + return testloggerInterface{ + t: t, Formatter: funcr.NewFormatter(funcr.Options{ LogTimestamp: opts.LogTimestamp, Verbosity: opts.Verbosity, }), - t: t, } - return logr.New(l) } // Underlier exposes access to the underlying testing.T instance. Since @@ -115,37 +111,17 @@ func logError(t TestingT, formatError func(error, string, []interface{}) (string t.Log(args) } +// This type exists to wrap and modify the method-set of testloggerInterface. +// In particular, it changes the GetUnderlying() method. type testlogger struct { - funcr.Formatter - t *testing.T -} - -func (l testlogger) WithName(name string) logr.LogSink { - l.Formatter.AddName(name) - return &l -} - -func (l testlogger) WithValues(kvList ...interface{}) logr.LogSink { - l.Formatter.AddValues(kvList) - return &l -} - -func (l testlogger) GetCallStackHelper() func() { - return l.t.Helper -} - -func (l testlogger) Info(level int, msg string, kvList ...interface{}) { - l.t.Helper() - logInfo(l.t, l.FormatInfo, level, msg, kvList...) -} - -func (l testlogger) Error(err error, msg string, kvList ...interface{}) { - l.t.Helper() - logError(l.t, l.FormatError, err, msg, kvList...) + testloggerInterface } func (l testlogger) GetUnderlying() *testing.T { - return l.t + // This method is defined on testlogger, so the only type this could + // possibly be is testing.T, even though that's not guaranteed by the type + // system itself. + return l.t.(*testing.T) //nolint:forcetypeassert } type testloggerInterface struct { diff --git a/testr/testr_test.go b/testr/testr_test.go index 4a737b7..252c4be 100644 --- a/testr/testr_test.go +++ b/testr/testr_test.go @@ -41,7 +41,7 @@ func TestLogger(t *testing.T) { underlier, ok := log.GetSink().(Underlier) if !ok { - t.Error("couldn't get underlier") + t.Fatal("couldn't get underlier") } if t != underlier.GetUnderlying() { t.Error("invalid underlier") From 5b49379682bf4ce8b92140de4300525f56ff62ed Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 20 Nov 2022 15:24:12 -0800 Subject: [PATCH 11/15] Fix comments on optional sink interfaces --- logr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/logr.go b/logr.go index 87886aa..6091b41 100644 --- a/logr.go +++ b/logr.go @@ -467,7 +467,7 @@ type LogSink interface { WithName(name string) LogSink } -// CallDepthLogSink represents a Logger that knows how to climb the call stack +// CallDepthLogSink represents a LogSink 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. @@ -492,7 +492,7 @@ type CallDepthLogSink interface { WithCallDepth(depth int) LogSink } -// CallStackHelperLogSink represents a Logger that knows how to climb +// CallStackHelperLogSink represents a LogSink that knows how to climb // the call stack to identify the original call site and can skip // intermediate helper functions if they mark themselves as // helper. Go's testing package uses that approach. From 18e8b879d207a0391c5ac4b5a7b8b423591156d4 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 20 Nov 2022 15:28:21 -0800 Subject: [PATCH 12/15] Make github assign PRs and issues --- .github/workflows/assign.yaml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 .github/workflows/assign.yaml diff --git a/.github/workflows/assign.yaml b/.github/workflows/assign.yaml new file mode 100644 index 0000000..e1bfb97 --- /dev/null +++ b/.github/workflows/assign.yaml @@ -0,0 +1,21 @@ +name: Assign + +on: + issues: + types: [opened, reopened] + pull_request_target: + types: [opened, reopened] + +jobs: + assign: + runs-on: ubuntu-latest + steps: + - uses: actions/github-script@v6 + with: + script: | + github.rest.issues.addAssignees({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + assignees: ['thockin', 'pohly'] + }) From b818fb2718906ece42dca289aa1be2cf8f8f5290 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 20 Nov 2022 15:53:25 -0800 Subject: [PATCH 13/15] Add examples for Logger methods --- example_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/example_test.go b/example_test.go index 351515b..d18c96d 100644 --- a/example_test.go +++ b/example_test.go @@ -27,7 +27,7 @@ import ( func NewStdoutLogger() logr.Logger { return funcr.New(func(prefix, args string) { if prefix != "" { - _ = fmt.Sprintf("%s: %s\n", prefix, args) + fmt.Printf("%s: %s\n", prefix, args) } else { fmt.Println(args) } @@ -44,3 +44,57 @@ func Example() { // "level"=0 "msg"="V(0) info log" "stringVal"="value" "intVal"=12345 // "msg"="error log" "error"="an error" "stringVal"="value" "intVal"=12345 } + +func ExampleLogger_Info() { + l := NewStdoutLogger() + l.Info("this is a V(0)-equivalent info log", "stringVal", "value", "intVal", 12345) + // Output: + // "level"=0 "msg"="this is a V(0)-equivalent info log" "stringVal"="value" "intVal"=12345 +} + +func ExampleLogger_Error() { + l := NewStdoutLogger() + l.Error(fmt.Errorf("the error"), "this is an error log", "stringVal", "value", "intVal", 12345) + l.Error(nil, "this is an error log with nil error", "stringVal", "value", "intVal", 12345) + // Output: + // "msg"="this is an error log" "error"="the error" "stringVal"="value" "intVal"=12345 + // "msg"="this is an error log with nil error" "error"=null "stringVal"="value" "intVal"=12345 +} + +func ExampleLogger_WithName() { + l := NewStdoutLogger() + l = l.WithName("name1") + l.Info("this is an info log", "stringVal", "value", "intVal", 12345) + l = l.WithName("name2") + l.Info("this is an info log", "stringVal", "value", "intVal", 12345) + // Output: + // name1: "level"=0 "msg"="this is an info log" "stringVal"="value" "intVal"=12345 + // name1/name2: "level"=0 "msg"="this is an info log" "stringVal"="value" "intVal"=12345 +} + +func ExampleLogger_WithValues() { + l := NewStdoutLogger() + l = l.WithValues("stringVal", "value", "intVal", 12345) + l = l.WithValues("boolVal", true) + l.Info("this is an info log", "floatVal", 3.1415) + // Output: + // "level"=0 "msg"="this is an info log" "stringVal"="value" "intVal"=12345 "boolVal"=true "floatVal"=3.1415 +} + +func ExampleLogger_V() { + l := NewStdoutLogger() + l.V(0).Info("V(0) info log") + l.V(1).Info("V(1) info log") + l.V(2).Info("V(2) info log") + // Output: + // "level"=0 "msg"="V(0) info log" +} + +func ExampleLogger_Enabled() { + l := NewStdoutLogger() + if loggerV := l.V(5); loggerV.Enabled() { + // Do something expensive. + loggerV.Info("this is an expensive log message") + } + // Output: +} From 4d25940030733aad917e7750e705dd8273ab7707 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 18 Nov 2022 13:39:23 +0100 Subject: [PATCH 14/15] funcr: optimize WithValues/WithName/WithCallDepth All of these functions indirectly copied the entire Options struct because it gets stored by value in the Formatter and thus fnlogger. The struct is read-only, therefore sharing a single copy by pointer via different logger instances is possible. Performance for the context value benchmark got better. --- funcr/funcr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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. From 4da5305ff29a64c62f54ad43ebbfcb5e1b015fb2 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 28 Nov 2022 10:56:35 +0100 Subject: [PATCH 15/15] 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") } }