log/experimental_levels: review feedback

- Unexport level key and default level values
- Migrate Allow vars to functions
- By default, un-leveled log events are allowed

Author: peterbourgon

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(),
 	}))
 }
 

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...)
 }

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,28 +34,28 @@ 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"}`,
 				`{"level":"error","this is":"error log"}`,
 			}, "\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 {