fix(node): Ensure status is correct for http server span errors (#12477)

I noticed that we were not correctly setting a status message based on
http status for errored http.server span.
The problem was in our logic, where we already had the status set to `{
code: 2, message: undefined }`, and whenever the code was not 0 (unset),
we'd never look at the attributes to infer the proper status message.

Now, if we have code: 2 but not message, we try to infer the message
from the attributes, if possible.

Author: mydea

dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts

@@ -12,7 +12,6 @@ test('Sends an API route transaction', async ({ baseURL }) => {
   await fetch(`${baseURL}/test-transaction`);
 
   const transactionEvent = await pageloadTransactionEventPromise;
-  const transactionEventId = transactionEvent.event_id;
 
   expect(transactionEvent.contexts?.trace).toEqual({
     data: {
@@ -119,3 +118,84 @@ test('Sends an API route transaction', async ({ baseURL }) => {
     trace_id: expect.any(String),
   });
 });
+
+test('Sends an API route transaction for an errored route', async ({ baseURL }) => {
+  const transactionEventPromise = waitForTransaction('node-express', transactionEvent => {
+    return (
+      transactionEvent.contexts?.trace?.op === 'http.server' &&
+      transactionEvent.transaction === 'GET /test-exception/:id' &&
+      transactionEvent.request?.url === 'http://localhost:3030/test-exception/777'
+    );
+  });
+
+  await fetch(`${baseURL}/test-exception/777`);
+
+  const transactionEvent = await transactionEventPromise;
+
+  expect(transactionEvent.contexts?.trace?.op).toEqual('http.server');
+  expect(transactionEvent.transaction).toEqual('GET /test-exception/:id');
+  expect(transactionEvent.contexts?.trace?.status).toEqual('internal_error');
+  expect(transactionEvent.contexts?.trace?.data?.['http.status_code']).toEqual(500);
+
+  const spans = transactionEvent.spans || [];
+
+  expect(spans).toContainEqual({
+    data: {
+      'sentry.origin': 'auto.http.otel.express',
+      'sentry.op': 'middleware.express',
+      'http.route': '/',
+      'express.name': 'query',
+      'express.type': 'middleware',
+      'otel.kind': 'INTERNAL',
+    },
+    description: 'query',
+    op: 'middleware.express',
+    origin: 'auto.http.otel.express',
+    parent_span_id: expect.any(String),
+    span_id: expect.any(String),
+    start_timestamp: expect.any(Number),
+    status: 'ok',
+    timestamp: expect.any(Number),
+    trace_id: expect.any(String),
+  });
+
+  expect(spans).toContainEqual({
+    data: {
+      'sentry.origin': 'auto.http.otel.express',
+      'sentry.op': 'middleware.express',
+      'http.route': '/',
+      'express.name': 'expressInit',
+      'express.type': 'middleware',
+      'otel.kind': 'INTERNAL',
+    },
+    description: 'expressInit',
+    op: 'middleware.express',
+    origin: 'auto.http.otel.express',
+    parent_span_id: expect.any(String),
+    span_id: expect.any(String),
+    start_timestamp: expect.any(Number),
+    status: 'ok',
+    timestamp: expect.any(Number),
+    trace_id: expect.any(String),
+  });
+
+  expect(spans).toContainEqual({
+    data: {
+      'sentry.origin': 'auto.http.otel.express',
+      'sentry.op': 'request_handler.express',
+      'http.route': '/test-exception/:id',
+      'express.name': '/test-exception/:id',
+      'express.type': 'request_handler',
+      'otel.kind': 'INTERNAL',
+    },
+    description: '/test-exception/:id',
+    op: 'request_handler.express',
+    origin: 'auto.http.otel.express',
+    parent_span_id: expect.any(String),
+    span_id: expect.any(String),
+    start_timestamp: expect.any(Number),
+    status: 'ok',
+    timestamp: expect.any(Number),
+    trace_id: expect.any(String),
+  });
+});

packages/opentelemetry/src/utils/mapStatus.ts

@@ -1,7 +1,7 @@
 import { SpanStatusCode } from '@opentelemetry/api';
 import { SEMATTRS_HTTP_STATUS_CODE, SEMATTRS_RPC_GRPC_STATUS_CODE } from '@opentelemetry/semantic-conventions';
 import { SPAN_STATUS_ERROR, SPAN_STATUS_OK, getSpanStatusFromHttpCode } from '@sentry/core';
-import type { SpanStatus } from '@sentry/types';
+import type { SpanAttributes, SpanStatus } from '@sentry/types';
 
 import type { AbstractSpan } from '../types';
 import { spanHasAttributes, spanHasStatus } from './spanTypes';
@@ -43,14 +43,37 @@ export function mapStatus(span: AbstractSpan): SpanStatus {
       return { code: SPAN_STATUS_OK };
       // If the span is already marked as erroneous we return that exact status
     } else if (status.code === SpanStatusCode.ERROR) {
-      if (typeof status.message === 'undefined' || isStatusErrorMessageValid(status.message)) {
+      if (typeof status.message === 'undefined') {
+        const inferredStatus = inferStatusFromAttributes(attributes);
+        if (inferredStatus) {
+          return inferredStatus;
+        }
+      }
+
+      if (status.message && isStatusErrorMessageValid(status.message)) {
         return { code: SPAN_STATUS_ERROR, message: status.message };
       } else {
         return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
       }
     }
   }
 
+  // If the span status is UNSET, we try to infer it from HTTP or GRPC status codes.
+  const inferredStatus = inferStatusFromAttributes(attributes);
+
+  if (inferredStatus) {
+    return inferredStatus;
+  }
+
+  // We default to setting the spans status to ok.
+  if (status && status.code === SpanStatusCode.UNSET) {
+    return { code: SPAN_STATUS_OK };
+  } else {
+    return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
+  }
+}
+
+function inferStatusFromAttributes(attributes: SpanAttributes): SpanStatus | undefined {
   // If the span status is UNSET, we try to infer it from HTTP or GRPC status codes.
 
   const httpCodeAttribute = attributes[SEMATTRS_HTTP_STATUS_CODE];
@@ -63,18 +86,13 @@ export function mapStatus(span: AbstractSpan): SpanStatus {
         ? parseInt(httpCodeAttribute)
         : undefined;
 
-  if (numberHttpCode) {
+  if (typeof numberHttpCode === 'number') {
     return getSpanStatusFromHttpCode(numberHttpCode);
   }
 
   if (typeof grpcCodeAttribute === 'string') {
     return { code: SPAN_STATUS_ERROR, message: canonicalGrpcErrorCodesMap[grpcCodeAttribute] || 'unknown_error' };
   }
 
-  // We default to setting the spans status to ok.
-  if (status && status.code === SpanStatusCode.UNSET) {
-    return { code: SPAN_STATUS_OK };
-  } else {
-    return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
-  }
+  return undefined;
 }

packages/opentelemetry/test/utils/mapStatus.test.ts

@@ -91,6 +91,19 @@ describe('mapStatus', () => {
     expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'invalid_argument' });
   });
 
+  it('returns error status when span already has error status without message', () => {
+    const span = createSpan();
+    span.setStatus({ code: 2 }); // ERROR
+    expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'unknown_error' });
+  });
+
+  it('infers error status form attributes when span already has error status without message', () => {
+    const span = createSpan();
+    span.setAttribute(SEMATTRS_HTTP_STATUS_CODE, 500);
+    span.setStatus({ code: 2 }); // ERROR
+    expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
+  });
+
   it('returns unknown error status when code is unknown', () => {
     const span = createSpan();
     span.setStatus({ code: -1 as 0 });