Start showing inlined functions in stack trace #208
Conversation
DariaKunoichi
force-pushed
the
daria-PLAT-7225
branch
from
f92d042
to
1e35788
Compare
DariaKunoichi
force-pushed
the
daria-PLAT-7225
branch
from
7069695
to
a41982d
Compare
| @@ -64,7 +65,7 @@ func New(e interface{}, skip int) *Error { | |||
| trace := e.StackTrace() | |||
| stack := make([]uintptr, len(trace)) | |||
| for i, ptr := range trace { | |||
| stack[i] = uintptr(ptr) - 1 | |||
There was a problem hiding this comment.
Can you include some justification on the PR description for this change? It seems fairly fundamental to all our stacktrace reporting, so I'm surprised it's incorrect.
There was a problem hiding this comment.
It is actually moving the whole stack to ignore the newest item. With the -1 added the stack for example looks like:
PTR: main.A
STACK[0]: main.B
PTR: main.B
STACK[1]: main.C
PTR: main.C
STACK[2]: main.main
PTR: main.main
STACK[3]: main.main
PTR: runtime.main
STACK[4]: runtime.main
PTR: runtime.goexit
STACK[5]: runtime.goexit
after changing to not modify the uintptr address:
PTR: main.A
STACK[0]: main.A
PTR: main.B
STACK[1]: main.B
PTR: main.C
STACK[2]: main.C
PTR: main.main
STACK[3]: main.main
PTR: runtime.main
STACK[4]: runtime.main
PTR: runtime.goexit
STACK[5]: runtime.goexit
| @@ -187,6 +249,7 @@ func assertStackframesMatch(t *testing.T, expected []errors.StackFrame) { | |||
| if strings.HasSuffix(file, expectedFrame.File) && expectedFrame.Name == method { | |||
| lastmatch = index | |||
| matched++ | |||
| break | |||
There was a problem hiding this comment.
Can you also provide rationale for this change in the PR description? Why did it pass before and not now?
There was a problem hiding this comment.
It passed before because the stack was:
- func.1
- func
and each function got matched only once somatchedcount was correct.
When I added expansion of inlines the stack became:
- func.1 (for go1.11 and crash for everything else)
- func.1
- func
that caused incorrect matching of actual and expected stacks - each actual stackframe could be matched many times - we did not stop comparing after finding first match.
Matched count exceeded expected stack length and panic happened on the next log. (out of bounds idx)
Notice that error_test.go has the break in the same place while matching stacks.
## 2.3.0 (2024-03-05) ### Bug fixes * Start showing inlined functions in stack trace [#208](#208) * Handle complex structs in metadata [#215](#215) [Chris Duncan](https://github.com/veqryn) * Stop trimming everything before "main.go" on main packages [#217](#217) [Chris Duncan](https://github.com/veqryn)
|
Great Job! Thanks |
Goal
Customers requested for inline functions to be visible in the stacktrace in bugsnag UI.
Issue: #183
Design
In golang it's recommended to process stacktraces using runtime.CallersFrames.
But unfortunately for inlined functions the frame.Func field is going to be nil.
In this situation we can extract inlined function data from underlying low level data using FuncForPC.
Changeset
For inline functions extract stack data from PC.
Processing frames from stack now does not exclude newest item on this line.
Tests for notifier do not match one expected line multiple times on this line
For the example in linked issue the Bugsnag UI reported this:
and after this change it now looks like this:
Testing
Changed unit tests that required us to ignore inlined functions - they are now not ignored.