Add application err category #1925
Conversation
THardy98
force-pushed
the
add_application_err_category
branch
from
dd61c43 to
8700e2a
Compare
THardy98
force-pushed
the
add_application_err_category
branch
6 times, most recently
from
9522536 to
49536fa
Compare
THardy98
force-pushed
the
add_application_err_category
branch
from
49536fa to
388c9ba
Compare
| @@ -137,6 +140,7 @@ type ( | |||
| // | |||
| // NOTE: This option is supported by Temporal Server >= v1.24.2 older version will ignore this value. | |||
| NextRetryDelay time.Duration | |||
| Category ApplicationErrorCategory | |||
| "Benign failure", | ||
| "", | ||
| temporal.ApplicationErrorOptions{ | ||
| Category: internal.ErrorCategoryBenign, |
There was a problem hiding this comment.
test should not refer to internal types, you need to export ErrorCategoryBenign
| @@ -137,6 +140,7 @@ type ( | |||
| // | |||
| // NOTE: This option is supported by Temporal Server >= v1.24.2 older version will ignore this value. | |||
| NextRetryDelay time.Duration | |||
| Category ApplicationErrorCategory | |||
There was a problem hiding this comment.
What is the release status of temporalio/features#614? Is it GA?
| incrementWorkflowTaskFailureCounter(metricsHandler, failureReason) | ||
| completedRequest = failWorkflowTask | ||
| wtp.logger.Warn("Failed to process workflow task.", |
There was a problem hiding this comment.
What is the goal of this change?
| @@ -119,6 +119,9 @@ Workflow consumers will get an instance of *WorkflowExecutionError. This error w | |||
| */ | |||
|
|
|||
| type ( | |||
| // Category of the error. Maps to logging/metrics behaviours. | |||
| ApplicationErrorCategory string | |||
There was a problem hiding this comment.
Needs to be exposed outside of internal
| @@ -119,6 +119,9 @@ Workflow consumers will get an instance of *WorkflowExecutionError. This error w | |||
| */ | |||
|
|
|||
| type ( | |||
| // Category of the error. Maps to logging/metrics behaviours. | |||
| ApplicationErrorCategory string | |||
There was a problem hiding this comment.
Hrmm, I wonder if we really want this to be a string vs just a number (based on the proto number) and can have a String() on it. A string may make users think it can accept any string literal. Also other enumerates we traditionally do the iota approach. And if/when you do change to integer here, make sure there is an unspecified form representing 0.
| @@ -380,6 +385,11 @@ var ( | |||
| ErrMissingWorkflowID = errors.New("workflow ID is unset for Nexus operation") | |||
| ) | |||
|
|
|||
| const ( | |||
| // ErrorCategoryBenign indicates an error that is expected under normal operation and should not trigger alerts. | |||
| ErrorCategoryBenign ApplicationErrorCategory = "BENIGN" | |||
There was a problem hiding this comment.
| ErrorCategoryBenign ApplicationErrorCategory = "BENIGN" | |
| ApplicationErrorCategoryBenign ApplicationErrorCategory = "BENIGN" |
Need to qualify the const IMO
| case "": | ||
| // Zero value maps to unspecified | ||
| return enumspb.APPLICATION_ERROR_CATEGORY_UNSPECIFIED | ||
| default: |
There was a problem hiding this comment.
If/when we change to integer enums we can/should just use the enum given to us in both directions since both Go and proto allow "open-ended" enums.
| } | ||
| } | ||
|
|
||
| func IsBenignApplicationError(err error) bool { |
There was a problem hiding this comment.
| func IsBenignApplicationError(err error) bool { | |
| func isBenignApplicationError(err error) bool { |
No need to expose I assume unless it's a helper we really need to expose, but I don't believe we do. A user can easily check for any value on application error without a helper.
| ts.Error(err) | ||
| var appErr *temporal.ApplicationError | ||
| ts.True(errors.As(err, &appErr)) | ||
| ts.False(internal.IsBenignApplicationError(err)) |
There was a problem hiding this comment.
Try to avoid using internal in integration tests. Users can't use it.
| @@ -474,19 +474,21 @@ func (wtp *workflowTaskPoller) RespondTaskCompletedWithMetrics( | |||
| ) (response *workflowservice.RespondWorkflowTaskCompletedResponse, err error) { | |||
| metricsHandler := wtp.metricsHandler.WithTags(metrics.WorkflowTags(task.WorkflowType.GetName())) | |||
| if taskErr != nil { | |||
| wtp.logger.Warn("Failed to process workflow task.", | |||
There was a problem hiding this comment.
Are any of the changes made inside of this method needed?
| @@ -119,6 +119,9 @@ Workflow consumers will get an instance of *WorkflowExecutionError. This error w | |||
| */ | |||
|
|
|||
| type ( | |||
| // Category of the error. Maps to logging/metrics behaviours. | |||
| ApplicationErrorCategory string | |||
There was a problem hiding this comment.
Is ApplicationErrorCategory intended to be an enum?
What was changed
Upgrade api-go to 1.48.0
Added support for
ApplicationErrorCategory, allowing users to specify benign application errors.Why?
Part of the benign exceptions work.
Closes Apply application failure logging and metrics behaviour according to ApplicationErrorCategory #1908
How was this tested:
Modify existing failure conversion tests.
Any docs updates needed?
Not sure