Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor to reuse Log method #1128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor to reuse Log method #1128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 22 additions & 31 deletions logger.go
View file
Open in desktop
Expand Up @@ -187,47 +187,46 @@ func (log *Logger) With(fields ...Field) *Logger {
// is enabled. It's a completely optional optimization; in high-performance
// applications, Check can help avoid allocating a slice to hold fields.
func (log *Logger) Check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry {
return log.check(lvl, msg)
return log.check(lvl, msg, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: https://github.com/uber-go/guide/blob/master/style.md#avoid-naked-parameters

Suggested change
return log.check(lvl, msg, 0)
return log.check(lvl, msg, 0 /* skip */)

same for all below callsites where we pass a number

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misreading, but should this be 1 instead? And for log methods below, should those be 3? (There was previously a const offset of 2, but we've since added another intermediate call.)

Can we add a test to validate the skips?

}

func (log *Logger) log(lvl zapcore.Level, msg string, skip int, fields ...Field) {
if ce := log.check(lvl, msg, skip); ce != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should the log call add +1 to the skip so that callers don't need to specify 2

ce.Write(fields...)
}
Comment on lines +193 to +196
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Let's move Logger.log below the exported definitions (maybe right above or below Logger.check).

}

// Log logs a message at the specified level. The message includes any fields
// passed at the log site, as well as any fields accumulated on the logger.
func (log *Logger) Log(lvl zapcore.Level, msg string, fields ...Field) {
if ce := log.check(lvl, msg); ce != nil {
ce.Write(fields...)
}
// Skip 2 frames in Log and all Debug/Info/... methods
// below for the public method as well as the private log
// method.
log.log(lvl, msg, 2, fields...)
}

// Debug logs a message at DebugLevel. The message includes any fields passed
// at the log site, as well as any fields accumulated on the logger.
func (log *Logger) Debug(msg string, fields ...Field) {
if ce := log.check(DebugLevel, msg); ce != nil {
ce.Write(fields...)
}
log.log(DebugLevel, msg, 2, fields...)
}

// Info logs a message at InfoLevel. The message includes any fields passed
// at the log site, as well as any fields accumulated on the logger.
func (log *Logger) Info(msg string, fields ...Field) {
if ce := log.check(InfoLevel, msg); ce != nil {
ce.Write(fields...)
}
log.log(InfoLevel, msg, 2, fields...)
}

// Warn logs a message at WarnLevel. The message includes any fields passed
// at the log site, as well as any fields accumulated on the logger.
func (log *Logger) Warn(msg string, fields ...Field) {
if ce := log.check(WarnLevel, msg); ce != nil {
ce.Write(fields...)
}
log.log(WarnLevel, msg, 2, fields...)
}

// Error logs a message at ErrorLevel. The message includes any fields passed
// at the log site, as well as any fields accumulated on the logger.
func (log *Logger) Error(msg string, fields ...Field) {
if ce := log.check(ErrorLevel, msg); ce != nil {
ce.Write(fields...)
}
log.log(ErrorLevel, msg, 2, fields...)
}

// DPanic logs a message at DPanicLevel. The message includes any fields
Expand All @@ -237,19 +236,15 @@ func (log *Logger) Error(msg string, fields ...Field) {
// "development panic"). This is useful for catching errors that are
// recoverable, but shouldn't ever happen.
func (log *Logger) DPanic(msg string, fields ...Field) {
if ce := log.check(DPanicLevel, msg); ce != nil {
ce.Write(fields...)
}
log.log(DPanicLevel, msg, 2, fields...)
}

// Panic logs a message at PanicLevel. The message includes any fields passed
// at the log site, as well as any fields accumulated on the logger.
//
// The logger then panics, even if logging at PanicLevel is disabled.
func (log *Logger) Panic(msg string, fields ...Field) {
if ce := log.check(PanicLevel, msg); ce != nil {
ce.Write(fields...)
}
log.log(PanicLevel, msg, 2, fields...)
}

// Fatal logs a message at FatalLevel. The message includes any fields passed
Expand All @@ -258,9 +253,7 @@ func (log *Logger) Panic(msg string, fields ...Field) {
// The logger then calls os.Exit(1), even if logging at FatalLevel is
// disabled.
func (log *Logger) Fatal(msg string, fields ...Field) {
if ce := log.check(FatalLevel, msg); ce != nil {
ce.Write(fields...)
}
log.log(FatalLevel, msg, 2, fields...)
}

// Sync calls the underlying Core's Sync method, flushing any buffered log
Expand All @@ -279,12 +272,10 @@ func (log *Logger) clone() *Logger {
return &copy
}

func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry {
// Logger.check must always be called directly by a method in the
// Logger interface (e.g., Check, Info, Fatal).
// This skips Logger.check and the Info/Fatal/Check/etc. method that
// called it.
const callerSkipOffset = 2
func (log *Logger) check(lvl zapcore.Level, msg string, skip int) *zapcore.CheckedEntry {
// This skips Logger.check method on top of any additional
// skips from the caller.
callerSkipOffset := skip + 1

// Check the level first to reduce the cost of disabled log calls.
// Since Panic and higher may exit, we skip the optimization for those levels.
Expand Down
4 changes: 2 additions & 2 deletions sugar.go
View file
Open in desktop
Expand Up @@ -279,7 +279,7 @@ func (s *SugaredLogger) log(lvl zapcore.Level, template string, fmtArgs []interf
}

msg := getMessage(template, fmtArgs)
if ce := s.base.Check(lvl, msg); ce != nil {
if ce := s.base.check(lvl, msg, 0); ce != nil {
ce.Write(s.sweetenFields(context)...)
}
}
Expand All @@ -291,7 +291,7 @@ func (s *SugaredLogger) logln(lvl zapcore.Level, template string, fmtArgs []inte
}

msg := getMessageln(fmtArgs)
if ce := s.base.Check(lvl, msg); ce != nil {
if ce := s.base.check(lvl, msg, 0); ce != nil {
ce.Write(s.sweetenFields(context)...)
}
}
Expand Down