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.

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

Logger: add Log method #1118

Merged
merged 3 commits into from
Jun 22, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,14 @@ func (log *Logger) Check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry {
return log.check(lvl, msg)
}

// 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...)
}
}

// 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) {
Expand Down
33 changes: 31 additions & 2 deletions logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ func TestLoggerLogPanic(t *testing.T) {
should bool
expected string
}{
{func(logger *Logger) { logger.Check(PanicLevel, "bar").Write() }, true, "bar"},
{func(logger *Logger) { logger.Check(PanicLevel, "foo").Write() }, true, "foo"},
{func(logger *Logger) { logger.Log(PanicLevel, "bar") }, true, "bar"},
{func(logger *Logger) { logger.Panic("baz") }, true, "baz"},
} {
withLogger(t, DebugLevel, nil, func(logger *Logger, logs *observer.ObservedLogs) {
Expand Down Expand Up @@ -147,7 +148,8 @@ func TestLoggerLogFatal(t *testing.T) {
do func(*Logger)
expected string
}{
{func(logger *Logger) { logger.Check(FatalLevel, "bar").Write() }, "bar"},
{func(logger *Logger) { logger.Check(FatalLevel, "foo").Write() }, "foo"},
{func(logger *Logger) { logger.Log(FatalLevel, "bar") }, "bar"},
{func(logger *Logger) { logger.Fatal("baz") }, "baz"},
} {
withLogger(t, DebugLevel, nil, func(logger *Logger, logs *observer.ObservedLogs) {
Expand Down Expand Up @@ -194,12 +196,36 @@ func TestLoggerLeveledMethods(t *testing.T) {
})
}

func TestLoggerLogLevels(t *testing.T) {
withLogger(t, DebugLevel, nil, func(logger *Logger, logs *observer.ObservedLogs) {
levels := []zapcore.Level{
DebugLevel,
InfoLevel,
WarnLevel,
ErrorLevel,
DPanicLevel,
}
for i, level := range levels {
logger.Log(level, "")
output := logs.AllUntimed()
assert.Equal(t, i+1, len(output), "Unexpected number of logs.")
assert.Equal(t, 0, len(output[i].Context), "Unexpected context on first log.")
assert.Equal(
t,
zapcore.Entry{Level: level},
output[i].Entry,
"Unexpected output from %s-level logger method.", level)
}
})
}

func TestLoggerAlwaysPanics(t *testing.T) {
// Users can disable writing out panic-level logs, but calls to logger.Panic()
// should still call panic().
withLogger(t, FatalLevel, nil, func(logger *Logger, logs *observer.ObservedLogs) {
msg := "Even if output is disabled, logger.Panic should always panic."
assert.Panics(t, func() { logger.Panic("foo") }, msg)
assert.Panics(t, func() { logger.Log(PanicLevel, "foo") }, msg)
assert.Panics(t, func() {
if ce := logger.Check(PanicLevel, "foo"); ce != nil {
ce.Write()
Expand All @@ -216,6 +242,9 @@ func TestLoggerAlwaysFatals(t *testing.T) {
stub := exit.WithStub(func() { logger.Fatal("") })
assert.True(t, stub.Exited, "Expected calls to logger.Fatal to terminate process.")

stub = exit.WithStub(func() { logger.Log(FatalLevel, "") })
assert.True(t, stub.Exited, "Expected calls to logger.Fatal to terminate process.")

stub = exit.WithStub(func() {
if ce := logger.Check(FatalLevel, ""); ce != nil {
ce.Write()
Expand Down
8 changes: 2 additions & 6 deletions sugar.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,9 +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 {
ce.Write(s.sweetenFields(context)...)
}
s.base.Log(lvl, msg, s.sweetenFields(context)...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would revert this and the other change in this file.

The Check version has the advantage that we don't spend CPU on sweetenFields unless we're actually going to log the message. So if the log level is too low or the message gets sampled, we won't pay to serialize the fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that is a good point. Thank you! Reverted.

}

// logln message with Sprintln
Expand All @@ -291,9 +289,7 @@ func (s *SugaredLogger) logln(lvl zapcore.Level, template string, fmtArgs []inte
}

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

// getMessage format with Sprint, Sprintf, or neither.
Expand Down