From fa46ba075de0c7fb0a3eb3d939386e9775cfc0d0 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 12 Jan 2023 12:40:51 +0100 Subject: [PATCH 1/5] ref(replay): Extract replay sending into functions --- packages/replay/src/constants.ts | 3 + packages/replay/src/replay.ts | 214 +----------------- packages/replay/src/types.ts | 2 + packages/replay/src/util/sendReplay.ts | 74 ++++++ packages/replay/src/util/sendReplayRequest.ts | 114 ++++++++++ .../replay/test/integration/events.test.ts | 8 - .../replay/test/integration/flush.test.ts | 11 +- .../test/integration/sendReplayEvent.test.ts | 29 +-- 8 files changed, 221 insertions(+), 234 deletions(-) create mode 100644 packages/replay/src/util/sendReplay.ts create mode 100644 packages/replay/src/util/sendReplayRequest.ts diff --git a/packages/replay/src/constants.ts b/packages/replay/src/constants.ts index b0176293e13c..187d729cb6f5 100644 --- a/packages/replay/src/constants.ts +++ b/packages/replay/src/constants.ts @@ -33,3 +33,6 @@ export const MASK_ALL_TEXT_SELECTOR = 'body *:not(style), body *:not(script)'; export const DEFAULT_FLUSH_MIN_DELAY = 5_000; export const DEFAULT_FLUSH_MAX_DELAY = 15_000; export const INITIAL_FLUSH_DELAY = 5_000; + +export const RETRY_BASE_INTERVAL = 5000; +export const RETRY_MAX_COUNT = 3; diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 06a8ae58f64a..7fe02a04c80a 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -1,18 +1,10 @@ /* eslint-disable max-lines */ // TODO: We might want to split this file up -import { addGlobalEventProcessor, captureException, getCurrentHub, setContext } from '@sentry/core'; -import type { Breadcrumb, ReplayEvent, ReplayRecordingMode, TransportMakeRequestResponse } from '@sentry/types'; -import type { RateLimits } from '@sentry/utils'; -import { addInstrumentationHandler, disabledUntil, isRateLimited, logger, updateRateLimits } from '@sentry/utils'; +import { addGlobalEventProcessor, captureException, getCurrentHub } from '@sentry/core'; +import type { Breadcrumb, ReplayRecordingMode } from '@sentry/types'; +import { addInstrumentationHandler, logger } from '@sentry/utils'; import { EventType, record } from 'rrweb'; -import { - MAX_SESSION_LIFE, - REPLAY_EVENT_NAME, - SESSION_IDLE_DURATION, - UNABLE_TO_SEND_REPLAY, - VISIBILITY_CHANGE_TIMEOUT, - WINDOW, -} from './constants'; +import { MAX_SESSION_LIFE, SESSION_IDLE_DURATION, VISIBILITY_CHANGE_TIMEOUT, WINDOW } from './constants'; import { breadcrumbHandler } from './coreHandlers/breadcrumbHandler'; import { handleFetchSpanListener } from './coreHandlers/handleFetch'; import { handleGlobalEventListener } from './coreHandlers/handleGlobalEvent'; @@ -34,7 +26,6 @@ import type { RecordingOptions, ReplayContainer as ReplayContainerInterface, ReplayPluginOptions, - SendReplay, Session, } from './types'; import { addEvent } from './util/addEvent'; @@ -42,20 +33,11 @@ import { addMemoryEntry } from './util/addMemoryEntry'; import { createBreadcrumb } from './util/createBreadcrumb'; import { createPerformanceEntries } from './util/createPerformanceEntries'; import { createPerformanceSpans } from './util/createPerformanceSpans'; -import { createRecordingData } from './util/createRecordingData'; -import { createReplayEnvelope } from './util/createReplayEnvelope'; import { debounce } from './util/debounce'; import { isExpired } from './util/isExpired'; import { isSessionExpired } from './util/isSessionExpired'; import { overwriteRecordDroppedEvent, restoreRecordDroppedEvent } from './util/monkeyPatchRecordDroppedEvent'; -import { prepareReplayEvent } from './util/prepareReplayEvent'; - -/** - * Returns true to return control to calling function, otherwise continue with normal batching - */ - -const BASE_RETRY_INTERVAL = 5000; -const MAX_RETRY_COUNT = 3; +import { sendReplay } from './util/sendReplay'; /** * The main replay container class, which holds all the state and methods for recording and sending replays. @@ -86,9 +68,6 @@ export class ReplayContainer implements ReplayContainerInterface { private _performanceObserver: PerformanceObserver | null = null; - private _retryCount: number = 0; - private _retryInterval: number = BASE_RETRY_INTERVAL; - private _debouncedFlush: ReturnType; private _flushLock: Promise | null = null; @@ -837,12 +816,14 @@ export class ReplayContainer implements ReplayContainerInterface { const segmentId = this.session.segmentId++; this._maybeSaveSession(); - await this._sendReplay({ + await sendReplay({ replayId, events: recordingData, segmentId, includeReplayStartTimestamp: segmentId === 0, eventContext, + session: this.session, + options: this.getOptions(), }); } catch (err) { this._handleException(err); @@ -897,185 +878,6 @@ export class ReplayContainer implements ReplayContainerInterface { } }; - /** - * Send replay attachment using `fetch()` - */ - private async _sendReplayRequest({ - events, - replayId, - segmentId: segment_id, - includeReplayStartTimestamp, - eventContext, - timestamp = new Date().getTime(), - }: SendReplay): Promise { - const recordingData = createRecordingData({ - events, - headers: { - segment_id, - }, - }); - - const { urls, errorIds, traceIds, initialTimestamp } = eventContext; - - const hub = getCurrentHub(); - const client = hub.getClient(); - const scope = hub.getScope(); - const transport = client && client.getTransport(); - const dsn = client?.getDsn(); - - if (!client || !scope || !transport || !dsn || !this.session || !this.session.sampled) { - return; - } - - const baseEvent: ReplayEvent = { - // @ts-ignore private api - type: REPLAY_EVENT_NAME, - ...(includeReplayStartTimestamp ? { replay_start_timestamp: initialTimestamp / 1000 } : {}), - timestamp: timestamp / 1000, - error_ids: errorIds, - trace_ids: traceIds, - urls, - replay_id: replayId, - segment_id, - replay_type: this.session.sampled, - }; - - const replayEvent = await prepareReplayEvent({ scope, client, replayId, event: baseEvent }); - - if (!replayEvent) { - // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions - client.recordDroppedEvent('event_processor', 'replay_event', baseEvent); - __DEBUG_BUILD__ && logger.log('An event processor returned `null`, will not send event.'); - return; - } - - replayEvent.tags = { - ...replayEvent.tags, - sessionSampleRate: this._options.sessionSampleRate, - errorSampleRate: this._options.errorSampleRate, - }; - - /* - For reference, the fully built event looks something like this: - { - "type": "replay_event", - "timestamp": 1670837008.634, - "error_ids": [ - "errorId" - ], - "trace_ids": [ - "traceId" - ], - "urls": [ - "https://example.com" - ], - "replay_id": "eventId", - "segment_id": 3, - "replay_type": "error", - "platform": "javascript", - "event_id": "eventId", - "environment": "production", - "sdk": { - "integrations": [ - "BrowserTracing", - "Replay" - ], - "name": "sentry.javascript.browser", - "version": "7.25.0" - }, - "sdkProcessingMetadata": {}, - "tags": { - "sessionSampleRate": 1, - "errorSampleRate": 0, - } - } - */ - - const envelope = createReplayEnvelope(replayEvent, recordingData, dsn, client.getOptions().tunnel); - - try { - const response = await transport.send(envelope); - // TODO (v8): we can remove this guard once transport.send's type signature doesn't include void anymore - if (response) { - this._rateLimits = updateRateLimits(this._rateLimits, response); - if (isRateLimited(this._rateLimits, 'replay')) { - this._handleRateLimit(); - } - } - return response; - } catch { - throw new Error(UNABLE_TO_SEND_REPLAY); - } - } - - /** - * Reset the counter of retries for sending replays. - */ - private _resetRetries(): void { - this._retryCount = 0; - this._retryInterval = BASE_RETRY_INTERVAL; - } - - /** - * Finalize and send the current replay event to Sentry - */ - private async _sendReplay({ - replayId, - events, - segmentId, - includeReplayStartTimestamp, - eventContext, - }: SendReplay): Promise { - // short circuit if there's no events to upload (this shouldn't happen as _runFlush makes this check) - if (!events.length) { - return; - } - - try { - await this._sendReplayRequest({ - events, - replayId, - segmentId, - includeReplayStartTimestamp, - eventContext, - }); - this._resetRetries(); - return true; - } catch (err) { - // Capture error for every failed replay - setContext('Replays', { - _retryCount: this._retryCount, - }); - this._handleException(err); - - // If an error happened here, it's likely that uploading the attachment - // failed, we'll can retry with the same events payload - if (this._retryCount >= MAX_RETRY_COUNT) { - throw new Error(`${UNABLE_TO_SEND_REPLAY} - max retries exceeded`); - } - - // will retry in intervals of 5, 10, 30 - this._retryInterval = ++this._retryCount * this._retryInterval; - - return await new Promise((resolve, reject) => { - setTimeout(async () => { - try { - await this._sendReplay({ - replayId, - events, - segmentId, - includeReplayStartTimestamp, - eventContext, - }); - resolve(true); - } catch (err) { - reject(err); - } - }, this._retryInterval); - }); - } - } - /** Save the session, if it is sticky */ private _maybeSaveSession(): void { if (this.session && this._options.stickySession) { diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index 21a528575380..ac7c411919a5 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -16,6 +16,8 @@ export interface SendReplay { includeReplayStartTimestamp: boolean; eventContext: PopEventContext; timestamp?: number; + session: Session; + options: ReplayPluginOptions; } export type InstrumentationTypeBreadcrumb = 'dom' | 'scope'; diff --git a/packages/replay/src/util/sendReplay.ts b/packages/replay/src/util/sendReplay.ts new file mode 100644 index 000000000000..7ced6f7488f2 --- /dev/null +++ b/packages/replay/src/util/sendReplay.ts @@ -0,0 +1,74 @@ +import { captureException, setContext } from '@sentry/core'; + +import { RETRY_BASE_INTERVAL, RETRY_MAX_COUNT, UNABLE_TO_SEND_REPLAY } from '../constants'; +import type { SendReplay } from '../types'; +import { sendReplayRequest } from './sendReplayRequest'; + +/** + * Finalize and send the current replay event to Sentry + */ +export async function sendReplay( + { replayId, events, segmentId, includeReplayStartTimestamp, eventContext, session, options }: SendReplay, + retryConfig = { + count: 0, + interval: RETRY_BASE_INTERVAL, + }, +): Promise { + // short circuit if there's no events to upload (this shouldn't happen as _runFlush makes this check) + if (!events.length) { + return; + } + + try { + await sendReplayRequest({ + events, + replayId, + segmentId, + includeReplayStartTimestamp, + eventContext, + session, + options, + }); + return true; + } catch (err) { + // Capture error for every failed replay + setContext('Replays', { + _retryCount: retryConfig.count, + }); + + if (__DEBUG_BUILD__ && options._experiments && options._experiments.captureExceptions) { + captureException(err); + } + + // If an error happened here, it's likely that uploading the attachment + // failed, we'll can retry with the same events payload + if (retryConfig.count >= RETRY_MAX_COUNT) { + throw new Error(`${UNABLE_TO_SEND_REPLAY} - max retries exceeded`); + } + + // will retry in intervals of 5, 10, 30 + retryConfig.interval *= ++retryConfig.count; + + return await new Promise((resolve, reject) => { + setTimeout(async () => { + try { + await sendReplay( + { + replayId, + events, + segmentId, + includeReplayStartTimestamp, + eventContext, + session, + options, + }, + retryConfig, + ); + resolve(true); + } catch (err) { + reject(err); + } + }, retryConfig.interval); + }); + } +} diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts new file mode 100644 index 000000000000..6e124918d7e7 --- /dev/null +++ b/packages/replay/src/util/sendReplayRequest.ts @@ -0,0 +1,114 @@ +import { getCurrentHub } from '@sentry/core'; +import type { ReplayEvent, TransportMakeRequestResponse } from '@sentry/types'; +import { logger } from '@sentry/utils'; + +import { REPLAY_EVENT_NAME, UNABLE_TO_SEND_REPLAY } from '../constants'; +import type { SendReplay } from '../types'; +import { createRecordingData } from './createRecordingData'; +import { createReplayEnvelope } from './createReplayEnvelope'; +import { prepareReplayEvent } from './prepareReplayEvent'; + +/** + * Send replay attachment using `fetch()` + */ +export async function sendReplayRequest({ + events, + replayId, + segmentId: segment_id, + includeReplayStartTimestamp, + eventContext, + timestamp = new Date().getTime(), + session, + options, +}: SendReplay): Promise { + const recordingData = createRecordingData({ + events, + headers: { + segment_id, + }, + }); + + const { urls, errorIds, traceIds, initialTimestamp } = eventContext; + + const hub = getCurrentHub(); + const client = hub.getClient(); + const scope = hub.getScope(); + const transport = client && client.getTransport(); + const dsn = client?.getDsn(); + + if (!client || !scope || !transport || !dsn || !session.sampled) { + return; + } + + const baseEvent: ReplayEvent = { + // @ts-ignore private api + type: REPLAY_EVENT_NAME, + ...(includeReplayStartTimestamp ? { replay_start_timestamp: initialTimestamp / 1000 } : {}), + timestamp: timestamp / 1000, + error_ids: errorIds, + trace_ids: traceIds, + urls, + replay_id: replayId, + segment_id, + replay_type: session.sampled, + }; + + const replayEvent = await prepareReplayEvent({ scope, client, replayId, event: baseEvent }); + + if (!replayEvent) { + // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions + client.recordDroppedEvent('event_processor', 'replay_event', baseEvent); + __DEBUG_BUILD__ && logger.log('An event processor returned `null`, will not send event.'); + return; + } + + replayEvent.tags = { + ...replayEvent.tags, + sessionSampleRate: options.sessionSampleRate, + errorSampleRate: options.errorSampleRate, + }; + + /* + For reference, the fully built event looks something like this: + { + "type": "replay_event", + "timestamp": 1670837008.634, + "error_ids": [ + "errorId" + ], + "trace_ids": [ + "traceId" + ], + "urls": [ + "https://example.com" + ], + "replay_id": "eventId", + "segment_id": 3, + "replay_type": "error", + "platform": "javascript", + "event_id": "eventId", + "environment": "production", + "sdk": { + "integrations": [ + "BrowserTracing", + "Replay" + ], + "name": "sentry.javascript.browser", + "version": "7.25.0" + }, + "sdkProcessingMetadata": {}, + "tags": { + "sessionSampleRate": 1, + "errorSampleRate": 0, + } + } + */ + + const envelope = createReplayEnvelope(replayEvent, recordingData, dsn, client.getOptions().tunnel); + + try { + return await transport.send(envelope); + } catch { + throw new Error(UNABLE_TO_SEND_REPLAY); + } +} diff --git a/packages/replay/test/integration/events.test.ts b/packages/replay/test/integration/events.test.ts index 9bfe34484ce6..a71fe10f4f41 100644 --- a/packages/replay/test/integration/events.test.ts +++ b/packages/replay/test/integration/events.test.ts @@ -23,9 +23,6 @@ describe('Integration | events', () => { let mockTransportSend: jest.SpyInstance; const prevLocation = WINDOW.location; - type MockSendReplayRequest = jest.MockedFunction; - let mockSendReplayRequest: MockSendReplayRequest; - beforeAll(async () => { jest.setSystemTime(new Date(BASE_TIMESTAMP)); jest.runAllTimers(); @@ -40,15 +37,11 @@ describe('Integration | events', () => { mockTransportSend = jest.spyOn(getCurrentHub().getClient()!.getTransport()!, 'send'); - // @ts-ignore private API - mockSendReplayRequest = jest.spyOn(replay, '_sendReplayRequest'); - // Create a new session and clear mocks because a segment (from initial // checkout) will have already been uploaded by the time the tests run clearSession(replay); replay['_loadSession']({ expiry: 0 }); mockTransportSend.mockClear(); - mockSendReplayRequest.mockClear(); }); afterEach(async () => { @@ -60,7 +53,6 @@ describe('Integration | events', () => { }); clearSession(replay); jest.clearAllMocks(); - mockSendReplayRequest.mockRestore(); mockRecord.takeFullSnapshot.mockClear(); replay.stop(); }); diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index f2760ef43371..ff9ec6c1058f 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -6,6 +6,7 @@ import type { EventBuffer } from '../../src/types'; import * as AddMemoryEntry from '../../src/util/addMemoryEntry'; import { createPerformanceEntries } from '../../src/util/createPerformanceEntries'; 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'; @@ -17,7 +18,7 @@ async function advanceTimers(time: number) { await new Promise(process.nextTick); } -type MockSendReplay = jest.MockedFunction; +type MockSendReplay = jest.MockedFunction; type MockAddPerformanceEntries = jest.MockedFunction; type MockAddMemoryEntry = jest.SpyInstance; type MockEventBufferFinish = jest.MockedFunction; @@ -48,8 +49,7 @@ describe('Integration | flush', () => { ({ replay } = await mockSdk()); - // @ts-ignore private API - mockSendReplay = jest.spyOn(replay, '_sendReplay'); + mockSendReplay = jest.spyOn(SendReplay, 'sendReplay'); mockSendReplay.mockImplementation( jest.fn(async () => { return; @@ -175,12 +175,15 @@ describe('Integration | flush', () => { // debouncedFlush, which will call `flush` in 1 second expect(mockFlush).toHaveBeenCalledTimes(4); // sendReplay is called with replayId, events, segment + expect(mockSendReplay).toHaveBeenLastCalledWith({ events: expect.any(String), replayId: expect.any(String), includeReplayStartTimestamp: true, segmentId: 0, eventContext: expect.anything(), + session: expect.any(Object), + options: expect.any(Object), }); // Add this to test that segment ID increases @@ -228,6 +231,8 @@ describe('Integration | flush', () => { includeReplayStartTimestamp: false, segmentId: 1, eventContext: expect.anything(), + session: expect.any(Object), + options: expect.any(Object), }); // Make sure there's no other calls diff --git a/packages/replay/test/integration/sendReplayEvent.test.ts b/packages/replay/test/integration/sendReplayEvent.test.ts index ade380a1605a..12e06603752f 100644 --- a/packages/replay/test/integration/sendReplayEvent.test.ts +++ b/packages/replay/test/integration/sendReplayEvent.test.ts @@ -1,10 +1,11 @@ -import { getCurrentHub } from '@sentry/core'; +import * as SentryCore from '@sentry/core'; import type { Transport } from '@sentry/types'; import * as SentryUtils from '@sentry/utils'; import { DEFAULT_FLUSH_MIN_DELAY, SESSION_IDLE_DURATION, WINDOW } from '../../src/constants'; 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'; @@ -17,12 +18,11 @@ async function advanceTimers(time: number) { } type MockTransportSend = jest.MockedFunction; -type MockSendReplayRequest = jest.MockedFunction; describe('Integration | sendReplayEvent', () => { let replay: ReplayContainer; let mockTransportSend: MockTransportSend; - let mockSendReplayRequest: MockSendReplayRequest; + let mockSendReplayRequest: jest.SpyInstance; let domHandler: (args: any) => any; const { record: mockRecord } = mockRrweb(); @@ -37,14 +37,16 @@ describe('Integration | sendReplayEvent', () => { ({ replay } = await mockSdk({ replayOptions: { stickySession: false, + _experiments: { + captureExceptions: true, + }, }, })); - // @ts-ignore private API - mockSendReplayRequest = jest.spyOn(replay, '_sendReplayRequest'); + mockSendReplayRequest = jest.spyOn(SendReplayRequest, 'sendReplayRequest'); jest.runAllTimers(); - mockTransportSend = getCurrentHub()?.getClient()?.getTransport()?.send as MockTransportSend; + mockTransportSend = SentryCore.getCurrentHub()?.getClient()?.getTransport()?.send as MockTransportSend; }); beforeEach(() => { @@ -330,7 +332,7 @@ describe('Integration | sendReplayEvent', () => { expect(replay.session?.segmentId).toBe(1); }); - it('fails to upload data on first two calls and succeeds on the third', async () => { + it('fails to upload data on first two calls and succeeds on the third xxx', async () => { expect(replay.session?.segmentId).toBe(0); const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; @@ -383,20 +385,15 @@ describe('Integration | sendReplayEvent', () => { it('fails to upload data and hits retry max and stops', async () => { const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; - // @ts-ignore private API - const spySendReplay = jest.spyOn(replay, '_sendReplay'); + const spyHandleException = jest.spyOn(SentryCore, 'captureException'); // Suppress console.errors const mockConsole = jest.spyOn(console, 'error').mockImplementation(jest.fn()); - // @ts-ignore privaye api - Check errors - const spyHandleException = jest.spyOn(replay, '_handleException'); - expect(replay.session?.segmentId).toBe(0); - // fail the first and second requests and pass the third one - mockSendReplayRequest.mockReset(); - mockSendReplayRequest.mockImplementation(() => { + // fail all requests + mockSendReplayRequest.mockImplementation(async () => { throw new Error('Something bad happened'); }); mockRecord._emitter(TEST_EVENT); @@ -414,14 +411,12 @@ describe('Integration | sendReplayEvent', () => { await advanceTimers(30000); expect(mockSendReplayRequest).toHaveBeenCalledTimes(4); - expect(spySendReplay).toHaveBeenCalledTimes(4); mockConsole.mockReset(); // Make sure it doesn't retry again jest.runAllTimers(); expect(mockSendReplayRequest).toHaveBeenCalledTimes(4); - expect(spySendReplay).toHaveBeenCalledTimes(4); // Retries = 3 (total tries = 4 including initial attempt) // + last exception is max retries exceeded From 7113183df455763bb97cb9b7b8858c311e8ca8eb Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 12 Jan 2023 14:18:15 +0100 Subject: [PATCH 2/5] fix(replay): Ensure timestamp is always consistent, even after retries --- packages/replay/src/replay.ts | 1 + packages/replay/src/types.ts | 2 +- packages/replay/src/util/sendReplay.ts | 27 ++++--------------- packages/replay/src/util/sendReplayRequest.ts | 2 +- .../replay/test/integration/flush.test.ts | 2 ++ .../test/integration/sendReplayEvent.test.ts | 6 ++--- 6 files changed, 13 insertions(+), 27 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 7fe02a04c80a..366f18e132df 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -824,6 +824,7 @@ export class ReplayContainer implements ReplayContainerInterface { eventContext, session: this.session, options: this.getOptions(), + timestamp: new Date().getTime(), }); } catch (err) { this._handleException(err); diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index ac7c411919a5..bd3c6af80eda 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -15,7 +15,7 @@ export interface SendReplay { segmentId: number; includeReplayStartTimestamp: boolean; eventContext: PopEventContext; - timestamp?: number; + timestamp: number; session: Session; options: ReplayPluginOptions; } diff --git a/packages/replay/src/util/sendReplay.ts b/packages/replay/src/util/sendReplay.ts index 7ced6f7488f2..d15968fc747c 100644 --- a/packages/replay/src/util/sendReplay.ts +++ b/packages/replay/src/util/sendReplay.ts @@ -8,27 +8,21 @@ import { sendReplayRequest } from './sendReplayRequest'; * Finalize and send the current replay event to Sentry */ export async function sendReplay( - { replayId, events, segmentId, includeReplayStartTimestamp, eventContext, session, options }: SendReplay, + replayData: SendReplay, retryConfig = { count: 0, interval: RETRY_BASE_INTERVAL, }, ): Promise { + const { events, options } = replayData; + // short circuit if there's no events to upload (this shouldn't happen as _runFlush makes this check) if (!events.length) { return; } try { - await sendReplayRequest({ - events, - replayId, - segmentId, - includeReplayStartTimestamp, - eventContext, - session, - options, - }); + await sendReplayRequest(replayData); return true; } catch (err) { // Capture error for every failed replay @@ -52,18 +46,7 @@ export async function sendReplay( return await new Promise((resolve, reject) => { setTimeout(async () => { try { - await sendReplay( - { - replayId, - events, - segmentId, - includeReplayStartTimestamp, - eventContext, - session, - options, - }, - retryConfig, - ); + await sendReplay(replayData, retryConfig); resolve(true); } catch (err) { reject(err); diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts index 6e124918d7e7..57be831f66cd 100644 --- a/packages/replay/src/util/sendReplayRequest.ts +++ b/packages/replay/src/util/sendReplayRequest.ts @@ -17,7 +17,7 @@ export async function sendReplayRequest({ segmentId: segment_id, includeReplayStartTimestamp, eventContext, - timestamp = new Date().getTime(), + timestamp, session, options, }: SendReplay): Promise { diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index ff9ec6c1058f..87daadb3c64f 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -184,6 +184,7 @@ describe('Integration | flush', () => { eventContext: expect.anything(), session: expect.any(Object), options: expect.any(Object), + timestamp: expect.any(Number), }); // Add this to test that segment ID increases @@ -233,6 +234,7 @@ describe('Integration | flush', () => { eventContext: expect.anything(), session: expect.any(Object), options: expect.any(Object), + timestamp: expect.any(Number), }); // Make sure there's no other calls diff --git a/packages/replay/test/integration/sendReplayEvent.test.ts b/packages/replay/test/integration/sendReplayEvent.test.ts index 12e06603752f..62d8c9a1a3b7 100644 --- a/packages/replay/test/integration/sendReplayEvent.test.ts +++ b/packages/replay/test/integration/sendReplayEvent.test.ts @@ -332,7 +332,7 @@ describe('Integration | sendReplayEvent', () => { expect(replay.session?.segmentId).toBe(1); }); - it('fails to upload data on first two calls and succeeds on the third xxx', async () => { + it('fails to upload data on first two calls and succeeds on the third', async () => { expect(replay.session?.segmentId).toBe(0); const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; @@ -363,8 +363,8 @@ describe('Integration | sendReplayEvent', () => { error_ids: [], replay_id: expect.any(String), replay_start_timestamp: BASE_TIMESTAMP / 1000, - // 20seconds = Add up all of the previous `advanceTimers()` - timestamp: (BASE_TIMESTAMP + 20000) / 1000 + 0.02, + // timestamp is set on first try, after 5s flush + timestamp: (BASE_TIMESTAMP + 5000) / 1000, trace_ids: [], urls: ['http://localhost/'], }), From e87a5c14683952327807f34f79c75f383acf8f73 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 12 Jan 2023 14:39:22 +0100 Subject: [PATCH 3/5] ref: Fix `SendReplayData` type name --- packages/replay/src/types.ts | 2 +- packages/replay/src/util/sendReplay.ts | 4 ++-- packages/replay/src/util/sendReplayRequest.ts | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index bd3c6af80eda..c470241a6445 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -9,7 +9,7 @@ export type RecordedEvents = Uint8Array | string; export type AllPerformanceEntry = PerformancePaintTiming | PerformanceResourceTiming | PerformanceNavigationTiming; -export interface SendReplay { +export interface SendReplayData { events: RecordedEvents; replayId: string; segmentId: number; diff --git a/packages/replay/src/util/sendReplay.ts b/packages/replay/src/util/sendReplay.ts index d15968fc747c..8c40b7859cae 100644 --- a/packages/replay/src/util/sendReplay.ts +++ b/packages/replay/src/util/sendReplay.ts @@ -1,14 +1,14 @@ import { captureException, setContext } from '@sentry/core'; import { RETRY_BASE_INTERVAL, RETRY_MAX_COUNT, UNABLE_TO_SEND_REPLAY } from '../constants'; -import type { SendReplay } from '../types'; +import type { SendReplayData } from '../types'; import { sendReplayRequest } from './sendReplayRequest'; /** * Finalize and send the current replay event to Sentry */ export async function sendReplay( - replayData: SendReplay, + replayData: SendReplayData, retryConfig = { count: 0, interval: RETRY_BASE_INTERVAL, diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts index 57be831f66cd..dc7c0dd7e185 100644 --- a/packages/replay/src/util/sendReplayRequest.ts +++ b/packages/replay/src/util/sendReplayRequest.ts @@ -3,7 +3,7 @@ import type { ReplayEvent, TransportMakeRequestResponse } from '@sentry/types'; import { logger } from '@sentry/utils'; import { REPLAY_EVENT_NAME, UNABLE_TO_SEND_REPLAY } from '../constants'; -import type { SendReplay } from '../types'; +import type { SendReplayData } from '../types'; import { createRecordingData } from './createRecordingData'; import { createReplayEnvelope } from './createReplayEnvelope'; import { prepareReplayEvent } from './prepareReplayEvent'; @@ -20,7 +20,7 @@ export async function sendReplayRequest({ timestamp, session, options, -}: SendReplay): Promise { +}: SendReplayData): Promise { const recordingData = createRecordingData({ events, headers: { From dff1a447caf29215051c6ae2d8eb495122ffa4b8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 12 Jan 2023 14:40:24 +0100 Subject: [PATCH 4/5] ref: Rename `events` to `recordingData` for replays --- packages/replay/jest.setup.ts | 8 +++---- packages/replay/src/replay.ts | 2 +- packages/replay/src/types.ts | 4 +--- ...cordingData.ts => prepareRecordingData.ts} | 18 +++++++-------- packages/replay/src/util/sendReplay.ts | 4 ++-- packages/replay/src/util/sendReplayRequest.ts | 10 ++++----- .../test/integration/errorSampleRate.test.ts | 16 +++++++------- .../replay/test/integration/events.test.ts | 2 +- .../replay/test/integration/flush.test.ts | 4 ++-- .../test/integration/sendReplayEvent.test.ts | 22 +++++++++---------- .../replay/test/integration/session.test.ts | 6 ++--- packages/replay/test/integration/stop.test.ts | 2 +- 12 files changed, 47 insertions(+), 51 deletions(-) rename packages/replay/src/util/{createRecordingData.ts => prepareRecordingData.ts} (59%) diff --git a/packages/replay/jest.setup.ts b/packages/replay/jest.setup.ts index db17db23f761..26f9ea346a0d 100644 --- a/packages/replay/jest.setup.ts +++ b/packages/replay/jest.setup.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ import { getCurrentHub } from '@sentry/core'; -import type { ReplayRecordingData,Transport } from '@sentry/types'; +import type { ReplayRecordingData, Transport } from '@sentry/types'; import type { ReplayContainer, Session } from './src/types'; @@ -34,7 +34,7 @@ type SentReplayExpected = { replayEventPayload?: ReplayEventPayload; recordingHeader?: RecordingHeader; recordingPayloadHeader?: RecordingPayloadHeader; - events?: ReplayRecordingData; + recordingData?: ReplayRecordingData; }; // eslint-disable-next-line @typescript-eslint/explicit-function-return-type @@ -79,7 +79,7 @@ function checkCallForSentReplay( const [[replayEventHeader, replayEventPayload], [recordingHeader, recordingPayload] = []] = envelopeItems; // @ts-ignore recordingPayload is always a string in our tests - const [recordingPayloadHeader, events] = recordingPayload?.split('\n') || []; + const [recordingPayloadHeader, recordingData] = recordingPayload?.split('\n') || []; const actualObj: Required = { // @ts-ignore Custom envelope @@ -91,7 +91,7 @@ function checkCallForSentReplay( // @ts-ignore Custom envelope recordingHeader: recordingHeader, recordingPayloadHeader: recordingPayloadHeader && JSON.parse(recordingPayloadHeader), - events, + recordingData, }; const isObjectContaining = expected && 'sample' in expected && 'inverse' in expected; diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 366f18e132df..801372eea92e 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -818,7 +818,7 @@ export class ReplayContainer implements ReplayContainerInterface { await sendReplay({ replayId, - events: recordingData, + recordingData, segmentId, includeReplayStartTimestamp: segmentId === 0, eventContext, diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index c470241a6445..5d97766d6d00 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -5,12 +5,10 @@ import type { eventWithTime, recordOptions } from './types/rrweb'; export type RecordingEvent = eventWithTime; export type RecordingOptions = recordOptions; -export type RecordedEvents = Uint8Array | string; - export type AllPerformanceEntry = PerformancePaintTiming | PerformanceResourceTiming | PerformanceNavigationTiming; export interface SendReplayData { - events: RecordedEvents; + recordingData: ReplayRecordingData; replayId: string; segmentId: number; includeReplayStartTimestamp: boolean; diff --git a/packages/replay/src/util/createRecordingData.ts b/packages/replay/src/util/prepareRecordingData.ts similarity index 59% rename from packages/replay/src/util/createRecordingData.ts rename to packages/replay/src/util/prepareRecordingData.ts index a0d9835c1094..2ee3391d6f42 100644 --- a/packages/replay/src/util/createRecordingData.ts +++ b/packages/replay/src/util/prepareRecordingData.ts @@ -1,15 +1,13 @@ import type { ReplayRecordingData } from '@sentry/types'; -import type { RecordedEvents } from '../types'; - /** - * Create the recording data ready to be sent. + * Prepare the recording data ready to be sent. */ -export function createRecordingData({ - events, +export function prepareRecordingData({ + recordingData, headers, }: { - events: RecordedEvents; + recordingData: ReplayRecordingData; headers: Record; }): ReplayRecordingData { let payloadWithSequence; @@ -18,16 +16,16 @@ export function createRecordingData({ const replayHeaders = `${JSON.stringify(headers)} `; - if (typeof events === 'string') { - payloadWithSequence = `${replayHeaders}${events}`; + if (typeof recordingData === 'string') { + payloadWithSequence = `${replayHeaders}${recordingData}`; } else { const enc = new TextEncoder(); // XXX: newline is needed to separate sequence id from events const sequence = enc.encode(replayHeaders); // Merge the two Uint8Arrays - payloadWithSequence = new Uint8Array(sequence.length + events.length); + payloadWithSequence = new Uint8Array(sequence.length + recordingData.length); payloadWithSequence.set(sequence); - payloadWithSequence.set(events, sequence.length); + payloadWithSequence.set(recordingData, sequence.length); } return payloadWithSequence; diff --git a/packages/replay/src/util/sendReplay.ts b/packages/replay/src/util/sendReplay.ts index 8c40b7859cae..8b58d1a2b85b 100644 --- a/packages/replay/src/util/sendReplay.ts +++ b/packages/replay/src/util/sendReplay.ts @@ -14,10 +14,10 @@ export async function sendReplay( interval: RETRY_BASE_INTERVAL, }, ): Promise { - const { events, options } = replayData; + const { recordingData, options } = replayData; // short circuit if there's no events to upload (this shouldn't happen as _runFlush makes this check) - if (!events.length) { + if (!recordingData.length) { return; } diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts index dc7c0dd7e185..40ba86a9bef2 100644 --- a/packages/replay/src/util/sendReplayRequest.ts +++ b/packages/replay/src/util/sendReplayRequest.ts @@ -4,15 +4,15 @@ import { logger } from '@sentry/utils'; import { REPLAY_EVENT_NAME, UNABLE_TO_SEND_REPLAY } from '../constants'; import type { SendReplayData } from '../types'; -import { createRecordingData } from './createRecordingData'; import { createReplayEnvelope } from './createReplayEnvelope'; +import { prepareRecordingData } from './prepareRecordingData'; import { prepareReplayEvent } from './prepareReplayEvent'; /** * Send replay attachment using `fetch()` */ export async function sendReplayRequest({ - events, + recordingData, replayId, segmentId: segment_id, includeReplayStartTimestamp, @@ -21,8 +21,8 @@ export async function sendReplayRequest({ session, options, }: SendReplayData): Promise { - const recordingData = createRecordingData({ - events, + const preparedRecordingData = prepareRecordingData({ + recordingData, headers: { segment_id, }, @@ -104,7 +104,7 @@ export async function sendReplayRequest({ } */ - const envelope = createReplayEnvelope(replayEvent, recordingData, dsn, client.getOptions().tunnel); + const envelope = createReplayEnvelope(replayEvent, preparedRecordingData, dsn, client.getOptions().tunnel); try { return await transport.send(envelope); diff --git a/packages/replay/test/integration/errorSampleRate.test.ts b/packages/replay/test/integration/errorSampleRate.test.ts index 2730560093a4..2b52e6c7d96c 100644 --- a/packages/replay/test/integration/errorSampleRate.test.ts +++ b/packages/replay/test/integration/errorSampleRate.test.ts @@ -68,7 +68,7 @@ describe('Integration | errorSampleRate', () => { sessionSampleRate: 0, }), }), - events: JSON.stringify([ + recordingData: JSON.stringify([ { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT, { @@ -98,7 +98,7 @@ describe('Integration | errorSampleRate', () => { sessionSampleRate: 0, }), }), - events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5020, type: 2 }]), + recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5020, type: 2 }]), }); jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); @@ -106,7 +106,7 @@ describe('Integration | errorSampleRate', () => { // New checkout when we call `startRecording` again after uploading segment // after an error occurs expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([ + recordingData: JSON.stringify([ { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY + 20, @@ -124,7 +124,7 @@ describe('Integration | errorSampleRate', () => { await new Promise(process.nextTick); expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([ + recordingData: JSON.stringify([ { type: 5, timestamp: BASE_TIMESTAMP + 10000 + 40, @@ -304,7 +304,7 @@ describe('Integration | errorSampleRate', () => { await new Promise(process.nextTick); expect(replay).toHaveSentReplay({ - events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]), + recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]), replayEventPayload: expect.objectContaining({ replay_start_timestamp: BASE_TIMESTAMP / 1000, // the exception happens roughly 10 seconds after BASE_TIMESTAMP @@ -360,7 +360,7 @@ describe('Integration | errorSampleRate', () => { // Make sure the old performance event is thrown out replay_start_timestamp: (BASE_TIMESTAMP + ELAPSED + 20) / 1000, }), - events: JSON.stringify([ + recordingData: JSON.stringify([ { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + ELAPSED + 20, @@ -406,12 +406,12 @@ it('sends a replay after loading the session multiple times', async () => { await new Promise(process.nextTick); expect(replay).toHaveSentReplay({ - events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]), + recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]), }); // Latest checkout when we call `startRecording` again after uploading segment // after an error occurs (e.g. when we switch to session replay recording) expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5020, type: 2 }]), + recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5020, type: 2 }]), }); }); diff --git a/packages/replay/test/integration/events.test.ts b/packages/replay/test/integration/events.test.ts index a71fe10f4f41..5168426dfd79 100644 --- a/packages/replay/test/integration/events.test.ts +++ b/packages/replay/test/integration/events.test.ts @@ -176,7 +176,7 @@ describe('Integration | events', () => { // Make sure the old performance event is thrown out replay_start_timestamp: BASE_TIMESTAMP / 1000, }), - events: JSON.stringify([ + recordingData: JSON.stringify([ TEST_EVENT, { type: 5, diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index 87daadb3c64f..c0c143b255e4 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -177,7 +177,7 @@ describe('Integration | flush', () => { // sendReplay is called with replayId, events, segment expect(mockSendReplay).toHaveBeenLastCalledWith({ - events: expect.any(String), + recordingData: expect.any(String), replayId: expect.any(String), includeReplayStartTimestamp: true, segmentId: 0, @@ -227,7 +227,7 @@ describe('Integration | flush', () => { expect(mockFlush).toHaveBeenCalledTimes(5); expect(mockRunFlush).toHaveBeenCalledTimes(2); expect(mockSendReplay).toHaveBeenLastCalledWith({ - events: expect.any(String), + recordingData: expect.any(String), replayId: expect.any(String), includeReplayStartTimestamp: false, segmentId: 1, diff --git a/packages/replay/test/integration/sendReplayEvent.test.ts b/packages/replay/test/integration/sendReplayEvent.test.ts index 62d8c9a1a3b7..9a4c62fa8ede 100644 --- a/packages/replay/test/integration/sendReplayEvent.test.ts +++ b/packages/replay/test/integration/sendReplayEvent.test.ts @@ -96,7 +96,7 @@ describe('Integration | sendReplayEvent', () => { expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); // Session's last activity is not updated because we do not consider // visibilitystate as user being active @@ -136,7 +136,7 @@ describe('Integration | sendReplayEvent', () => { expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); // No user activity to trigger an update expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP); @@ -160,7 +160,7 @@ describe('Integration | sendReplayEvent', () => { await new Promise(process.nextTick); expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([...Array(5)].map(() => TEST_EVENT)), + recordingData: JSON.stringify([...Array(5)].map(() => TEST_EVENT)), }); // There should also not be another attempt at an upload 5 seconds after the last replay event @@ -177,7 +177,7 @@ describe('Integration | sendReplayEvent', () => { mockTransportSend.mockClear(); mockRecord._emitter(TEST_EVENT); await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); }); it('uploads a replay event when WINDOW is blurred', async () => { @@ -211,7 +211,7 @@ describe('Integration | sendReplayEvent', () => { await new Promise(process.nextTick); expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([TEST_EVENT, hiddenBreadcrumb]), + recordingData: JSON.stringify([TEST_EVENT, hiddenBreadcrumb]), }); // Session's last activity should not be updated expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP); @@ -238,7 +238,7 @@ describe('Integration | sendReplayEvent', () => { await new Promise(process.nextTick); expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); // Session's last activity is not updated because we do not consider // visibilitystate as user being active @@ -256,7 +256,7 @@ describe('Integration | sendReplayEvent', () => { expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); expect(mockTransportSend).toHaveBeenCalledTimes(1); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); // No user activity to trigger an update expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP); @@ -280,7 +280,7 @@ describe('Integration | sendReplayEvent', () => { await new Promise(process.nextTick); expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([...Array(5)].map(() => TEST_EVENT)), + recordingData: JSON.stringify([...Array(5)].map(() => TEST_EVENT)), }); // There should also not be another attempt at an upload 5 seconds after the last replay event @@ -298,7 +298,7 @@ describe('Integration | sendReplayEvent', () => { mockTransportSend.mockClear(); mockRecord._emitter(TEST_EVENT); await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); }); it('uploads a dom breadcrumb 5 seconds after listener receives an event', async () => { @@ -311,7 +311,7 @@ describe('Integration | sendReplayEvent', () => { await advanceTimers(ELAPSED); expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([ + recordingData: JSON.stringify([ { type: 5, timestamp: BASE_TIMESTAMP, @@ -369,7 +369,7 @@ describe('Integration | sendReplayEvent', () => { urls: ['http://localhost/'], }), recordingPayloadHeader: { segment_id: 0 }, - events: JSON.stringify([TEST_EVENT]), + recordingData: JSON.stringify([TEST_EVENT]), }); mockTransportSend.mockClear(); diff --git a/packages/replay/test/integration/session.test.ts b/packages/replay/test/integration/session.test.ts index c07bac150751..0a8a79dc5467 100644 --- a/packages/replay/test/integration/session.test.ts +++ b/packages/replay/test/integration/session.test.ts @@ -149,7 +149,7 @@ describe('Integration | session', () => { const breadcrumbTimestamp = newTimestamp + 20; // I don't know where this 20ms comes from expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([ + recordingData: JSON.stringify([ { data: { isCheckout: true }, timestamp: newTimestamp, type: 2 }, { type: 5, @@ -309,7 +309,7 @@ describe('Integration | session', () => { expect(replay).toHaveLastSentReplay({ recordingPayloadHeader: { segment_id: 0 }, - events: JSON.stringify([ + recordingData: JSON.stringify([ { data: { isCheckout: true }, timestamp: newTimestamp, type: 2 }, { type: 5, @@ -420,7 +420,7 @@ describe('Integration | session', () => { expect(replay).toHaveLastSentReplay({ recordingPayloadHeader: { segment_id: 0 }, - events: JSON.stringify([ + recordingData: JSON.stringify([ { data: { isCheckout: true }, timestamp: newTimestamp, type: 2 }, { type: 5, diff --git a/packages/replay/test/integration/stop.test.ts b/packages/replay/test/integration/stop.test.ts index 42ccbe09ca68..55f8dafd9289 100644 --- a/packages/replay/test/integration/stop.test.ts +++ b/packages/replay/test/integration/stop.test.ts @@ -110,7 +110,7 @@ describe('Integration | stop', () => { jest.runAllTimers(); await new Promise(process.nextTick); expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([ + recordingData: JSON.stringify([ // This event happens when we call `replay.start` { data: { isCheckout: true }, From caec2f76965f56d2c40a01861f8484278461c5cb Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 12 Jan 2023 16:55:51 +0100 Subject: [PATCH 5/5] ref: Bring back rate limiting behavior --- packages/replay/src/replay.ts | 16 +++--- packages/replay/src/util/sendReplay.ts | 6 ++- packages/replay/src/util/sendReplayRequest.ts | 28 +++++++++- .../test/integration/rateLimiting.test.ts | 51 ++++++++----------- 4 files changed, 60 insertions(+), 41 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 801372eea92e..241d330dd412 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -1,7 +1,8 @@ /* eslint-disable max-lines */ // TODO: We might want to split this file up import { addGlobalEventProcessor, captureException, getCurrentHub } from '@sentry/core'; import type { Breadcrumb, ReplayRecordingMode } from '@sentry/types'; -import { addInstrumentationHandler, logger } from '@sentry/utils'; +import type { RateLimits } from '@sentry/utils'; +import { addInstrumentationHandler, disabledUntil, logger } from '@sentry/utils'; import { EventType, record } from 'rrweb'; import { MAX_SESSION_LIFE, SESSION_IDLE_DURATION, VISIBILITY_CHANGE_TIMEOUT, WINDOW } from './constants'; @@ -38,6 +39,7 @@ import { isExpired } from './util/isExpired'; import { isSessionExpired } from './util/isSessionExpired'; import { overwriteRecordDroppedEvent, restoreRecordDroppedEvent } from './util/monkeyPatchRecordDroppedEvent'; import { sendReplay } from './util/sendReplay'; +import { RateLimitError } from './util/sendReplayRequest'; /** * The main replay container class, which holds all the state and methods for recording and sending replays. @@ -108,11 +110,6 @@ export class ReplayContainer implements ReplayContainerInterface { initialUrl: '', }; - /** - * A RateLimits object holding the rate-limit durations in case a sent replay event was rate-limited. - */ - private _rateLimits: RateLimits = {}; - public constructor({ options, recordingOptions, @@ -827,6 +824,9 @@ export class ReplayContainer implements ReplayContainerInterface { timestamp: new Date().getTime(), }); } catch (err) { + if (err instanceof RateLimitError) { + this._handleRateLimit(err.rateLimits); + } this._handleException(err); } } @@ -889,14 +889,14 @@ export class ReplayContainer implements ReplayContainerInterface { /** * Pauses the replay and resumes it after the rate-limit duration is over. */ - private _handleRateLimit(): void { + private _handleRateLimit(rateLimits: RateLimits): void { // in case recording is already paused, we don't need to do anything, as we might have already paused because of a // rate limit if (this.isPaused()) { return; } - const rateLimitEnd = disabledUntil(this._rateLimits, 'replay'); + const rateLimitEnd = disabledUntil(rateLimits, 'replay'); const rateLimitDuration = rateLimitEnd - Date.now(); if (rateLimitDuration > 0) { diff --git a/packages/replay/src/util/sendReplay.ts b/packages/replay/src/util/sendReplay.ts index 8b58d1a2b85b..aa2e4649dda7 100644 --- a/packages/replay/src/util/sendReplay.ts +++ b/packages/replay/src/util/sendReplay.ts @@ -2,7 +2,7 @@ import { captureException, setContext } from '@sentry/core'; import { RETRY_BASE_INTERVAL, RETRY_MAX_COUNT, UNABLE_TO_SEND_REPLAY } from '../constants'; import type { SendReplayData } from '../types'; -import { sendReplayRequest } from './sendReplayRequest'; +import { RateLimitError, sendReplayRequest } from './sendReplayRequest'; /** * Finalize and send the current replay event to Sentry @@ -25,6 +25,10 @@ export async function sendReplay( await sendReplayRequest(replayData); return true; } catch (err) { + if (err instanceof RateLimitError) { + throw err; + } + // Capture error for every failed replay setContext('Replays', { _retryCount: retryConfig.count, diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts index 40ba86a9bef2..23df14ffcdda 100644 --- a/packages/replay/src/util/sendReplayRequest.ts +++ b/packages/replay/src/util/sendReplayRequest.ts @@ -1,6 +1,7 @@ import { getCurrentHub } from '@sentry/core'; import type { ReplayEvent, TransportMakeRequestResponse } from '@sentry/types'; -import { logger } from '@sentry/utils'; +import type { RateLimits } from '@sentry/utils'; +import { isRateLimited, logger, updateRateLimits } from '@sentry/utils'; import { REPLAY_EVENT_NAME, UNABLE_TO_SEND_REPLAY } from '../constants'; import type { SendReplayData } from '../types'; @@ -106,9 +107,32 @@ export async function sendReplayRequest({ const envelope = createReplayEnvelope(replayEvent, preparedRecordingData, dsn, client.getOptions().tunnel); + let response: void | TransportMakeRequestResponse; + try { - return await transport.send(envelope); + response = await transport.send(envelope); } catch { throw new Error(UNABLE_TO_SEND_REPLAY); } + + // TODO (v8): we can remove this guard once transport.send's type signature doesn't include void anymore + if (response) { + const rateLimits = updateRateLimits({}, response); + if (isRateLimited(rateLimits, 'replay')) { + throw new RateLimitError(rateLimits); + } + } + return response; +} + +/** + * This error indicates that we hit a rate limit API error. + */ +export class RateLimitError extends Error { + public rateLimits: RateLimits; + + public constructor(rateLimits: RateLimits) { + super('Rate limit hit'); + this.rateLimits = rateLimits; + } } diff --git a/packages/replay/test/integration/rateLimiting.test.ts b/packages/replay/test/integration/rateLimiting.test.ts index 1270d7c9d780..31e15638c314 100644 --- a/packages/replay/test/integration/rateLimiting.test.ts +++ b/packages/replay/test/integration/rateLimiting.test.ts @@ -3,6 +3,7 @@ import type { Transport, TransportMakeRequestResponse } from '@sentry/types'; import { DEFAULT_FLUSH_MIN_DELAY, SESSION_IDLE_DURATION } from '../../src/constants'; 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'; @@ -16,12 +17,11 @@ async function advanceTimers(time: number) { } type MockTransportSend = jest.MockedFunction; -type MockSendReplayRequest = jest.MockedFunction; describe('Integration | rate-limiting behaviour', () => { let replay: ReplayContainer; let mockTransportSend: MockTransportSend; - let mockSendReplayRequest: MockSendReplayRequest; + let mockSendReplayRequest: jest.MockedFunction; const { record: mockRecord } = mockRrweb(); beforeAll(async () => { @@ -33,12 +33,9 @@ describe('Integration | rate-limiting behaviour', () => { }, })); - // @ts-ignore private API - jest.spyOn(replay, '_sendReplayRequest'); - jest.runAllTimers(); mockTransportSend = getCurrentHub()?.getClient()?.getTransport()?.send as MockTransportSend; - mockSendReplayRequest = replay['_sendReplayRequest'] as MockSendReplayRequest; + mockSendReplayRequest = jest.spyOn(SendReplayRequest, 'sendReplayRequest'); }); beforeEach(() => { @@ -52,8 +49,6 @@ describe('Integration | rate-limiting behaviour', () => { replay['_loadSession']({ expiry: 0 }); mockSendReplayRequest.mockClear(); - - replay['_rateLimits'] = {}; }); afterEach(async () => { @@ -92,15 +87,13 @@ describe('Integration | rate-limiting behaviour', () => { }, }, ] as TransportMakeRequestResponse[])( - 'pauses recording and flushing a rate limit is hit and resumes both after the rate limit duration is over', + 'pauses recording and flushing a rate limit is hit and resumes both after the rate limit duration is over %j', async rateLimitResponse => { expect(replay.session?.segmentId).toBe(0); jest.spyOn(replay, 'pause'); jest.spyOn(replay, 'resume'); // @ts-ignore private API jest.spyOn(replay, '_handleRateLimit'); - // @ts-ignore private API - jest.spyOn(replay, '_sendReplay'); const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; @@ -115,7 +108,7 @@ describe('Integration | rate-limiting behaviour', () => { expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); expect(mockTransportSend).toHaveBeenCalledTimes(1); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); expect(replay['_handleRateLimit']).toHaveBeenCalledTimes(1); // resume() was called once before we even started @@ -136,7 +129,7 @@ describe('Integration | rate-limiting behaviour', () => { mockRecord._emitter(ev); await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); expect(replay.isPaused()).toBe(true); - expect(replay['_sendReplay']).toHaveBeenCalledTimes(1); + expect(mockSendReplayRequest).toHaveBeenCalledTimes(1); expect(mockTransportSend).toHaveBeenCalledTimes(1); } @@ -148,9 +141,9 @@ describe('Integration | rate-limiting behaviour', () => { expect(replay.resume).toHaveBeenCalledTimes(1); expect(replay.isPaused()).toBe(false); - expect(replay['_sendReplay']).toHaveBeenCalledTimes(2); + expect(mockSendReplayRequest).toHaveBeenCalledTimes(2); expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([ + recordingData: JSON.stringify([ { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY * 7, type: 2 }, ]), }); @@ -165,14 +158,14 @@ describe('Integration | rate-limiting behaviour', () => { // T = base + 40 await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); - expect(replay['_sendReplay']).toHaveBeenCalledTimes(3); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT3]) }); + expect(mockSendReplayRequest).toHaveBeenCalledTimes(3); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT3]) }); // nothing should happen afterwards // T = base + 60 await advanceTimers(20_000); - expect(replay['_sendReplay']).toHaveBeenCalledTimes(3); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT3]) }); + expect(mockSendReplayRequest).toHaveBeenCalledTimes(3); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT3]) }); // events array should be empty expect(replay.eventBuffer?.pendingLength).toBe(0); @@ -185,8 +178,6 @@ describe('Integration | rate-limiting behaviour', () => { jest.spyOn(replay, 'resume'); // @ts-ignore private API jest.spyOn(replay, '_handleRateLimit'); - // @ts-ignore private API - jest.spyOn(replay, '_sendReplay'); const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; @@ -201,7 +192,7 @@ describe('Integration | rate-limiting behaviour', () => { expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); expect(mockTransportSend).toHaveBeenCalledTimes(1); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); expect(replay['_handleRateLimit']).toHaveBeenCalledTimes(1); // resume() was called once before we even started @@ -223,7 +214,7 @@ describe('Integration | rate-limiting behaviour', () => { mockRecord._emitter(ev); await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); expect(replay.isPaused()).toBe(true); - expect(replay['_sendReplay']).toHaveBeenCalledTimes(1); + expect(mockSendReplayRequest).toHaveBeenCalledTimes(1); expect(mockTransportSend).toHaveBeenCalledTimes(1); } @@ -235,9 +226,9 @@ describe('Integration | rate-limiting behaviour', () => { expect(replay.resume).toHaveBeenCalledTimes(1); expect(replay.isPaused()).toBe(false); - expect(replay['_sendReplay']).toHaveBeenCalledTimes(2); + expect(mockSendReplayRequest).toHaveBeenCalledTimes(2); expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([ + recordingData: JSON.stringify([ { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY * 13, type: 2 }, ]), }); @@ -252,14 +243,14 @@ describe('Integration | rate-limiting behaviour', () => { // T = base + 65 await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); - expect(replay['_sendReplay']).toHaveBeenCalledTimes(3); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT3]) }); + expect(mockSendReplayRequest).toHaveBeenCalledTimes(3); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT3]) }); // nothing should happen afterwards // T = base + 85 await advanceTimers(20_000); - expect(replay['_sendReplay']).toHaveBeenCalledTimes(3); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT3]) }); + expect(mockSendReplayRequest).toHaveBeenCalledTimes(3); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT3]) }); // events array should be empty expect(replay.eventBuffer?.pendingLength).toBe(0); @@ -291,7 +282,7 @@ describe('Integration | rate-limiting behaviour', () => { expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); expect(mockTransportSend).toHaveBeenCalledTimes(1); - expect(replay).toHaveLastSentReplay({ events: JSON.stringify([TEST_EVENT]) }); + expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) }); expect(replay['_handleRateLimit']).toHaveBeenCalledTimes(1); expect(replay.resume).not.toHaveBeenCalled();