*: add pingcap/log encoder as the default encoder #62
Conversation
Signed-off-by: xhe <xw897002528@gmail.com>
cmd/weirproxy/main.go
Outdated
| @@ -33,7 +33,7 @@ func main() { | |||
| } | |||
|
|
|||
| configFile := rootCmd.PersistentFlags().String("config", "conf/weirproxy.yaml", "weir proxy config file path") | |||
| logEncoder := rootCmd.PersistentFlags().String("log_encoder", "", "log in format of tidb, console, or json") | |||
| logEncoder := rootCmd.PersistentFlags().String("log_encoder", "", "log in format of tidb, newtidb, console, or json") | |||
There was a problem hiding this comment.
This is shown for users, don't show this. It just confuses users.
Why don't you trust log.NewTextEncoder? Did you fix any bugs on your version?
There was a problem hiding this comment.
This is shown for users, don't show this. It just confuses users.
Removed from the description.
Why don't you trust log.NewTextEncoder?
I've trust log.NewTextEncoder much that I just modified the encoder instead of writing a new one from the scratch.
The modification of encoder is seen as a minor improvement that should not have problems. And it is indeed a minor change. Even if it has problems, like now, I can fix it easily.
Did you fix any bugs on your version?
I don't, neither the old encoder.
Most bugs of pingcap/log is not in the encoder, but comes from its buggy API. I did not even include one line of other components. The old encoder code have not been modified since Mar 2019.
But there was a bug in the function log.NewTextEncoder. I would say that the encoder itself is indeed stable(or more like unmaintained, should have special processors for key encodings or errors), but not other parts.
Signed-off-by: xhe <xw897002528@gmail.com>
| @@ -32,6 +33,9 @@ var registerEncoders sync.Once | |||
| func RunRootCommand(rootCmd *cobra.Command) { | |||
| registerEncoders.Do(func() { | |||
| zap.RegisterEncoder("tidb", func(cfg zapcore.EncoderConfig) (zapcore.Encoder, error) { | |||
| return log.NewTextEncoder(&log.Config{}) | |||
There was a problem hiding this comment.
Have a manual test to verify that the log configurations in the configuration file can work.
There was a problem hiding this comment.
LogLevel has no relation with Encoder. zapcore will handle loglevel and logfile. That said, there is no options in our yaml files, or options from EncoderConfig can be applied to log.NewTextEncoder. If we want to apply all zap config, either we need to modify pingcap/log, or we need to copy the whole file.
LogFile support is not added. If I rememered it correct, it is not added in the original weir, too. I can add it to my TODO list. This should be a trivial change.
There was a problem hiding this comment.
LogLevel tested, LogFile not working expectedly.
There was a problem hiding this comment.
File an issue to support LogFile.
There was a problem hiding this comment.
File an issue to support
LogFile.
It is recorded in the list already.
Signed-off-by: xhe <xw897002528@gmail.com>
| @@ -57,6 +57,7 @@ func main() { | |||
|
|
|||
| zapcfg := zap.NewDevelopmentConfig() | |||
| zapcfg.Encoding = cfg.Log.Encoder | |||
| zapcfg.DisableStacktrace = true | |||
There was a problem hiding this comment.
While Encoder can ensure there is no multiline string, the stacktrace from zap iteself may escape.
Signed-off-by: xhe xw897002528@gmail.com
What problem does this PR solve?
Issue Number: close None
Problem Summary: To reduce the risk of GA, the old encoder is brought back as the default.
What is changed and how it works:
StackTraceby zap is multiline, this is not compatible withtidbformat.Check List
Tests
Notable changes
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.