Add application category #853

THardy98 · 2025-05-01T04:58:32Z

What was changed

Added a category field to ApplicationError, allowing users to configure the severity (and corresponding logging/metrics behavior) of their ApplicationError.

Activity errors that are BENIGN application errors do not log.

Why?

Part of benign exceptions work.

Closes Apply application failure logging and metrics behaviour according to ApplicationErrorCategory #820
How was this tested:
Simple integration test.
Any docs updates needed?
Maybe

…tivities

THardy98 · 2025-05-01T05:34:38Z

Was unsure - do we also want to filter logs on workflow task handling failures @cretz @dandavison :

sdk-python/temporalio/worker/_workflow.py

Lines 308 to 323 in e360398

    
           except Exception as err: 
        
               if isinstance(err, _DeadlockError): 
        
                   err.swap_traceback() 
        
               logger.exception( 
        
                   "Failed handling activation on workflow with run ID %s", act.run_id 
        
               ) 
        
               # Set completion failure 
        
               completion.failed.failure.SetInParent() 
        
               try: 
        
                   self._data_converter.failure_converter.to_failure( 
        
                       err, 
        
                       self._data_converter.payload_converter, 
        
                       completion.failed.failure, 
        
                   )

cretz

Only minor things. We should add followup issues on all SDKs once they are released to go update their "polling" samples to start using benign exceptions.

Was unsure - do we also want to filter logs on workflow task handling failures

This should not get hit on application error, so I don't think we need to add anything for it

temporalio/worker/_activity.py

tests/worker/test_workflow.py

temporalio/worker/_activity.py

temporalio/exceptions.py

cretz · 2025-05-01T12:44:55Z

temporalio/service.py

+        self.create_workflow_rule = client._new_call(
+            "create_workflow_rule",
+            wsv1.CreateWorkflowRuleRequest,
+            wsv1.CreateWorkflowRuleRequest,
+        )
+        self.delete_workflow_rule = client._new_call(
+            "delete_workflow_rule",
+            wsv1.DeleteWorkflowRuleRequest,
+            wsv1.DeleteWorkflowRuleResponse,
+        )
+        self.describe_workflow_rule = client._new_call(
+            "describe_workflow_rule",
+            wsv1.DescribeWorkflowRuleRequest,
+            wsv1.DescribeWorkflowRuleResponse,
+        )
+        self.list_workflow_rules = client._new_call(
+            "list_workflow_rules",
+            wsv1.ListWorkflowRulesRequest,
+            wsv1.ListWorkflowRulesResponse,
+        )
+        self.trigger_workflow_rule = client._new_call(
+            "trigger_workflow_rule",
+            wsv1.TriggerWorkflowRuleRequest,
+            wsv1.TriggerWorkflowRuleResponse,
+        )


You may need to also add these in client.rs (sorry it's such a manual process, we didn't expect RPCs to get added so regularly to justify auto-generating this, though we probably should)

We have a test_all_grpc_calls_present that is supposed to catch if these are missing, I wonder why it is not. Hrmm, I may need to investigate that...

added
from what I can tell, test_all_grpc_calls_present only compares the python client vs the GRPC service it should implement, there's not comparison with the rust bridge client

assert_all_calls_present( client.workflow_service, temporalio.api.workflowservice.v1, temporalio.api.workflowservice.v1.WorkflowServiceStub, )

Ah, yeah, it's just testing the pure Python gRPC client stub creation matches what we've set in Python, it does not actually make the call like some SDKs do. My mistake. There's a gap there, but meh.

cretz · 2025-05-02T14:13:07Z

temporalio/bridge/src/client.rs

+                "create_workflow_rule" => {
+                    rpc_call!(retry_client, call, create_workflow_rule)
+                }
+                "delete_workflow_rule" => {
+                    rpc_call!(retry_client, call, delete_workflow_rule)
+                }
+                "describe_workflow_rule" => {
+                    rpc_call!(retry_client, call, describe_workflow_rule)
+                }
+                "list_workflow_rules" => {
+                    rpc_call!(retry_client, call, list_workflow_rules)
+                }
+                "trigger_workflow_rule" => {
+                    rpc_call!(retry_client, call, trigger_workflow_rule)
+                }


Gonna sound pedantic, sorry, but can we keep these in alphabetical order with the rest? Same for their placement in service.py

cretz · 2025-05-02T14:14:12Z

temporalio/exceptions.py

+        temporalio.api.enums.v1.ApplicationErrorCategory.APPLICATION_ERROR_CATEGORY_UNSPECIFIED
+    )
+
+    """BENIGN category errors emit DEBUG level logs and do not record metrics"""


I think a Python docstring has to be below the item

ah, my mistake
a bit atypical docstring convention

cretz · 2025-05-02T14:15:24Z

temporalio/exceptions.py

+        temporalio.api.enums.v1.ApplicationErrorCategory.APPLICATION_ERROR_CATEGORY_BENIGN
+    )
+
+
 class ApplicationError(FailureError):


Can you adjust the default failure converter to properly populate category in both directions (to/from proto)? And can you add (or adjust) a test where, if you raise a benign application error from a workflow, the client can see the category in the application error (it'll be the cause of workflow failed).

Yeah - I had done this with go/java and forgot to do so here

…t failure converter

cretz

LGTM, feel free to merge when CI passes (there are a couple of flakes, you may have to "re-run failed jobs" a time or two)

temporalio/converter.py

update core

a2fa635

THardy98 requested a review from a team as a code owner May 1, 2025 04:58

add category to application error, do not log on benign errors for ac…

47d7e26

…tivities

THardy98 force-pushed the add_application_category branch from 33b6b2a to 47d7e26 Compare May 1, 2025 04:59

THardy98 added 2 commits April 30, 2025 22:03

formatting

8ce5e9c

implement missing wf service calls

50cbd55

cretz reviewed May 1, 2025

View reviewed changes

address pr review

9d0e7e0

cretz reviewed May 2, 2025

View reviewed changes

THardy98 added 2 commits May 2, 2025 14:47

alphabetically order new wf service calls, convert category in defaul…

f2fa777

…t failure converter

formatting

fe46d0b

THardy98 requested a review from cretz May 2, 2025 19:45

cretz approved these changes May 2, 2025

View reviewed changes

temporalio/converter.py Outdated Show resolved Hide resolved

remove 0 from category conversion

ec26398

THardy98 marked this pull request as draft May 5, 2025 16:48

THardy98 added 2 commits May 5, 2025 12:48

commenting out test, checking for CI flake

2facd4b

uncomment test

01673bc

THardy98 marked this pull request as ready for review May 5, 2025 17:51

THardy98 and others added 3 commits May 15, 2025 07:47

Merge branch 'main' into add_application_category

7fd5d99

format

63a3cb2

Merge branch 'main' into add_application_category

9ec36ef

THardy98 merged commit 8449a35 into main May 15, 2025
18 checks passed

THardy98 deleted the add_application_category branch May 15, 2025 23:09

Add application category #853

Add application category #853

Uh oh!

Conversation

THardy98 commented May 1, 2025

What was changed

Why?

Uh oh!

THardy98 commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cretz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cretz May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

cretz May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

cretz May 2, 2025

Choose a reason for hiding this comment

Uh oh!

THardy98 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

cretz May 2, 2025

Choose a reason for hiding this comment

Uh oh!

THardy98 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

cretz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

THardy98 commented May 1, 2025 •

edited

Loading

cretz left a comment •

edited

Loading

cretz May 1, 2025 •

edited

Loading

cretz May 2, 2025 •

edited

Loading

cretz May 2, 2025 •

edited

Loading

cretz left a comment •

edited

Loading