Add support for custom failure converters #924

Quinn-With-Two-Ns · 2022-09-29T20:47:53Z

Adds an interface called FailureConverter that allows users to customize failure serialization and deserialization.

What was changed

Adds a FailureConverter interface to allow users to customize how errors are serialized and deserialized.

See also:
temporalio/sdk-typescript#887
https://github.com/temporalio/api/blob/master/temporal/api/failure/v1/message.proto#L81

Checklist

Closes
How was this tested:

Any docs updates needed?

Adds an interface called `FailureConverter` that allows users to customize failure serialization and deserialization.

CLAassistant · 2022-09-29T20:47:58Z

All committers have signed the CLA.

CLAassistant · 2022-09-29T20:47:58Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

bergundy

I didn't look at the implementation, leaving that to @cretz.
The API LGTM.

cretz · 2022-09-30T12:29:34Z

converter/failure_converter.go

+import failurepb "go.temporal.io/api/failure/v1"
+
+type (
+	FailureConverter interface {


All exported types need godoc.

Also, don't have to put this in a type ( block if you'd rather just type FailureConverter interface {. Consistency for this isn't that important.

cretz · 2022-09-30T12:33:08Z

internal/client.go

+		// default: defaultFailureConverter, does not encode any fields of the error. See `temporal/default_failure_converter` for
+		// options to configure or create a custom converter.


Suggested change

// default: defaultFailureConverter, does not encode any fields of the error. See `temporal/default_failure_converter` for

// options to configure or create a custom converter.

// default: temporal.DefaultFailureConverter, does not encode any fields of the error. Use temporal.NewDefaultFailureConverter

// to configure or create a custom converter.

Reference the type name

Ask people to read Godoc for doc on how to do things instead of code (and if we're missing Godoc, can add it)

Don't use markdown-style formatting in Godoc

internal/default_failure_converter.go

internal/failure_converter.go

cretz · 2022-09-30T12:41:06Z

internal/failure_converter.go

+		err := dfc.dataConverter.FromPayload(failure.GetEncodedAttributes(), &ea)
+		if err != nil {
+			return nil
+		}
+		failure.Message = ea.Message
+		failure.StackTrace = ea.StackTrace


Suggested change

err := dfc.dataConverter.FromPayload(failure.GetEncodedAttributes(), &ea)

if err != nil {

return nil

}

failure.Message = ea.Message

failure.StackTrace = ea.StackTrace

if dfc.dataConverter.FromPayload(failure.GetEncodedAttributes(), &ea) == nil {

failure.Message = ea.Message

failure.StackTrace = ea.StackTrace

}

If we can't decode the attributes into that type we should probably continue with our work. They may have encoded their own attributes.

cretz · 2022-09-30T12:47:56Z

internal/failure_converter.go

+		failure.FailureInfo = &failurepb.Failure_ApplicationFailureInfo{ApplicationFailureInfo: failureInfo}
+	}
+
+	failure.Cause = InternalErrorToFailure(errors.Unwrap(err), dc)


This does not properly encode nested attributes.

Instead of walking the causes after this is done and to avoid these mistakes in the codebase in general, lets get rid of these two top-level functions and put them inside the methods of the default converter itself. If someone wants to call these from their converter, they can call GetDefaultFailureConverter().ErrorToFailure or GetDefaultFailureConverter().FailureToError to get default behavior.

(we don't have to make custom converters work for nested errors, so long as they are called for the root one, custom converters can do whatever recursive work they choose to)

ah good catch, i'll refactor

internal/internal_worker.go

cretz · 2022-09-30T12:55:08Z

internalbindings/internalbindings.go

@@ -79,14 +79,14 @@ type (
 	ContinueAsNewError = internal.ContinueAsNewError
 )

-// ConvertErrorToFailure converts Go error to the correspondent Failure protobuf.


This internalbindings package is here to support https://github.com/temporalio/roadrunner-temporal I believe. I think we can 1) remove all error conversion APIs here, and 2) open an issue over there pointing to this PR on how to use the now-public default error conversion API.

done: temporalio/roadrunner-temporal#277

temporal/default_failure_converter.go

cretz

Looks great! If/when CI passes, merge whenever you'd like (or make that minor change if you want).

Don't forget to give https://github.com/temporalio/roadrunner-temporal a heads up on the removal of their internalbindings call at least via an issue.

cretz · 2022-09-30T18:52:21Z

test/bindings_workflows_test.go

@@ -109,7 +110,7 @@ func (d *SingleActivityWorkflowDefinition) Execute(env bindings.WorkflowEnvironm
 						env.Complete(nil, errors.New("error expected"))
 						return
 					}
-					failure := bindings.ConvertErrorToFailure(err, converter.GetDefaultDataConverter())
+					failure := internal.GetDefaultFailureConverter().ErrorToFailure(err)


Suggested change

failure := internal.GetDefaultFailureConverter().ErrorToFailure(err)

failure := temporal.GetDefaultFailureConverter().ErrorToFailure(err)

No biggie though

Quinn-With-Two-Ns · 2022-09-30T18:57:08Z

Looks great! If/when CI passes, merge whenever you'd like (or make that minor change if you want).

Don't forget to give https://github.com/temporalio/roadrunner-temporal a heads up on the removal of their internalbindings call at least via an issue.

Thanks, yep I made a issue over there temporalio/roadrunner-temporal#277

Add support for custom failure converters

073ca57

Adds an interface called `FailureConverter` that allows users to customize failure serialization and deserialization.

Quinn-With-Two-Ns requested a review from cretz September 29, 2022 20:47

Quinn-With-Two-Ns requested a review from a team as a code owner September 29, 2022 20:47

bergundy approved these changes Sep 29, 2022

View reviewed changes

cretz reviewed Sep 30, 2022

View reviewed changes

Address PR review comments

8a8cf9f

Quinn-With-Two-Ns mentioned this pull request Sep 30, 2022

[💡 FEATURE REQUEST]: Support FailureConverter API temporalio/roadrunner-temporal#277

Closed

cretz approved these changes Sep 30, 2022

View reviewed changes

Quinn-With-Two-Ns merged commit e6a06f2 into temporalio:master Sep 30, 2022

This was referenced Oct 11, 2022

Support failure encoding in proxy gRPC interceptor #930

Closed

[Feature Request] Custom failure converters temporalio/sdk-python#152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom failure converters #924

Add support for custom failure converters #924

Quinn-With-Two-Ns commented Sep 29, 2022

CLAassistant commented Sep 29, 2022 •

edited

Loading

CLAassistant commented Sep 29, 2022

bergundy left a comment

cretz Sep 30, 2022

cretz Sep 30, 2022

cretz Sep 30, 2022

cretz Sep 30, 2022 •

edited

Loading

Quinn-With-Two-Ns Sep 30, 2022

cretz Sep 30, 2022 •

edited

Loading

Quinn-With-Two-Ns Sep 30, 2022

cretz left a comment

cretz Sep 30, 2022 •

edited

Loading

Quinn-With-Two-Ns commented Sep 30, 2022

		// default: defaultFailureConverter, does not encode any fields of the error. See `temporal/default_failure_converter` for
		// options to configure or create a custom converter.

	failure := internal.GetDefaultFailureConverter().ErrorToFailure(err)
	failure := temporal.GetDefaultFailureConverter().ErrorToFailure(err)

Add support for custom failure converters #924

Add support for custom failure converters #924

Conversation

Quinn-With-Two-Ns commented Sep 29, 2022

What was changed

Checklist

CLAassistant commented Sep 29, 2022 • edited Loading

CLAassistant commented Sep 29, 2022

bergundy left a comment

Choose a reason for hiding this comment

cretz Sep 30, 2022

Choose a reason for hiding this comment

cretz Sep 30, 2022

Choose a reason for hiding this comment

cretz Sep 30, 2022

Choose a reason for hiding this comment

cretz Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Quinn-With-Two-Ns Sep 30, 2022

Choose a reason for hiding this comment

cretz Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Quinn-With-Two-Ns Sep 30, 2022

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

cretz Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Sep 30, 2022

CLAassistant commented Sep 29, 2022 •

edited

Loading

cretz Sep 30, 2022 •

edited

Loading

cretz Sep 30, 2022 •

edited

Loading

cretz Sep 30, 2022 •

edited

Loading