Don't set error status on otel spans for benign exceptions (#1786)

Author: THardy98

packages/interceptors-opentelemetry/src/instrumentation.ts

@@ -3,7 +3,7 @@
  * @module
  */
 import * as otel from '@opentelemetry/api';
-import { Headers, defaultPayloadConverter } from '@temporalio/common';
+import { ApplicationFailure, ApplicationFailureCategory, Headers, defaultPayloadConverter } from '@temporalio/common';
 
 /** Default trace header for opentelemetry interceptors */
 export const TRACE_HEADER = '_tracer-data';
@@ -43,8 +43,10 @@ async function wrapWithSpan<T>(
     span.setStatus({ code: otel.SpanStatusCode.OK });
     return ret;
   } catch (err: any) {
+    const isBenignErr = err instanceof ApplicationFailure && err.category === ApplicationFailureCategory.BENIGN;
     if (acceptableErrors === undefined || !acceptableErrors(err)) {
-      span.setStatus({ code: otel.SpanStatusCode.ERROR, message: err instanceof Error ? err.message : String(err) });
+      const statusCode = isBenignErr ? otel.SpanStatusCode.UNSET : otel.SpanStatusCode.ERROR;
+      span.setStatus({ code: statusCode, message: (err as Error).message ?? String(err) });
       span.recordException(err);
     } else {
       span.setStatus({ code: otel.SpanStatusCode.OK });

packages/test/src/activities/index.ts

@@ -1,7 +1,7 @@
 /* eslint-disable @typescript-eslint/no-empty-function */
 /* eslint-disable @typescript-eslint/explicit-module-boundary-types */
-import { Context } from '@temporalio/activity';
-import { ApplicationFailure } from '@temporalio/common';
+import { activityInfo, Context } from '@temporalio/activity';
+import { ApplicationFailure, ApplicationFailureCategory } from '@temporalio/common';
 import { ProtoActivityInput, ProtoActivityResult } from '../../protos/root';
 import { cancellableFetch as cancellableFetchInner } from './cancellable-fetch';
 import { fakeProgress as fakeProgressInner } from './fake-progress';
@@ -93,3 +93,12 @@ export async function progressiveSleep(): Promise<void> {
 export async function protoActivity(args: ProtoActivityInput): Promise<ProtoActivityResult> {
   return ProtoActivityResult.create({ sentence: `${args.name} is ${args.age} years old.` });
 }
+
+export async function throwMaybeBenign(): Promise<void> {
+  if (activityInfo().attempt === 1) {
+    throw ApplicationFailure.create({ message: 'not benign' });
+  }
+  if (activityInfo().attempt === 2) {
+    throw ApplicationFailure.create({ message: 'benign', category: ApplicationFailureCategory.BENIGN });
+  }
+}

packages/test/src/test-otel.ts

@@ -470,4 +470,43 @@ if (RUN_INTEGRATION_TESTS) {
     const exceptionEvents = span.events.filter((event) => event.name === 'exception');
     t.is(exceptionEvents.length, 1);
   });
+
+  test('Otel workflow omits ApplicationError with BENIGN category', async (t) => {
+    const memoryExporter = new InMemorySpanExporter();
+    const provider = new BasicTracerProvider();
+    provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter));
+    provider.register();
+    const tracer = provider.getTracer('test-error-tracer');
+
+    const worker = await Worker.create({
+      workflowsPath: require.resolve('./workflows'),
+      activities,
+      taskQueue: 'test-otel-benign-err',
+      interceptors: {
+        activity: [
+          (ctx) => {
+            return { inbound: new OpenTelemetryActivityInboundInterceptor(ctx, { tracer }) };
+          },
+        ],
+      },
+    });
+
+    const client = new WorkflowClient();
+
+    await worker.runUntil(
+      client.execute(workflows.throwMaybeBenignErr, {
+        taskQueue: 'test-otel-benign-err',
+        workflowId: uuid4(),
+        retry: { maximumAttempts: 3 },
+      })
+    );
+
+    const spans = memoryExporter.getFinishedSpans();
+    t.is(spans.length, 3);
+    t.is(spans[0].status.code, SpanStatusCode.ERROR);
+    t.is(spans[0].status.message, 'not benign');
+    t.is(spans[1].status.code, SpanStatusCode.UNSET);
+    t.is(spans[1].status.message, 'benign');
+    t.is(spans[2].status.code, SpanStatusCode.OK);
+  });
 }

packages/test/src/workflows/index.ts

@@ -79,6 +79,7 @@ export * from './swc';
 export * from './tasks-and-microtasks';
 export * from './text-encoder-decoder';
 export * from './throw-async';
+export * from './throw-maybe-benign';
 export * from './trailing-timer';
 export * from './try-to-continue-after-completion';
 export * from './two-strings';

packages/test/src/workflows/throw-maybe-benign.ts

@@ -0,0 +1,11 @@
+import * as workflow from '@temporalio/workflow';
+import * as activities from '../activities';
+
+const { throwMaybeBenign } = workflow.proxyActivities<typeof activities>({
+  startToCloseTimeout: '5s',
+  retry: { maximumAttempts: 3, backoffCoefficient: 1, initialInterval: 500 },
+});
+
+export async function throwMaybeBenignErr(): Promise<void> {
+  await throwMaybeBenign();
+}