From db373c92bf4c1ea75c9567a2ed3f103470140284 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 21 Jan 2022 15:17:13 -0800 Subject: [PATCH 1/3] renormalize only if stringifying goes wrong --- packages/core/src/baseclient.ts | 4 --- packages/core/src/request.ts | 45 +++++++++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index a06283fcc4cc..85d192c75e29 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -428,10 +428,6 @@ export abstract class BaseClient implement normalized.contexts.trace = event.contexts.trace; } - const { _experiments = {} } = this.getOptions(); - if (_experiments.ensureNoCircularStructures) { - return normalize(normalized); - } return normalized; } diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts index f483cbc596c7..82259147d1f5 100644 --- a/packages/core/src/request.ts +++ b/packages/core/src/request.ts @@ -1,5 +1,5 @@ import { Event, SdkInfo, SentryRequest, SentryRequestType, Session, SessionAggregates } from '@sentry/types'; -import { dsnToString } from '@sentry/utils'; +import { dsnToString, normalize } from '@sentry/utils'; import { APIDetails, getEnvelopeEndpointWithUrlEncodedAuth, getStoreEndpointWithUrlEncodedAuth } from './api'; @@ -58,11 +58,52 @@ export function eventToSentryRequest(event: Event, api: APIDetails): SentryReque 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 and + // https://github.com/getsentry/sentry-javascript/pull/4425. 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 `skippedNormalization` and pull out the companion hack in the browser playwright tests + enhanceEventWithSdkInfo(event, api.metadata.sdk); + event.tags = event.tags || {}; + event.extra = event.extra || {}; + // prevent this data from being sent to sentry + // TODO: This is NOT part of the hack - DO NOT DELETE delete event.sdkProcessingMetadata; + let body; + try { + // 99.9% of events should get through just fine - no change in behavior for them + body = JSON.stringify(event); + } catch (err) { + // Record data about the error without replacing original event data, then force renormalization + event.tags.JSONStringifyError = true; + event.extra.JSONStringifyError = err; + try { + body = JSON.stringify(normalize(event)); + } catch (newErr) { + // At this point even renormalization hasn't worked, meaning something about the event data has gone very wrong. + // Time to cut our losses and record only the new error. With luck, even in the problematic cases we're trying to + // debug with this hack, we won't ever land here. + const innerErr = newErr as Error; + body = JSON.stringify({ + message: 'JSON.stringify error after renormalization', + // setting `extra: { innerErr }` here for some reason results in an empty object, so unpack manually + extra: { message: innerErr.message, stack: innerErr.stack }, + }); + } + } + const req: SentryRequest = { - body: JSON.stringify(sdkInfo ? enhanceEventWithSdkInfo(event, api.metadata.sdk) : event), + // this is the relevant line of code before the hack was added, to make it easy to undo said hack once we've solved + // the mystery + // body: JSON.stringify(sdkInfo ? enhanceEventWithSdkInfo(event, api.metadata.sdk) : event), + body, type: eventType, url: useEnvelope ? getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel) From f24334db1713bf0c602109fa5771a2d03862a1b1 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 21 Jan 2022 15:18:20 -0800 Subject: [PATCH 2/3] tag events which have skipped normalization --- packages/core/src/baseclient.ts | 1 + packages/core/src/request.ts | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 85d192c75e29..5b2cc020e07f 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -428,6 +428,7 @@ export abstract class BaseClient implement normalized.contexts.trace = event.contexts.trace; } + event.sdkProcessingMetadata = { ...event.sdkProcessingMetadata, baseClientNormalized: true }; return normalized; } diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts index 82259147d1f5..cd785260a320 100644 --- a/packages/core/src/request.ts +++ b/packages/core/src/request.ts @@ -72,6 +72,12 @@ export function eventToSentryRequest(event: Event, api: APIDetails): SentryReque 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 + if (!(event.sdkProcessingMetadata && event.sdkProcessingMetadata.baseClientNormalized)) { + event.tags.skippedNormalization = true; + } + // prevent this data from being sent to sentry // TODO: This is NOT part of the hack - DO NOT DELETE delete event.sdkProcessingMetadata; From e9ae54d0f67f7080998a702d059e9edade1ed429 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 21 Jan 2022 15:19:56 -0800 Subject: [PATCH 3/3] fix playwright tests --- packages/integration-tests/utils/helpers.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/integration-tests/utils/helpers.ts b/packages/integration-tests/utils/helpers.ts index e5c7fa32d5a2..90365b11228d 100644 --- a/packages/integration-tests/utils/helpers.ts +++ b/packages/integration-tests/utils/helpers.ts @@ -87,7 +87,24 @@ async function getMultipleRequests( if (urlRgx.test(request.url())) { try { reqCount -= 1; - requestData.push(requestParser(request)); + + // 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 (parsedRequest.tags.skippedNormalization && Object.keys(parsedRequest.tags).length === 1) { + 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)); + if (reqCount === 0) { resolve(requestData); }