responses to review comments

- empty string as marker for failure to discover calling function
 - tighten up logger usage - don't rely on std logger internally

Also fix ordering of expected/got in logrus_test.go to ensure correct
output form test failures.
This commit is contained in:
Dave Clendenan 2016-11-30 14:07:10 -08:00
parent 4575b7a64d
commit a5c845c224
7 changed files with 63 additions and 53 deletions

View File

@ -123,7 +123,7 @@ func getCaller() (method string) {
} }
// if we got here, we failed to find the caller's context // if we got here, we failed to find the caller's context
return "UNKNOWN_CALLER" return ""
} }
// This function is not declared with a pointer value because otherwise // This function is not declared with a pointer value because otherwise
@ -133,7 +133,7 @@ func (entry Entry) log(level Level, msg string) {
entry.Time = time.Now() entry.Time = time.Now()
entry.Level = level entry.Level = level
entry.Message = msg entry.Message = msg
if ReportCaller() { if entry.Logger.ReportCaller {
entry.Caller = getCaller() entry.Caller = getCaller()
} }
if err := entry.Logger.Hooks.Fire(level, &entry); err != nil { if err := entry.Logger.Hooks.Fire(level, &entry); err != nil {

View File

@ -27,20 +27,14 @@ func SetFormatter(formatter Formatter) {
std.Formatter = formatter std.Formatter = formatter
} }
// SetReportCaller sets whether to include the calling method as a field // SetReportCaller sets whether the standard logger will include the calling
// method as a field.
func SetReportCaller(include bool) { func SetReportCaller(include bool) {
std.mu.Lock() std.mu.Lock()
defer std.mu.Unlock() defer std.mu.Unlock()
std.ReportCaller = include std.ReportCaller = include
} }
// ReportCaller returns the 'include calling method' state
func ReportCaller() bool {
std.mu.Lock()
defer std.mu.Unlock()
return std.ReportCaller
}
// SetLevel sets the standard logger level. // SetLevel sets the standard logger level.
func SetLevel(level Level) { func SetLevel(level Level) {
std.mu.Lock() std.mu.Lock()

View File

@ -31,7 +31,7 @@ type Formatter interface {
// //
// It's not exported because it's still using Data in an opinionated way. It's to // It's not exported because it's still using Data in an opinionated way. It's to
// avoid code duplication between the two default formatters. // avoid code duplication between the two default formatters.
func prefixFieldClashes(data Fields) { func prefixFieldClashes(data Fields, reportCaller bool) {
if t, ok := data["time"]; ok { if t, ok := data["time"]; ok {
data["fields.time"] = t data["fields.time"] = t
} }
@ -44,8 +44,8 @@ func prefixFieldClashes(data Fields) {
data["fields.level"] = l data["fields.level"] = l
} }
// If Reportmethod is not set, 'method' will not conflict. // If reportCaller is not set, 'method' will not conflict.
if ReportCaller() { if reportCaller {
if l, ok := data["method"]; ok { if l, ok := data["method"]; ok {
data["fields.method"] = l data["fields.method"] = l
} }

View File

@ -52,7 +52,13 @@ func (f *JSONFormatter) Format(entry *Entry) ([]byte, error) {
data[k] = v data[k] = v
} }
} }
prefixFieldClashes(data)
reportCaller := false
if entry.Logger != nil {
reportCaller = entry.Logger.ReportCaller
}
prefixFieldClashes(data, reportCaller)
timestampFormat := f.TimestampFormat timestampFormat := f.TimestampFormat
if timestampFormat == "" { if timestampFormat == "" {
@ -62,7 +68,7 @@ func (f *JSONFormatter) Format(entry *Entry) ([]byte, error) {
data[f.FieldMap.resolve(FieldKeyTime)] = entry.Time.Format(timestampFormat) data[f.FieldMap.resolve(FieldKeyTime)] = entry.Time.Format(timestampFormat)
data[f.FieldMap.resolve(FieldKeyMsg)] = entry.Message data[f.FieldMap.resolve(FieldKeyMsg)] = entry.Message
data[f.FieldMap.resolve(FieldKeyLevel)] = entry.Level.String() data[f.FieldMap.resolve(FieldKeyLevel)] = entry.Level.String()
if ReportCaller() { if reportCaller {
data[f.FieldMap.resolve(FieldKeyCaller)] = entry.Caller data[f.FieldMap.resolve(FieldKeyCaller)] = entry.Caller
} }
serialized, err := json.Marshal(data) serialized, err := json.Marshal(data)

View File

@ -193,6 +193,7 @@ func TestFieldDoesNotClashWithCaller(t *testing.T) {
func TestFieldClashWithCaller(t *testing.T) { func TestFieldClashWithCaller(t *testing.T) {
SetReportCaller(true) SetReportCaller(true)
formatter := &JSONFormatter{} formatter := &JSONFormatter{}
std.ReportCaller = true
b, err := formatter.Format(WithField("method", "howdy pardner")) b, err := formatter.Format(WithField("method", "howdy pardner"))
if err != nil { if err != nil {

View File

@ -60,32 +60,30 @@ func LogAndAssertText(t *testing.T, log func(*Logger), assertions func(fields ma
// is added, and when it is unset it is not set or modified // is added, and when it is unset it is not set or modified
func TestReportCaller(t *testing.T) { func TestReportCaller(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
SetReportCaller(false) log.ReportCaller = false
log.Print("testNoCaller") log.Print("testNoCaller")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "testNoCaller") assert.Equal(t, "testNoCaller", fields["msg"])
assert.Equal(t, fields["level"], "info") assert.Equal(t, "info", fields["level"])
assert.Equal(t, fields["method"], nil) assert.Equal(t, nil, fields["method"])
}) })
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
SetReportCaller(true) log.ReportCaller = true
log.Print("testWithCaller") log.Print("testWithCaller")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "testWithCaller") assert.Equal(t, "testWithCaller", fields["msg"])
assert.Equal(t, fields["level"], "info") assert.Equal(t, "info", fields["level"])
assert.Equal(t, fields["method"], "testing.tRunner") assert.Equal(t, "testing.tRunner", fields["method"])
}) })
SetReportCaller(false) // return to default value
} }
func TestPrint(t *testing.T) { func TestPrint(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Print("test") log.Print("test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "test") assert.Equal(t, "test", fields["msg"])
assert.Equal(t, fields["level"], "info") assert.Equal(t, "info", fields["level"])
}) })
} }
@ -93,8 +91,8 @@ func TestInfo(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Info("test") log.Info("test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "test") assert.Equal(t, "test", fields["msg"])
assert.Equal(t, fields["level"], "info") assert.Equal(t, "info", fields["level"])
}) })
} }
@ -102,8 +100,8 @@ func TestWarn(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Warn("test") log.Warn("test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "test") assert.Equal(t, "test", fields["msg"])
assert.Equal(t, fields["level"], "warning") assert.Equal(t, "warning", fields["level"])
}) })
} }
@ -111,7 +109,7 @@ func TestInfolnShouldAddSpacesBetweenStrings(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Infoln("test", "test") log.Infoln("test", "test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "test test") assert.Equal(t, "test test", fields["msg"])
}) })
} }
@ -119,7 +117,7 @@ func TestInfolnShouldAddSpacesBetweenStringAndNonstring(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Infoln("test", 10) log.Infoln("test", 10)
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "test 10") assert.Equal(t, "test 10", fields["msg"])
}) })
} }
@ -127,7 +125,7 @@ func TestInfolnShouldAddSpacesBetweenTwoNonStrings(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Infoln(10, 10) log.Infoln(10, 10)
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "10 10") assert.Equal(t, "10 10", fields["msg"])
}) })
} }
@ -135,7 +133,7 @@ func TestInfoShouldAddSpacesBetweenTwoNonStrings(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Infoln(10, 10) log.Infoln(10, 10)
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "10 10") assert.Equal(t, "10 10", fields["msg"])
}) })
} }
@ -143,7 +141,7 @@ func TestInfoShouldNotAddSpacesBetweenStringAndNonstring(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Info("test", 10) log.Info("test", 10)
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "test10") assert.Equal(t, "test10", fields["msg"])
}) })
} }
@ -151,7 +149,7 @@ func TestInfoShouldNotAddSpacesBetweenStrings(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.Info("test", "test") log.Info("test", "test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "testtest") assert.Equal(t, "testtest", fields["msg"])
}) })
} }
@ -189,7 +187,7 @@ func TestUserSuppliedFieldDoesNotOverwriteDefaults(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.WithField("msg", "hello").Info("test") log.WithField("msg", "hello").Info("test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "test") assert.Equal(t, "test", fields["msg"])
}) })
} }
@ -197,8 +195,8 @@ func TestUserSuppliedMsgFieldHasPrefix(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.WithField("msg", "hello").Info("test") log.WithField("msg", "hello").Info("test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["msg"], "test") assert.Equal(t, "test", fields["msg"])
assert.Equal(t, fields["fields.msg"], "hello") assert.Equal(t, "hello", fields["fields.msg"])
}) })
} }
@ -206,7 +204,7 @@ func TestUserSuppliedTimeFieldHasPrefix(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.WithField("time", "hello").Info("test") log.WithField("time", "hello").Info("test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["fields.time"], "hello") assert.Equal(t, "hello", fields["fields.time"])
}) })
} }
@ -214,8 +212,8 @@ func TestUserSuppliedLevelFieldHasPrefix(t *testing.T) {
LogAndAssertJSON(t, func(log *Logger) { LogAndAssertJSON(t, func(log *Logger) {
log.WithField("level", 1).Info("test") log.WithField("level", 1).Info("test")
}, func(fields Fields) { }, func(fields Fields) {
assert.Equal(t, fields["level"], "info") assert.Equal(t, "info", fields["level"])
assert.Equal(t, fields["fields.level"], 1.0) // JSON has floats only assert.Equal(t, 1.0, fields["fields.level"]) // JSON has floats only
}) })
} }
@ -259,8 +257,8 @@ func TestDoubleLoggingDoesntPrefixPreviousFields(t *testing.T) {
err = json.Unmarshal(buffer.Bytes(), &fields) err = json.Unmarshal(buffer.Bytes(), &fields)
assert.NoError(t, err, "should have decoded second message") assert.NoError(t, err, "should have decoded second message")
assert.Equal(t, len(fields), 4, "should only have msg/time/level/context fields") assert.Equal(t, len(fields), 4, "should only have msg/time/level/context fields")
assert.Equal(t, fields["msg"], "omg it is!") assert.Equal(t, "omg it is!", fields["msg"])
assert.Equal(t, fields["context"], "eating raw fish") assert.Equal(t, "eating raw fish", fields["context"])
assert.Nil(t, fields["fields.msg"], "should not have prefixed previous `msg` entry") assert.Nil(t, fields["fields.msg"], "should not have prefixed previous `msg` entry")
} }
@ -269,10 +267,10 @@ func TestNestedLoggingReportsCorrectCaller(t *testing.T) {
var buffer bytes.Buffer var buffer bytes.Buffer
var fields Fields var fields Fields
SetReportCaller(true)
logger := New() logger := New()
logger.Out = &buffer logger.Out = &buffer
logger.Formatter = new(JSONFormatter) logger.Formatter = new(JSONFormatter)
logger.ReportCaller = true
llog := logger.WithField("context", "eating raw fish") llog := logger.WithField("context", "eating raw fish")
@ -281,9 +279,9 @@ func TestNestedLoggingReportsCorrectCaller(t *testing.T) {
err := json.Unmarshal(buffer.Bytes(), &fields) err := json.Unmarshal(buffer.Bytes(), &fields)
assert.NoError(t, err, "should have decoded first message") assert.NoError(t, err, "should have decoded first message")
assert.Equal(t, len(fields), 5, "should have msg/time/level/method/context fields") assert.Equal(t, len(fields), 5, "should have msg/time/level/method/context fields")
assert.Equal(t, fields["msg"], "looks delicious") assert.Equal(t, "looks delicious", fields["msg"])
assert.Equal(t, fields["context"], "eating raw fish") assert.Equal(t, "eating raw fish", fields["context"])
assert.Equal(t, fields["method"], "testing.tRunner") assert.Equal(t, "testing.tRunner", fields["method"])
buffer.Reset() buffer.Reset()
@ -311,7 +309,7 @@ func TestNestedLoggingReportsCorrectCaller(t *testing.T) {
assert.Nil(t, fields["fields.msg"], "should not have prefixed previous `msg` entry") assert.Nil(t, fields["fields.msg"], "should not have prefixed previous `msg` entry")
assert.Equal(t, "testing.tRunner", fields["method"]) assert.Equal(t, "testing.tRunner", fields["method"])
SetReportCaller(false) // return to default value logger.ReportCaller = false // return to default value
} }
func TestConvertLevelToString(t *testing.T) { func TestConvertLevelToString(t *testing.T) {

View File

@ -72,7 +72,12 @@ func (f *TextFormatter) Format(entry *Entry) ([]byte, error) {
b = &bytes.Buffer{} b = &bytes.Buffer{}
} }
prefixFieldClashes(entry.Data) reportCaller := false
if entry.Logger != nil {
reportCaller = entry.Logger.ReportCaller
}
prefixFieldClashes(entry.Data, reportCaller)
isColorTerminal := isTerminal && (runtime.GOOS != "windows") isColorTerminal := isTerminal && (runtime.GOOS != "windows")
isColored := (f.ForceColors || isColorTerminal) && !f.DisableColors isColored := (f.ForceColors || isColorTerminal) && !f.DisableColors
@ -88,7 +93,7 @@ func (f *TextFormatter) Format(entry *Entry) ([]byte, error) {
f.appendKeyValue(b, "time", entry.Time.Format(timestampFormat)) f.appendKeyValue(b, "time", entry.Time.Format(timestampFormat))
} }
f.appendKeyValue(b, "level", entry.Level.String()) f.appendKeyValue(b, "level", entry.Level.String())
if ReportCaller() { if reportCaller {
f.appendKeyValue(b, "method", entry.Caller) f.appendKeyValue(b, "method", entry.Caller)
} }
if entry.Message != "" { if entry.Message != "" {
@ -119,7 +124,13 @@ func (f *TextFormatter) printColored(b *bytes.Buffer, entry *Entry, keys []strin
levelText := strings.ToUpper(entry.Level.String())[0:4] levelText := strings.ToUpper(entry.Level.String())[0:4]
caller := "" caller := ""
if ReportCaller() {
reportCaller := false
if entry.Logger != nil {
reportCaller = entry.Logger.ReportCaller
}
if reportCaller {
caller = fmt.Sprintf(" %s()", entry.Caller) caller = fmt.Sprintf(" %s()", entry.Caller)
} }