dlib icon indicating copy to clipboard operation
dlib copied to clipboard

Add SetMaxLogLevel method to LoggerWithMaxLevel interface

Open haq204 opened this issue 2 years ago • 1 comments

Follow up to #39

Propose to add SetMaxLogLevel to LoggerWithMaxLevel interface to complete the leveled logging add-in interface for loggers that support it or add leveled logging for logger implementations that don't natively support it.

The main motivation is that there are situations where the loglevel wants to be set dynamically at runtime (ex. from an environment variable) but do not want to be dependent on a particular logging implementation or has to reference a global logger

An example is in emissary where logrus.Logger is exposed globally from the package

var logrusLogger *logrus.Logger
...

// Setting global state
func SetLogLevel(lvl logrus.Level) {
	logrusLogger.SetLevel(lvl)
}

// Getting package global state
func GetLogLevel() logrus.Level {
	return logrusLogger.GetLevel()
}

With this new method to the interface we can set the log level for the logger passed in through the context without being dependent on a particular implementation and without pulling the logger out of the context which can lead to drift.

logger := dlog.WrapLogrus(logrusLogger).
ctx := dlog.WithLogger(context.Background(), logger)

// later on possibly in separate package
doSomething(ctx) {
  switch os.Getenv("LOGLEVEL") {
      case "WARN":
        dlog.SetMaxLogLevel(ctx, dlog.LogLevelWarn)
      case "ERROR":
        dlog.SetMaxLogLevel(ctx, dlog.LogLevelError)
      default:
        dlog.SetMaxLogLevel(ctx, dlog.LogLevelInfo)
  }
  ...
}

I think this offers a cleaner api if we're passing loggers through context.

haq204 avatar Aug 03 '22 14:08 haq204

I can see the benefit of making it scoped instead of modifying the global logger.

I went back and forth whether it should be a separate interface. It seems weird to me to be able to get a level but not set one. I guess a more versatile approach would be:

interface LoggerWithMaxLevel {
  MaxLevel
}

interface MaxLevelSetter {
  SetMaxLevel
}

interface LeveledLogger{
  LoggerWithMaxLevel
  MaxLevelSetter
}

and leave it to the logger implementation to decide which to implement. But I didn't think it was strictly necessary at this time

haq204 avatar Aug 10 '22 19:08 haq204