From 5641265022590e1f72a806e293ba5b2df121b16e Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 4 Apr 2023 14:50:54 -0400 Subject: [PATCH 01/12] feat(replay): Change `stop()` to flush and remove current session `stop()` will now flush the eventBuffer before clearing it, as well as removing the session from Session Storage. Due to the flushing, `stop()` is now async. Ref: https://github.com/getsentry/sentry-javascript/issues/7738 --- packages/replay/src/integration.ts | 4 +- packages/replay/src/replay.ts | 31 +++++++++++++-- .../utils => src/session}/clearSession.ts | 0 packages/replay/src/util/addEvent.ts | 2 +- .../test/integration/errorSampleRate.test.ts | 2 +- .../replay/test/integration/events.test.ts | 2 +- .../replay/test/integration/flush.test.ts | 2 +- .../test/integration/rateLimiting.test.ts | 5 +-- .../test/integration/sendReplayEvent.test.ts | 11 ++---- .../replay/test/integration/session.test.ts | 2 +- packages/replay/test/integration/stop.test.ts | 39 ++++++++++++++----- .../replay/test/utils/setupReplayContainer.ts | 2 +- 12 files changed, 69 insertions(+), 33 deletions(-) rename packages/replay/{test/utils => src/session}/clearSession.ts (100%) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index c08eb0431cac..da966a29eefc 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -207,12 +207,12 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, * Currently, this needs to be manually called (e.g. for tests). Sentry SDK * does not support a teardown */ - public stop(): void { + public async stop(): Promise { if (!this._replay) { return; } - this._replay.stop(); + return this._replay.stop(); } /** diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 8bb426874442..6e4ef2df682a 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -7,6 +7,7 @@ import { logger } from '@sentry/utils'; import { ERROR_CHECKOUT_TIME, MAX_SESSION_LIFE, SESSION_IDLE_DURATION, WINDOW } from './constants'; import { setupPerformanceObserver } from './coreHandlers/performanceObserver'; import { createEventBuffer } from './eventBuffer'; +import { clearSession } from './session/clearSession'; import { getSession } from './session/getSession'; import { saveSession } from './session/saveSession'; import type { @@ -85,6 +86,11 @@ export class ReplayContainer implements ReplayContainerInterface { */ private _isEnabled: boolean = false; + /** + * If true, will flush regardless of `_isEnabled` property + */ + private _shouldFinalFlush: boolean = false; + /** * Paused is a state where: * - DOM Recording is not listening at all @@ -237,7 +243,7 @@ export class ReplayContainer implements ReplayContainerInterface { * Currently, this needs to be manually called (e.g. for tests). Sentry SDK * does not support a teardown */ - public stop(reason?: string): void { + public async stop(reason?: string): Promise { if (!this._isEnabled) { return; } @@ -253,14 +259,31 @@ export class ReplayContainer implements ReplayContainerInterface { log(msg); } + // Set this property so that it ignores `_isEnabled` = false + // We can't move `_isEnabled` after awaiting a flush, otherwise we can + // enter into an infinite loop when `stop()` is called while flushing. + this._shouldFinalFlush = true; this._isEnabled = false; this._removeListeners(); this.stopRecording(); + + // Flush event buffer before stopping + await this.flushImmediate() + + this._shouldFinalFlush = false; + + // After flush, destroy event buffer this.eventBuffer && this.eventBuffer.destroy(); this.eventBuffer = null; - this._debouncedFlush.cancel(); + + // Clear session from session storage, note this means if a new session + // is started after, it will not have `previousSessionId` + clearSession(this); + } catch (err) { this._handleException(err); + } finally { + this._shouldFinalFlush = false; } } @@ -760,7 +783,7 @@ export class ReplayContainer implements ReplayContainerInterface { // This means we retried 3 times and all of them failed, // or we ran into a problem we don't want to retry, like rate limiting. // In this case, we want to completely stop the replay - otherwise, we may get inconsistent segments - this.stop('sendReplay'); + void this.stop('sendReplay'); const client = getCurrentHub().getClient(); @@ -775,7 +798,7 @@ export class ReplayContainer implements ReplayContainerInterface { * can be active at a time. Do not call this directly. */ private _flush: () => Promise = async () => { - if (!this._isEnabled) { + if (!this._isEnabled && !this._shouldFinalFlush) { // This can happen if e.g. the replay was stopped because of exceeding the retry limit return; } diff --git a/packages/replay/test/utils/clearSession.ts b/packages/replay/src/session/clearSession.ts similarity index 100% rename from packages/replay/test/utils/clearSession.ts rename to packages/replay/src/session/clearSession.ts diff --git a/packages/replay/src/util/addEvent.ts b/packages/replay/src/util/addEvent.ts index b32050665519..17e933557d83 100644 --- a/packages/replay/src/util/addEvent.ts +++ b/packages/replay/src/util/addEvent.ts @@ -46,7 +46,7 @@ export async function addEvent( return await replay.eventBuffer.addEvent(event, isCheckout); } catch (error) { __DEBUG_BUILD__ && logger.error(error); - replay.stop('addEvent'); + await replay.stop('addEvent'); const client = getCurrentHub().getClient(); diff --git a/packages/replay/test/integration/errorSampleRate.test.ts b/packages/replay/test/integration/errorSampleRate.test.ts index 85f154e523f4..3690da1126a8 100644 --- a/packages/replay/test/integration/errorSampleRate.test.ts +++ b/packages/replay/test/integration/errorSampleRate.test.ts @@ -15,8 +15,8 @@ import type { RecordMock } from '../index'; import { BASE_TIMESTAMP } from '../index'; import { resetSdkMock } from '../mocks/resetSdkMock'; import type { DomHandler } from '../types'; -import { clearSession } from '../utils/clearSession'; import { useFakeTimers } from '../utils/use-fake-timers'; +import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/events.test.ts b/packages/replay/test/integration/events.test.ts index 57c49ba1e245..edd618deb9fe 100644 --- a/packages/replay/test/integration/events.test.ts +++ b/packages/replay/test/integration/events.test.ts @@ -7,8 +7,8 @@ import { PerformanceEntryResource } from '../fixtures/performanceEntry/resource' import type { RecordMock } from '../index'; import { BASE_TIMESTAMP } from '../index'; import { resetSdkMock } from '../mocks/resetSdkMock'; -import { clearSession } from '../utils/clearSession'; import { useFakeTimers } from '../utils/use-fake-timers'; +import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index 5d91edf483a3..a696e926d74d 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -8,8 +8,8 @@ import { createPerformanceEntries } from '../../src/util/createPerformanceEntrie import { createPerformanceSpans } from '../../src/util/createPerformanceSpans'; import * as SendReplay from '../../src/util/sendReplay'; import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; -import { clearSession } from '../utils/clearSession'; import { useFakeTimers } from '../utils/use-fake-timers'; +import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/rateLimiting.test.ts b/packages/replay/test/integration/rateLimiting.test.ts index fcd170b31784..ea8122d6c55e 100644 --- a/packages/replay/test/integration/rateLimiting.test.ts +++ b/packages/replay/test/integration/rateLimiting.test.ts @@ -6,8 +6,8 @@ import type { ReplayContainer } from '../../src/replay'; import * as SendReplayRequest from '../../src/util/sendReplayRequest'; import { BASE_TIMESTAMP, mockSdk } from '../index'; import { mockRrweb } from '../mocks/mockRrweb'; -import { clearSession } from '../utils/clearSession'; import { useFakeTimers } from '../utils/use-fake-timers'; +import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); @@ -86,8 +86,7 @@ describe('Integration | rate-limiting behaviour', () => { expect(replay.stop).toHaveBeenCalledTimes(1); // No user activity to trigger an update - expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP); - expect(replay.session?.segmentId).toBe(1); + expect(replay.session).toBe(undefined); // let's simulate the default rate-limit time of inactivity (60secs) and check that we // don't do anything in the meantime or after the time has passed diff --git a/packages/replay/test/integration/sendReplayEvent.test.ts b/packages/replay/test/integration/sendReplayEvent.test.ts index 867499890bb7..831e16dd146d 100644 --- a/packages/replay/test/integration/sendReplayEvent.test.ts +++ b/packages/replay/test/integration/sendReplayEvent.test.ts @@ -7,8 +7,8 @@ import type { ReplayContainer } from '../../src/replay'; import { addEvent } from '../../src/util/addEvent'; import * as SendReplayRequest from '../../src/util/sendReplayRequest'; import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; -import { clearSession } from '../utils/clearSession'; import { useFakeTimers } from '../utils/use-fake-timers'; +import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); @@ -396,13 +396,8 @@ describe('Integration | sendReplayEvent', () => { 'Something bad happened', ); - // No activity has occurred, session's last activity should remain the same - expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP); - - // segmentId increases despite error - expect(replay.session?.segmentId).toBe(1); - - // Replay should be completely stopped now + // Replay has stopped, no session should exist + expect(replay.session).toBe(undefined); expect(replay.isEnabled()).toBe(false); // Events are ignored now, because we stopped diff --git a/packages/replay/test/integration/session.test.ts b/packages/replay/test/integration/session.test.ts index 4720e7b65bc6..1bbeffc24000 100644 --- a/packages/replay/test/integration/session.test.ts +++ b/packages/replay/test/integration/session.test.ts @@ -15,8 +15,8 @@ import { createPerformanceSpans } from '../../src/util/createPerformanceSpans'; import { BASE_TIMESTAMP } from '../index'; import type { RecordMock } from '../mocks/mockRrweb'; import { resetSdkMock } from '../mocks/resetSdkMock'; -import { clearSession } from '../utils/clearSession'; import { useFakeTimers } from '../utils/use-fake-timers'; +import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/stop.test.ts b/packages/replay/test/integration/stop.test.ts index 05772ecab6c7..c6c245714b66 100644 --- a/packages/replay/test/integration/stop.test.ts +++ b/packages/replay/test/integration/stop.test.ts @@ -6,11 +6,13 @@ import type { ReplayContainer } from '../../src/replay'; import { addEvent } from '../../src/util/addEvent'; // mock functions need to be imported first import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; -import { clearSession } from '../utils/clearSession'; import { useFakeTimers } from '../utils/use-fake-timers'; +import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); +type MockRunFlush = jest.MockedFunction; + describe('Integration | stop', () => { let replay: ReplayContainer; let integration: Replay; @@ -20,6 +22,7 @@ describe('Integration | stop', () => { const { record: mockRecord } = mockRrweb(); let mockAddInstrumentationHandler: MockAddInstrumentationHandler; + let mockRunFlush: MockRunFlush; beforeAll(async () => { jest.setSystemTime(new Date(BASE_TIMESTAMP)); @@ -29,6 +32,10 @@ describe('Integration | stop', () => { ) as MockAddInstrumentationHandler; ({ replay, integration } = await mockSdk()); + + // @ts-ignore private API + mockRunFlush = jest.spyOn(replay, '_runFlush'); + jest.runAllTimers(); }); @@ -68,9 +75,10 @@ describe('Integration | stop', () => { // Not sure where the 20ms comes from tbh const EXTRA_TICKS = 20; const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const previousSessionId = replay.session?.id; // stop replays - integration.stop(); + await integration.stop(); // Pretend 5 seconds have passed jest.advanceTimersByTime(ELAPSED); @@ -80,14 +88,17 @@ describe('Integration | stop', () => { await new Promise(process.nextTick); expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); expect(replay).not.toHaveLastSentReplay(); - // Session's last activity should not be updated - expect(replay.session?.lastActivity).toEqual(BASE_TIMESTAMP); + // Session's does not exist + expect(replay.session).toEqual(undefined); // eventBuffer is destroyed expect(replay.eventBuffer).toBe(null); // re-enable replay integration.start(); + // will be different session + expect(replay.session?.id).not.toEqual(previousSessionId) + jest.advanceTimersByTime(ELAPSED); const timestamp = +new Date(BASE_TIMESTAMP + ELAPSED + ELAPSED + EXTRA_TICKS) / 1000; @@ -126,12 +137,16 @@ describe('Integration | stop', () => { expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP + ELAPSED + 20); }); - it('does not buffer events when stopped', async function () { - WINDOW.dispatchEvent(new Event('blur')); + it('does not buffer new events after being stopped', async function () { + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + addEvent(replay, TEST_EVENT); expect(replay.eventBuffer?.hasEvents).toBe(true); + expect(mockRunFlush).toHaveBeenCalledTimes(0); // stop replays - integration.stop(); + await integration.stop(); + + expect(mockRunFlush).toHaveBeenCalledTimes(1); expect(replay.eventBuffer).toBe(null); @@ -139,14 +154,18 @@ describe('Integration | stop', () => { await new Promise(process.nextTick); expect(replay.eventBuffer).toBe(null); - expect(replay).not.toHaveLastSentReplay(); + expect(replay).toHaveLastSentReplay({ + recordingData: JSON.stringify([ + TEST_EVENT, + ]), + }); }); it('does not call core SDK `addInstrumentationHandler` after initial setup', async function () { // NOTE: We clear addInstrumentationHandler mock after every test - integration.stop(); + await integration.stop(); integration.start(); - integration.stop(); + await integration.stop(); integration.start(); expect(mockAddInstrumentationHandler).not.toHaveBeenCalled(); diff --git a/packages/replay/test/utils/setupReplayContainer.ts b/packages/replay/test/utils/setupReplayContainer.ts index e6a427e19638..d61ca6237730 100644 --- a/packages/replay/test/utils/setupReplayContainer.ts +++ b/packages/replay/test/utils/setupReplayContainer.ts @@ -1,7 +1,7 @@ import { createEventBuffer } from '../../src/eventBuffer'; import { ReplayContainer } from '../../src/replay'; import type { RecordingOptions, ReplayPluginOptions } from '../../src/types'; -import { clearSession } from './clearSession'; +import { clearSession } from '../../src/session/clearSession'; export function setupReplayContainer({ options, From 14b4b5d715ca7dd247ffc21c675216729eba4ffd Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 4 Apr 2023 16:13:50 -0400 Subject: [PATCH 02/12] lint --- packages/replay/src/replay.ts | 5 ++--- packages/replay/src/session/clearSession.ts | 5 ++++- packages/replay/test/integration/errorSampleRate.test.ts | 2 +- packages/replay/test/integration/events.test.ts | 2 +- packages/replay/test/integration/flush.test.ts | 2 +- packages/replay/test/integration/rateLimiting.test.ts | 2 +- packages/replay/test/integration/sendReplayEvent.test.ts | 2 +- packages/replay/test/integration/session.test.ts | 2 +- packages/replay/test/integration/stop.test.ts | 8 +++----- packages/replay/test/utils/setupReplayContainer.ts | 2 +- 10 files changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 6e4ef2df682a..18c4c9b80295 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -268,7 +268,7 @@ export class ReplayContainer implements ReplayContainerInterface { this.stopRecording(); // Flush event buffer before stopping - await this.flushImmediate() + await this.flushImmediate(); this._shouldFinalFlush = false; @@ -279,7 +279,6 @@ export class ReplayContainer implements ReplayContainerInterface { // Clear session from session storage, note this means if a new session // is started after, it will not have `previousSessionId` clearSession(this); - } catch (err) { this._handleException(err); } finally { @@ -489,7 +488,7 @@ export class ReplayContainer implements ReplayContainerInterface { this.session = session; if (!this.session.sampled) { - this.stop('session unsampled'); + void this.stop('session unsampled'); return false; } diff --git a/packages/replay/src/session/clearSession.ts b/packages/replay/src/session/clearSession.ts index b5b64ac04531..2cb092638d51 100644 --- a/packages/replay/src/session/clearSession.ts +++ b/packages/replay/src/session/clearSession.ts @@ -1,7 +1,10 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../../src/constants'; import type { ReplayContainer } from '../../src/types'; -export function clearSession(replay: ReplayContainer) { +/** + * + */ +export function clearSession(replay: ReplayContainer): void { deleteSession(); replay.session = undefined; } diff --git a/packages/replay/test/integration/errorSampleRate.test.ts b/packages/replay/test/integration/errorSampleRate.test.ts index 3690da1126a8..b713fe36b17c 100644 --- a/packages/replay/test/integration/errorSampleRate.test.ts +++ b/packages/replay/test/integration/errorSampleRate.test.ts @@ -9,6 +9,7 @@ import { WINDOW, } from '../../src/constants'; import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; import { addEvent } from '../../src/util/addEvent'; import { PerformanceEntryResource } from '../fixtures/performanceEntry/resource'; import type { RecordMock } from '../index'; @@ -16,7 +17,6 @@ import { BASE_TIMESTAMP } from '../index'; import { resetSdkMock } from '../mocks/resetSdkMock'; import type { DomHandler } from '../types'; import { useFakeTimers } from '../utils/use-fake-timers'; -import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/events.test.ts b/packages/replay/test/integration/events.test.ts index edd618deb9fe..4fffc5fadbfa 100644 --- a/packages/replay/test/integration/events.test.ts +++ b/packages/replay/test/integration/events.test.ts @@ -2,13 +2,13 @@ import { getCurrentHub } from '@sentry/core'; import { WINDOW } from '../../src/constants'; import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; import { addEvent } from '../../src/util/addEvent'; import { PerformanceEntryResource } from '../fixtures/performanceEntry/resource'; import type { RecordMock } from '../index'; import { BASE_TIMESTAMP } from '../index'; import { resetSdkMock } from '../mocks/resetSdkMock'; import { useFakeTimers } from '../utils/use-fake-timers'; -import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index a696e926d74d..9b11a414d4b5 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -2,6 +2,7 @@ import * as SentryUtils from '@sentry/utils'; import { DEFAULT_FLUSH_MIN_DELAY, WINDOW } from '../../src/constants'; import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; import type { EventBuffer } from '../../src/types'; import * as AddMemoryEntry from '../../src/util/addMemoryEntry'; import { createPerformanceEntries } from '../../src/util/createPerformanceEntries'; @@ -9,7 +10,6 @@ import { createPerformanceSpans } from '../../src/util/createPerformanceSpans'; import * as SendReplay from '../../src/util/sendReplay'; import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; import { useFakeTimers } from '../utils/use-fake-timers'; -import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/rateLimiting.test.ts b/packages/replay/test/integration/rateLimiting.test.ts index ea8122d6c55e..723dc682d100 100644 --- a/packages/replay/test/integration/rateLimiting.test.ts +++ b/packages/replay/test/integration/rateLimiting.test.ts @@ -3,11 +3,11 @@ import type { Transport } from '@sentry/types'; import { DEFAULT_FLUSH_MIN_DELAY } from '../../src/constants'; import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; import * as SendReplayRequest from '../../src/util/sendReplayRequest'; import { BASE_TIMESTAMP, mockSdk } from '../index'; import { mockRrweb } from '../mocks/mockRrweb'; import { useFakeTimers } from '../utils/use-fake-timers'; -import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/sendReplayEvent.test.ts b/packages/replay/test/integration/sendReplayEvent.test.ts index 831e16dd146d..d7a9974bcaa9 100644 --- a/packages/replay/test/integration/sendReplayEvent.test.ts +++ b/packages/replay/test/integration/sendReplayEvent.test.ts @@ -4,11 +4,11 @@ import * as SentryUtils from '@sentry/utils'; import { DEFAULT_FLUSH_MIN_DELAY, WINDOW } from '../../src/constants'; import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; import { addEvent } from '../../src/util/addEvent'; import * as SendReplayRequest from '../../src/util/sendReplayRequest'; import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; import { useFakeTimers } from '../utils/use-fake-timers'; -import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/session.test.ts b/packages/replay/test/integration/session.test.ts index 1bbeffc24000..0dede22edfca 100644 --- a/packages/replay/test/integration/session.test.ts +++ b/packages/replay/test/integration/session.test.ts @@ -9,6 +9,7 @@ import { WINDOW, } from '../../src/constants'; import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; import type { Session } from '../../src/types'; import { addEvent } from '../../src/util/addEvent'; import { createPerformanceSpans } from '../../src/util/createPerformanceSpans'; @@ -16,7 +17,6 @@ import { BASE_TIMESTAMP } from '../index'; import type { RecordMock } from '../mocks/mockRrweb'; import { resetSdkMock } from '../mocks/resetSdkMock'; import { useFakeTimers } from '../utils/use-fake-timers'; -import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); diff --git a/packages/replay/test/integration/stop.test.ts b/packages/replay/test/integration/stop.test.ts index c6c245714b66..a477ee8e044f 100644 --- a/packages/replay/test/integration/stop.test.ts +++ b/packages/replay/test/integration/stop.test.ts @@ -3,11 +3,11 @@ import * as SentryUtils from '@sentry/utils'; import type { Replay } from '../../src'; import { WINDOW } from '../../src/constants'; import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; import { addEvent } from '../../src/util/addEvent'; // mock functions need to be imported first import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; import { useFakeTimers } from '../utils/use-fake-timers'; -import { clearSession } from '../../src/session/clearSession'; useFakeTimers(); @@ -97,7 +97,7 @@ describe('Integration | stop', () => { integration.start(); // will be different session - expect(replay.session?.id).not.toEqual(previousSessionId) + expect(replay.session?.id).not.toEqual(previousSessionId); jest.advanceTimersByTime(ELAPSED); @@ -155,9 +155,7 @@ describe('Integration | stop', () => { expect(replay.eventBuffer).toBe(null); expect(replay).toHaveLastSentReplay({ - recordingData: JSON.stringify([ - TEST_EVENT, - ]), + recordingData: JSON.stringify([TEST_EVENT]), }); }); diff --git a/packages/replay/test/utils/setupReplayContainer.ts b/packages/replay/test/utils/setupReplayContainer.ts index d61ca6237730..fbc9ebd594b8 100644 --- a/packages/replay/test/utils/setupReplayContainer.ts +++ b/packages/replay/test/utils/setupReplayContainer.ts @@ -1,7 +1,7 @@ import { createEventBuffer } from '../../src/eventBuffer'; import { ReplayContainer } from '../../src/replay'; -import type { RecordingOptions, ReplayPluginOptions } from '../../src/types'; import { clearSession } from '../../src/session/clearSession'; +import type { RecordingOptions, ReplayPluginOptions } from '../../src/types'; export function setupReplayContainer({ options, From cd3c1d69631fae963928d4a23acd83949901fa25 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 5 Apr 2023 10:41:39 -0400 Subject: [PATCH 03/12] remove async --- packages/replay/src/integration.ts | 2 +- packages/replay/src/types.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index da966a29eefc..3a19bc883633 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -207,7 +207,7 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, * Currently, this needs to be manually called (e.g. for tests). Sentry SDK * does not support a teardown */ - public async stop(): Promise { + public stop(): Promise | void { if (!this._replay) { return; } diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index f6d4566d2d7c..af355a3426d3 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -437,7 +437,7 @@ export interface ReplayContainer { isPaused(): boolean; getContext(): InternalEventContext; start(): void; - stop(reason?: string): void; + stop(reason?: string): Promise; pause(): void; resume(): void; startRecording(): void; From ac8a4b6a38cd216a14826b45b0372469c21e3b2e Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 5 Apr 2023 10:42:27 -0400 Subject: [PATCH 04/12] remove redundant flag change --- packages/replay/src/replay.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 18c4c9b80295..89985fd09933 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -270,8 +270,6 @@ export class ReplayContainer implements ReplayContainerInterface { // Flush event buffer before stopping await this.flushImmediate(); - this._shouldFinalFlush = false; - // After flush, destroy event buffer this.eventBuffer && this.eventBuffer.destroy(); this.eventBuffer = null; From 3fab0b266a872a4bb341dee9d5b5bad9eee2f7d4 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 5 Apr 2023 10:43:57 -0400 Subject: [PATCH 05/12] Update packages/replay/src/session/clearSession.ts --- packages/replay/src/session/clearSession.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/replay/src/session/clearSession.ts b/packages/replay/src/session/clearSession.ts index 2cb092638d51..d084764c2fb9 100644 --- a/packages/replay/src/session/clearSession.ts +++ b/packages/replay/src/session/clearSession.ts @@ -2,7 +2,7 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../../src/constants'; import type { ReplayContainer } from '../../src/types'; /** - * + * Removes the session from Session Storage and unsets session in replay instance */ export function clearSession(replay: ReplayContainer): void { deleteSession(); From 45996ed47376ab17542172ee3d04c253a4b703a9 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Fri, 14 Apr 2023 17:21:54 -0400 Subject: [PATCH 06/12] return promise --- packages/replay/src/integration.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index 3a19bc883633..b77505854b73 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -207,9 +207,9 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, * Currently, this needs to be manually called (e.g. for tests). Sentry SDK * does not support a teardown */ - public stop(): Promise | void { + public stop(): Promise { if (!this._replay) { - return; + return Promise.resolve(); } return this._replay.stop(); @@ -218,9 +218,9 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, /** * Immediately send all pending events. */ - public flush(): Promise | void { + public flush(): Promise { if (!this._replay || !this._replay.isEnabled()) { - return; + return Promise.resolve(); } return this._replay.flushImmediate(); From 756f7165e4ac5afc5881b59579124fabc1c87816 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 24 Apr 2023 13:43:55 +0200 Subject: [PATCH 07/12] remove shouldFinalFlush --- packages/replay/src/replay.ts | 20 +++++++------------- packages/replay/src/types.ts | 9 +++++++++ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 89985fd09933..073acfb59dd5 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -19,6 +19,7 @@ import type { PopEventContext, RecordingOptions, ReplayContainer as ReplayContainerInterface, + ReplayFlushOptions, ReplayPluginOptions, Session, Timeouts, @@ -86,11 +87,6 @@ export class ReplayContainer implements ReplayContainerInterface { */ private _isEnabled: boolean = false; - /** - * If true, will flush regardless of `_isEnabled` property - */ - private _shouldFinalFlush: boolean = false; - /** * Paused is a state where: * - DOM Recording is not listening at all @@ -259,16 +255,16 @@ export class ReplayContainer implements ReplayContainerInterface { log(msg); } - // Set this property so that it ignores `_isEnabled` = false // We can't move `_isEnabled` after awaiting a flush, otherwise we can // enter into an infinite loop when `stop()` is called while flushing. - this._shouldFinalFlush = true; this._isEnabled = false; this._removeListeners(); this.stopRecording(); - // Flush event buffer before stopping - await this.flushImmediate(); + this._debouncedFlush.cancel(); + // See comment above re: `_isEnabled`, we "force" a flush, ignoring the + // `_isEnabled` state of the plugin since it was disabled above. + await this._flush({force: true}) // After flush, destroy event buffer this.eventBuffer && this.eventBuffer.destroy(); @@ -279,8 +275,6 @@ export class ReplayContainer implements ReplayContainerInterface { clearSession(this); } catch (err) { this._handleException(err); - } finally { - this._shouldFinalFlush = false; } } @@ -794,8 +788,8 @@ export class ReplayContainer implements ReplayContainerInterface { * Flush recording data to Sentry. Creates a lock so that only a single flush * can be active at a time. Do not call this directly. */ - private _flush: () => Promise = async () => { - if (!this._isEnabled && !this._shouldFinalFlush) { + private _flush = async ({force = false}: ReplayFlushOptions = {}): Promise => { + if (!this._isEnabled && !force) { // This can happen if e.g. the replay was stopped because of exceeding the retry limit return; } diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index af355a3426d3..9a83cf0d29f4 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -451,6 +451,15 @@ export interface ReplayContainer { setInitialState(): void; } +export interface ReplayFlushOptions { + /** + * If true, flush while ignoring the `_isEnabled` state of +* Replay integration. (By default, flush is noop if integration +* is stopped). + */ + force?: boolean; +} + export interface ReplayPerformanceEntry { /** * One of these types https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry/entryType From aa34bc5313c70aa3013be3a53e00085ae2ff6fd3 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 24 Apr 2023 13:46:52 +0200 Subject: [PATCH 08/12] prettier --- packages/replay/src/replay.ts | 4 ++-- packages/replay/src/types.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 409adf6af2e1..750bdf228cce 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -278,7 +278,7 @@ export class ReplayContainer implements ReplayContainerInterface { this._debouncedFlush.cancel(); // See comment above re: `_isEnabled`, we "force" a flush, ignoring the // `_isEnabled` state of the plugin since it was disabled above. - await this._flush({force: true}) + await this._flush({ force: true }); // After flush, destroy event buffer this.eventBuffer && this.eventBuffer.destroy(); @@ -801,7 +801,7 @@ export class ReplayContainer implements ReplayContainerInterface { * Flush recording data to Sentry. Creates a lock so that only a single flush * can be active at a time. Do not call this directly. */ - private _flush = async ({force = false}: ReplayFlushOptions = {}): Promise => { + private _flush = async ({ force = false }: ReplayFlushOptions = {}): Promise => { if (!this._isEnabled && !force) { // This can happen if e.g. the replay was stopped because of exceeding the retry limit return; diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index af0034d479c5..d463726bb11f 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -477,8 +477,8 @@ export interface ReplayContainer { export interface ReplayFlushOptions { /** * If true, flush while ignoring the `_isEnabled` state of -* Replay integration. (By default, flush is noop if integration -* is stopped). + * Replay integration. (By default, flush is noop if integration + * is stopped). */ force?: boolean; } From b43de34085446d4f0a65df614f1cf2f9b199b344 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 24 Apr 2023 14:10:19 +0200 Subject: [PATCH 09/12] await --- packages/browser-integration-tests/suites/replay/dsc/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/dsc/test.ts b/packages/browser-integration-tests/suites/replay/dsc/test.ts index 83e95d84b9d5..22a6a628cf92 100644 --- a/packages/browser-integration-tests/suites/replay/dsc/test.ts +++ b/packages/browser-integration-tests/suites/replay/dsc/test.ts @@ -57,8 +57,8 @@ sentryTest( const url = await getLocalTestPath({ testDir: __dirname }); await page.goto(url); - await page.evaluate(() => { - (window as unknown as TestWindow).Replay.stop(); + await page.evaluate(async () => { + await (window as unknown as TestWindow).Replay.stop(); (window as unknown as TestWindow).Sentry.configureScope(scope => { scope.setUser({ id: 'user123', segment: 'segmentB' }); From b2c8ffcbb034197281a3dd7248a1242b206378ff Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 24 Apr 2023 16:02:18 +0200 Subject: [PATCH 10/12] remove await --- packages/browser-integration-tests/suites/replay/dsc/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/dsc/test.ts b/packages/browser-integration-tests/suites/replay/dsc/test.ts index 22a6a628cf92..bde86c91688a 100644 --- a/packages/browser-integration-tests/suites/replay/dsc/test.ts +++ b/packages/browser-integration-tests/suites/replay/dsc/test.ts @@ -57,8 +57,8 @@ sentryTest( const url = await getLocalTestPath({ testDir: __dirname }); await page.goto(url); - await page.evaluate(async () => { - await (window as unknown as TestWindow).Replay.stop(); + await page.evaluate(() => { + void (window as unknown as TestWindow).Replay.stop(); (window as unknown as TestWindow).Sentry.configureScope(scope => { scope.setUser({ id: 'user123', segment: 'segmentB' }); From 061ead972a9da7e1ef507b32b5771e3fc2942607 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 25 Apr 2023 09:45:58 +0200 Subject: [PATCH 11/12] need to await stop --- .../suites/replay/dsc/test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/dsc/test.ts b/packages/browser-integration-tests/suites/replay/dsc/test.ts index bde86c91688a..810305711a15 100644 --- a/packages/browser-integration-tests/suites/replay/dsc/test.ts +++ b/packages/browser-integration-tests/suites/replay/dsc/test.ts @@ -54,11 +54,19 @@ sentryTest( 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); - await page.evaluate(() => { - void (window as unknown as TestWindow).Replay.stop(); + await page.evaluate(async () => { + await (window as unknown as TestWindow).Replay.stop(); (window as unknown as TestWindow).Sentry.configureScope(scope => { scope.setUser({ id: 'user123', segment: 'segmentB' }); From fd448a6bf48f28aa044cc2ea141d6a3176b02d41 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 25 Apr 2023 11:21:44 +0200 Subject: [PATCH 12/12] inline flush options --- packages/replay/src/replay.ts | 12 ++++++++++-- packages/replay/src/types.ts | 9 --------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 750bdf228cce..36a4cbff833c 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -20,7 +20,6 @@ import type { RecordingOptions, ReplayContainer as ReplayContainerInterface, ReplayExperimentalPluginOptions, - ReplayFlushOptions, ReplayPluginOptions, Session, Timeouts, @@ -801,7 +800,16 @@ export class ReplayContainer implements ReplayContainerInterface { * Flush recording data to Sentry. Creates a lock so that only a single flush * can be active at a time. Do not call this directly. */ - private _flush = async ({ force = false }: ReplayFlushOptions = {}): Promise => { + private _flush = async ({ + force = false, + }: { + /** + * If true, flush while ignoring the `_isEnabled` state of + * Replay integration. (By default, flush is noop if integration + * is stopped). + */ + force?: boolean; + } = {}): Promise => { if (!this._isEnabled && !force) { // This can happen if e.g. the replay was stopped because of exceeding the retry limit return; diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index d463726bb11f..c353c74a7ab4 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -474,15 +474,6 @@ export interface ReplayContainer { setInitialState(): void; } -export interface ReplayFlushOptions { - /** - * If true, flush while ignoring the `_isEnabled` state of - * Replay integration. (By default, flush is noop if integration - * is stopped). - */ - force?: boolean; -} - export interface ReplayPerformanceEntry { /** * One of these types https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry/entryType