MindscapeHQ/raygun4go

Incomplete stacktrace when using go-errors

IcaroG opened this issue · 8 comments

I just started using this lib alongside the lib go-errors (supported as mentioned here) and noticed that the stacktrace when using the error type from go-errors lib is missing the first line (where the error is being created). It seems that the Parse method is not correct when using go-errors.Stack() as parameter.

Hi IcaroG

Only had time to have a quick look for now and wondering if it's due to this line here:

for index, line := range lines[1:] {

I believe this would be done because the first line of a stack trace string can be something like "goroutine 5 [running]:".

If this can be confirmed to be the issue, a fix could be to provide an overload for the Parse function to take in a number of lines to skip, or a boolean to signal that the first line should be skipped. Alternatively the Parse method could be changed to detect whether or not the first line should be skipped. Just throwing some initial ideas around.

I'm not sure when we'd be able to look into this further, but we do accept pull requests which could help speed this up. First step is to see if that line I linked is related or if it's caused by something else.

Yes, this line is indeed the cause. I did some testing locally and what I found is that the stacktrace provided by the go-errors lib is different from what the Parse method expects.
This is how the stack looks like when calling Current:

github.com/MindscapeHQ/raygun4go.Current(0x7211e0, 0xc00000e240)
/home/icaro.guerra/go/src/github.com/MindscapeHQ/raygun4go/stack2struct.go:36 +0x6d
main.loco(0xc00005df48)
/home/icaro.guerra/test.go:12 +0x89
main.main()
/home/icaro.guerra/test.go:19 +0x26

and this is what the stack looks like when using go-errors.New():

/home/icaro.guerra/test.go:14 (0x63e840)
loco: err := goerrors.New("this is an error")
/home/icaro.guerra/test.go:19 (0x63e8a6)
main: err := loco()
/home/linuxbrew/.linuxbrew/Cellar/go/1.14.1/libexec/src/runtime/proc.go:203 (0x436592)
main: fn()
/home/linuxbrew/.linuxbrew/Cellar/go/1.14.1/libexec/src/runtime/asm_amd64.s:1373 (0x462ae1)
goexit: BYTE	$0x90	// NOP

This causes the packageName/methodName parse to not work as intended. So, something like a ParseGoError() method needs to be created. I could create the PR too to fix this.

Thanks for testing this out and clarifying the cause of the issue - and for proving the stacktrace examples. A ParseGoError method to handle this sounds good to me. Do go ahead with creating a PR if you like and we'll take a look when ready.

How would you like the packageName missing from the go-error lib stacktrace to be handled?

Any time a package name cannot be resolved, I think it would be fine to use nil instead, or if this causes issues, use an empty string. Does that make sense to do?

That seems good. It is also possible implement a method ToString(frames []goerrors.StackFrame) to format the stacktrace as we want and use the same Parse method. Which one do you prefer? Something like this https://play.golang.org/p/byDBVWOMfS4

I'd avoid needing to create a ToString method. That's got me thinking though - Raygun is using the go-errors Stack function to get the stack string which is then parsed. Perhaps the StackFrames function could be used instead (like in your playground example) to get the function name and line number and so on to be passed into the AddEntry function that's used to build up the Raygun specific stack frame model:

func (s *StackTrace) AddEntry(lineNumber int, packageName, fileName, methodName string) {

This avoids any string creation + parsing involved when dealing with go-errors.

Thanks again for the PR, I'm closing this issue now 👍