Add stack trace to error handler's HandlePanicFunc
#423
Conversation
This file contains 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
3a5c57b
to
fabdde1
Compare
bgentry
approved these changes
Jul 4, 2024
CHANGELOG.md
Outdated
| @@ -11,6 +11,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| - `Config.TestOnly` has been added. It disables various features in the River client like staggered maintenance service start that are useful in production, but may be somewhat harmful in tests because they make start/stop slower. [PR #414](https://github.com/riverqueue/river/pull/414). | |||
|
|
|||
| ### 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. | |||
job_executor.go
Outdated
| @@ -234,7 +234,7 @@ func (e *jobExecutor) invokeErrorHandler(ctx context.Context, res *jobExecutorRe | |||
|
|
|||
| 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)) | |||
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?
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.
fabdde1
to
aeb3b97
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.