[DRAFT] Improve error modeling and serialization#65
Conversation
| // chain. | ||
| // [Failure] instances are converted to [FailureError] to allow access to the full failure metadata and details if | ||
| // available. | ||
| func DefaultFailureConverter() FailureConverter { |
There was a problem hiding this comment.
Does it make sense to allow this to be composed with custom failure converters?
| // Error message. | ||
| Message string | ||
| // Stack trace which may be set if this error was generated by a language that supports it. | ||
| StackTrace string |
There was a problem hiding this comment.
Stack traces can be quite long, do we need to worry about truncation?
There was a problem hiding this comment.
I don't think we should worry about that at this level. If that comes up as an issue, that would have to be handled at the transport level.
| State: OperationStateCanceled, | ||
| Cause: fmt.Errorf(format, args...), | ||
| State: OperationStateCanceled, | ||
| Message: fmt.Sprintf(format, args...), |
There was a problem hiding this comment.
Switching from fmt.Errorf to fmt.Sprintf is losing some of the built-in unwrapping that errors.As and related methods do.
| Cause: err, | ||
| } | ||
| // Set if this error is constructed from a failure object. | ||
| OriginalFailure *Failure |
There was a problem hiding this comment.
Do we have any constructor helpers for setting this? I don't see it getting set anywhere but maybe I missed it.
| } | ||
| } | ||
|
|
||
| func (e *HandlerError) retryBehaviorAsOptionalBool() *bool { |
There was a problem hiding this comment.
Why optional? I would default to non-retryable to make it simpler.
There was a problem hiding this comment.
That's in line with what we did in other SDKs. That is, we want a way to know if the retry behavior was explicitly set, rather than only get the effective behavior after default.
| // OperationState represents the variable states of an operation. | ||
| type OperationState string | ||
|
|
||
| // FIXME(JWH): Should we remove running and succeeded? They are no longer pertinent as public APIs. |
There was a problem hiding this comment.
AFAICT we don't need running. Succeeded might still be useful for completion calls that want to differentiate from failed.
| Cause *Failure `json:"cause,omitempty"` | ||
| } | ||
|
|
||
| // FIXME(JWH): Do we still want the FailureError type? |
There was a problem hiding this comment.
I think it's still useful to be able to represent any arbitrary error as a failure, as a fallback/default. But no strong opinion.
- 💥 **BREAKING CHANGE** 💥 Removes the experimental `FailureConverter` interface. - Enhances `HandlerError`, `OperationError`, and `FailureError` to support separate `Cause` and `Message` fields as well as `StackTrace`. - Deprecates `HandlerErrorf` in favor of `NewHandlerErrorf`. - Deprecates `OperationErrorf`, `OperationCanceledErrorf`, `OperationFailedErrorf`, `NewOperationFailedError`, `NewOperationCanceledError` in favor of `NewOperationErrorf`, `NewOperationFailedErrorf` and `NewOperationCanceledErrorf`. Revives the work from #56 and #65 --------- Co-authored-by: Roey Berman <roey.berman@gmail.com>
3 participants
Description
This is rough attempt at restoring @bergundy work on improving the
HandlerError,OperationErrorandFailuretypes to support carrying both a message and a cause at the same time.