[Logs] Consider accepting a zap.Logger rather than zap.Config in embed & client

[Quentin-M]

Embed currently accepts ZapLoggerBuilder, and clientv3 accepts a LogConfig, which in both cases basically takes a zap.Config. However, in order for clients to be able to filter out noisy messages (e.g. clientv3/retry_interceptor.go, or LB Health Checks spamming with bogus TLS connections), a wrapped zapcore.Core must be used, which requires calling (cfg Config) Build(opts ...Option) (*Logger, error), which constructs a full logger out of a config struct. Ops could then very simply pass assign the logger a filtering core. A zapcore.Core override could also be used by Ops to do things such as exporting extra metrics based on logs, etc.

It really should not change much on the etcd code base side of things - especially as 3.5 is not out yet, but will allow the amount of log noise configuration that once was available with capnslog. For instance:

capnslog.MustRepoLogger("go.etcd.io/etcd").SetLogLevel(map[string]capnslog.LogLevel{"etcdserver/api/v3rpc": capnslog.CRITICAL})

x-ref #11147

edit May 2021: client.WithLogger lets us override a zap.Logger onto clientv3, that's amazing! The very same method could be implemented on Embed.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

[ptabor]

Agree. I don't see reasons why we just not support passing logger to embedded & clientv3.

clientv3 requires 'wrapping' with:
c := c.WithLogger(logger) that is error prone (as some objects take the logger before wrapping).

I think I already did a lot of logging simplifications such that RAFT logs and GRPC loggers are
derived from the zap.Logger and not from the core configs.

[ptabor]

@serathius

[serathius]

I will take a look into implementing this

[gyuho]

I don't see reasons why we just not support passing logger to embedded & clientv3.

As long as we support both, sounds good.

If logger is not empty, we can overwrite the existing zap log config?

[serathius]

Hmm, looked through embed config code

etcd/server/embed/config.go

Lines 347 to 364 in 450fd9a

	// Logger is logger options: currently only supports "zap".
	// "capnslog" is removed in v3.5.
	Logger string `json:"logger"`
	// LogLevel configures log level. Only supports debug, info, warn, error, panic, or fatal. Default 'info'.
	LogLevel string `json:"log-level"`
	// LogOutputs is either:
	// - "default" as os.Stderr,
	// - "stderr" as os.Stderr,
	// - "stdout" as os.Stdout,
	// - file path to append server logs to.
	// It can be multiple when "Logger" is zap.
	LogOutputs []string `json:"log-outputs"`
	// EnableLogRotation enables log rotation of a single LogOutputs file target.
	EnableLogRotation bool `json:"enable-log-rotation"`
	// LogRotationConfigJSON is a passthrough allowing a log rotation JSON config to be passed directly.
	LogRotationConfigJSON string `json:"log-rotation-config-json"`
	// ZapLoggerBuilder is used to build the zap logger.
	ZapLoggerBuilder func(*Config) error

logger field is protected by mutex loggerMu, so I don't think we should expose it.
On the other hand, ZapLoggerBuilder already allows overriding logger using NewzapCoreLoggerBuilder function.

etcd/server/embed/config_logging.go

Lines 201 to 211 in 450fd9a

	func NewZapCoreLoggerBuilder(lg *zap.Logger) func(*Config) error {
	return func(cfg *Config) error {
	cfg.loggerMu.Lock()
	defer cfg.loggerMu.Unlock()
	cfg.logger = lg
	cfg.loggerConfig = nil
	cfg.loggerCore = nil
	cfg.loggerWriteSyncer = nil
	return nil
	}
	}

c := embed.NewConfig()
c.ZapLoggerBuilder = embed.NewZapCoreLoggerBuilder(logger)

[ptabor]

@gyuho:

I wonder whether there are resons to build RaftLogger from LoggerConfig or LoggerCore.
If we always created it as 'derived' from the main logger like in line 605, a lot
of state would not need to be persisted in our cfg data-structures. The fields would be only used to create the original logger in the first place (in embed) and never accessed in the etcdserver.

etcd/server/etcdserver/raft.go

Lines 596 to 606 in 70f7e0d

	func getRaftLogger(cfg config.ServerConfig) (raft.Logger, error) {
	if cfg.Logger != nil {
	// called after capnslog setting in "init" function
	if cfg.LoggerConfig != nil {
	return NewRaftLogger(cfg.LoggerConfig)
	} else if cfg.LoggerCore != nil && cfg.LoggerWriteSyncer != nil {
	return NewRaftLoggerFromZapCore(cfg.LoggerCore, cfg.LoggerWriteSyncer), nil
	} else {
	return NewRaftLoggerZap(cfg.Logger.Named("raft")), nil
	}

[gyuho]

I wonder whether there are resons to build RaftLogger from LoggerConfig or LoggerCore

No specific reason. Please feel free to make change. The only reason why we required log config is that we wanted to have consistent "default" logging format.

[lilic]

@serathius is there anything else left for this issue, as I noticed it has a 3.5 nice to have label and with the release approaching. Thank you!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

[Quentin-M]

@lilic Yes, while clientv3 and server allows passing a zap.Logger directly, embed still does not, it still only supports being passed a ZapLoggerBuilder which is pretty pointless when a logger could just be given directly.