From 6cabf8e017464104e609ac8c5993d444d487411b Mon Sep 17 00:00:00 2001 From: Peter Bourgon Date: Wed, 7 Sep 2016 11:09:54 +0200 Subject: [PATCH] log/experimental_level: review feedback - Unexport level key and default level values - Migrate Allow vars to functions - By default, un-leveled log events are allowed --- log/experimental_level/benchmark_test.go | 12 +-- log/experimental_level/level.go | 132 ++++++++++++----------- log/experimental_level/level_test.go | 30 +++--- 3 files changed, 88 insertions(+), 86 deletions(-) diff --git a/log/experimental_level/benchmark_test.go b/log/experimental_level/benchmark_test.go index 6e22b3c46..176af248d 100644 --- a/log/experimental_level/benchmark_test.go +++ b/log/experimental_level/benchmark_test.go @@ -14,13 +14,13 @@ func BenchmarkNopBaseline(b *testing.B) { func BenchmarkNopDisallowedLevel(b *testing.B) { benchmarkRunner(b, level.New(log.NewNopLogger(), level.Config{ - Allowed: level.AllowInfoAndAbove, + Allowed: level.AllowInfoAndAbove(), })) } func BenchmarkNopAllowedLevel(b *testing.B) { benchmarkRunner(b, level.New(log.NewNopLogger(), level.Config{ - Allowed: level.AllowAll, + Allowed: level.AllowAll(), })) } @@ -30,13 +30,13 @@ func BenchmarkJSONBaseline(b *testing.B) { func BenchmarkJSONDisallowedLevel(b *testing.B) { benchmarkRunner(b, level.New(log.NewJSONLogger(ioutil.Discard), level.Config{ - Allowed: level.AllowInfoAndAbove, + Allowed: level.AllowInfoAndAbove(), })) } func BenchmarkJSONAllowedLevel(b *testing.B) { benchmarkRunner(b, level.New(log.NewJSONLogger(ioutil.Discard), level.Config{ - Allowed: level.AllowAll, + Allowed: level.AllowAll(), })) } @@ -46,13 +46,13 @@ func BenchmarkLogfmtBaseline(b *testing.B) { func BenchmarkLogfmtDisallowedLevel(b *testing.B) { benchmarkRunner(b, level.New(log.NewLogfmtLogger(ioutil.Discard), level.Config{ - Allowed: level.AllowInfoAndAbove, + Allowed: level.AllowInfoAndAbove(), })) } func BenchmarkLogfmtAllowedLevel(b *testing.B) { benchmarkRunner(b, level.New(log.NewLogfmtLogger(ioutil.Discard), level.Config{ - Allowed: level.AllowAll, + Allowed: level.AllowAll(), })) } diff --git a/log/experimental_level/level.go b/log/experimental_level/level.go index 7a2378c5b..6210d5c14 100644 --- a/log/experimental_level/level.go +++ b/log/experimental_level/level.go @@ -5,86 +5,88 @@ import ( ) var ( - // LevelKey is the key part of a level keyval. - LevelKey = "level" - - // ErrorLevelValue is the val part of an error-level keyval. - ErrorLevelValue = "error" - - // WarnLevelValue is the val part of a warn-level keyval. - WarnLevelValue = "warn" - - // InfoLevelValue is the val part of an info-level keyval. - InfoLevelValue = "info" - - // DebugLevelValue is the val part of a debug-level keyval. - DebugLevelValue = "debug" + levelKey = "level" + errorLevelValue = "error" + warnLevelValue = "warn" + infoLevelValue = "info" + debugLevelValue = "debug" ) -var ( - // AllowAll is an alias for AllowDebugAndAbove. - AllowAll = AllowDebugAndAbove +// AllowAll is an alias for AllowDebugAndAbove. +func AllowAll() []string { + return AllowDebugAndAbove() +} - // AllowDebugAndAbove allows all of the four default log levels. - // It may be provided as the Allowed parameter in the Config struct. - AllowDebugAndAbove = []string{ErrorLevelValue, WarnLevelValue, InfoLevelValue, DebugLevelValue} +// AllowDebugAndAbove allows all of the four default log levels. +// Its return value may be provided as the Allowed parameter in the Config. +func AllowDebugAndAbove() []string { + return []string{errorLevelValue, warnLevelValue, infoLevelValue, debugLevelValue} +} - // AllowInfoAndAbove allows the default info, warn, and error log levels. - // It may be provided as the Allowed parameter in the Config struct. - AllowInfoAndAbove = []string{ErrorLevelValue, WarnLevelValue, InfoLevelValue} +// AllowInfoAndAbove allows the default info, warn, and error log levels. +// Its return value may be provided as the Allowed parameter in the Config. +func AllowInfoAndAbove() []string { + return []string{errorLevelValue, warnLevelValue, infoLevelValue} +} - // AllowWarnAndAbove allows the default warn and error log levels. - // It may be provided as the Allowed parameter in the Config struct. - AllowWarnAndAbove = []string{ErrorLevelValue, WarnLevelValue} +// AllowWarnAndAbove allows the default warn and error log levels. +// Its return value may be provided as the Allowed parameter in the Config. +func AllowWarnAndAbove() []string { + return []string{errorLevelValue, warnLevelValue} +} - // AllowErrorOnly allows only the default error log level. - // It may be provided as the Allowed parameter in the Config struct. - AllowErrorOnly = []string{ErrorLevelValue} +// AllowErrorOnly allows only the default error log level. +// Its return value may be provided as the Allowed parameter in the Config. +func AllowErrorOnly() []string { + return []string{errorLevelValue} +} - // AllowNone allows none of the default log levels. - // It may be provided as the Allowed parameter in the Config struct. - AllowNone = []string{} -) +// AllowNone allows none of the default log levels. +// Its return value may be provided as the Allowed parameter in the Config. +func AllowNone() []string { + return []string{} +} -// Error returns a logger with the LevelKey set to ErrorLevelValue. +// Error returns a logger with the level key set to ErrorLevelValue. func Error(logger log.Logger) log.Logger { - return log.NewContext(logger).With(LevelKey, ErrorLevelValue) + return log.NewContext(logger).With(levelKey, errorLevelValue) } -// Warn returns a logger with the LevelKey set to WarnLevelValue. +// Warn returns a logger with the level key set to WarnLevelValue. func Warn(logger log.Logger) log.Logger { - return log.NewContext(logger).With(LevelKey, WarnLevelValue) + return log.NewContext(logger).With(levelKey, warnLevelValue) } -// Info returns a logger with the LevelKey set to InfoLevelValue. +// Info returns a logger with the level key set to InfoLevelValue. func Info(logger log.Logger) log.Logger { - return log.NewContext(logger).With(LevelKey, InfoLevelValue) + return log.NewContext(logger).With(levelKey, infoLevelValue) } -// Debug returns a logger with the LevelKey set to DebugLevelValue. +// Debug returns a logger with the level key set to DebugLevelValue. func Debug(logger log.Logger) log.Logger { - return log.NewContext(logger).With(LevelKey, DebugLevelValue) + return log.NewContext(logger).With(levelKey, debugLevelValue) } // Config parameterizes the leveled logger. type Config struct { // Allowed enumerates the accepted log levels. If a log event is encountered - // with a LevelKey set to a value that isn't explicitly allowed, the event - // will be squelched, and ErrSquelched returned. + // with a level key set to a value that isn't explicitly allowed, the event + // will be squelched, and ErrNotAllowed returned. Allowed []string - // ErrSquelched is returned to the caller when Log is invoked with a - // LevelKey that hasn't been explicitly allowed. By default, ErrSquelched is + // ErrNotAllowed is returned to the caller when Log is invoked with a level + // key that hasn't been explicitly allowed. By default, ErrNotAllowed is // nil; in this case, the log event is squelched with no error. - ErrSquelched error + ErrNotAllowed error - // AllowNoLevel will allow log events with no LevelKey to proceed through to - // the wrapped logger without error. By default, log events with no LevelKey - // will be squelched, and ErrNoLevel returned. - AllowNoLevel bool + // SquelchNoLevel will squelch log events with no level key, so that they + // don't proceed through to the wrapped logger. If SquelchNoLevel is set to + // true and a log event is squelched in this way, ErrNoLevel is returned to + // the caller. + SquelchNoLevel bool - // ErrNoLevel is returned to the caller when AllowNoLevel is false, and Log - // is invoked without a LevelKey. By default, ErrNoLevel is nil; in this + // ErrNoLevel is returned to the caller when SquelchNoLevel is true, and Log + // is invoked without a level key. By default, ErrNoLevel is nil; in this // case, the log event is squelched with no error. ErrNoLevel error } @@ -93,26 +95,26 @@ type Config struct { // Config object for a detailed description of how to configure levels. func New(next log.Logger, config Config) log.Logger { return &logger{ - next: next, - allowed: makeSet(config.Allowed), - errSquelched: config.ErrSquelched, - allowNoLevel: config.AllowNoLevel, - errNoLevel: config.ErrNoLevel, + next: next, + allowed: makeSet(config.Allowed), + errNotAllowed: config.ErrNotAllowed, + squelchNoLevel: config.SquelchNoLevel, + errNoLevel: config.ErrNoLevel, } } type logger struct { - next log.Logger - allowed map[string]struct{} - errSquelched error - allowNoLevel bool - errNoLevel error + next log.Logger + allowed map[string]struct{} + errNotAllowed error + squelchNoLevel bool + errNoLevel error } func (l *logger) Log(keyvals ...interface{}) error { var hasLevel, levelAllowed bool for i := 0; i < len(keyvals); i += 2 { - if k, ok := keyvals[i].(string); !ok || k != LevelKey { + if k, ok := keyvals[i].(string); !ok || k != levelKey { continue } hasLevel = true @@ -126,11 +128,11 @@ func (l *logger) Log(keyvals ...interface{}) error { _, levelAllowed = l.allowed[v] break } - if !hasLevel && !l.allowNoLevel { + if !hasLevel && l.squelchNoLevel { return l.errNoLevel } if hasLevel && !levelAllowed { - return l.errSquelched + return l.errNotAllowed } return l.next.Log(keyvals...) } diff --git a/log/experimental_level/level_test.go b/log/experimental_level/level_test.go index 3a5e9d8cb..741d3980c 100644 --- a/log/experimental_level/level_test.go +++ b/log/experimental_level/level_test.go @@ -16,7 +16,7 @@ func TestVariousLevels(t *testing.T) { want string }{ { - level.AllowAll, + level.AllowAll(), strings.Join([]string{ `{"level":"debug","this is":"debug log"}`, `{"level":"info","this is":"info log"}`, @@ -25,7 +25,7 @@ func TestVariousLevels(t *testing.T) { }, "\n"), }, { - level.AllowDebugAndAbove, + level.AllowDebugAndAbove(), strings.Join([]string{ `{"level":"debug","this is":"debug log"}`, `{"level":"info","this is":"info log"}`, @@ -34,7 +34,7 @@ func TestVariousLevels(t *testing.T) { }, "\n"), }, { - level.AllowInfoAndAbove, + level.AllowInfoAndAbove(), strings.Join([]string{ `{"level":"info","this is":"info log"}`, `{"level":"warn","this is":"warn log"}`, @@ -42,20 +42,20 @@ func TestVariousLevels(t *testing.T) { }, "\n"), }, { - level.AllowWarnAndAbove, + level.AllowWarnAndAbove(), strings.Join([]string{ `{"level":"warn","this is":"warn log"}`, `{"level":"error","this is":"error log"}`, }, "\n"), }, { - level.AllowErrorOnly, + level.AllowErrorOnly(), strings.Join([]string{ `{"level":"error","this is":"error log"}`, }, "\n"), }, { - level.AllowNone, + level.AllowNone(), ``, }, } { @@ -73,11 +73,11 @@ func TestVariousLevels(t *testing.T) { } } -func TestErrSquelch(t *testing.T) { +func TestErrNotAllowed(t *testing.T) { myError := errors.New("squelched!") logger := level.New(log.NewNopLogger(), level.Config{ - Allowed: level.AllowWarnAndAbove, - ErrSquelched: myError, + Allowed: level.AllowWarnAndAbove(), + ErrNotAllowed: myError, }) if want, have := myError, level.Info(logger).Log("foo", "bar"); want != have { @@ -94,8 +94,8 @@ func TestErrNoLevel(t *testing.T) { var buf bytes.Buffer logger := level.New(log.NewJSONLogger(&buf), level.Config{ - AllowNoLevel: false, - ErrNoLevel: myError, + SquelchNoLevel: true, + ErrNoLevel: myError, }) if want, have := myError, logger.Log("foo", "bar"); want != have { @@ -109,8 +109,8 @@ func TestErrNoLevel(t *testing.T) { func TestAllowNoLevel(t *testing.T) { var buf bytes.Buffer logger := level.New(log.NewJSONLogger(&buf), level.Config{ - AllowNoLevel: true, - ErrNoLevel: errors.New("I should never be returned!"), + SquelchNoLevel: false, + ErrNoLevel: errors.New("I should never be returned!"), }) if want, have := error(nil), logger.Log("foo", "bar"); want != have { @@ -128,7 +128,7 @@ func TestLevelContext(t *testing.T) { // log.DefaultCaller as per normal. var logger log.Logger logger = log.NewLogfmtLogger(&buf) - logger = level.New(logger, level.Config{Allowed: level.AllowAll}) + logger = level.New(logger, level.Config{Allowed: level.AllowAll()}) logger = log.NewContext(logger).With("caller", log.DefaultCaller) level.Info(logger).Log("foo", "bar") @@ -145,7 +145,7 @@ func TestContextLevel(t *testing.T) { var logger log.Logger logger = log.NewLogfmtLogger(&buf) logger = log.NewContext(logger).With("caller", log.Caller(5)) - logger = level.New(logger, level.Config{Allowed: level.AllowAll}) + logger = level.New(logger, level.Config{Allowed: level.AllowAll()}) level.Info(logger).Log("foo", "bar") if want, have := `caller=level_test.go:150 level=info foo=bar`, strings.TrimSpace(buf.String()); want != have {