fix(remix): Prevent capturing pending promises as exceptions. (#6129)

The reason we have been getting exceptions from Remix SDK captured as, `{}` even if the response is not an empty object was the pending Promises returned from `extractData`.

Implemented the async logic and added another abstraction specifically for error responses, to make sure the errors do not end up misnamed.

Based on Remix examples of [how to throw a `json(...)`](https://github.com/remix-run/remix/blob/471ab259fba5adbdd70de71ee7bc7e874c1f4db9/docs/api/conventions.md), this implementation favours:

1 - `response` (if `response` is a `string`)
2 - `response.statusText`
3 - The extracted `response` object, which will eventually be handled by [Node SDK's `eventbuilder`](https://github.com/getsentry/sentry-javascript/blob/4aff9f14bee54a65105c2ec65170ecef19c05b33/packages/node/src/eventbuilder.ts#L60-L63)

While extracting data from an error response.

Author: onurtemizkan

packages/remix/src/utils/instrumentServer.ts

@@ -57,7 +57,7 @@ function isCatchResponse(response: Response): boolean {
 
 // Based on Remix Implementation
 // https://github.com/remix-run/remix/blob/7688da5c75190a2e29496c78721456d6e12e3abe/packages/remix-server-runtime/data.ts#L131-L145
-function extractData(response: Response): Promise<unknown> {
+async function extractData(response: Response): Promise<unknown> {
   const contentType = response.headers.get('Content-Type');
 
   // Cloning the response to avoid consuming the original body stream
@@ -70,14 +70,28 @@ function extractData(response: Response): Promise<unknown> {
   return responseClone.text();
 }
 
-function captureRemixServerException(err: Error, name: string, request: Request): void {
+async function extractResponseError(response: Response): Promise<unknown> {
+  const responseData = await extractData(response);
+
+  if (typeof responseData === 'string') {
+    return responseData;
+  }
+
+  if (response.statusText) {
+    return response.statusText;
+  }
+
+  return responseData;
+}
+
+async function captureRemixServerException(err: Error, name: string, request: Request): Promise<void> {
   // Skip capturing if the thrown error is not a 5xx response
   // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders
   if (isResponse(err) && err.status < 500) {
     return;
   }
 
-  captureException(isResponse(err) ? extractData(err) : err, scope => {
+  captureException(isResponse(err) ? await extractResponseError(err) : err, scope => {
     scope.setSDKProcessingMetadata({ request });
     scope.addEventProcessor(event => {
       addExceptionMechanism(event, {
@@ -128,7 +142,7 @@ function makeWrappedDocumentRequestFunction(
 
       span?.finish();
     } catch (err) {
-      captureRemixServerException(err, 'documentRequest', request);
+      await captureRemixServerException(err, 'documentRequest', request);
       throw err;
     }
 
@@ -165,7 +179,7 @@ function makeWrappedDataFunction(origFn: DataFunction, id: string, name: 'action
       currentScope.setSpan(activeTransaction);
       span?.finish();
     } catch (err) {
-      captureRemixServerException(err, name, args.request);
+      await captureRemixServerException(err, name, args.request);
       throw err;
     }
 

packages/remix/test/integration/app/routes/action-json-response/$id.tsx

@@ -17,6 +17,22 @@ export const action: ActionFunction = async ({ params: { id } }) => {
     throw redirect('/action-json-response/-1');
   }
 
+  if (id === '-3') {
+    throw json({}, { status: 500, statusText: 'Sentry Test Error' });
+  }
+
+  if (id === '-4') {
+    throw json({ data: 1234 }, { status: 500 });
+  }
+
+  if (id === '-5') {
+    throw json('Sentry Test Error [string body]', { status: 500 });
+  }
+
+  if (id === '-6') {
+    throw json({}, { status: 500 });
+  }
+
   return json({ test: 'test' });
 };
 

packages/remix/test/integration/test/server/action.test.ts

@@ -185,4 +185,204 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
       },
     });
   });
+
+  it('handles a thrown `json()` error response with `statusText`', async () => {
+    const env = await RemixTestEnv.init(adapter);
+    const url = `${env.url}/action-json-response/-3`;
+
+    const envelopes = await env.getMultipleEnvelopeRequest({
+      url,
+      count: 2,
+      method: 'post',
+      envelopeType: ['transaction', 'event'],
+    });
+
+    const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
+    const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
+
+    assertSentryTransaction(transaction[2], {
+      contexts: {
+        trace: {
+          op: 'http.server',
+          status: 'internal_error',
+          tags: {
+            method: 'POST',
+            'http.status_code': '500',
+          },
+        },
+      },
+      tags: {
+        transaction: 'routes/action-json-response/$id',
+      },
+    });
+
+    assertSentryEvent(event[2], {
+      exception: {
+        values: [
+          {
+            type: 'Error',
+            value: 'Sentry Test Error',
+            stacktrace: expect.any(Object),
+            mechanism: {
+              data: {
+                function: 'action',
+              },
+              handled: true,
+              type: 'instrument',
+            },
+          },
+        ],
+      },
+    });
+  });
+
+  it('handles a thrown `json()` error response without `statusText`', async () => {
+    const env = await RemixTestEnv.init(adapter);
+    const url = `${env.url}/action-json-response/-4`;
+
+    const envelopes = await env.getMultipleEnvelopeRequest({
+      url,
+      count: 2,
+      method: 'post',
+      envelopeType: ['transaction', 'event'],
+    });
+
+    const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
+    const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
+
+    assertSentryTransaction(transaction[2], {
+      contexts: {
+        trace: {
+          op: 'http.server',
+          status: 'internal_error',
+          tags: {
+            method: 'POST',
+            'http.status_code': '500',
+          },
+        },
+      },
+      tags: {
+        transaction: 'routes/action-json-response/$id',
+      },
+    });
+
+    assertSentryEvent(event[2], {
+      exception: {
+        values: [
+          {
+            type: 'Error',
+            value: 'Non-Error exception captured with keys: data',
+            stacktrace: expect.any(Object),
+            mechanism: {
+              data: {
+                function: 'action',
+              },
+              handled: true,
+              type: 'instrument',
+            },
+          },
+        ],
+      },
+    });
+  });
+
+  it('handles a thrown `json()` error response with string body', async () => {
+    const env = await RemixTestEnv.init(adapter);
+    const url = `${env.url}/action-json-response/-5`;
+
+    const envelopes = await env.getMultipleEnvelopeRequest({
+      url,
+      count: 2,
+      method: 'post',
+      envelopeType: ['transaction', 'event'],
+    });
+
+    const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
+    const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
+
+    assertSentryTransaction(transaction[2], {
+      contexts: {
+        trace: {
+          op: 'http.server',
+          status: 'internal_error',
+          tags: {
+            method: 'POST',
+            'http.status_code': '500',
+          },
+        },
+      },
+      tags: {
+        transaction: 'routes/action-json-response/$id',
+      },
+    });
+
+    assertSentryEvent(event[2], {
+      exception: {
+        values: [
+          {
+            type: 'Error',
+            value: 'Sentry Test Error [string body]',
+            stacktrace: expect.any(Object),
+            mechanism: {
+              data: {
+                function: 'action',
+              },
+              handled: true,
+              type: 'instrument',
+            },
+          },
+        ],
+      },
+    });
+  });
+
+  it('handles a thrown `json()` error response with an empty object', async () => {
+    const env = await RemixTestEnv.init(adapter);
+    const url = `${env.url}/action-json-response/-6`;
+
+    const envelopes = await env.getMultipleEnvelopeRequest({
+      url,
+      count: 2,
+      method: 'post',
+      envelopeType: ['transaction', 'event'],
+    });
+
+    const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
+    const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
+
+    assertSentryTransaction(transaction[2], {
+      contexts: {
+        trace: {
+          op: 'http.server',
+          status: 'internal_error',
+          tags: {
+            method: 'POST',
+            'http.status_code': '500',
+          },
+        },
+      },
+      tags: {
+        transaction: 'routes/action-json-response/$id',
+      },
+    });
+
+    assertSentryEvent(event[2], {
+      exception: {
+        values: [
+          {
+            type: 'Error',
+            value: 'Non-Error exception captured with keys: [object has no keys]',
+            stacktrace: expect.any(Object),
+            mechanism: {
+              data: {
+                function: 'action',
+              },
+              handled: true,
+              type: 'instrument',
+            },
+          },
+        ],
+      },
+    });
+  });
 });