Do not set OTel span error status for benign exceptions (#530)

Fixes #510

Author: cretz

src/Temporalio.Extensions.OpenTelemetry/TracingInterceptor.cs

@@ -8,6 +8,7 @@
 using OpenTelemetry.Trace;
 using Temporalio.Activities;
 using Temporalio.Api.Common.V1;
+using Temporalio.Api.Enums.V1;
 using Temporalio.Client;
 using Temporalio.Client.Interceptors;
 using Temporalio.Converters;
@@ -198,7 +199,16 @@ protected virtual IDictionary<string, Payload> HeadersFromContext(
 
         private static void RecordExceptionWithStatus(Activity? activity, Exception exception)
         {
-            activity?.SetStatus(ActivityStatusCode.Error, exception.Message);
+            // If the exception is a benign exception, we do not consider the status an error. Note,
+            // we intentionally only consider benign exceptions as not errors directly, not
+            // transitively. So when a client uses this helper for a benign exception from a
+            // workflow, or when a workflow uses this helper for a benign exception from an activity
+            // bubbled out, this will still be considered an error status because those are wrapped
+            // forms given to callers.
+            if ((exception as ApplicationFailureException)?.Category != ApplicationErrorCategory.Benign)
+            {
+                activity?.SetStatus(ActivityStatusCode.Error, exception.Message);
+            }
             activity?.RecordException(exception);
         }
 

tests/Temporalio.Tests/Extensions/OpenTelemetry/TracingInterceptorTests.cs

@@ -728,6 +728,30 @@ await client.ExecuteUpdateWithStartWorkflowAsync(
                 "UpdateWithStartWorkflow:TracingWorkflow", null));
     }
 
+    [Fact]
+    public async Task TracingInterceptor_BenignExceptions_DoNotHaveErrorStatus()
+    {
+        // Confirm non-benign fail shows up as error status
+        var (_, activities) = await ExecuteTracingWorkflowAsync(
+            new(new[] { new TracingWorkflowAction(AppFail: "Non-benign fail") }),
+            expectFail: true);
+        var act = activities.Single(act => act.OperationName == "CompleteWorkflow:TracingWorkflow");
+        Assert.EndsWith(
+            "Non-benign fail",
+            act.Events.Single().Tags.Single(t => t.Key == "exception.message").Value!.ToString());
+        Assert.Equal(ActivityStatusCode.Error, act.Status);
+
+        // Confirm benign fail is not error status
+        (_, activities) = await ExecuteTracingWorkflowAsync(
+            new(new[] { new TracingWorkflowAction(BenignAppFail: "Benign fail") }),
+            expectFail: true);
+        act = activities.Single(act => act.OperationName == "CompleteWorkflow:TracingWorkflow");
+        Assert.EndsWith(
+            "Benign fail",
+            act.Events.Single().Tags.Single(t => t.Key == "exception.message").Value!.ToString());
+        Assert.NotEqual(ActivityStatusCode.Error, act.Status);
+    }
+
     private static void AssertActivities(
         IReadOnlyCollection<Activity> activities, params ActivityAssertion[] assertions)
     {
@@ -907,6 +931,11 @@ public async Task RunAsync(TracingWorkflowParam param)
                 {
                     throw new ApplicationFailureException(action.AppFail);
                 }
+                if (action.BenignAppFail != null)
+                {
+                    throw new ApplicationFailureException(
+                        action.BenignAppFail, category: ApplicationErrorCategory.Benign);
+                }
                 if (action.FailOnNonReplay != null)
                 {
                     await RaiseOnNonReplayAsync(action.FailOnNonReplay);
@@ -1078,6 +1107,7 @@ public record TracingWorkflowParam(
 
     public record TracingWorkflowAction(
         string? AppFail = null,
+        string? BenignAppFail = null,
         string? FailOnNonReplay = null,
         TracingWorkflowActionChildWorkflow? ChildWorkflow = null,
         TracingWorkflowActionActivity? Activity = null,