chore(core): Remove circular JSON debugging hacks (#5267)

In #4425 and #4574, various hacks were added to the SDK in aid of debugging #2809. Now that that issue is closed, they (and the clean-up code in tests which was there to compensate for them) can come out.

Author: lobsterkatie

packages/core/src/baseclient.ts

@@ -446,14 +446,6 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     }
 
     return result.then(evt => {
-      if (evt) {
-        // TODO this is more of the hack trying to solve https://github.com/getsentry/sentry-javascript/issues/2809
-        // it is only attached as extra data to the event if the event somehow skips being normalized
-        evt.sdkProcessingMetadata = {
-          ...evt.sdkProcessingMetadata,
-          normalizeDepth: `${normalize(normalizeDepth)} (${typeof normalizeDepth})`,
-        };
-      }
       if (typeof normalizeDepth === 'number' && normalizeDepth > 0) {
         return this._normalizeEvent(evt, normalizeDepth, normalizeMaxBreadth);
       }
@@ -525,8 +517,6 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
       });
     }
 
-    normalized.sdkProcessingMetadata = { ...normalized.sdkProcessingMetadata, baseClientNormalized: true };
-
     return normalized;
   }
 

packages/core/src/envelope.ts

@@ -73,33 +73,12 @@ export function createEventEnvelope(
   const { transactionSampling } = event.sdkProcessingMetadata || {};
   const { method: samplingMethod, rate: sampleRate } = transactionSampling || {};
 
-  // TODO: Below is a temporary hack in order to debug a serialization error - see
-  // https://github.com/getsentry/sentry-javascript/issues/2809,
-  // https://github.com/getsentry/sentry-javascript/pull/4425, and
-  // https://github.com/getsentry/sentry-javascript/pull/4574.
-  //
-  // TL; DR: even though we normalize all events (which should prevent this), something is causing `JSON.stringify` to
-  // throw a circular reference error.
-  //
-  // When it's time to remove it:
-  // 1. Delete everything between here and where the request object `req` is created, EXCEPT the line deleting
-  //    `sdkProcessingMetadata`
-  // 2. Restore the original version of the request body, which is commented out
-  // 3. Search for either of the PR URLs above and pull out the companion hacks in the browser playwright tests and the
-  //    baseClient tests in this package
   enhanceEventWithSdkInfo(event, metadata && metadata.sdk);
-  event.tags = event.tags || {};
-  event.extra = event.extra || {};
 
-  // In theory, all events should be marked as having gone through normalization and so
-  // we should never set this tag/extra data
-  if (!(event.sdkProcessingMetadata && event.sdkProcessingMetadata.baseClientNormalized)) {
-    event.tags.skippedNormalization = true;
-    event.extra.normalizeDepth = event.sdkProcessingMetadata ? event.sdkProcessingMetadata.normalizeDepth : 'unset';
-  }
-
-  // prevent this data from being sent to sentry
-  // TODO: This is NOT part of the hack - DO NOT DELETE
+  // Prevent this data (which, if it exists, was used in earlier steps in the processing pipeline) from being sent to
+  // sentry. (Note: Our use of this property comes and goes with whatever we might be debugging, whatever hacks we may
+  // have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid
+  // of this `delete`, lest we miss putting it back in the next time the property is in use.)
   delete event.sdkProcessingMetadata;
 
   const envelopeHeaders = createEventEnvelopeHeaders(event, sdkInfo, tunnel, dsn);

packages/core/test/lib/base.test.ts

@@ -884,29 +884,9 @@ describe('BaseClient', () => {
       const normalizedTransaction = JSON.parse(JSON.stringify(transaction)); // deep-copy
 
       client.captureEvent(transaction);
-
-      // TODO: This is to compensate for a temporary debugging hack which adds data the tests aren't anticipating to the
-      // event. The code can be restored to its original form (the commented-out line below) once that hack is
-      // removed. See https://github.com/getsentry/sentry-javascript/pull/4425 and
-      // https://github.com/getsentry/sentry-javascript/pull/4574
       const capturedEvent = TestClient.instance!.event!;
-      if (capturedEvent.sdkProcessingMetadata?.normalizeDepth) {
-        if (Object.keys(capturedEvent.sdkProcessingMetadata).length === 1) {
-          delete capturedEvent.sdkProcessingMetadata;
-        } else {
-          delete capturedEvent.sdkProcessingMetadata.normalizeDepth;
-        }
-      }
-      if (capturedEvent.sdkProcessingMetadata?.baseClientNormalized) {
-        if (Object.keys(capturedEvent.sdkProcessingMetadata).length === 1) {
-          delete capturedEvent.sdkProcessingMetadata;
-        } else {
-          delete capturedEvent.sdkProcessingMetadata.baseClientNormalized;
-        }
-      }
 
       expect(capturedEvent).toEqual(normalizedTransaction);
-      // expect(TestClient.instance!.event!).toEqual(normalizedTransaction);
     });
 
     test('calls beforeSend and uses original event without any changes', () => {

packages/core/test/mocks/client.ts

@@ -69,6 +69,10 @@ export class TestClient extends BaseClient<TestClientOptions> {
 
   public sendEvent(event: Event, hint?: EventHint): void {
     this.event = event;
+
+    // In real life, this will get deleted as part of envelope creation.
+    delete event.sdkProcessingMetadata;
+
     if (this._options.enableSend) {
       super.sendEvent(event, hint);
       return;

packages/integration-tests/suites/public-api/configureScope/clear_scope/test.ts

@@ -9,18 +9,6 @@ sentryTest('should clear previously set properties of a scope', async ({ getLoca
 
   const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
 
-  // TODO: This is to compensate for a temporary debugging hack which adds data the tests aren't anticipating to the
-  // event. The code can be restored to its original form (the commented-out line below) once that hack is
-  // removed. See https://github.com/getsentry/sentry-javascript/pull/4425 and
-  // https://github.com/getsentry/sentry-javascript/pull/4574
-  if (eventData.extra) {
-    if (Object.keys(eventData.extra).length === 1) {
-      delete eventData.extra;
-    } else {
-      delete eventData.extra.normalizeDepth;
-    }
-  }
-
   expect(eventData.message).toBe('cleared_scope');
   expect(eventData.user).toBeUndefined();
   expect(eventData.tags).toBeUndefined();

packages/integration-tests/utils/helpers.ts

@@ -70,26 +70,7 @@ async function getMultipleRequests(
       if (urlRgx.test(request.url())) {
         try {
           reqCount -= 1;
-
-          // TODO: This is to compensate for a temporary debugging hack which adds data the tests aren't anticipating to
-          // the request. The code can be restored to its original form (the commented-out line below) once that hack is
-          // removed. See https://github.com/getsentry/sentry-javascript/pull/4425.
-          const parsedRequest = requestParser(request);
-          if (parsedRequest.tags) {
-            if (
-              Object.keys(parsedRequest.tags).length === 0 ||
-              (Object.keys(parsedRequest.tags).length === 1 && 'skippedNormalization' in parsedRequest.tags)
-            ) {
-              delete parsedRequest.tags;
-            } else {
-              delete parsedRequest.tags.skippedNormalization;
-            }
-          }
-          if (parsedRequest.extra && Object.keys(parsedRequest.extra).length === 0) {
-            delete parsedRequest.extra;
-          }
-          requestData.push(parsedRequest);
-          // requestData.push(requestParser(request));
+          requestData.push(requestParser(request));
 
           if (reqCount === 0) {
             if (timeoutId) {

packages/node-integration-tests/suites/public-api/configureScope/clear_scope/test.ts

@@ -8,8 +8,6 @@ test('should clear previously set properties of a scope', async () => {
 
   assertSentryEvent(envelope[2], {
     message: 'cleared_scope',
-    tags: {},
-    extra: {},
   });
 
   expect((envelope[2] as Event).user).not.toBeDefined();

packages/node-integration-tests/suites/public-api/withScope/nested-scopes/test.ts

@@ -11,7 +11,6 @@ test('should allow nested scoping', async () => {
     user: {
       id: 'qux',
     },
-    tags: {},
   });
 
   assertSentryEvent(envelopes[3][2], {
@@ -49,6 +48,5 @@ test('should allow nested scoping', async () => {
     user: {
       id: 'qux',
     },
-    tags: {},
   });
 });