Stack trace has confusing top frame when using sentry.CurrentHub().Recover(err)

[eric]

Summary

When using sentry.CurrentHub().Recover(err) to report an exception the stack trace contains that stack frame at the top.

Steps To Reproduce

Create recover block:

		defer func() {
			if err := recover(); err != nil {
				log.Printf("[ERR] Error: %v", err)
				sentry.CurrentHub().Recover(err)
			}
		}()

Have panic after that in a nested function.

Expected Behavior

I expect the top stack frame to be the frame that caused the panic. Instead, it is the stack frame that has the recover. This makes some logical sense, because it is the thing that is calling sentry.CurrentHub().Recover(err) but it is unintuitive.

Environment

SDK

• sentry-go version: 0.25.0
• Go version: go1.20.10
• Using Go Modules? yes

Sentry

• Using hosted Sentry in sentry.io? yes

[cleptric]

I wasn't able to reproduce this.

[eric]

Here is an example exception: https://fancy-bits-llc.sentry.io/issues/3485598464/

What does the recover handler look like in your example?

[cleptric]

https://github.com/getsentry/sentry-go/blob/master/gin/sentrygin.go#L94

The issue you linked uses Go SDK 0.13.0 btw.

[eric]

Ah, you are correct. Here is one from the 0.25.0 SDK: https://fancy-bits-llc.sentry.io/issues/4575378790/

[cleptric]

So to confirm, the panic stems from reflect/value.go in Value.Field at line 1272 in your linked issue?

[eric]

Yes. Line 341 of processor.go is:

				sentry.CurrentHub().Recover(err)

from the above example recover block.

[eric]

Your repro doesn't properly reproduce the issue because you are relying on the recover() that is within the sentry-go package:

sentry-go/stacktrace.go

Lines 323 to 327 in 8f8897d

	// Skip Sentry internal frames, except for frames in _test packages (for testing).
	if strings.HasPrefix(module, "github.com/getsentry/sentry-go") &&
	!strings.HasSuffix(module, "_test") {
	return true
	}

If you try to reproduce this in a separate repo that contains its own recover() call like the one in my original post, I expect you will be able to reproduce this easily.

[cleptric]

So I did

package main

import (
	"log"
	"time"

	"github.com/getsentry/sentry-go"
)

func main() {

	sentry.Init(sentry.ClientOptions{
		Dsn:              "<DSN>",
		Debug:            true,
		AttachStacktrace: true,
	})

	doStuff()

	defer sentry.Flush(2 * time.Second)
}

func doStuff() {
	defer func() {
		if err := recover(); err != nil {
			log.Printf("[ERR] Error: %v", err)
			sentry.CurrentHub().Recover(err)
		}
	}()

	doMoreStuff()
}

func doMoreStuff() {
	panic("oh no")
}

and got https://sentry-sdks.sentry.io/share/issue/fe6200a9013b4c65802251ee877b5d97/.

Hmmm, we could do some trickery if the last frame is something close to sentry.CurrentHub().Recover(err) and its various counterparts, but this feels a tad odd 😬

[eric]

I believe it's common in implementations to provide an argument for the number of stack frames to suppress instead of just blanket ignoring all frames from a given package. As the comment in shouldSkipFrame() said, the current implementation could hide other stack frames from inside sentry-go that caused a problem.

[eric]

Related: #649

[eric]

Related: #458