stacktrace: skip frames #525
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #525 +/- ##
==========================================
- Coverage 97.06% 97.03% -0.03%
==========================================
Files 39 39
Lines 2279 2262 -17
==========================================
- Hits 2212 2195 -17
Misses 59 59
Partials 8 8
Continue to review full report at Codecov.
|
|
Hi @prashantv, @akshayjshah. Any thoughts on this? Thanks! |
field.go
Outdated
| // stack frames via the skip argument. | ||
| // | ||
| // StackSkip("foo",0) is equivalent to Stack("foo"). | ||
| func StackSkip(key string, skip int) zapcore.Field { |
There was a problem hiding this comment.
I'm not sure if we should expose this right now -- I'd prefer to keep just Stack for now and think about whether we really need the skip version separately.
stacktrace.go
Outdated
| buffer := bufferpool.Get() | ||
| defer buffer.Free() | ||
| programCounters := _stacktracePool.Get().(*programCounters) | ||
| defer _stacktracePool.Put(programCounters) | ||
|
|
||
| var numFrames int | ||
| skip += 2 | ||
| for { | ||
| // Skip the call to runtime.Counters and takeStacktrace so that the |
There was a problem hiding this comment.
can you move this comment to where we do += 2
stacktrace_ext_test.go
Outdated
|
|
||
| assert.True(t, len(frames) > 6) | ||
|
|
||
| // The logging function should be the first frame ... |
There was a problem hiding this comment.
instead of testing partially, can we just test the entire result of frames?
There was a problem hiding this comment.
Sure, added. I was worried it'd be too brittle.
| @@ -63,19 +56,12 @@ func takeStacktrace() string { | |||
| } | |||
|
|
|||
| i := 0 | |||
| skipZapFrames := true // skip all consecutive zap frames at the beginning. | |||
There was a problem hiding this comment.
This behaviour was added on purpose, any reason why we're deleting this logic?
|
Thanks for the contribution @boz Can you provide more context about why we're removing the |
* Honor StackSkip option when including stacktrace. * Replace package-based stack pruning - the same behavior is achieved by skipping frames appropriately. fixes uber-go#512
|
Hi @prashantv As far as I can tell, accounting for frames correctly with |
|
Today, the offset for determining callers is separate from the stack-pruning behavior. That means that it's entirely possible to use a logger that never includes the calling function, but does occasionally log stacktraces. In that case, the caller skip likely wouldn't be configured, but stacktraces would still prune zap's internals. Merging this change would begin including zap's internals in the stacktraces, which would be unfortunate. Strictly speaking, that means that this change isn't backward-compatible (even though it does make the implementation simpler) :/ |
|
This has been open for a long time, and I'd prefer not to introduce even small backward-incompatible changes if they're not absolutely necessary. For housekeeping, I'm going to close this PR - @boz, happy to discuss further if you want to re-open it. |
skipping frames appropriately.
fixes #512