-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
feat: structured logs with uber/zap #848
Conversation
Pull Request Test Coverage Report for Build 3f60b86ad-PR-848
💛 - Coveralls |
fix: init log level chore: remove coverage.out :s
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.
Looking good in here. I have a few minor comments inline. The main thing I want to think through here is, do we really want to support 2 types of loggers? I guess that might be nice for a transition period, but it seems like we should maybe rally around a single logging implementation. Maybe we deprecate the old one in favor of the new?
What do you think @gdey?
// SetOutput... | ||
// To satisfy log.Interface | ||
func (z *ZapLogger) SetOutput(io.Writer) { | ||
return |
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.
Interesting. Does zap not support changing the output location?
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.
You can set the output location within the zap config before instantiating the logger. See here.
Something like:
zapconfig.OutputPaths = []string{
"/var/log/myproject/myproject.log",
}
would also be possible here.
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 think we should drop SetOutput from the Interface{}, we don't have any calls to it, and having the output set during the configuration of the logger makes the most sense.
return fmt.Errorf("invalid logger %s", logger) | ||
} | ||
} | ||
|
||
func getLogLevelFromString(level string) (log.Level, error) { | ||
switch level { | ||
case "TRACE": |
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.
Do we have some consts we can key off of for these log levels?
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.
Not sure if I understand your right here. Log levels are numbers ranging from -2 to 3 e.g. log.TRACE
equals -2 and we have to derive this from the users input string.
We could add additional consts like TRACE_STRING
for that switch statement I guess?
While zap is fast, text logs have the edge in terms of performance because json serialization has an overhead. Giving the user the option is only fair as not every users wants to reap the benefits of json logs, or doesn't even care at all. But ultimately that's your decision to make of course. :) |
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 think we should drop the SetOutput from the log.Interface
@@ -46,6 +58,8 @@ func init() { | |||
"path or http url to a config file, or \"-\" for stdin") | |||
RootCmd.PersistentFlags().StringVar(&logLevel, "log-level", "INFO", | |||
"work in progress - set log level to: TRACE, DEBUG, INFO, WARN or ERROR") | |||
RootCmd.PersistentFlags().StringVar(&logger, "logger", "standard", |
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.
minor nit, might want to use the log.STANDARD
constant here.
// SetOutput... | ||
// To satisfy log.Interface | ||
func (z *ZapLogger) SetOutput(io.Writer) { | ||
return |
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 think we should drop SetOutput from the Interface{}, we don't have any calls to it, and having the output set during the configuration of the logger makes the most sense.
Hi 👋
As discussed in #831 I added the option to use structured logs with uber/zap while falling back to the currently implemented default if
--logger
is not specifically used. Log levels conform to the current implementation with the exception of log levelTRACE
that is not supported byzap
. I useDebug
as a fall back here.Looking forward for your feedback