chore(logging)_: switch to zap.Logger as central logger #6025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (47)
|
osmaczko
force-pushed
the
feat/zap-logger-takes-control
branch
3 times, most recently
from
980b08f
to
0b320e3
Compare
osmaczko
force-pushed
the
feat/zap-logger-takes-control
branch
2 times, most recently
from
c6ca8b4
to
0aca460
Compare
osmaczko
changed the title
osmaczko
force-pushed
the
feat/zap-logger-takes-control
branch
from
0aca460
to
30da93f
Compare
Set zap.Logger as the primary logger for status-go. All geth logs are now proxied through zap.Logger. closes: #6029
osmaczko
force-pushed
the
feat/zap-logger-takes-control
branch
from
30da93f
to
282fcc1
Compare
igor-sirotin
approved these changes
Nov 4, 2024
| func lvlFromString(lvlString string) (zapcore.Level, error) { | ||
| switch strings.ToLower(lvlString) { | ||
| case "trace", "trce": | ||
| return zapcore.DebugLevel, nil // zap does not have a trace level, using DebugLevel as closest |
There was a problem hiding this comment.
I think we should skip trace messages. It will pollute debug logs too much.
|
|
||
| buffer.Reset() | ||
| log.Info("should be printed") | ||
| require.Regexp(t, `INFO\s+'INFO\s*\[.*\]\s*should be printed '`, buffer.String()) |
There was a problem hiding this comment.
Why is INFO duplicated in the log message? :think
| @@ -46,7 +45,6 @@ const ( | |||
| var ( | |||
| configFiles configFlags | |||
| logLevel = flag.String("log", "", `Log level, one of: "ERROR", "WARN", "INFO", "DEBUG", and "TRACE"`) | |||
| logWithoutColors = flag.Bool("log-without-color", false, "Disables log colors") | |||
There was a problem hiding this comment.
Probably better to keep the flag, but deprecate it. I'm not sure of all usages of our cmds.
Or ensure that it's not used anywhere. And mark the commit as a breaking change
There was a problem hiding this comment.
Pls don't remove it,I like the colorful output. Especially, we will frequently use the status backend server now.
|
|
||
| func defaultEncoder() zapcore.Encoder { | ||
| encoderConfig := zap.NewDevelopmentEncoderConfig() | ||
| encoderConfig.EncodeTime = utcTimeEncoder(encoderConfig.EncodeTime) |
| if fileOpts.Filename != "" { | ||
| if fileOpts.MaxBackups == 0 { | ||
|
|
||
| if settings.File != "" { |
There was a problem hiding this comment.
Can you please invrert the condition, so that the shorter branch goes first?
qfrank
reviewed
Nov 11, 2024
| @@ -73,8 +71,11 @@ func init() { | |||
|
|
|||
| // nolint:gocyclo | |||
| func main() { | |||
| colors := terminal.IsTerminal(int(os.Stdin.Fd())) | |||
| if err := logutils.OverrideRootLog(true, "ERROR", logutils.FileOptions{}, colors); err != nil { | |||
| @@ -46,7 +45,6 @@ const ( | |||
| var ( | |||
| configFiles configFlags | |||
| logLevel = flag.String("log", "", `Log level, one of: "ERROR", "WARN", "INFO", "DEBUG", and "TRACE"`) | |||
| logWithoutColors = flag.Bool("log-without-color", false, "Disables log colors") | |||
There was a problem hiding this comment.
Pls don't remove it,I like the colorful output. Especially, we will frequently use the status backend server now.
| @@ -53,6 +53,14 @@ func (core *Core) Enabled(lvl zapcore.Level) bool { | |||
| return core.level.Enabled(lvl) | |||
| } | |||
|
|
|||
| func (core *Core) Level() zapcore.Level { | |||
There was a problem hiding this comment.
can we rename the file to zap_core.go?
| handler := log.StreamHandler(buf, log.TerminalFormat(false)) | ||
| require.NoError(t, enableRootLog("debug", handler)) | ||
| log.Debug("hello") | ||
| require.Contains(t, buf.String(), "logutils/logger_test.go:16") |
There was a problem hiding this comment.
will it contain the origin info after replaced with zap?
| func OverrideRootLog(enabled bool, levelStr string, fileOpts FileOptions, terminal bool) error { | ||
| if !enabled { | ||
| disableRootLog() | ||
| if settings.MobileSystem { |
There was a problem hiding this comment.
it seems we never set MobileSystem to true in frontend, do you know why we need this ? 🤔
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #6029
NOTE: It will require testing on both platforms.