[FLI-148] posibility to configure Zap logger keys

[giddel]

Problem

Great thing to use Zap as logger. Unfortunately the logger keys are hardcoded in main.go. At least GCP looging cannot understand the logger keys. Alle entries at now are logged at INFO severity.

Ideal Solution

So it would be great to set the complete Zap logger config like this:

level: debug
development: false  # enables stacktrace!
encoding: json  # or 'console'
outputPaths:
  - stdout
errorOutputPaths:
  - stderr
encoderConfig:
  timeKey: time
  levelKey: level
  messageKey: msg
  levelEncoder: capital  # or 'lower'
  timeEncoder: iso8601  # o 'rfc3339'
  callerEncoder: full

Which then looks like this:

_FLI-148

[GeorgeMac]

This actually went by as a suggestion when we moved from logrus to zap in #1020.
And this is a great real world use-case for doing it, thanks for sharing!

I like the idea, I think it needs some thought r.e. backwards compatibility with existing config. Though, it should be achievable.

Here is where the default config for zap is currently defined:

flipt/cmd/flipt/main.go

Lines 57 to 78 in b3fc4b1

	loggerConfig = zap.Config{
	Level: zap.NewAtomicLevelAt(zap.InfoLevel),
	Development: false,
	Encoding: "console",
	EncoderConfig: zapcore.EncoderConfig{
	// Keys can be anything except the empty string.
	TimeKey: "T",
	LevelKey: "L",
	NameKey: "N",
	CallerKey: zapcore.OmitKey,
	FunctionKey: zapcore.OmitKey,
	MessageKey: "M",
	StacktraceKey: zapcore.OmitKey,
	LineEnding: zapcore.DefaultLineEnding,
	EncodeLevel: zapcore.CapitalColorLevelEncoder,
	EncodeTime: zapcore.RFC3339TimeEncoder,
	EncodeDuration: zapcore.StringDurationEncoder,
	EncodeCaller: zapcore.ShortCallerEncoder,
	},
	OutputPaths: []string{"stdout"},
	ErrorOutputPaths: []string{"stderr"},
	}

Then the following Flipt configuration values adjust that configuration slightly:

flipt/internal/config/log.go

Lines 14 to 19 in b3fc4b1

	type LogConfig struct {
	Level string `json:"level,omitempty" mapstructure:"level"`
	File string `json:"file,omitempty" mapstructure:"file"`
	Encoding LogEncoding `json:"encoding,omitempty" mapstructure:"encoding"`
	GRPCLevel string `json:"grpcLevel,omitempty" mapstructure:"grpc_level"`
	}

Once configuration is initialized, that config is adjusted down here:

flipt/cmd/flipt/main.go

Lines 159 to 186 in b3fc4b1

	cobra.OnInitialize(func() {
	// read in config
	res, err := config.Load(cfgPath)
	if err != nil {
	logger().Fatal("loading configuration", zap.Error(err))
	}
	cfg = res.Config
	cfgWarnings = res.Warnings
	// log to file if enabled
	if cfg.Log.File != "" {
	loggerConfig.OutputPaths = []string{cfg.Log.File}
	}
	// parse/set log level
	loggerConfig.Level, err = zap.ParseAtomicLevel(cfg.Log.Level)
	if err != nil {
	logger().Fatal("parsing log level", zap.String("level", cfg.Log.Level), zap.Error(err))
	}
	if cfg.Log.Encoding > config.LogEncodingConsole {
	loggerConfig.Encoding = cfg.Log.Encoding.String()
	// don't encode with colors if not using console log output
	loggerConfig.EncoderConfig.EncodeLevel = zapcore.CapitalLevelEncoder
	}
	})

Here are some thoughts on how it could be adjusted:

1. Interleave zap.Config with existing config.LogConfig.

We already use the same keys level and encoding for the same purposes.
Then the rest of the keys do not overlap in name.
However, there is a field with different names in both zap and flipts config, which effect the same construct (output destination).

flipt/cmd/flipt/main.go

Line 171 in b3fc4b1

loggerConfig.OutputPaths = []string{cfg.Log.File}

The File field sets the output path to the defined file.
In this situation, we would need to define how to reconcile when both file and output_paths are provided.
e.g. do we fail fast with an error, or attempt to combine them?

2. We nest the full zap config beneath a new key.

For example, zap or the word config:

log:
  zap:
    level: debug
    development: false
    # ...

Off the top of my head, I can't quite think of a nice name for this key. config is already heavily abused, and zap is a little too close to an implementation detail. However, I guess, embracing the entire zap config is itself taking the plunge on that implementation detail. So perhaps that doesn't matter.

This could be a mutually exclusive space. Whereby you can either set the original level, encoding and file or you can use the more fine-grained zap key.

grpc_level could remain as is (in both (1) and (2)) as it effects something outside the scope of the logging output configuration. Since it supports adjusting the level of the gRPC client's standard logging (which is very chatty at INFO).

3. Wait until we decide on a Flipt config v2.

Mark recently introduced a version to the entire config file.
The mindset being we could move away to something fresh in the future.
However, I suspect we might want some critical mass of desired changes for that to happen.
This could be one of those, but it would push the effort to some yet to be decided date.

[GeorgeMac]

One additional thought is we would need to consider how well it plays with viper.

I suspect we might be able to knit it altogether nicely:
https://github.com/uber-go/zap/blob/a55bdc32f526699c3b4cc51a2cc97e944d02fbbf/config.go#L58-L94

It might just require us to support something like that yaml tag here:
https://github.com/flipt-io/flipt/blob/main/internal/config/config.go#L161

Or to make an equivalent structure with appropriate annotations and copy the config across.

---

Update: I see that the zapcore config does do a lot of unmarshal text and yaml custom overrides:
https://github.com/uber-go/zap/blob/a55bdc32f526699c3b4cc51a2cc97e944d02fbbf/zapcore/encoder.go#L164-L212

This could likely need some attention to support it with mapstructure.
Mapstructure has support for UnmarshalText via a decoder:
https://pkg.go.dev/github.com/mitchellh/mapstructure#TextUnmarshallerHookFunc

However, I don't think it supports unmarshal yaml. This is where we might need to do a little surgery.

[giddel]

In my case, I use a separate config file for Zap configuration logger.yaml as shown in my initial post at top. I point to this file in my applications config. Then I Iet Zap do the work for me:

var Logger *zap.Logger

func initLoggerFromConfigFile(loggerConfigFile string) {
	var cfg zap.Config

	yamlFile, err := ioutil.ReadFile(loggerConfigFile)
	if err != nil {
		panic(fmt.Sprintf("error reading file %s: #%v ",loggerConfigFile, err))
	}

	if err := yaml.Unmarshal(yamlFile, &cfg); err != nil {
		panic(err)
	}

	initLoggerWithConfig(cfg)
}
func initLoggerWithConfig(config zap.Config) {
	var err error
	Logger, err = config.Build()
	if err != nil {
		panic(err)
	}
}

[GeorgeMac]

Thats a nice alternative idea. Could have a line in the Flipt config which points to the files location.
Similar to (2) in that it could be mutually exclusive with the existing level, encoding and file fields.

[markphelps]

@giddel FYI we're looking to support this in the 1.18 release. Perhaps we can chat about it in Discord?

[GeorgeMac]

Apologies, this got bumped into 1.19 as we had a lot of big changes queued up.

I am circling back to this now. After discussing with @markphelps we decided we liked to keep the solution abstracted away from Zap. We love Zap, we intend to keep it. However, we're also going to "never say never" on replacing it.
e.g. we might reconsider if anything ever comes of golang/go#56345
(https://pkg.go.dev/golang.org/x/exp/slog - which happens to default its message, time and level keys to msg, time and level respectively - which is handy)

So we would like to abstract the details from our own configuration as much as possible.
We still want to solve the problem of adding further configuration to better support logging frameworks like GCPs offering.

Ill attempt to share a proposal shortly of what the config could look like and see what we think.

[GeorgeMac]

I popped open the PR above (#1295) to address simply adding support to customize the three keys (time, level and message). Would this be enough to unblock you @giddel ?

[giddel]

I've tried it out locally and it looks good for me so far.
Thanks!

[markphelps]

@giddel we'll create a new release this week with these changes in it!

[giddel]

Great! I will check it as soon as possible.

[markphelps]

@giddel this has been released in https://github.com/flipt-io/flipt/releases/tag/v1.18.1. Please give it go and let us know what you think! We'll be updating docs today as well