Add stack trace to error handler's HandlePanicFunc - #423
Merged
Conversation
brandur
force-pushed
the
brandur-panic-trace
branch
from
July 4, 2024 05:40
3a5c57b to
fabdde1
Compare
bgentry
approved these changes
Jul 4, 2024
bgentry
left a comment
bgentry
left a comment
Contributor
There was a problem hiding this comment.
LGTM aside from comments. Thanks for jumping on this!
|
|
||
| ### Changed | ||
|
|
||
| ⚠️ Version 0.8.0 has a small breaking change in `ErrorHandler`. As before, we try never to make breaking changes, but this one was deemed quite important because `ErrorHandler` was fundamentally lacking important functionality. |
| case res.PanicVal != nil: | ||
| errorHandlerRes = invokeAndHandlePanic("HandlePanic", func() *ErrorHandlerResult { | ||
| return e.ErrorHandler.HandlePanic(ctx, e.JobRow, res.PanicVal) | ||
| return e.ErrorHandler.HandlePanic(ctx, e.JobRow, res.PanicVal, string(res.PanicTrace)) |
Contributor
There was a problem hiding this comment.
Should we convert this to string once when saving it to the result so that it doesn’t have to be cast here and by pgx when writing to the db?
Contributor
Author
There was a problem hiding this comment.
Yep, makes sense. Fixed.
This one in response to #418 in which although we persist a stack trace to a job row's errors property, we don't reveal it in a panic handler, which is quite inconvenient for purposes of logging or other telemetry (e.g. sending to Sentry). Here, `HandlePanic`'s signature changes to (`trace` is added): HandlePanic(ctx context.Context, job *rivertype.JobRow, panicVal any, trace string) *ErrorHandlerResult A couple notes on choices: * The naming of `trace` is reused from `AttemptError`. * The value type is `string`. This is a little non-obvious because Go exposes it as a `[]byte`, something I've never quite understood as to why, but I did `string` because for one it's more convenient to use, but more importantly, it's the same type on `AttemptError`. This is a breaking change, but it seems like being able to get a stack trace during panic is important enough that it's worth it, and with any luck there's few enough people using this feature that it won't break that many people. The fix is quite easy regardless and will easily be caught by the compiler. Fixes #418.
brandur
force-pushed
the
brandur-panic-trace
branch
from
July 4, 2024 14:26
fabdde1 to
aeb3b97
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This one in response to #418 in which although we persist a stack trace
to a job row's errors property, we don't reveal it in a panic handler,
which is quite inconvenient for purposes of logging or other telemetry
(e.g. sending to Sentry).
Here,
HandlePanic's signature changes to (traceis added):A couple notes on choices:
The naming of
traceis reused fromAttemptError.The value type is
string. This is a little non-obvious because Goexposes it as a
[]byte, something I've never quite understood as towhy, but I did
stringbecause for one it's more convenient to use,but more importantly, it's the same type on
AttemptError.This is a breaking change, but it seems like being able to get a stack
trace during panic is important enough that it's worth it, and with any
luck there's few enough people using this feature that it won't break
that many people. The fix is quite easy regardless and will easily be
caught by the compiler.
Fixes #418.