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
Honor CallerSkip when taking stack traces & add the StackSkip field #843
Changes from 7 commits
017057e
8e25507
c27b7e4
3410fce
33c1b16
80d2017
066ee2d
fc5ed1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -120,6 +120,31 @@ func TestStacktraceFiltersVendorZap(t *testing.T) { | |||||||
}) | ||||||||
} | ||||||||
|
||||||||
func TestStacktraceWithoutCallerSkip(t *testing.T) { | ||||||||
withLogger(t, func(logger *zap.Logger, out *bytes.Buffer) { | ||||||||
func() { | ||||||||
logger.Error("test log") | ||||||||
}() | ||||||||
|
||||||||
require.Contains(t, out.String(), "TestStacktraceWithoutCallerSkip.", "Should not skip too much") | ||||||||
require.Contains(t, out.String(), "TestStacktraceWithoutCallerSkip", "Should not skip too much") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we pass the first check, we'll always also pass the second check. We can probably just drop the second.
Suggested change
|
||||||||
verifyNoZap(t, out.String()) | ||||||||
}) | ||||||||
} | ||||||||
|
||||||||
func TestStacktraceWithCallerSkip(t *testing.T) { | ||||||||
withLogger(t, func(logger *zap.Logger, out *bytes.Buffer) { | ||||||||
logger = logger.WithOptions(zap.AddCallerSkip(2)) | ||||||||
func() { | ||||||||
logger.Error("test log") | ||||||||
}() | ||||||||
|
||||||||
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()) | ||||||||
}) | ||||||||
} | ||||||||
|
||||||||
// withLogger sets up a logger with a real encoder set up, so that any marshal functions are called. | ||||||||
// The inbuilt observer does not call Marshal for objects/arrays, which we need for some tests. | ||||||||
func withLogger(t *testing.T, fn func(logger *zap.Logger, out *bytes.Buffer)) { | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -29,47 +29,46 @@ import ( | |||||
) | ||||||
|
||||||
func TestTakeStacktrace(t *testing.T) { | ||||||
trace := takeStacktrace() | ||||||
trace := takeStacktrace(0) | ||||||
lines := strings.Split(trace, "\n") | ||||||
require.True(t, len(lines) > 0, "Expected stacktrace to have at least one frame.") | ||||||
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", | ||||||
} | ||||||
func TestTakeStacktraceWithSkip(t *testing.T) { | ||||||
trace := takeStacktrace(1) | ||||||
lines := strings.Split(trace, "\n") | ||||||
require.True(t, len(lines) > 0, "Expected stacktrace to have at least one frame.") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: Can use NotEmpty.
Suggested change
|
||||||
assert.Contains( | ||||||
t, | ||||||
lines[0], | ||||||
"testing.", | ||||||
"Expected stacktrace to start with the test runner (skipping our own frame) %s.", lines[0], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: Testify will include the
Suggested change
|
||||||
) | ||||||
} | ||||||
|
||||||
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 TestTakeStacktraceWithSkipInnerFunc(t *testing.T) { | ||||||
var trace string | ||||||
func() { | ||||||
trace = takeStacktrace(2) | ||||||
}() | ||||||
lines := strings.Split(trace, "\n") | ||||||
require.True(t, len(lines) > 0, "Expected stacktrace to have at least one frame.") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
assert.Contains( | ||||||
t, | ||||||
lines[0], | ||||||
"testing.", | ||||||
"Expected stacktrace to start with the test function (skipping the test function) %s.", lines[0], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
) | ||||||
} | ||||||
|
||||||
func BenchmarkTakeStacktrace(b *testing.B) { | ||||||
for i := 0; i < b.N; i++ { | ||||||
takeStacktrace() | ||||||
takeStacktrace(0) | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't appear to be any difference between this and TestStackSkipField. Did you mean to use zero for TestStackSkipField?