Nexus handler error translation #1626
Conversation
| // Temporal serviceerrors have a Status() method. | ||
| stGetter, ok := err.(interface{ Status() *status.Status }) | ||
| if !ok { | ||
| // Not a serviceerror, passthrough. |
There was a problem hiding this comment.
What if the error is wrapping a service error?
There was a problem hiding this comment.
I personally like only checking the outer one. I don't think we should treat errors the same just because they may have the error somewhere in their chain.
There was a problem hiding this comment.
Good call, switched to errors.As instead.
There was a problem hiding this comment.
There was a problem hiding this comment.
I was debating what to do here but I think most users will probably wrap just to provide more context but will expect to maintain the semantics.
There was a problem hiding this comment.
but internal is retryable right? Feels like it should be an
UnsuccessfulOperationError?
Yes, internal is retryable. What do you mean by "it"?
There was a problem hiding this comment.
"it" meaning the workflow error . If I have an operation handler that gets the result of a workflow and the workflow has failed wouldn't it make sense to not retry that?
There was a problem hiding this comment.
Decided to make it non retryable but I couldn't use UnsuccessfulOperationError because that's only usable in Start and GetResult. It's going to be a bad request for lack of better outcome.
There was a problem hiding this comment.
Also decided to not use errors.As and only consider errors directly returned by client methods.
If users wrap the errors, they should decide what error type they want.
Let's get user feedback on this and verify it matches their expectation before we remove the experimental label.
There was a problem hiding this comment.
Yeah I would really like to get feedback from users here if this is helpful and behaves how they expect.
| // Temporal serviceerrors have a Status() method. | ||
| stGetter, ok := err.(interface{ Status() *status.Status }) | ||
| if !ok { | ||
| // Not a serviceerror, passthrough. |
There was a problem hiding this comment.
I personally like only checking the outer one. I don't think we should treat errors the same just because they may have the error somewhere in their chain.
| // Roughly taken from https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto | ||
| // and | ||
| // https://github.com/grpc-ecosystem/grpc-gateway/blob/a7cf811e6ffabeaddcfb4ff65602c12671ff326e/runtime/errors.go#L56. | ||
| func convertServiceError(err error, exposeDetails bool) error { |
There was a problem hiding this comment.
Doesn't seem like there's much value in splitting convertServiceError and convertApplicationError, might as well just have convertError. Also doesn't seem like exposeDetails parameter has any value since it is never set to false on this private function. Can wait for the future need to hide details before adding that functionality, it has no value currently.
There was a problem hiding this comment.
Agree there's not much value, for the split. Initially there was more logic in the application error conversion since I thought we may want to handle other TemporalErrors but I can merge.
I already know that we'll want to allow hiding details and had this logic in the server codebase (where I ported this status error conversion logic from) so I figured I'd keep it in here.
bergundy
force-pushed
the
nexus-error-translation
branch
from
c1a64de to
4c04d9a
Compare
| // flag. | ||
| func convertApplicationError(err error) error { | ||
| var appErr *ApplicationError | ||
| if errors.As(err, &appErr) { |
There was a problem hiding this comment.
Normally the Go SDK explicitly does does not check the error chain for ApplicationError
There was a problem hiding this comment.
Same with the Server
There was a problem hiding this comment.
Hmm... so yeah, we should be consistent here too.
|
|
||
| // Default to internal error. This should only happen for codes.OK, which is unexpected for serviceerrors. | ||
| if !exposeDetails { | ||
| return nexus.HandlerErrorf(nexus.HandlerErrorTypeInternal, "internal error") |
There was a problem hiding this comment.
Dead code. Can you wait until you need it?
There was a problem hiding this comment.
I'd rather keep this because I know I will need it later when we add the ability to control the level of details exposed from handlers (and callers).
There was a problem hiding this comment.
I have no issue with the dead code, but It should be tested
bergundy
force-pushed
the
nexus-error-translation
branch
from
0d7e474 to
70903aa
Compare
|
|
||
| // convertKnownErrors converts known errors to corresponding Nexus HandlerError. | ||
| func convertKnownErrors(err error) error { | ||
| // Handle common errors returned from various client methods. |
There was a problem hiding this comment.
So will each SDK need to decided what is a common error and how to map it ?
* Nexus handler error translation * Address review comments
What was changed
serviceerrors,ApplicationErrors, and other expected client errors returned from Nexus handlers.Why?
Better out of the box developer experience.
Added integration tests.