Sentry Integration - Capture Manual panics with Sentry Exception and runtime panics with a wrapper on panic #4756
Conversation
…wrapper for everytheing else. env = ee/oss; scope = subcmd
x/histogram.go
Outdated
| @@ -41,7 +42,7 @@ type slidingHistogram struct { | |||
| // details. | |||
| func newSlidingHistogram(duration time.Duration, maxVal int64, sigFigs int) *slidingHistogram { | |||
| if duration <= 0 { | |||
| panic("cannot create a sliding histogram with nonpositive duration") | |||
| PanicWithSentryException(errors.New("cannot create a sliding histogram with nonpositive duration")) | |||
x/sentry_integration.go
Outdated
| os.Exit(1) | ||
| } | ||
|
|
||
| // WrapPanics is a wrapper on panics. We use it to send sentry events about panics and crash right after. |
graphql/e2e/common/common.go
Outdated
| @@ -139,41 +138,41 @@ type state struct { | |||
| func BootstrapServer(schema, data []byte) { | |||
| err := checkGraphQLLayerStarted(graphqlAdminURL) | |||
| if err != nil { | |||
| panic(fmt.Sprintf("Waited for GraphQL test server to become available, but it never did.\n"+ | |||
| x.PanicWithSentryException(errors.Errorf("Waited for GraphQL test server to become available, but it never did.\n"+ | |||
graphql/e2e/common/common.go
Outdated
| "Got last error %+v", err.Error())) | ||
| } | ||
|
|
||
| err = checkGraphQLLayerStarted(graphqlAdminTestAdminURL) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("Waited for GraphQL AdminTest server to become available, "+ | ||
| x.PanicWithSentryException(errors.Errorf("Waited for GraphQL AdminTest server to become available, "+ |
x/sentry_integration.go
Outdated
| os.Exit(1) | ||
| } | ||
|
|
||
| // WrapPanics is a wrapper on panics. We use it to send sentry events about panics |
There was a problem hiding this comment.
File is not gofmt-ed with -s (from gofmt)
| // WrapPanics is a wrapper on panics. We use it to send sentry events about panics | |
| // WrapPanics is a wrapper on panics. We use it to send sentry events about panics |
graphql/e2e/common/common.go
Outdated
| "Got last error %+v", err.Error())) | ||
| } | ||
|
|
||
| err = checkGraphQLLayerStarted(graphqlAdminTestAdminURL) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("Waited for GraphQL AdminTest server to become available, "+ | ||
| x.PanicWithSentryException(errors.Errorf( | ||
| "Waited for GraphQL AdminTest server to become available, " + |
There was a problem hiding this comment.
File is not gofmt-ed with -s (from gofmt)
parasssh
changed the title
There was a problem hiding this comment.
Reviewed 7 of 20 files at r1, 1 of 4 files at r2, 4 of 9 files at r3, 8 of 8 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)
dgraph/cmd/alpha/run.go, line 596 at r4 (raw file):
// Simulate exception, panic.... // x.CaptureSentryException(errors.New("alpha exception")) // x.PanicWithSentryException(errors.New("alpha manual panic will send 2 events"))
Make it clearer that the commented out code is example code. For example, change the first line. "Simulate an exception or a panic as shown below"
dgraph/cmd/zero/run.go, line 203 at r4 (raw file):
// x.CaptureSentryException(errors.New("zero exception")) // x.PanicWithSentryException(errors.New("zero manual panic will send 2 events"))
why is this commented out?
x/sentry_integration.go, line 31 at r4 (raw file):
var env string // InitSentry intiializes the sentry machinery
minor: end comments with periods. Also in the other comments in this file.
x/sentry_integration.go, line 88 at r4 (raw file):
Dont
Don't
There was a problem hiding this comment.
Dismissed @martinmr from 4 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @danielmai, @golangcibot, @manishrjain, and @parasssh)
dgraph/cmd/alpha/run.go, line 596 at r4 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
// Simulate exception, panic.... // x.CaptureSentryException(errors.New("alpha exception")) // x.PanicWithSentryException(errors.New("alpha manual panic will send 2 events"))Make it clearer that the commented out code is example code. For example, change the first line. "Simulate an exception or a panic as shown below"
Done.
dgraph/cmd/zero/run.go, line 203 at r4 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
// x.CaptureSentryException(errors.New("zero exception")) // x.PanicWithSentryException(errors.New("zero manual panic will send 2 events"))why is this commented out?
This is also an example to simulate a sentry exception or panic. I will add a comment to make this clear.
x/sentry_integration.go, line 93 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 105 characters (from
lll)
Done.
x/sentry_integration.go, line 93 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
File is not
gofmt-ed with-s(fromgofmt)// WrapPanics is a wrapper on panics. We use it to send sentry events about panics
Done.
There was a problem hiding this comment.
Reviewable status: 17 of 20 files reviewed, 6 unresolved discussions (waiting on @danielmai, @golangcibot, @manishrjain, @martinmr, and @parasssh)
graphql/e2e/common/common.go, line 141 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 117 characters (from
lll)
Done.
graphql/e2e/common/common.go, line 147 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 103 characters (from
lll)
Done.
graphql/e2e/common/common.go, line 148 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
File is not
gofmt-ed with-s(fromgofmt)
Done.
There was a problem hiding this comment.
Dismissed @golangcibot from 6 discussions.
Reviewable status: 17 of 20 files reviewed, all discussions resolved (waiting on @danielmai, @manishrjain, and @martinmr)
There was a problem hiding this comment.
Looks like it only catches when we panic ourselves. What if our code panics because of say slice out of boundary? Is that caught by this change?
Reviewable status: 17 of 20 files reviewed, 1 unresolved discussion (waiting on @danielmai, @martinmr, and @parasssh)
graphql/e2e/common/common.go, line 154 at r5 (raw file):
err := checkGraphQLStarted(graphqlAdminURL) if err != nil { x.PanicWithSentryException(errors.Errorf(
can just be called x.Panic.
There was a problem hiding this comment.
It captures those as well. See x.WrapPanics() method which is called once at alpha/zero run() that sets a custom handler on panic. Thereafter whenever there is a panic, the custom handler is called.
The one big downside with this approach is that now we will have 2x the processes (alphas, zeros) since this wrapper basically spawns a monitoring process to capture any panics and calls the handler to send the Sentry event.
Dismissed @manishrjain from a discussion.
Reviewable status: 17 of 20 files reviewed, all discussions resolved (waiting on @danielmai and @martinmr)
graphql/e2e/common/common.go, line 154 at r5 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
can just be called x.Panic.
Done.
There was a problem hiding this comment.
I think x.Checks could also be switched over to this.
Reviewed 15 of 22 files at r6, 7 of 7 files at r7.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @danielmai)
|
|
||
| func initSentry() { | ||
| if err := sentry.Init(sentry.ClientOptions{ | ||
| Dsn: "https://58a035f0d85a4c1c80aee0a3e72f3899@sentry.io/1805390", |
There was a problem hiding this comment.
I'm confused. Why isn't there a way to change Sentry's DSN so I can get notified when a panic occurs?
Or is this feature meant only for developers (Dgraph Labs)?
This patch is a basic integration with Sentry.
All unit tests pass.
This change is
Docs Preview: