Skip to content
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.

Sign up for GitHub

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

fix: do not start recording buffer without id #1215

Merged
merged 2 commits into from
Jun 3, 2024
Merged

fix: do not start recording buffer without id #1215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2d3631
fix: do not start recording buffer without id
pauldambra Jun 1, 2024
022253b
correct field declaration
pauldambra Jun 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 46 additions & 15 deletions src/__tests__/extensions/replay/sessionrecording.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,11 @@ describe('SessionRecording', () => {
sessionRecording.startIfEnabledOrStop()
expect(loadScript).toHaveBeenCalled()
expect(sessionRecording['status']).toBe('buffering')
expect(sessionRecording['buffer']).toEqual(EMPTY_BUFFER)
expect(sessionRecording['buffer']).toEqual({
...EMPTY_BUFFER,
sessionId: sessionId,
windowId: 'windowId',
})

const metaSnapshot = createMetaSnapshot({ data: { href: 'https://example.com' } })
_emit(metaSnapshot)
Expand All @@ -290,7 +294,11 @@ describe('SessionRecording', () => {
sessionRecording.startIfEnabledOrStop()
expect(loadScript).toHaveBeenCalled()
expect(sessionRecording['status']).toBe('buffering')
expect(sessionRecording['buffer']).toEqual(EMPTY_BUFFER)
expect(sessionRecording['buffer']).toEqual({
...EMPTY_BUFFER,
sessionId: sessionId,
windowId: 'windowId',
})

const fullSnapshot = createFullSnapshot()
_emit(fullSnapshot)
Expand All @@ -306,7 +314,11 @@ describe('SessionRecording', () => {
sessionRecording.startIfEnabledOrStop()
expect(loadScript).toHaveBeenCalled()
expect(sessionRecording['status']).toBe('buffering')
expect(sessionRecording['buffer']).toEqual(EMPTY_BUFFER)
expect(sessionRecording['buffer']).toEqual({
...EMPTY_BUFFER,
sessionId: sessionId,
windowId: 'windowId',
})

const incrementalSnapshot = createIncrementalSnapshot({ data: { source: 1 } })
_emit(incrementalSnapshot)
Expand All @@ -322,7 +334,11 @@ describe('SessionRecording', () => {
sessionRecording.startIfEnabledOrStop()
expect(loadScript).toHaveBeenCalled()
expect(sessionRecording['status']).toBe('buffering')
expect(sessionRecording['buffer']).toEqual(EMPTY_BUFFER)
expect(sessionRecording['buffer']).toEqual({
...EMPTY_BUFFER,
sessionId: sessionId,
windowId: 'windowId',
})

const incrementalSnapshot = createIncrementalSnapshot({ data: { source: 1 } })
_emit(incrementalSnapshot)
Expand Down Expand Up @@ -717,7 +733,7 @@ describe('SessionRecording', () => {
sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } }))
sessionRecording.startIfEnabledOrStop()

expect(sessionRecording['buffer']?.sessionId).toEqual(null)
expect(sessionRecording['buffer']?.sessionId).toEqual(sessionId)

_emit(createIncrementalSnapshot({ emit: 1 }))

Expand Down Expand Up @@ -883,6 +899,11 @@ describe('SessionRecording', () => {
it('does not send a full snapshot if there is not a new session or window id', () => {
assignableWindow.rrweb.record.takeFullSnapshot.mockClear()

// we always take a full snapshot when there hasn't been one
// and use _fullSnapshotTimer to track that
// we want to avoid that behavior here, so we set it to any value
sessionRecording['_fullSnapshotTimer'] = 1

sessionIdGeneratorMock.mockImplementation(() => 'old-session-id')
windowIdGeneratorMock.mockImplementation(() => 'old-window-id')
sessionManager.resetSessionId()
Expand Down Expand Up @@ -1153,9 +1174,9 @@ describe('SessionRecording', () => {
// the buffer starts out empty
expect(sessionRecording['buffer']).toEqual({
data: [],
sessionId: null,
sessionId: sessionId,
size: 0,
windowId: null,
windowId: 'windowId',
})

// options will have been emitted
Expand All @@ -1164,22 +1185,32 @@ describe('SessionRecording', () => {
})

it('does not emit when idle', () => {
const emptyBuffer = {
...EMPTY_BUFFER,
sessionId: sessionId,
windowId: 'windowId',
}

// force idle state
sessionRecording['isIdle'] = true
// buffer is empty
expect(sessionRecording['buffer']).toEqual(EMPTY_BUFFER)
expect(sessionRecording['buffer']).toEqual(emptyBuffer)
// a plugin event doesn't count as returning from idle
sessionRecording.onRRwebEmit(createPluginSnapshot({}) as unknown as eventWithTime)

// buffer is still empty
expect(sessionRecording['buffer']).toEqual(EMPTY_BUFFER)
expect(sessionRecording['buffer']).toEqual(emptyBuffer)
})

it('emits custom events even when idle', () => {
// force idle state
sessionRecording['isIdle'] = true
// buffer is empty
expect(sessionRecording['buffer']).toEqual(EMPTY_BUFFER)
expect(sessionRecording['buffer']).toEqual({
...EMPTY_BUFFER,
sessionId: sessionId,
windowId: 'windowId',
})

sessionRecording.onRRwebEmit(createCustomSnapshot({}) as unknown as eventWithTime)

Expand All @@ -1194,9 +1225,9 @@ describe('SessionRecording', () => {
type: 5,
},
],
sessionId: null,
sessionId: sessionId,
size: 47,
windowId: null,
windowId: 'windowId',
})
})

Expand Down Expand Up @@ -1609,12 +1640,12 @@ describe('SessionRecording', () => {
expect(sessionRecording['_fullSnapshotTimer']).toBe(undefined)
})

it('schedules a snapshot on start', () => {
it('does not schedule a snapshot on start', () => {
sessionRecording.startIfEnabledOrStop()
expect(sessionRecording['_fullSnapshotTimer']).not.toBe(undefined)
expect(sessionRecording['_fullSnapshotTimer']).toBe(undefined)
})

it('reschedules a snapshot, when we take a full snapshot', () => {
it('schedules a snapshot, when we take a full snapshot', () => {
sessionRecording.startIfEnabledOrStop()
const startTimer = sessionRecording['_fullSnapshotTimer']

Expand Down
16 changes: 8 additions & 8 deletions src/extensions/replay/sessionrecording.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ export class SessionRecording {

private _linkedFlagSeen: boolean = false
private _lastActivityTimestamp: number = Date.now()
private windowId: string | null = null
private sessionId: string | null = null
private windowId: string
private sessionId: string
private _linkedFlag: string | FlagVariant | null = null

private _fullSnapshotTimer?: ReturnType<typeof setInterval>
Expand Down Expand Up @@ -281,6 +281,11 @@ export class SessionRecording {
throw new Error(LOGGER_PREFIX + ' started without valid sessionManager. This is a bug.')
}

// we know there's a sessionManager, so don't need to start without a session id
const { sessionId, windowId } = this.sessionManager.checkAndGetSessionAndWindowId()
this.sessionId = sessionId
this.windowId = windowId

this.buffer = this.clearBuffer()

// on reload there might be an already sampled session that should be continued before decide response,
Expand Down Expand Up @@ -554,7 +559,7 @@ export class SessionRecording {
if (
returningFromIdle ||
([FULL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE].indexOf(event.type) === -1 &&
(windowIdChanged || sessionIdChanged))
(windowIdChanged || sessionIdChanged || isUndefined(this._fullSnapshotTimer)))
) {
this._tryTakeFullSnapshot()
}
Expand Down Expand Up @@ -645,11 +650,6 @@ export class SessionRecording {
},
})

// rrweb takes a snapshot on initialization,
// we want to take one in five minutes
// if nothing else happens to reset the timer
this._scheduleFullSnapshot()

const activePlugins = this._gatherRRWebPlugins()
this.stopRrweb = this.rrwebRecord({
emit: (event) => {
Expand Down
Loading