Add application err category #1925

THardy98 · 2025-04-22T00:47:40Z

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

Quinn-With-Two-Ns · 2025-04-22T13:57:26Z

internal/error.go

@@ -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


Quinn-With-Two-Ns · 2025-04-22T13:59:25Z

test/integration_test.go

+			"Benign failure",
+			"",
+			temporal.ApplicationErrorOptions{
+				Category: internal.ErrorCategoryBenign,


test should not refer to internal types, you need to export ErrorCategoryBenign

Exported/exposed in common error.

Quinn-With-Two-Ns · 2025-04-22T14:00:49Z

internal/error.go

@@ -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


What is the release status of temporalio/features#614? Is it GA?

That's a good question - I'll ask drew

internal/internal_task_pollers.go

cretz · 2025-04-22T13:54:17Z

internal/error.go

@@ -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


Needs to be exposed outside of internal

Exposed in common error

cretz · 2025-04-22T13:54:49Z

internal/error.go

@@ -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


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.

Yeah not sure why I opted for a string... changed to an int enum

cretz · 2025-04-22T13:56:08Z

internal/error.go

@@ -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"


Suggested change

ErrorCategoryBenign ApplicationErrorCategory = "BENIGN"

ApplicationErrorCategoryBenign ApplicationErrorCategory = "BENIGN"

Need to qualify the const IMO

cretz · 2025-04-22T13:59:34Z

internal/error.go

+	case "":
+		// Zero value maps to unspecified
+		return enumspb.APPLICATION_ERROR_CATEGORY_UNSPECIFIED
+	default:


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.

Yup, removed

cretz · 2025-04-22T14:00:24Z

internal/error.go

+	}
+}
+
+func IsBenignApplicationError(err error) bool {


Suggested change

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.

Didn't mean to export this, unexported.

Did you want this removed btw? Just do the check inline wherever it's used?
Alternatively, should we put this check under a different name (i.e. shouldRecordFailureMetric or something similar, probably move it to metrics), in case we adopt more conditional logic for failure metrics in the future?

cretz · 2025-04-22T14:00:55Z

test/integration_test.go

+	ts.Error(err)
+	var appErr *temporal.ApplicationError
+	ts.True(errors.As(err, &appErr))
+	ts.False(internal.IsBenignApplicationError(err))


Try to avoid using internal in integration tests. Users can't use it.

Done - inline boolean check.

cretz · 2025-04-22T14:04:10Z

internal/internal_task_pollers.go

@@ -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.",


Are any of the changes made inside of this method needed?

Nope, old change I forgot to revert

Quinn-With-Two-Ns · 2025-04-22T14:06:35Z

internal/error.go

@@ -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


Is ApplicationErrorCategory intended to be an enum?

- fully qualified names ApplicationErrorCategory<x> - export category for external usage... - make enum int instead of string

cretz · 2025-04-24T20:21:03Z

internal/error.go

+	var appError *ApplicationError
+	return errors.As(err, &appError) && appError.Category() == ApplicationErrorCategoryBenign


I don't think we should assume causes no matter the depth as benign. Explained at temporalio/sdk-java#2485 (comment).

Suggested change

var appError *ApplicationError

return errors.As(err, &appError) && appError.Category() == ApplicationErrorCategoryBenign

appError, _ := err.(*ApplicationError)

return appError != nil && appError.Category() == ApplicationErrorCategoryBenign

Yes we should only look at the top level error, no error part of the error is inspected at any depth

Quinn-With-Two-Ns · 2025-04-24T21:54:36Z

internal/error.go

@@ -137,6 +137,8 @@ type (
 		//
 		// NOTE: This option is supported by Temporal Server >= v1.24.2 older version will ignore this value.
 		NextRetryDelay time.Duration
+		// Category of the error. Maps to logging/metrics behaviours.


Suggested change

// Category of the error. Maps to logging/metrics behaviours.

// Category of the error. Maps to SDK logging/metrics behaviours.

This Category does not effect server side metrics right?

Can you update the comment then if this is correct?

cretz

Look great! Would like @Quinn-With-Two-Ns's approval before merge though.

upgrade api-go to 1.48.0

e77d632

THardy98 requested a review from a team as a code owner April 22, 2025 00:47

THardy98 force-pushed the add_application_err_category branch from dd61c43 to 8700e2a Compare April 22, 2025 00:50

add support for benign application error category

f011e54

THardy98 force-pushed the add_application_err_category branch 6 times, most recently from 9522536 to 49536fa Compare April 22, 2025 03:12

go mod tidy all go.mod files, linting/formatting fixes, ci fixes

388c9ba

THardy98 force-pushed the add_application_err_category branch from 49536fa to 388c9ba Compare April 22, 2025 03:58

Quinn-With-Two-Ns reviewed Apr 22, 2025

View reviewed changes

internal/internal_task_pollers.go Outdated Show resolved Hide resolved

cretz reviewed Apr 22, 2025

View reviewed changes

Quinn-With-Two-Ns reviewed Apr 22, 2025

View reviewed changes

fixes, added tests

d96a992

THardy98 force-pushed the add_application_err_category branch 3 times, most recently from c914631 to 8f0368d Compare April 24, 2025 17:41

Address PR review:

5646e0a

- fully qualified names ApplicationErrorCategory<x> - export category for external usage... - make enum int instead of string

THardy98 force-pushed the add_application_err_category branch from 8f0368d to 5646e0a Compare April 24, 2025 17:43

THardy98 requested review from cretz and Quinn-With-Two-Ns April 24, 2025 18:07

THardy98 mentioned this pull request Apr 24, 2025

Add application err category temporalio/sdk-java#2485

Merged

cretz reviewed Apr 24, 2025

View reviewed changes

Quinn-With-Two-Ns reviewed Apr 24, 2025

View reviewed changes

only check top-level error

b9bdbf8

THardy98 requested review from cretz and Quinn-With-Two-Ns April 25, 2025 16:50

cretz approved these changes Apr 25, 2025

View reviewed changes

update comment

4b3e520

Quinn-With-Two-Ns approved these changes Apr 25, 2025

View reviewed changes

Merge branch 'master' into add_application_err_category

3b74603

THardy98 merged commit ee78d25 into master Apr 25, 2025
17 checks passed

THardy98 deleted the add_application_err_category branch April 25, 2025 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add application err category #1925

Add application err category #1925

THardy98 commented Apr 22, 2025

Quinn-With-Two-Ns Apr 22, 2025

Quinn-With-Two-Ns Apr 22, 2025

THardy98 Apr 24, 2025

Quinn-With-Two-Ns Apr 22, 2025 •

edited

Loading

THardy98 Apr 24, 2025

cretz Apr 22, 2025

THardy98 Apr 24, 2025

cretz Apr 22, 2025

THardy98 Apr 24, 2025

cretz Apr 22, 2025

THardy98 Apr 24, 2025

cretz Apr 22, 2025

THardy98 Apr 24, 2025

cretz Apr 22, 2025

THardy98 Apr 24, 2025

THardy98 Apr 24, 2025 •

edited

Loading

cretz Apr 22, 2025

THardy98 Apr 24, 2025

cretz Apr 22, 2025

THardy98 Apr 24, 2025

Quinn-With-Two-Ns Apr 22, 2025

THardy98 Apr 24, 2025

cretz Apr 24, 2025

Quinn-With-Two-Ns Apr 24, 2025 •

edited

Loading

Quinn-With-Two-Ns Apr 24, 2025

THardy98 Apr 24, 2025

Quinn-With-Two-Ns Apr 25, 2025

THardy98 Apr 25, 2025

cretz left a comment

	ErrorCategoryBenign ApplicationErrorCategory = "BENIGN"
	ApplicationErrorCategoryBenign ApplicationErrorCategory = "BENIGN"

	func IsBenignApplicationError(err error) bool {
	func isBenignApplicationError(err error) bool {

		var appError *ApplicationError
		return errors.As(err, &appError) && appError.Category() == ApplicationErrorCategoryBenign

	// Category of the error. Maps to logging/metrics behaviours.
	// Category of the error. Maps to SDK logging/metrics behaviours.

Add application err category #1925

Add application err category #1925

Conversation

THardy98 commented Apr 22, 2025

What was changed

Why?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

THardy98 Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns Apr 22, 2025 •

edited

Loading

THardy98 Apr 24, 2025 •

edited

Loading

Quinn-With-Two-Ns Apr 24, 2025 •

edited

Loading