Browse Source

ref(js): Improve handling of erroring API responses with 200 (#49253)

This makes several improvements to the way our frontend API client handles errors coming from responses with a 200 status.

- Since we are now capturing an event specifically about this situation[1], don't also send a `RequestError` to Sentry.
- In the capturing of that event:
  - Stop logging redundant data in extras
  - Only log extras if the error is a different one than `responseText` being undefined, which we already know about
  - Add `errorReason` as a tag, so we can see a breakdown of the different values
  - Use error message as fingerprint, for simplicity and to force the non-"`ResponseText` is undefined" errors into a new group

[1] https://github.com/getsentry/sentry/pull/49060
Katie Byers 1 year ago
parent
commit
32512533fb

+ 15 - 18
static/app/api.tsx

@@ -521,33 +521,30 @@ export class Client {
           // tons of events from this codepath with a 200 status nonetheless.
           // tons of events from this codepath with a 200 status nonetheless.
           // Until we know why, let's do what is essentially some very fancy print debugging.
           // Until we know why, let's do what is essentially some very fancy print debugging.
           if (status === 200) {
           if (status === 200) {
+            const responseTextUndefined = responseText === undefined;
+
             // Pass a scope object rather than using `withScope` to avoid even
             // Pass a scope object rather than using `withScope` to avoid even
             // the possibility of scope bleed.
             // the possibility of scope bleed.
             const scope = new Sentry.Scope();
             const scope = new Sentry.Scope();
+            scope.setTags({errorReason});
+
+            if (!responseTextUndefined) {
+              // Grab everything that could conceivably be helpful to know
+              scope.setExtras({
+                twoHundredErrorReason,
+                responseJSON,
+                responseText,
+                responseContentType,
+                errorReason,
+              });
+            }
 
 
-            // Grab everything that could conceivably be helpful to know
-            scope.setExtras({
-              twoHundredErrorReason,
-              path,
-              method,
-              status,
-              statusText,
-              responseJSON,
-              responseText,
-              responseContentType,
-              errorReason,
-            });
-
-            const responseTextUndefined = responseText === undefined;
-            const fingerprint = responseTextUndefined
-              ? '200 with undefined responseText'
-              : '200 as error';
             const message = responseTextUndefined
             const message = responseTextUndefined
               ? '200 API response with undefined responseText'
               ? '200 API response with undefined responseText'
               : '200 treated as error';
               : '200 treated as error';
 
 
             // Make sure all of these errors group, so we don't produce a bunch of noise
             // Make sure all of these errors group, so we don't produce a bunch of noise
-            scope.setFingerprint([fingerprint]);
+            scope.setFingerprint([message]);
 
 
             Sentry.captureException(new Error(`${message}: ${method} ${path}`), scope);
             Sentry.captureException(new Error(`${message}: ${method} ${path}`), scope);
           }
           }

+ 10 - 0
static/app/bootstrap/initializeSdk.spec.jsx

@@ -1,6 +1,16 @@
 import {isFilteredRequestErrorEvent} from './initializeSdk';
 import {isFilteredRequestErrorEvent} from './initializeSdk';
 
 
 describe('isFilteredRequestErrorEvent', () => {
 describe('isFilteredRequestErrorEvent', () => {
+  it.each(['GET', 'POST', 'PUT', 'DELETE'])('filters 200 %s events', method => {
+    const requestErrorEvent = {
+      exception: {
+        values: [{type: 'RequestError', value: `${method} /dogs/are/great/ 200`}],
+      },
+    };
+
+    expect(isFilteredRequestErrorEvent(requestErrorEvent)).toBeTruthy();
+  });
+
   it.each(['GET', 'POST', 'PUT', 'DELETE'])('filters 401 %s events', method => {
   it.each(['GET', 'POST', 'PUT', 'DELETE'])('filters 401 %s events', method => {
     const unauthorizedErrorEvent = {
     const unauthorizedErrorEvent = {
       exception: {
       exception: {

+ 3 - 1
static/app/bootstrap/initializeSdk.tsx

@@ -176,6 +176,8 @@ export function isFilteredRequestErrorEvent(event: Event): boolean {
 
 
   const {type = '', value = ''} = mainError;
   const {type = '', value = ''} = mainError;
 
 
+  const is200 =
+    ['RequestError'].includes(type) && !!value.match('(GET|POST|PUT|DELETE) .* 200');
   const is401 =
   const is401 =
     ['UnauthorizedError', 'RequestError'].includes(type) &&
     ['UnauthorizedError', 'RequestError'].includes(type) &&
     !!value.match('(GET|POST|PUT|DELETE) .* 401');
     !!value.match('(GET|POST|PUT|DELETE) .* 401');
@@ -186,5 +188,5 @@ export function isFilteredRequestErrorEvent(event: Event): boolean {
     ['NotFoundError', 'RequestError'].includes(type) &&
     ['NotFoundError', 'RequestError'].includes(type) &&
     !!value.match('(GET|POST|PUT|DELETE) .* 404');
     !!value.match('(GET|POST|PUT|DELETE) .* 404');
 
 
-  return is401 || is403 || is404;
+  return is200 || is401 || is403 || is404;
 }
 }