-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(replay): Add beforeAddRecordingEvent
Replay option
#8124
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -183,6 +183,10 @@ export interface WorkerResponse { | |
|
||
export type AddEventResult = void; | ||
|
||
export interface BeforeAddRecoringEvent { | ||
(event: RecordingEvent): RecordingEvent | null | undefined; | ||
} | ||
|
||
export interface ReplayNetworkOptions { | ||
/** | ||
* Capture request/response details for XHR/Fetch requests that match the given URLs. | ||
|
@@ -267,6 +271,11 @@ export interface ReplayPluginOptions extends ReplayNetworkOptions { | |
*/ | ||
maskAllText: boolean; | ||
|
||
/** | ||
* Callback before adding a recording event | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l: Let's add a sentence about what to use this function for and what to return(i.e. returning null will drop the event). Just to make it a little clearer to users There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, we should probably point out that dropping one event here won't result in the entire replay being dropped, wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've also added the condition that we only call this callback for custom events, e.g. only when event.type == 5 (otherwise we risk user error breaking the replay), thoughts on that @Lms24? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that sounds good to me 👍 less chances for folks to screw things up with the actual replay! Let's just make sure we point this out in the JSDoc |
||
beforeAddRecordingEvent?: BeforeAddRecoringEvent; | ||
|
||
/** | ||
* _experiments allows users to enable experimental or internal features. | ||
* We don't consider such features as part of the public API and hence we don't guarantee semver for them. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
import * as SentryCore from '@sentry/core'; | ||
import type { Transport } from '@sentry/types'; | ||
import * as SentryUtils from '@sentry/utils'; | ||
|
||
import type { Replay } from '../../src'; | ||
import type { ReplayContainer } from '../../src/replay'; | ||
import { clearSession } from '../../src/session/clearSession'; | ||
import * as SendReplayRequest from '../../src/util/sendReplayRequest'; | ||
import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; | ||
import { useFakeTimers } from '../utils/use-fake-timers'; | ||
|
||
useFakeTimers(); | ||
|
||
async function advanceTimers(time: number) { | ||
jest.advanceTimersByTime(time); | ||
await new Promise(process.nextTick); | ||
} | ||
|
||
type MockTransportSend = jest.MockedFunction<Transport['send']>; | ||
|
||
describe('Integration | beforeAddRecordingEvent', () => { | ||
let replay: ReplayContainer; | ||
let integration: Replay; | ||
let mockTransportSend: MockTransportSend; | ||
let mockSendReplayRequest: jest.SpyInstance<any>; | ||
let domHandler: (args: any) => any; | ||
const { record: mockRecord } = mockRrweb(); | ||
|
||
beforeAll(async () => { | ||
jest.setSystemTime(new Date(BASE_TIMESTAMP)); | ||
jest.spyOn(SentryUtils, 'addInstrumentationHandler').mockImplementation((type, handler: (args: any) => any) => { | ||
if (type === 'dom') { | ||
domHandler = handler; | ||
} | ||
}); | ||
|
||
({ replay, integration } = await mockSdk({ | ||
replayOptions: { | ||
beforeAddRecordingEvent: event => { | ||
const eventData = event.data as Record<string, any>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Types are not ideal here :/ I'll work on a follow-up to see how we can improve them. |
||
|
||
if (eventData.tag === 'breadcrumb' && eventData.payload.category === 'ui.click') { | ||
return { | ||
...event, | ||
data: { | ||
...eventData, | ||
payload: { | ||
...eventData.payload, | ||
message: 'beforeAddRecordingEvent', | ||
}, | ||
}, | ||
}; | ||
} | ||
|
||
if (eventData.tag === 'options') { | ||
return null; | ||
} | ||
|
||
return event; | ||
}, | ||
_experiments: { | ||
captureExceptions: true, | ||
}, | ||
}, | ||
})); | ||
|
||
mockSendReplayRequest = jest.spyOn(SendReplayRequest, 'sendReplayRequest'); | ||
|
||
jest.runAllTimers(); | ||
mockTransportSend = SentryCore.getCurrentHub()?.getClient()?.getTransport()?.send as MockTransportSend; | ||
}); | ||
|
||
beforeEach(() => { | ||
jest.setSystemTime(new Date(BASE_TIMESTAMP)); | ||
mockRecord.takeFullSnapshot.mockClear(); | ||
mockTransportSend.mockClear(); | ||
|
||
// 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['_loadAndCheckSession'](); | ||
|
||
mockSendReplayRequest.mockClear(); | ||
}); | ||
|
||
afterEach(async () => { | ||
jest.runAllTimers(); | ||
await new Promise(process.nextTick); | ||
jest.setSystemTime(new Date(BASE_TIMESTAMP)); | ||
clearSession(replay); | ||
replay['_loadAndCheckSession'](); | ||
}); | ||
|
||
afterAll(() => { | ||
integration && integration.stop(); | ||
}); | ||
|
||
it('changes click breadcrumbs message', async () => { | ||
domHandler({ | ||
name: 'click', | ||
}); | ||
|
||
await advanceTimers(5000); | ||
|
||
expect(replay).toHaveLastSentReplay({ | ||
recordingPayloadHeader: { segment_id: 0 }, | ||
recordingData: JSON.stringify([ | ||
{ | ||
type: 5, | ||
timestamp: BASE_TIMESTAMP, | ||
data: { | ||
tag: 'breadcrumb', | ||
payload: { | ||
timestamp: BASE_TIMESTAMP / 1000, | ||
type: 'default', | ||
category: 'ui.click', | ||
message: 'beforeAddRecordingEvent', | ||
data: {}, | ||
}, | ||
}, | ||
}, | ||
]), | ||
}); | ||
}); | ||
|
||
it('filters out the options event', async () => { | ||
mockTransportSend.mockClear(); | ||
await integration.stop(); | ||
|
||
integration.start(); | ||
|
||
jest.runAllTimers(); | ||
await new Promise(process.nextTick); | ||
expect(replay).toHaveLastSentReplay({ | ||
recordingPayloadHeader: { segment_id: 0 }, | ||
recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }]), | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No action required, just as a side note: You don't have to add this entry to the unreleased section. We always create a changelog PR when we release which is when we add all entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah thought it would make it easier for whoever is publishing