From a5c845c2247a1798ddfb903dd3af2d5e062f6c7e Mon Sep 17 00:00:00 2001 From: Dave Clendenan Date: Wed, 30 Nov 2016 14:07:10 -0800 Subject: [PATCH] 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. --- entry.go | 4 +-- exported.go | 10 ++----- formatter.go | 6 ++-- json_formatter.go | 10 +++++-- json_formatter_test.go | 1 + logrus_test.go | 68 ++++++++++++++++++++---------------------- text_formatter.go | 17 +++++++++-- 7 files changed, 63 insertions(+), 53 deletions(-) diff --git a/entry.go b/entry.go index 850b958..9e86687 100644 --- a/entry.go +++ b/entry.go @@ -123,7 +123,7 @@ func getCaller() (method string) { } // 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 @@ -133,7 +133,7 @@ func (entry Entry) log(level Level, msg string) { entry.Time = time.Now() entry.Level = level entry.Message = msg - if ReportCaller() { + if entry.Logger.ReportCaller { entry.Caller = getCaller() } if err := entry.Logger.Hooks.Fire(level, &entry); err != nil { diff --git a/exported.go b/exported.go index 356d4a9..3585f94 100644 --- a/exported.go +++ b/exported.go @@ -27,20 +27,14 @@ func SetFormatter(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) { std.mu.Lock() defer std.mu.Unlock() 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. func SetLevel(level Level) { std.mu.Lock() diff --git a/formatter.go b/formatter.go index 9e94689..773b359 100644 --- a/formatter.go +++ b/formatter.go @@ -31,7 +31,7 @@ type Formatter interface { // // 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. -func prefixFieldClashes(data Fields) { +func prefixFieldClashes(data Fields, reportCaller bool) { if t, ok := data["time"]; ok { data["fields.time"] = t } @@ -44,8 +44,8 @@ func prefixFieldClashes(data Fields) { data["fields.level"] = l } - // If Reportmethod is not set, 'method' will not conflict. - if ReportCaller() { + // If reportCaller is not set, 'method' will not conflict. + if reportCaller { if l, ok := data["method"]; ok { data["fields.method"] = l } diff --git a/json_formatter.go b/json_formatter.go index 2bb3a6a..46498da 100644 --- a/json_formatter.go +++ b/json_formatter.go @@ -52,7 +52,13 @@ func (f *JSONFormatter) Format(entry *Entry) ([]byte, error) { data[k] = v } } - prefixFieldClashes(data) + + reportCaller := false + if entry.Logger != nil { + reportCaller = entry.Logger.ReportCaller + } + + prefixFieldClashes(data, reportCaller) timestampFormat := f.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(FieldKeyMsg)] = entry.Message data[f.FieldMap.resolve(FieldKeyLevel)] = entry.Level.String() - if ReportCaller() { + if reportCaller { data[f.FieldMap.resolve(FieldKeyCaller)] = entry.Caller } serialized, err := json.Marshal(data) diff --git a/json_formatter_test.go b/json_formatter_test.go index ab52095..8235224 100644 --- a/json_formatter_test.go +++ b/json_formatter_test.go @@ -193,6 +193,7 @@ func TestFieldDoesNotClashWithCaller(t *testing.T) { func TestFieldClashWithCaller(t *testing.T) { SetReportCaller(true) formatter := &JSONFormatter{} + std.ReportCaller = true b, err := formatter.Format(WithField("method", "howdy pardner")) if err != nil { diff --git a/logrus_test.go b/logrus_test.go index ec31850..40cbd64 100644 --- a/logrus_test.go +++ b/logrus_test.go @@ -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 func TestReportCaller(t *testing.T) { LogAndAssertJSON(t, func(log *Logger) { - SetReportCaller(false) + log.ReportCaller = false log.Print("testNoCaller") }, func(fields Fields) { - assert.Equal(t, fields["msg"], "testNoCaller") - assert.Equal(t, fields["level"], "info") - assert.Equal(t, fields["method"], nil) + assert.Equal(t, "testNoCaller", fields["msg"]) + assert.Equal(t, "info", fields["level"]) + assert.Equal(t, nil, fields["method"]) }) LogAndAssertJSON(t, func(log *Logger) { - SetReportCaller(true) + log.ReportCaller = true log.Print("testWithCaller") }, func(fields Fields) { - assert.Equal(t, fields["msg"], "testWithCaller") - assert.Equal(t, fields["level"], "info") - assert.Equal(t, fields["method"], "testing.tRunner") + assert.Equal(t, "testWithCaller", fields["msg"]) + assert.Equal(t, "info", fields["level"]) + assert.Equal(t, "testing.tRunner", fields["method"]) }) - - SetReportCaller(false) // return to default value } func TestPrint(t *testing.T) { LogAndAssertJSON(t, func(log *Logger) { log.Print("test") }, func(fields Fields) { - assert.Equal(t, fields["msg"], "test") - assert.Equal(t, fields["level"], "info") + assert.Equal(t, "test", fields["msg"]) + assert.Equal(t, "info", fields["level"]) }) } @@ -93,8 +91,8 @@ func TestInfo(t *testing.T) { LogAndAssertJSON(t, func(log *Logger) { log.Info("test") }, func(fields Fields) { - assert.Equal(t, fields["msg"], "test") - assert.Equal(t, fields["level"], "info") + assert.Equal(t, "test", fields["msg"]) + assert.Equal(t, "info", fields["level"]) }) } @@ -102,8 +100,8 @@ func TestWarn(t *testing.T) { LogAndAssertJSON(t, func(log *Logger) { log.Warn("test") }, func(fields Fields) { - assert.Equal(t, fields["msg"], "test") - assert.Equal(t, fields["level"], "warning") + assert.Equal(t, "test", fields["msg"]) + assert.Equal(t, "warning", fields["level"]) }) } @@ -111,7 +109,7 @@ func TestInfolnShouldAddSpacesBetweenStrings(t *testing.T) { LogAndAssertJSON(t, func(log *Logger) { log.Infoln("test", "test") }, 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) { log.Infoln("test", 10) }, 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) { log.Infoln(10, 10) }, 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) { log.Infoln(10, 10) }, 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) { log.Info("test", 10) }, 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) { log.Info("test", "test") }, 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) { log.WithField("msg", "hello").Info("test") }, 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) { log.WithField("msg", "hello").Info("test") }, func(fields Fields) { - assert.Equal(t, fields["msg"], "test") - assert.Equal(t, fields["fields.msg"], "hello") + assert.Equal(t, "test", fields["msg"]) + assert.Equal(t, "hello", fields["fields.msg"]) }) } @@ -206,7 +204,7 @@ func TestUserSuppliedTimeFieldHasPrefix(t *testing.T) { LogAndAssertJSON(t, func(log *Logger) { log.WithField("time", "hello").Info("test") }, 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) { log.WithField("level", 1).Info("test") }, func(fields Fields) { - assert.Equal(t, fields["level"], "info") - assert.Equal(t, fields["fields.level"], 1.0) // JSON has floats only + assert.Equal(t, "info", fields["level"]) + 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) 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, fields["msg"], "omg it is!") - assert.Equal(t, fields["context"], "eating raw fish") + assert.Equal(t, "omg it is!", fields["msg"]) + assert.Equal(t, "eating raw fish", fields["context"]) 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 fields Fields - SetReportCaller(true) logger := New() logger.Out = &buffer logger.Formatter = new(JSONFormatter) + logger.ReportCaller = true llog := logger.WithField("context", "eating raw fish") @@ -281,9 +279,9 @@ func TestNestedLoggingReportsCorrectCaller(t *testing.T) { err := json.Unmarshal(buffer.Bytes(), &fields) 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, fields["msg"], "looks delicious") - assert.Equal(t, fields["context"], "eating raw fish") - assert.Equal(t, fields["method"], "testing.tRunner") + assert.Equal(t, "looks delicious", fields["msg"]) + assert.Equal(t, "eating raw fish", fields["context"]) + assert.Equal(t, "testing.tRunner", fields["method"]) 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.Equal(t, "testing.tRunner", fields["method"]) - SetReportCaller(false) // return to default value + logger.ReportCaller = false // return to default value } func TestConvertLevelToString(t *testing.T) { diff --git a/text_formatter.go b/text_formatter.go index b003451..91715f8 100644 --- a/text_formatter.go +++ b/text_formatter.go @@ -72,7 +72,12 @@ func (f *TextFormatter) Format(entry *Entry) ([]byte, error) { 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") 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, "level", entry.Level.String()) - if ReportCaller() { + if reportCaller { f.appendKeyValue(b, "method", entry.Caller) } 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] caller := "" - if ReportCaller() { + + reportCaller := false + if entry.Logger != nil { + reportCaller = entry.Logger.ReportCaller + } + + if reportCaller { caller = fmt.Sprintf(" %s()", entry.Caller) }