From ee697be1ec91220afcb8257b62ae85b99cf0cd9a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 7 Oct 2024 17:11:49 +0200 Subject: [PATCH 1/3] fix(replay): Ensure `replay_id` is removed from frozen DSC --- .../suites/replay/dsc/subject.js | 5 ++ .../suites/replay/dsc/test.ts | 71 ++++++++++++++++++- .../utils/helpers.ts | 12 +++- packages/replay-internal/src/replay.ts | 3 + .../resetReplayIdOnDynamicSamplingContext.ts | 20 ++++++ 5 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/replay/dsc/subject.js create mode 100644 packages/replay-internal/src/util/resetReplayIdOnDynamicSamplingContext.ts diff --git a/dev-packages/browser-integration-tests/suites/replay/dsc/subject.js b/dev-packages/browser-integration-tests/suites/replay/dsc/subject.js new file mode 100644 index 000000000000..01227681eb61 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/replay/dsc/subject.js @@ -0,0 +1,5 @@ +import * as Sentry from '@sentry/browser'; + +window._triggerError = function (errorCount) { + Sentry.captureException(new Error(`This is error #${errorCount}`)); +}; diff --git a/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts b/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts index e46c958497f6..d4721ea9998c 100644 --- a/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts @@ -3,7 +3,12 @@ import type * as Sentry from '@sentry/browser'; import type { EventEnvelopeHeaders } from '@sentry/types'; import { sentryTest } from '../../../utils/fixtures'; -import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../utils/helpers'; +import { + envelopeRequestParser, + shouldSkipTracingTest, + waitForErrorRequest, + waitForTransactionRequest, +} from '../../../utils/helpers'; import { getReplaySnapshot, shouldSkipReplayTest, waitForReplayRunning } from '../../../utils/replayHelpers'; type TestWindow = Window & { @@ -216,3 +221,67 @@ sentryTest( }); }, ); + +sentryTest('should add replay_id to error DSC while replay is active', async ({ getLocalTestPath, page }) => { + if (shouldSkipReplayTest()) { + sentryTest.skip(); + } + + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ id: 'test-id' }), + }); + }); + + const url = await getLocalTestPath({ testDir: __dirname }); + await page.goto(url); + + const error1Req = waitForErrorRequest(page, event => event.exception?.values?.[0].value === 'This is error #1'); + const error2Req = waitForErrorRequest(page, event => event.exception?.values?.[0].value === 'This is error #2'); + + // We want to wait for the transaction to be done, to ensure we have a consistent test + const transactionReq = shouldSkipTracingTest() ? Promise.resolve() : waitForTransactionRequest(page); + + // Wait for this to be available + await page.waitForFunction('!!window.Replay'); + + // We have to start replay before we finish the transaction, otherwise the DSC will not be frozen with the Replay ID + await page.evaluate('window.Replay.start();'); + await waitForReplayRunning(page); + await transactionReq; + + await page.evaluate('window._triggerError(1)'); + + const error1Header = envelopeRequestParser(await error1Req, 0) as EventEnvelopeHeaders; + const replay = await getReplaySnapshot(page); + + expect(replay.session?.id).toBeDefined(); + + expect(error1Header.trace).toBeDefined(); + expect(error1Header.trace).toEqual({ + environment: 'production', + sample_rate: '1', + trace_id: expect.any(String), + public_key: 'public', + replay_id: replay.session?.id, + sampled: 'true', + }); + + // Now end replay and trigger another error, it should not have a replay_id in DSC anymore + await page.evaluate('window.Replay.stop();'); + await page.waitForFunction('!window.Replay.getReplayId();'); + await page.evaluate('window._triggerError(2)'); + + const error2Header = envelopeRequestParser(await error2Req, 0) as EventEnvelopeHeaders; + + expect(error2Header.trace).toBeDefined(); + expect(error2Header.trace).toEqual({ + environment: 'production', + sample_rate: '1', + trace_id: expect.any(String), + public_key: 'public', + sampled: 'true', + }); +}); diff --git a/dev-packages/browser-integration-tests/utils/helpers.ts b/dev-packages/browser-integration-tests/utils/helpers.ts index 898cef6a67dd..3163ef8d6a60 100644 --- a/dev-packages/browser-integration-tests/utils/helpers.ts +++ b/dev-packages/browser-integration-tests/utils/helpers.ts @@ -199,7 +199,7 @@ export async function waitForTransactionRequestOnUrl(page: Page, url: string): P return req; } -export function waitForErrorRequest(page: Page): Promise { +export function waitForErrorRequest(page: Page, callback?: (event: Event) => boolean): Promise { return page.waitForRequest(req => { const postData = req.postData(); if (!postData) { @@ -209,7 +209,15 @@ export function waitForErrorRequest(page: Page): Promise { try { const event = envelopeRequestParser(req); - return !event.type; + if (event.type) { + return false; + } + + if (callback) { + return callback(event); + } + + return true; } catch { return false; } diff --git a/packages/replay-internal/src/replay.ts b/packages/replay-internal/src/replay.ts index ea7df8b7afa7..563938fe4d43 100644 --- a/packages/replay-internal/src/replay.ts +++ b/packages/replay-internal/src/replay.ts @@ -53,6 +53,7 @@ import { debounce } from './util/debounce'; import { getHandleRecordingEmit } from './util/handleRecordingEmit'; import { isExpired } from './util/isExpired'; import { isSessionExpired } from './util/isSessionExpired'; +import { resetReplayIdOnDynamicSamplingContext } from './util/resetReplayIdOnDynamicSamplingContext'; import { sendReplay } from './util/sendReplay'; import { RateLimitError } from './util/sendReplayRequest'; import type { SKIPPED } from './util/throttle'; @@ -446,6 +447,8 @@ export class ReplayContainer implements ReplayContainerInterface { try { DEBUG_BUILD && logger.info(`Stopping Replay${reason ? ` triggered by ${reason}` : ''}`); + resetReplayIdOnDynamicSamplingContext(); + this._removeListeners(); this.stopRecording(); diff --git a/packages/replay-internal/src/util/resetReplayIdOnDynamicSamplingContext.ts b/packages/replay-internal/src/util/resetReplayIdOnDynamicSamplingContext.ts new file mode 100644 index 000000000000..b38de9e3312d --- /dev/null +++ b/packages/replay-internal/src/util/resetReplayIdOnDynamicSamplingContext.ts @@ -0,0 +1,20 @@ +import { getActiveSpan, getCurrentScope, getDynamicSamplingContextFromSpan } from '@sentry/core'; +import type { DynamicSamplingContext } from '@sentry/types'; + +/** + * Reset the `replay_id` field on the DSC. + */ +export function resetReplayIdOnDynamicSamplingContext(): void { + // Reset DSC on the current scope, if there is one + const dsc = getCurrentScope().getPropagationContext().dsc; + if (dsc) { + delete dsc.replay_id; + } + + // Clear it from frozen DSC on the active span + const activeSpan = getActiveSpan(); + if (activeSpan) { + const dsc = getDynamicSamplingContextFromSpan(activeSpan); + delete (dsc as Partial).replay_id; + } +} From 99d0bda2918a2664745e0cfd8d5f140f56eada64 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 8 Oct 2024 13:03:02 +0200 Subject: [PATCH 2/3] fix & increase size limit --- .size-limit.js | 2 +- .../suites/replay/dsc/test.ts | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/.size-limit.js b/.size-limit.js index 5da293511976..8a7670e6d638 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -180,7 +180,7 @@ module.exports = [ name: 'CDN Bundle (incl. Tracing, Replay)', path: createCDNPath('bundle.tracing.replay.min.js'), gzip: true, - limit: '73 KB', + limit: '74 KB', }, { name: 'CDN Bundle (incl. Tracing, Replay, Feedback)', diff --git a/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts b/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts index d4721ea9998c..be3a900edea2 100644 --- a/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts @@ -227,6 +227,8 @@ sentryTest('should add replay_id to error DSC while replay is active', async ({ sentryTest.skip(); } + const hasTracing = !shouldSkipTracingTest(); + await page.route('https://dsn.ingest.sentry.io/**/*', route => { return route.fulfill({ status: 200, @@ -242,7 +244,7 @@ sentryTest('should add replay_id to error DSC while replay is active', async ({ const error2Req = waitForErrorRequest(page, event => event.exception?.values?.[0].value === 'This is error #2'); // We want to wait for the transaction to be done, to ensure we have a consistent test - const transactionReq = shouldSkipTracingTest() ? Promise.resolve() : waitForTransactionRequest(page); + const transactionReq = hasTracing ? waitForTransactionRequest(page) : Promise.resolve(); // Wait for this to be available await page.waitForFunction('!!window.Replay'); @@ -262,11 +264,15 @@ sentryTest('should add replay_id to error DSC while replay is active', async ({ expect(error1Header.trace).toBeDefined(); expect(error1Header.trace).toEqual({ environment: 'production', - sample_rate: '1', trace_id: expect.any(String), public_key: 'public', replay_id: replay.session?.id, - sampled: 'true', + ...(hasTracing + ? { + sample_rate: '1', + sampled: 'true', + } + : {}), }); // Now end replay and trigger another error, it should not have a replay_id in DSC anymore From 5462c2b4f107413d4a78599588009dcc0570433f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 8 Oct 2024 13:49:40 +0200 Subject: [PATCH 3/3] fix more test --- .../browser-integration-tests/suites/replay/dsc/test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts b/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts index be3a900edea2..b6bb4da00abb 100644 --- a/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/dsc/test.ts @@ -285,9 +285,13 @@ sentryTest('should add replay_id to error DSC while replay is active', async ({ expect(error2Header.trace).toBeDefined(); expect(error2Header.trace).toEqual({ environment: 'production', - sample_rate: '1', trace_id: expect.any(String), public_key: 'public', - sampled: 'true', + ...(hasTracing + ? { + sample_rate: '1', + sampled: 'true', + } + : {}), }); });