From fe2a182bfde8a8ca53c9dedc8e9477768dc5430e Mon Sep 17 00:00:00 2001 From: Craig Pastro Date: Wed, 5 Oct 2022 16:07:12 -0700 Subject: [PATCH 1/4] SugaredLogger: Turn error into zap.Error Resolves #1184. --- sugar.go | 9 ++++++++- sugar_test.go | 6 ++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/sugar.go b/sugar.go index e42b694ed..6ac6c4c32 100644 --- a/sugar.go +++ b/sugar.go @@ -349,7 +349,14 @@ func (s *SugaredLogger) sweetenFields(args []interface{}) []Field { continue } - // Make sure this element isn't a dangling key. + // If it is an error. + if err, ok := args[i].(error); ok { + fields = append(fields, Error(err)) + i++ + continue + } + + // If it is not a singular error, make sure this element isn't a dangling key. if i == len(args)-1 { s.base.Error(_oddNumberErrMsg, Any("ignored", args[i])) break diff --git a/sugar_test.go b/sugar_test.go index 9c067cbab..b2266dbb7 100644 --- a/sugar_test.go +++ b/sugar_test.go @@ -21,6 +21,7 @@ package zap import ( + "errors" "testing" "go.uber.org/zap/internal/exit" @@ -198,9 +199,10 @@ func TestSugarStructuredLogging(t *testing.T) { } // Common to all test cases. + err := errors.New("qux") context := []interface{}{"foo", "bar"} - extra := []interface{}{"baz", false} - expectedFields := []Field{String("foo", "bar"), Bool("baz", false)} + extra := []interface{}{"baz", false, err} + expectedFields := []Field{String("foo", "bar"), Bool("baz", false), Error(err)} for _, tt := range tests { withSugar(t, DebugLevel, nil, func(logger *SugaredLogger, logs *observer.ObservedLogs) { From 3d9205860fd2262195e88e44699477c11e460b73 Mon Sep 17 00:00:00 2001 From: Craig Pastro Date: Wed, 5 Oct 2022 17:48:16 -0700 Subject: [PATCH 2/4] better comment --- sugar.go | 4 ++-- sugar_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sugar.go b/sugar.go index 6ac6c4c32..7eae8ffb7 100644 --- a/sugar.go +++ b/sugar.go @@ -349,14 +349,14 @@ func (s *SugaredLogger) sweetenFields(args []interface{}) []Field { continue } - // If it is an error. + // If it is an error, consume it and move on. if err, ok := args[i].(error); ok { fields = append(fields, Error(err)) i++ continue } - // If it is not a singular error, make sure this element isn't a dangling key. + // Make sure this element isn't a dangling key. if i == len(args)-1 { s.base.Error(_oddNumberErrMsg, Any("ignored", args[i])) break diff --git a/sugar_test.go b/sugar_test.go index b2266dbb7..6fdef38ca 100644 --- a/sugar_test.go +++ b/sugar_test.go @@ -201,8 +201,8 @@ func TestSugarStructuredLogging(t *testing.T) { // Common to all test cases. err := errors.New("qux") context := []interface{}{"foo", "bar"} - extra := []interface{}{"baz", false, err} - expectedFields := []Field{String("foo", "bar"), Bool("baz", false), Error(err)} + extra := []interface{}{err, "baz", false} + expectedFields := []Field{String("foo", "bar"), Error(err), Bool("baz", false)} for _, tt := range tests { withSugar(t, DebugLevel, nil, func(logger *SugaredLogger, logs *observer.ObservedLogs) { From 03bdfb2ee3ff0156475620bde33be14c1a3af53f Mon Sep 17 00:00:00 2001 From: Craig Pastro Date: Wed, 5 Oct 2022 18:11:11 -0700 Subject: [PATCH 3/4] watch for multiple errors --- sugar.go | 9 ++++++++- sugar_test.go | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/sugar.go b/sugar.go index 7eae8ffb7..61076c985 100644 --- a/sugar.go +++ b/sugar.go @@ -31,6 +31,7 @@ import ( const ( _oddNumberErrMsg = "Ignored key without a value." _nonStringKeyErrMsg = "Ignored key-value pairs with non-string keys." + _multipleErrMsg = "Multiple errors without a key." ) // A SugaredLogger wraps the base Logger functionality in a slower, but less @@ -340,6 +341,7 @@ func (s *SugaredLogger) sweetenFields(args []interface{}) []Field { // fields, we shouldn't penalize them with extra allocations. fields := make([]Field, 0, len(args)) var invalid invalidPairs + var seenError bool for i := 0; i < len(args); { // This is a strongly-typed field. Consume it and move on. @@ -351,7 +353,12 @@ func (s *SugaredLogger) sweetenFields(args []interface{}) []Field { // If it is an error, consume it and move on. if err, ok := args[i].(error); ok { - fields = append(fields, Error(err)) + if !seenError { + seenError = true + fields = append(fields, Error(err)) + } else { + s.base.Error(_multipleErrMsg, Error(err)) + } i++ continue } diff --git a/sugar_test.go b/sugar_test.go index 6fdef38ca..d4daf665e 100644 --- a/sugar_test.go +++ b/sugar_test.go @@ -47,6 +47,12 @@ func TestSugarWith(t *testing.T) { Context: []Field{Array("invalid", invalidPairs(pairs))}, } } + ignoredError := func(err error) observer.LoggedEntry { + return observer.LoggedEntry{ + Entry: zapcore.Entry{Level: ErrorLevel, Message: _multipleErrMsg}, + Context: []Field{Error(err)}, + } + } tests := []struct { desc string @@ -123,6 +129,15 @@ func TestSugarWith(t *testing.T) { nonString(invalidPair{2, true, "bar"}, invalidPair{5, 42, "reversed"}), }, }, + { + desc: "multiple errors", + args: []interface{}{errors.New("first"), errors.New("second"), errors.New("third")}, + expected: []Field{Error(errors.New("first"))}, + errLogs: []observer.LoggedEntry{ + ignoredError(errors.New("second")), + ignoredError(errors.New("third")), + }, + }, } for _, tt := range tests { From e5798824279ff8253d401060e4775f5fe3d496e5 Mon Sep 17 00:00:00 2001 From: Craig Pastro Date: Sat, 15 Oct 2022 09:13:09 -0700 Subject: [PATCH 4/4] combine vars into block --- sugar.go | 12 +++++++----- sugar_test.go | 12 +++++++----- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/sugar.go b/sugar.go index 61076c985..ac387b3e4 100644 --- a/sugar.go +++ b/sugar.go @@ -337,11 +337,13 @@ func (s *SugaredLogger) sweetenFields(args []interface{}) []Field { return nil } - // Allocate enough space for the worst case; if users pass only structured - // fields, we shouldn't penalize them with extra allocations. - fields := make([]Field, 0, len(args)) - var invalid invalidPairs - var seenError bool + var ( + // Allocate enough space for the worst case; if users pass only structured + // fields, we shouldn't penalize them with extra allocations. + fields = make([]Field, 0, len(args)) + invalid invalidPairs + seenError bool + ) for i := 0; i < len(args); { // This is a strongly-typed field. Consume it and move on. diff --git a/sugar_test.go b/sugar_test.go index d4daf665e..602c4326a 100644 --- a/sugar_test.go +++ b/sugar_test.go @@ -214,11 +214,13 @@ func TestSugarStructuredLogging(t *testing.T) { } // Common to all test cases. - err := errors.New("qux") - context := []interface{}{"foo", "bar"} - extra := []interface{}{err, "baz", false} - expectedFields := []Field{String("foo", "bar"), Error(err), Bool("baz", false)} - + var ( + err = errors.New("qux") + context = []interface{}{"foo", "bar"} + extra = []interface{}{err, "baz", false} + expectedFields = []Field{String("foo", "bar"), Error(err), Bool("baz", false)} + ) + for _, tt := range tests { withSugar(t, DebugLevel, nil, func(logger *SugaredLogger, logs *observer.ObservedLogs) { logger.With(context...).Debugw(tt.msg, extra...)