From 65a07b80a33196e063601cb97b51e45dcbd7d66c Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 18 Sep 2017 18:01:14 -0700 Subject: [PATCH] change logger interface to not return errors (Refs #50) See https://github.com/go-kit/kit/issues/164 for discussion of why kitlog returns an error. ``` Package log is designed to be used for more than simple application info/warning/error logging; it's suitable for log-structured data in an e.g. Lambda architecture, where each invocation is important. I agree with you that if we were doing only application logging the error would be more noise than signal. But the scope of the package is larger than that. ``` Since we are doing only application logging and we're not checking errors, it is safe to get rid them. --- log/filter.go | 31 +++++++++++-------------------- log/filter_test.go | 18 ------------------ log/logger.go | 6 +++--- log/nop_logger.go | 14 +++----------- log/nop_logger_test.go | 18 ------------------ log/tm_logger.go | 12 ++++++------ log/tm_logger_test.go | 11 ----------- log/tracing_logger.go | 12 ++++++------ 8 files changed, 29 insertions(+), 93 deletions(-) delete mode 100644 log/nop_logger_test.go diff --git a/log/filter.go b/log/filter.go index f6198b2b..768c09b8 100644 --- a/log/filter.go +++ b/log/filter.go @@ -14,7 +14,6 @@ type filter struct { next Logger allowed level // XOR'd levels for default case allowedKeyvals map[keyval]level // When key-value match, use this level - errNotAllowed error } type keyval struct { @@ -37,28 +36,28 @@ func NewFilter(next Logger, options ...Option) Logger { return l } -func (l *filter) Info(msg string, keyvals ...interface{}) error { +func (l *filter) Info(msg string, keyvals ...interface{}) { levelAllowed := l.allowed&levelInfo != 0 if !levelAllowed { - return l.errNotAllowed + return } - return l.next.Info(msg, keyvals...) + l.next.Info(msg, keyvals...) } -func (l *filter) Debug(msg string, keyvals ...interface{}) error { +func (l *filter) Debug(msg string, keyvals ...interface{}) { levelAllowed := l.allowed&levelDebug != 0 if !levelAllowed { - return l.errNotAllowed + return } - return l.next.Debug(msg, keyvals...) + l.next.Debug(msg, keyvals...) } -func (l *filter) Error(msg string, keyvals ...interface{}) error { +func (l *filter) Error(msg string, keyvals ...interface{}) { levelAllowed := l.allowed&levelError != 0 if !levelAllowed { - return l.errNotAllowed + return } - return l.next.Error(msg, keyvals...) + l.next.Error(msg, keyvals...) } // With implements Logger by constructing a new filter with a keyvals appended @@ -80,11 +79,11 @@ func (l *filter) With(keyvals ...interface{}) Logger { for i := len(keyvals) - 2; i >= 0; i -= 2 { for kv, allowed := range l.allowedKeyvals { if keyvals[i] == kv.key && keyvals[i+1] == kv.value { - return &filter{next: l.next.With(keyvals...), allowed: allowed, errNotAllowed: l.errNotAllowed, allowedKeyvals: l.allowedKeyvals} + return &filter{next: l.next.With(keyvals...), allowed: allowed, allowedKeyvals: l.allowedKeyvals} } } } - return &filter{next: l.next.With(keyvals...), allowed: l.allowed, errNotAllowed: l.errNotAllowed, allowedKeyvals: l.allowedKeyvals} + return &filter{next: l.next.With(keyvals...), allowed: l.allowed, allowedKeyvals: l.allowedKeyvals} } //-------------------------------------------------------------------------------- @@ -138,14 +137,6 @@ func allowed(allowed level) Option { return func(l *filter) { l.allowed = allowed } } -// ErrNotAllowed sets the error to return from Log when it squelches a log -// event disallowed by the configured Allow[Level] option. By default, -// ErrNotAllowed is nil; in this case the log event is squelched with no -// error. -func ErrNotAllowed(err error) Option { - return func(l *filter) { l.errNotAllowed = err } -} - // AllowDebugWith allows error, info and debug level log events to pass for a specific key value pair. func AllowDebugWith(key interface{}, value interface{}) Option { return func(l *filter) { l.allowedKeyvals[keyval{key, value}] = levelError | levelInfo | levelDebug } diff --git a/log/filter_test.go b/log/filter_test.go index 4665db3d..fafafacb 100644 --- a/log/filter_test.go +++ b/log/filter_test.go @@ -2,7 +2,6 @@ package log_test import ( "bytes" - "errors" "strings" "testing" @@ -71,23 +70,6 @@ func TestVariousLevels(t *testing.T) { } } -func TestErrNotAllowed(t *testing.T) { - myError := errors.New("squelched!") - opts := []log.Option{ - log.AllowError(), - log.ErrNotAllowed(myError), - } - logger := log.NewFilter(log.NewNopLogger(), opts...) - - if want, have := myError, logger.Info("foo", "bar", "baz"); want != have { - t.Errorf("want %#+v, have %#+v", want, have) - } - - if want, have := error(nil), logger.Error("foo", "bar", "baz"); want != have { - t.Errorf("want %#+v, have %#+v", want, have) - } -} - func TestLevelContext(t *testing.T) { var buf bytes.Buffer diff --git a/log/logger.go b/log/logger.go index be273f48..ddb187bc 100644 --- a/log/logger.go +++ b/log/logger.go @@ -8,9 +8,9 @@ import ( // Logger is what any Tendermint library should take. type Logger interface { - Debug(msg string, keyvals ...interface{}) error - Info(msg string, keyvals ...interface{}) error - Error(msg string, keyvals ...interface{}) error + Debug(msg string, keyvals ...interface{}) + Info(msg string, keyvals ...interface{}) + Error(msg string, keyvals ...interface{}) With(keyvals ...interface{}) Logger } diff --git a/log/nop_logger.go b/log/nop_logger.go index 306a8405..12d75abe 100644 --- a/log/nop_logger.go +++ b/log/nop_logger.go @@ -8,17 +8,9 @@ var _ Logger = (*nopLogger)(nil) // NewNopLogger returns a logger that doesn't do anything. func NewNopLogger() Logger { return &nopLogger{} } -func (nopLogger) Info(string, ...interface{}) error { - return nil -} - -func (nopLogger) Debug(string, ...interface{}) error { - return nil -} - -func (nopLogger) Error(string, ...interface{}) error { - return nil -} +func (nopLogger) Info(string, ...interface{}) {} +func (nopLogger) Debug(string, ...interface{}) {} +func (nopLogger) Error(string, ...interface{}) {} func (l *nopLogger) With(...interface{}) Logger { return l diff --git a/log/nop_logger_test.go b/log/nop_logger_test.go deleted file mode 100644 index d2009fdf..00000000 --- a/log/nop_logger_test.go +++ /dev/null @@ -1,18 +0,0 @@ -package log_test - -import ( - "testing" - - "github.com/tendermint/tmlibs/log" -) - -func TestNopLogger(t *testing.T) { - t.Parallel() - logger := log.NewNopLogger() - if err := logger.Info("Hello", "abc", 123); err != nil { - t.Error(err) - } - if err := logger.With("def", "ghi").Debug(""); err != nil { - t.Error(err) - } -} diff --git a/log/tm_logger.go b/log/tm_logger.go index a903dbe8..dc6932dd 100644 --- a/log/tm_logger.go +++ b/log/tm_logger.go @@ -50,21 +50,21 @@ func NewTMLoggerWithColorFn(w io.Writer, colorFn func(keyvals ...interface{}) te } // Info logs a message at level Info. -func (l *tmLogger) Info(msg string, keyvals ...interface{}) error { +func (l *tmLogger) Info(msg string, keyvals ...interface{}) { lWithLevel := kitlevel.Info(l.srcLogger) - return kitlog.With(lWithLevel, msgKey, msg).Log(keyvals...) + kitlog.With(lWithLevel, msgKey, msg).Log(keyvals...) } // Debug logs a message at level Debug. -func (l *tmLogger) Debug(msg string, keyvals ...interface{}) error { +func (l *tmLogger) Debug(msg string, keyvals ...interface{}) { lWithLevel := kitlevel.Debug(l.srcLogger) - return kitlog.With(lWithLevel, msgKey, msg).Log(keyvals...) + kitlog.With(lWithLevel, msgKey, msg).Log(keyvals...) } // Error logs a message at level Error. -func (l *tmLogger) Error(msg string, keyvals ...interface{}) error { +func (l *tmLogger) Error(msg string, keyvals ...interface{}) { lWithLevel := kitlevel.Error(l.srcLogger) - return kitlog.With(lWithLevel, msgKey, msg).Log(keyvals...) + kitlog.With(lWithLevel, msgKey, msg).Log(keyvals...) } // With returns a new contextual logger with keyvals prepended to those passed diff --git a/log/tm_logger_test.go b/log/tm_logger_test.go index 15c940ce..8cd2f827 100644 --- a/log/tm_logger_test.go +++ b/log/tm_logger_test.go @@ -7,17 +7,6 @@ import ( "github.com/tendermint/tmlibs/log" ) -func TestTMLogger(t *testing.T) { - t.Parallel() - logger := log.NewTMLogger(ioutil.Discard) - if err := logger.Info("Hello", "abc", 123); err != nil { - t.Error(err) - } - if err := logger.With("def", "ghi").Debug(""); err != nil { - t.Error(err) - } -} - func BenchmarkTMLoggerSimple(b *testing.B) { benchmarkRunner(b, log.NewTMLogger(ioutil.Discard), baseInfoMessage) } diff --git a/log/tracing_logger.go b/log/tracing_logger.go index 794bdaeb..d2a6ff44 100644 --- a/log/tracing_logger.go +++ b/log/tracing_logger.go @@ -28,16 +28,16 @@ type tracingLogger struct { next Logger } -func (l *tracingLogger) Info(msg string, keyvals ...interface{}) error { - return l.next.Info(msg, formatErrors(keyvals)...) +func (l *tracingLogger) Info(msg string, keyvals ...interface{}) { + l.next.Info(msg, formatErrors(keyvals)...) } -func (l *tracingLogger) Debug(msg string, keyvals ...interface{}) error { - return l.next.Debug(msg, formatErrors(keyvals)...) +func (l *tracingLogger) Debug(msg string, keyvals ...interface{}) { + l.next.Debug(msg, formatErrors(keyvals)...) } -func (l *tracingLogger) Error(msg string, keyvals ...interface{}) error { - return l.next.Error(msg, formatErrors(keyvals)...) +func (l *tracingLogger) Error(msg string, keyvals ...interface{}) { + l.next.Error(msg, formatErrors(keyvals)...) } func (l *tracingLogger) With(keyvals ...interface{}) Logger {