-
Notifications
You must be signed in to change notification settings - Fork 817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix don't swallow the error when building zap logger #4102
Fix don't swallow the error when building zap logger #4102
Conversation
Currently, start the cadence server would be panic when failed to build the zap logger. It will cause confusion since the error was swallowed.
tools/cli/domainUtils.go
Outdated
@@ -299,7 +299,8 @@ func initializeDomainHandler( | |||
func initializeLogger( | |||
serviceConfig *config.Config, | |||
) log.Logger { | |||
return loggerimpl.NewLogger(serviceConfig.Log.NewZapLogger()) | |||
zapLogger, _ := serviceConfig.Log.NewZapLogger() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log fatal or panic here in case of error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you don't have a logger, so the only thing you can do is to panic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have used the ErrorAndExited
to log the error and exit
Huh. Why don't we have this passed in? Constructing a logger seems doomed to be over-complicated (as we expose more and more logger-configuration fields) and less flexible (zap loggers can do effectively anything, there's no reason for us to limit them). Accepting a logger would also simplify the |
Which one are you referring to? I think we have to do this way because they are in from different |
@Groxx I merged this PR but if you see a way that we can improve by passing around logger, we make do a refactoring |
What changed?
Fix don't swallow the error when building zap logger
Why?
Currently, start the cadence server would be panic when
failed to build the zap logger. It will cause confusion since
the error was swallowed.
How did you test it?
test local with no permission log dir
Potential risks
no