Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make errors support verbose formatting to print wrapped errors #1396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make errors support verbose formatting to print wrapped errors #1396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions internal/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ package internal
import (
"errors"
"fmt"
"io"
"reflect"
"strings"
"time"
Expand Down Expand Up @@ -551,6 +552,10 @@ func (e *ApplicationError) Message() string {
return e.msg
}

func (e *ApplicationError) Format(s fmt.State, verb rune) {
formatError(e, s, verb, e.Unwrap())
}

// Type returns error type represented as string.
// This type can be passed explicitly to ApplicationError constructor.
// Also any other Go error is converted to ApplicationError and type is set automatically using reflection.
Expand Down Expand Up @@ -610,6 +615,10 @@ func (e *TimeoutError) TimeoutType() enumspb.TimeoutType {
return e.timeoutType
}

func (e *TimeoutError) Format(s fmt.State, verb rune) {
formatError(e, s, verb, e.Unwrap())
}

// HasLastHeartbeatDetails return if this error has strong typed detail data.
func (e *TimeoutError) HasLastHeartbeatDetails() bool {
return e.lastHeartbeatDetails != nil && e.lastHeartbeatDetails.HasValues()
Expand Down Expand Up @@ -667,6 +676,10 @@ func (e *PanicError) StackTrace() string {
return e.stackTrace
}

func (e *PanicError) Format(s fmt.State, verb rune) {
formatError(e, s, verb, e.StackTrace())
}

// Error from error interface
func (e *workflowPanicError) Error() string {
return fmt.Sprintf("%v", e.value)
Expand All @@ -677,6 +690,10 @@ func (e *workflowPanicError) StackTrace() string {
return e.stackTrace
}

func (e *workflowPanicError) Format(s fmt.State, verb rune) {
formatError(e, s, verb, e.StackTrace())
}

// Error from error interface
func (e *ContinueAsNewError) Error() string {
return e.message()
Expand Down Expand Up @@ -732,6 +749,10 @@ func (e *ServerError) Unwrap() error {
return e.cause
}

func (e *ServerError) Format(s fmt.State, verb rune) {
formatError(e, s, verb, e.Unwrap())
}

func (e *ActivityError) Error() string {
msg := fmt.Sprintf("%s (type: %s, scheduledEventID: %d, startedEventID: %d, identity: %s)", e.message(), e.activityType.GetName(), e.scheduledEventID, e.startedEventID, e.identity)
if e.cause != nil {
Expand Down Expand Up @@ -788,6 +809,10 @@ func (e *ChildWorkflowExecutionError) Error() string {
return msg
}

func (e *ChildWorkflowExecutionError) Format(s fmt.State, verb rune) {
formatError(e, s, verb, e.Unwrap())
}

func (e *ChildWorkflowExecutionError) message() string {
return "child workflow execution error"
}
Expand Down Expand Up @@ -820,6 +845,24 @@ func (e *WorkflowExecutionError) Unwrap() error {
return e.cause
}

func formatError(e error, s fmt.State, verb rune, verboseValue any) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear the future proof-ness of this. If Go adds new print flags that'd work for errors, we'd silently not support them.

I wonder if it'd be better to change where you're formatting these errors to handle more advanced types instead of adding a formatting feature to a few of the errors here. I think that gives you the most flexibility instead of assuming exactly what people want when %+v is put (you want stack trace, but another might want struct fields). We tend to put accessors on the errors for users to extract and display the way they prefer.

(having said that, I'm not completely against the change, I think it just makes too many assumptions of what users may want to see instead of encouraging them to extract the information themselves when they're displaying)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right it might be different what people want - I just copied the behavior from pkg/errors since that seems to be what a lot of people (including myself) are used to. I'd personally say that %+v means max verbose, so dump anything that can be dumped, but again that might be a personal preference...

switch verb {
case 'v':
if s.Flag('+') {
fmt.Fprintf(s, "%+v\n", verboseValue)
io.WriteString(s, e.Error())
return
}
fallthrough
case 's', 'q':
io.WriteString(s, e.Error())
}
}

func (e *WorkflowExecutionError) Format(s fmt.State, verb rune) {
formatError(e, s, verb, e.Unwrap())
}

func (e *ActivityNotRegisteredError) Error() string {
supported := strings.Join(e.supportedTypes, ", ")
return fmt.Sprintf("unable to find activityType=%v. Supported types: [%v]", e.activityType, supported)
Expand Down
41 changes: 41 additions & 0 deletions internal/error_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1245,3 +1245,44 @@ func Test_convertFailureToError_SaveFailure(t *testing.T) {
require.Equal("SomeJavaException", f2.GetCause().GetApplicationFailureInfo().GetType())
require.Equal(true, f2.GetCause().GetApplicationFailureInfo().GetNonRetryable())
}

func Test_verbose_error_formatting(t *testing.T) {
tests := []struct {
name string
err error
expected string
}{
{
name: "WorkflowExecutionError",
err: NewWorkflowExecutionError("wid", "rid", "workflowType", newWorkflowPanicError("test message", "stack trace")),
expected: "stack trace\ntest message\nworkflow execution error (type: workflowType, workflowID: wid, runID: rid): test message",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems odd to show inner error information before outer error information

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically copied the behavior from github.com/pkg/errors - I did also think the same but for consistency it seemed like the best approach

},
{
name: "ApplicationError",
err: NewApplicationError("test message", "customType", true, errors.New("cause error"), "details", 2208),
expected: "cause error\ntest message (type: customType, retryable: false): cause error",
},
{
name: "TimeoutError",
err: NewTimeoutError("timeout", enumspb.TIMEOUT_TYPE_START_TO_CLOSE, errors.New("cause error")),
expected: "cause error\ntimeout (type: StartToClose): cause error",
},
{
name: "ServerError",
err: NewServerError("message", true, errors.New("cause error")),
expected: "cause error\nmessage: cause error",
},
{
name: "ChildWorkflowExecutionError",
err: NewChildWorkflowExecutionError("namespace", "wID", "rID", "wfType", 8, 22, enumspb.RETRY_STATE_NON_RETRYABLE_FAILURE, NewApplicationError("test message", "customType", true, errors.New("cause error"))),
expected: "cause error\ntest message (type: customType, retryable: false): cause error\nchild workflow execution error (type: wfType, workflowID: wID, runID: rID, initiatedEventID: 8, startedEventID: 22): test message (type: customType, retryable: false): cause error",
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
str := fmt.Sprintf("%+v", test.err)
require.Equal(t, test.expected, str)
})
}
}
Loading