Fix bug with issues [17464] fix with zap logger redirection #17510
Conversation
|
Hi @LLiuJJ. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Colin <jay_jieliu@outlook.com>
Signed-off-by: Colin <jay_jieliu@outlook.com>
Signed-off-by: Colin <jay_jieliu@outlook.com>
Signed-off-by: Colin <jay_jieliu@outlook.com>
|
/ok-to-test |
|
/cc @ivanvc |
client/pkg/logutil/zap.go
Outdated
| lcfg.Development = false | ||
| lcfg.Encoding = DefaultLogFormat | ||
| lcfg.Sampling = &zap.SamplingConfig{ | ||
| Initial: 100, | ||
| Thereafter: 100, | ||
| } |
There was a problem hiding this comment.
These values are already defined in DefaultZapLoggerConfig; any reason why to redefine them here?
There was a problem hiding this comment.
( ⋂‿⋂’) Thank you for your review, Your suggestion is right, I will resubmit my fix code.
client/pkg/logutil/zap.go
Outdated
| return level == zapcore.InfoLevel | ||
| }) | ||
| errorFatalLevel := zap.LevelEnablerFunc(func(level zapcore.Level) bool { | ||
| return level == zapcore.ErrorLevel || level == zapcore.FatalLevel |
There was a problem hiding this comment.
The problem with this is that it's not handling the WarningLevel, nor PanicLevel. It is an enumeration, so it would be useful to compare using greater than.
client/pkg/logutil/zap.go
Outdated
| @@ -33,6 +34,39 @@ func CreateDefaultZapLogger(level zapcore.Level) (*zap.Logger, error) { | |||
| return c, nil | |||
| } | |||
|
|
|||
| // CreateUtilZapLogger creates a logger with default zap configuration can redirect log to /dev/null | |||
| func CreateUtilZapLogger(level zapcore.Level) *zap.Logger { | |||
There was a problem hiding this comment.
I was able to keep returning the *zap.Logger and an error, by writing it like so:
func CreateUtilZapLogger(level zapcore.Level) (*zap.Logger, error) {
lcfg := DefaultZapLoggerConfig
lcfg.Level = zap.NewAtomicLevelAt(level)
infoLevel := zap.LevelEnablerFunc(func(level zapcore.Level) bool {
return level <= zapcore.InfoLevel
})
errorFatalLevel := zap.LevelEnablerFunc(func(level zapcore.Level) bool {
return level > zapcore.InfoLevel
})
stdoutSyncer := zapcore.Lock(os.Stdout)
stderrSyncer := zapcore.Lock(os.Stderr)
opts := []zap.Option{
zap.WrapCore(func(core zapcore.Core) zapcore.Core {
return zapcore.NewTee(
zapcore.NewCore(
zapcore.NewJSONEncoder(lcfg.EncoderConfig),
stdoutSyncer,
infoLevel,
),
zapcore.NewCore(
zapcore.NewJSONEncoder(lcfg.EncoderConfig),
stderrSyncer,
errorFatalLevel,
),
)
}),
}
return lcfg.Build(opts...)
}Which also makes me think if this should be an extension to CreateDefaultZapLogger(), and allow it to receive zap.Options.
There was a problem hiding this comment.
I'm not too fond of the name CreateUtilZapLogger. But I don't want to request more changes, being respectful of the contributor's time, as we don't have a conclusion on the parent issue 🙂.
| if err != nil { | ||
| cobrautl.ExitWithError(cobrautl.ExitError, err) | ||
| } | ||
| lg := logutil.CreateUtilZapLogger(zap.InfoLevel) |
There was a problem hiding this comment.
The problem I spot here is that it will only fix the issue for snapshot save. I'm curious about what's the conclusion of issue #17464. So, maybe this fix should expand to other commands, not only this one.
There was a problem hiding this comment.
There's still one instance in which it still calls CreateDefaultZapLogger. Refer to etcdctl/ctlv3/command/global.go:141.
Signed-off-by: Colin <jay_jieliu@outlook.com>
|
/retest |
Signed-off-by: Colin <jay_jieliu@outlook.com>
|
/retest |
1 similar comment
|
/retest |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
etcdctl: fix etcdctl zap logger redirection bug
This bug was report with issues #17464
Here is my test log
Signed-off-by: Colin jay_jieliu@outlook.com