From e48738f8985f3dbf15fda84da4bd1adda5d59919 Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Tue, 7 Mar 2017 10:02:58 -0800 Subject: [PATCH] Remove some now-unnecessary changes --- config_test.go | 24 ++++++++++------------ field.go | 4 ++-- field_test.go | 12 +++++------ logger_test.go | 30 +++++++++++++--------------- options.go | 3 +-- stacktrace.go | 14 +++++-------- stacktrace_test.go | 50 ++++++++++------------------------------------ 7 files changed, 48 insertions(+), 89 deletions(-) diff --git a/config_test.go b/config_test.go index 1fcb4e0d7..293f87151 100644 --- a/config_test.go +++ b/config_test.go @@ -68,21 +68,19 @@ func TestConfig(t *testing.T) { logger, err := tt.cfg.Build(Hooks(hook)) require.NoError(t, err, "Unexpected error constructing logger.") - withNoStacktraceIgnorePrefixes(func() { - logger.Debug("debug") - logger.Info("info") - logger.Warn("warn") + logger.Debug("debug") + logger.Info("info") + logger.Warn("warn") - byteContents, err := ioutil.ReadAll(temp) - require.NoError(t, err, "Couldn't read log contents from temp file.") - logs := string(byteContents) - assert.Regexp(t, tt.expectRe, logs, "Unexpected log output.") + byteContents, err := ioutil.ReadAll(temp) + require.NoError(t, err, "Couldn't read log contents from temp file.") + logs := string(byteContents) + assert.Regexp(t, tt.expectRe, logs, "Unexpected log output.") - for i := 0; i < 200; i++ { - logger.Info("sampling") - } - assert.Equal(t, tt.expectN, count.Load(), "Hook called an unexpected number of times.") - }) + for i := 0; i < 200; i++ { + logger.Info("sampling") + } + assert.Equal(t, tt.expectN, count.Load(), "Hook called an unexpected number of times.") }) } } diff --git a/field.go b/field.go index f04cb04de..905defe87 100644 --- a/field.go +++ b/field.go @@ -191,8 +191,8 @@ func NamedError(key string, err error) zapcore.Field { // Stack constructs a field that stores a stacktrace of the current goroutine // under provided key. Keep in mind that taking a stacktrace is eager and -// extremely expensive (relatively speaking); this function both makes an -// allocation and takes ~10 microseconds. +// expensive (relatively speaking); this function both makes an allocation and +// takes about two microseconds. func Stack(key string) zapcore.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 diff --git a/field_test.go b/field_test.go index 29567b868..7c1a1295f 100644 --- a/field_test.go +++ b/field_test.go @@ -164,11 +164,9 @@ func TestFieldConstructors(t *testing.T) { } func TestStackField(t *testing.T) { - withNoStacktraceIgnorePrefixes(func() { - f := Stack("stacktrace") - assert.Equal(t, "stacktrace", f.Key, "Unexpected field key.") - assert.Equal(t, zapcore.StringType, f.Type, "Unexpected field type.") - assert.Contains(t, f.String, "zap.TestStackField", "Expected stacktrace to contain caller.") - assertCanBeReused(t, f) - }) + f := Stack("stacktrace") + assert.Equal(t, "stacktrace", f.Key, "Unexpected field key.") + assert.Equal(t, zapcore.StringType, f.Type, "Unexpected field type.") + assert.Contains(t, f.String, "zap.TestStackField", "Expected stacktrace to contain caller.") + assertCanBeReused(t, f) } diff --git a/logger_test.go b/logger_test.go index 39ccc3858..1c220a2f5 100644 --- a/logger_test.go +++ b/logger_test.go @@ -360,24 +360,22 @@ func TestLoggerAddCallerFail(t *testing.T) { } func TestLoggerAddStacktrace(t *testing.T) { - withNoStacktraceIgnorePrefixes(func() { - assertHasStack := func(t testing.TB, obs observer.LoggedEntry) { - assert.Contains(t, obs.Entry.Stack, "zap.TestLoggerAddStacktrace", "Expected to find test function in stacktrace.") - } - withLogger(t, DebugLevel, opts(AddStacktrace(InfoLevel)), func(logger *Logger, logs *observer.ObservedLogs) { - logger.Debug("") - assert.Empty( - t, - logs.AllUntimed()[0].Entry.Stack, - "Unexpected stacktrack at DebugLevel.", - ) + assertHasStack := func(t testing.TB, obs observer.LoggedEntry) { + assert.Contains(t, obs.Entry.Stack, "zap.TestLoggerAddStacktrace", "Expected to find test function in stacktrace.") + } + withLogger(t, DebugLevel, opts(AddStacktrace(InfoLevel)), func(logger *Logger, logs *observer.ObservedLogs) { + logger.Debug("") + assert.Empty( + t, + logs.AllUntimed()[0].Entry.Stack, + "Unexpected stacktrack at DebugLevel.", + ) - logger.Info("") - assertHasStack(t, logs.AllUntimed()[1]) + logger.Info("") + assertHasStack(t, logs.AllUntimed()[1]) - logger.Warn("") - assertHasStack(t, logs.AllUntimed()[2]) - }) + logger.Warn("") + assertHasStack(t, logs.AllUntimed()[2]) }) } diff --git a/options.go b/options.go index 405a1b34c..29fdbf84e 100644 --- a/options.go +++ b/options.go @@ -92,8 +92,7 @@ func AddCallerSkip(skip int) Option { } // AddStacktrace configures the Logger to record a stack trace for all messages at -// or above a given level. Keep in mind that taking a stacktrace takes several -// microseconds; relative to the cost of logging, this is quite slow. +// or above a given level. func AddStacktrace(lvl zapcore.LevelEnabler) Option { return optionFunc(func(log *Logger) { log.addStack = lvl diff --git a/stacktrace.go b/stacktrace.go index 3b9552cd1..2726d84c7 100644 --- a/stacktrace.go +++ b/stacktrace.go @@ -40,9 +40,6 @@ var ( } ) -// takeStacktrace attempts to use the provided byte slice to take a stacktrace. -// If the provided slice isn't large enough, takeStacktrace will allocate -// successively larger slices until it can capture the whole stack. func takeStacktrace() string { buffer := bufferpool.Get() defer bufferpool.Put(buffer) @@ -50,16 +47,15 @@ func takeStacktrace() string { defer _stacktracePool.Put(programCounters) for { - // skipping 2 skips the call to runtime.Counters and takeStacktrace - // so that the program counters start at the caller of takeStacktrace + // Skip the call to runtime.Counters and takeStacktrace so that the + // program counters start at the caller of takeStacktrace. n := runtime.Callers(2, programCounters.pcs) - if n < len(programCounters.pcs) { + if n < cap(programCounters.pcs) { programCounters.pcs = programCounters.pcs[:n] break } - // Do not put programCounters back in pool, will put larger buffer in - // This is in case our size is always wrong, we optimize to pul - // correctly-sized buffers back in the pool + // Don't put the too-short counter slice back into the pool; this lets + // the pool adjust if we consistently take deep stacktraces. programCounters = newProgramCounters(len(programCounters.pcs) * 2) } diff --git a/stacktrace_test.go b/stacktrace_test.go index 38b03de93..1d5357637 100644 --- a/stacktrace_test.go +++ b/stacktrace_test.go @@ -22,50 +22,20 @@ package zap import ( "strings" - "sync" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -var ( - _stacktraceTestLock sync.Mutex -) - -func TestEmptyStacktrace(t *testing.T) { - withStacktraceIgnorePrefixes( - []string{ - "go.uber.org/zap", - "runtime.", - "testing.", - }, - func() { - assert.Empty(t, takeStacktrace(), "stacktrace should be empty if we ignore runtime, testing, and functions in this package") - }, - ) -} - -func TestAllStacktrace(t *testing.T) { - withNoStacktraceIgnorePrefixes( - func() { - stacktrace := takeStacktrace() - assert.True(t, strings.HasPrefix(stacktrace, "go.uber.org/zap.TestAllStacktrace"), - "stacktrace should start with TestAllStacktrace if no prefixes set, but is %s", stacktrace) - }, +func TestTakeStacktrace(t *testing.T) { + trace := takeStacktrace() + lines := strings.Split(trace, "\n") + require.True(t, len(lines) > 0, "Expected stacktrace to have at least one frame.") + assert.Contains( + t, + lines[0], + "TestTakeStacktrace", + "Expected stacktrace to start with this test function, but top frame is %s.", lines[0], ) } - -func withNoStacktraceIgnorePrefixes(f func()) { - withStacktraceIgnorePrefixes([]string{}, f) -} - -func withStacktraceIgnorePrefixes(prefixes []string, f func()) { - _stacktraceTestLock.Lock() - defer _stacktraceTestLock.Unlock() - existing := _stacktraceIgnorePrefixes - _stacktraceIgnorePrefixes = prefixes - defer func() { - _stacktraceIgnorePrefixes = existing - }() - f() -}