getsentry / sentry-go

The official Go SDK for Sentry (sentry.io)
https://docs.sentry.io/platforms/go/
MIT License
906 stars 211 forks source link

Default behavior of sentry-go silently swallows panics #506

Open claytoneast opened 1 year ago

claytoneast commented 1 year ago

Summary

The sentry-go http library by default recovers panics. The relevant library code is here, https://github.com/getsentry/sentry-go/blob/v0.13.0/http/sentryhttp.go#L107

It does this because it is an error reporting SDK, and thus it is assumed that the error has gone to Sentry and is visible there, but this is obviously untrue in environments where Sentry is not actually phoning home, such as test or development. Thus, for all intents and purposes, anything panic-y in non-production environments is silently swallowed.

I'm filing this as a bug because it seems counterproductive that the default behavior is "We will silently hide from you the most catastrophic kind of error you may encounter if you aren't in a production environment and checking sentry."

I realize that there is a Repanic option. I do not think Repanic should be false by default, since if you don't already know Sentry swallows the error, and that Repanic exists, you will spend hours trying to figure out what is happening in your program.

Steps To Reproduce

Our sentry setup is very basic.

// Sentry configures Sentry error logging then passes control to the next http.Handler
func Sentry(next http.Handler) http.Handler {
    sentryHandler := sentryhttp.New(sentryhttp.Options{}) // Yes, I realize that Repanic goes here

    return sentryHandler.Handle(next)
}

Expected Behavior

I would expect the worst possible kind of error in my Go programs to be visible to me, the person writing the program, when I'm developing the program, and encountering said worst possible kinds of errors.

Environment

SDK

Sentry

cleptric commented 1 year ago

Thanks for writing in.

I do agree that the UX is not great, especially in a local environment. Our other SDKs normally NOP if you do not supply a DSN, which is the case in most local envs. I'm wondering if our integrations should have the same behavior.

Changing the default will be a big breaking change, something I would rather do in 1.0.0.

claytoneast commented 1 year ago

I think that would be a great thing to do (noop w/o DSN) 👍

cleptric commented 1 year ago

So to confirm, you did not have a DSN set when encountering this locally?

claytoneast commented 1 year ago

Yep, DSN is unset in the program locally.