diff --git a/config_test.go b/config_test.go index 19af60795..ac098aafe 100644 --- a/config_test.go +++ b/config_test.go @@ -52,7 +52,7 @@ func TestConfig(t *testing.T) { expectRe: "DEBUG\tzap/config_test.go:" + `\d+` + "\tdebug\t" + `{"k": "v", "z": "zz"}` + "\n" + "INFO\tzap/config_test.go:" + `\d+` + "\tinfo\t" + `{"k": "v", "z": "zz"}` + "\n" + "WARN\tzap/config_test.go:" + `\d+` + "\twarn\t" + `{"k": "v", "z": "zz"}` + "\n" + - `testing.\w+`, + `go.uber.org/zap.TestConfig.\w+`, }, } diff --git a/field.go b/field.go index d1672db7a..3c0d7d957 100644 --- a/field.go +++ b/field.go @@ -364,26 +364,17 @@ func Timep(key string, val *time.Time) Field { // expensive (relatively speaking); this function both makes an allocation and // takes about two microseconds. func Stack(key string) Field { - // Returning the stacktrace as a string costs an allocation, but saves us - // from expanding the zapcore.Field union struct to include a byte slice. Since - // taking a stacktrace is already so expensive (~10us), the extra allocation - // is okay. - return String(key, takeStacktrace(0)) + return StackSkip(key, 1) // skip Stack } // StackSkip constructs a field similarly to Stack, but also skips the given // number of frames from the top of the stacktrace. func StackSkip(key string, skip int) Field { - // Skip the call to StackSkip - if skip != 0 { - skip++ - } - // Returning the stacktrace as a string costs an allocation, but saves us // from expanding the zapcore.Field union struct to include a byte slice. Since // taking a stacktrace is already so expensive (~10us), the extra allocation // is okay. - return String(key, takeStacktrace(skip)) + return String(key, takeStacktrace(skip+1)) // skip StackSkip } // Duration constructs a field with the given key and value. The encoder diff --git a/field_test.go b/field_test.go index 71d485ad9..08c1bf45b 100644 --- a/field_test.go +++ b/field_test.go @@ -23,6 +23,7 @@ package zap import ( "math" "net" + "regexp" "sync" "testing" "time" @@ -259,15 +260,16 @@ func TestStackField(t *testing.T) { f := Stack("stacktrace") assert.Equal(t, "stacktrace", f.Key, "Unexpected field key.") assert.Equal(t, zapcore.StringType, f.Type, "Unexpected field type.") - assert.Equal(t, takeStacktrace(0), f.String, "Unexpected stack trace") + r := regexp.MustCompile(`field_test.go:(\d+)`) + assert.Equal(t, r.ReplaceAllString(takeStacktrace(0), "field_test.go"), r.ReplaceAllString(f.String, "field_test.go"), "Unexpected stack trace") assertCanBeReused(t, f) } func TestStackSkipField(t *testing.T) { - f := StackSkip("stacktrace", 0) + f := StackSkip("stacktrace", 1) assert.Equal(t, "stacktrace", f.Key, "Unexpected field key.") assert.Equal(t, zapcore.StringType, f.Type, "Unexpected field type.") - assert.Equal(t, takeStacktrace(0), f.String, "Unexpected stack trace") + assert.Equal(t, takeStacktrace(1), f.String, "Unexpected stack trace") assertCanBeReused(t, f) } diff --git a/logger.go b/logger.go index 0c8ac05e3..532968d22 100644 --- a/logger.go +++ b/logger.go @@ -48,8 +48,7 @@ type Logger struct { addCaller bool addStack zapcore.LevelEnabler - callerSkip int - useCallerSkipForStack bool + callerSkip int } // New constructs a new Logger from the provided zapcore.Core and Options. If @@ -305,11 +304,7 @@ func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry { } } if log.addStack.Enabled(ce.Entry.Level) { - if log.useCallerSkipForStack { - ce.Entry.Stack = StackSkip("", log.callerSkip+callerSkipOffset).String - } else { - ce.Entry.Stack = Stack("").String - } + ce.Entry.Stack = StackSkip("", log.callerSkip+callerSkipOffset).String } return ce diff --git a/options.go b/options.go index 074566a3d..c05b38117 100644 --- a/options.go +++ b/options.go @@ -119,14 +119,6 @@ func AddStacktrace(lvl zapcore.LevelEnabler) Option { }) } -// UseCallerSkipForStacktrace configures the logger to use the number of frames -// that should be skipped for caller (CallerSkip) annotation when taking stacktraces. -func UseCallerSkipForStacktrace(enabled bool) Option { - return optionFunc(func(log *Logger) { - log.useCallerSkipForStack = enabled - }) -} - // IncreaseLevel increase the level of the logger. It has no effect if // the passed in level tries to decrease the level of the logger. func IncreaseLevel(lvl zapcore.LevelEnabler) Option { diff --git a/stacktrace.go b/stacktrace.go index c3f4f55a3..d9d1db93a 100644 --- a/stacktrace.go +++ b/stacktrace.go @@ -22,7 +22,6 @@ package zap import ( "runtime" - "strings" "sync" "go.uber.org/zap/internal/bufferpool" @@ -62,8 +61,6 @@ func takeStacktrace(skip int) string { programCounters = newProgramCounters(len(programCounters.pcs) * 2) } - // Skip all consecutive zap frames at the beginning if no skip is set. - skipZapFrames := skip == 0 i := 0 frames := runtime.CallersFrames(programCounters.pcs[:numFrames]) @@ -71,12 +68,6 @@ func takeStacktrace(skip int) string { // frame, but we ignore this frame. The last frame is a a runtime frame which // adds noise, since it's only either runtime.main or runtime.goexit. for frame, more := frames.Next(); more; frame, more = frames.Next() { - if skipZapFrames && isZapFrame(frame.Function) { - continue - } else { - skipZapFrames = false - } - if skip > 0 { skip-- continue @@ -97,24 +88,6 @@ func takeStacktrace(skip int) string { return buffer.String() } -func isZapFrame(function string) bool { - for _, prefix := range _zapStacktracePrefixes { - if strings.HasPrefix(function, prefix) { - return true - } - } - - // We can't use a prefix match here since the location of the vendor - // directory affects the prefix. Instead we do a contains match. - for _, contains := range _zapStacktraceVendorContains { - if strings.Contains(function, contains) { - return true - } - } - - return false -} - type programCounters struct { pcs []uintptr } diff --git a/stacktrace_ext_test.go b/stacktrace_ext_test.go index 97c8a81b7..c37e2d65a 100644 --- a/stacktrace_ext_test.go +++ b/stacktrace_ext_test.go @@ -120,28 +120,27 @@ func TestStacktraceFiltersVendorZap(t *testing.T) { }) } -func TestStacktraceUseCallerSkipForStacktraceWithoutCallerSkip(t *testing.T) { +func TestStacktraceWithoutCallerSkip(t *testing.T) { withLogger(t, func(logger *zap.Logger, out *bytes.Buffer) { - logger = logger.WithOptions(zap.UseCallerSkipForStacktrace(true)) func() { logger.Error("test log") }() - require.Contains(t, out.String(), "TestStacktraceUseCallerSkipForStacktraceWithoutCallerSkip.", "Should not skip too much") - require.Contains(t, out.String(), "TestStacktraceUseCallerSkipForStacktraceWithoutCallerSkip", "Should not skip too much") + require.Contains(t, out.String(), "TestStacktraceWithoutCallerSkip.", "Should not skip too much") + require.Contains(t, out.String(), "TestStacktraceWithoutCallerSkip", "Should not skip too much") verifyNoZap(t, out.String()) }) } -func TestStacktraceUseCallerSkipForStacktraceWithCallerSkip(t *testing.T) { +func TestStacktraceWithCallerSkip(t *testing.T) { withLogger(t, func(logger *zap.Logger, out *bytes.Buffer) { - logger = logger.WithOptions(zap.UseCallerSkipForStacktrace(true), zap.AddCallerSkip(2)) + logger = logger.WithOptions(zap.AddCallerSkip(2)) func() { logger.Error("test log") }() - require.NotContains(t, out.String(), "TestStacktraceUseCallerSkipForStacktraceWithCallerSkip.", "Should skip as requested by caller skip") - require.Contains(t, out.String(), "TestStacktraceUseCallerSkipForStacktraceWithCallerSkip", "Should not skip too much") + require.NotContains(t, out.String(), "TestStacktraceWithCallerSkip.", "Should skip as requested by caller skip") + require.Contains(t, out.String(), "TestStacktraceWithCallerSkip", "Should not skip too much") verifyNoZap(t, out.String()) }) } diff --git a/stacktrace_test.go b/stacktrace_test.go index c67426119..6482082be 100644 --- a/stacktrace_test.go +++ b/stacktrace_test.go @@ -35,39 +35,11 @@ func TestTakeStacktrace(t *testing.T) { assert.Contains( t, lines[0], - "testing.", - "Expected stacktrace to start with the test runner (zap frames are filtered out) %s.", lines[0], + "go.uber.org/zap.TestTakeStacktrace", + "Expected stacktrace to start with the test %s.", lines[0], ) } -func TestIsZapFrame(t *testing.T) { - zapFrames := []string{ - "go.uber.org/zap.Stack", - "go.uber.org/zap.(*SugaredLogger).log", - "go.uber.org/zap/zapcore.(ArrayMarshalerFunc).MarshalLogArray", - "github.com/uber/tchannel-go/vendor/go.uber.org/zap.Stack", - "github.com/uber/tchannel-go/vendor/go.uber.org/zap.(*SugaredLogger).log", - "github.com/uber/tchannel-go/vendor/go.uber.org/zap/zapcore.(ArrayMarshalerFunc).MarshalLogArray", - } - nonZapFrames := []string{ - "github.com/uber/tchannel-go.NewChannel", - "go.uber.org/not-zap.New", - "go.uber.org/zapext.ctx", - "go.uber.org/zap_ext/ctx.New", - } - - t.Run("zap frames", func(t *testing.T) { - for _, f := range zapFrames { - require.True(t, isZapFrame(f), f) - } - }) - t.Run("non-zap frames", func(t *testing.T) { - for _, f := range nonZapFrames { - require.False(t, isZapFrame(f), f) - } - }) -} - func TestTakeStacktraceWithSkip(t *testing.T) { trace := takeStacktrace(1) lines := strings.Split(trace, "\n")