Skip to content

Commit

Permalink
fix: session id should start null (#878)
Browse files Browse the repository at this point in the history 
* fix: session id nulliness

* fiddle
  • Loading branch information
@pauldambra
pauldambra authored Nov 8, 2023
1 parent 10fd7f4 commit 5417b82
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 31 deletions.
65 changes: 52 additions & 13 deletions src/__tests__/extensions/replay/sessionrecording.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,15 +408,32 @@ describe('SessionRecording', () => {

_emit(createIncrementalSnapshot({ data: { source: 1 } }))
expect(posthog.capture).not.toHaveBeenCalled()
expect(sessionRecording['buffer']?.data.length).toEqual(1)

expect(sessionRecording['buffer']).toEqual({
data: [
{
data: {
source: 1,
},
type: 3,
},
],
// session id and window id are not null 🚀
sessionId: sessionId,
size: 30,
windowId: 'windowId',
})

sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } }))

// next call to emit won't flush the buffer
// the events aren't big enough
_emit(createIncrementalSnapshot({ data: { source: 2 } }))

// access private method 🤯so we don't need to wait for the timer
sessionRecording['_flushBuffer']()

expect(sessionRecording['buffer']?.data.length).toEqual(undefined)

expect(posthog.capture).toHaveBeenCalledTimes(1)
expect(posthog.capture).toHaveBeenCalledWith(
'$snapshot',
Expand Down Expand Up @@ -525,20 +542,27 @@ describe('SessionRecording', () => {
sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } }))
sessionRecording.startRecordingIfEnabled()

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

_emit(createIncrementalSnapshot({ emit: 1 }))

expect(posthog.capture).not.toHaveBeenCalled()
expect(sessionRecording['buffer']?.sessionId).toEqual(sessionId)
expect(sessionRecording['buffer']?.sessionId).not.toEqual(null)
expect(sessionRecording['buffer']?.data).toEqual([{ data: { source: 1 }, emit: 1, type: 3 }])

// Not exactly right but easier to test than rotating the session id
// this simulates as the session id changing _after_ it has initially been set
// i.e. the data in the buffer should be sent with 'otherSessionId'
sessionRecording['buffer']!.sessionId = 'otherSessionId'
_emit(createIncrementalSnapshot())
expect(posthog.capture).toHaveBeenCalled()
_emit(createIncrementalSnapshot({ emit: 2 }))

expect(posthog.capture).toHaveBeenCalledWith(
'$snapshot',
{
$session_id: 'otherSessionId',
$window_id: 'windowId',
$snapshot_data: [{ type: 3, data: { source: 1 } }],
$snapshot_bytes: 30,
$snapshot_data: [{ data: { source: 1 }, emit: 1, type: 3 }],
$snapshot_bytes: 39,
},
{
method: 'POST',
Expand All @@ -549,6 +573,22 @@ describe('SessionRecording', () => {
_metrics: expect.anything(),
}
)

// and the rrweb event emitted _after_ the session id change should be sent yet
expect(sessionRecording['buffer']).toEqual({
data: [
{
data: {
source: 1,
},
emit: 2,
type: 3,
},
],
sessionId: sessionId,
size: 39,
windowId: 'windowId',
})
})

it("doesn't load recording script if already loaded", () => {
Expand Down Expand Up @@ -933,8 +973,7 @@ describe('SessionRecording', () => {
expect(sessionRecording['isIdle']).toEqual(false)
expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100)

// TODO check this with Ben, this was being called because of session id being null
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)

_emit({
event: 123,
Expand All @@ -946,7 +985,7 @@ describe('SessionRecording', () => {
})
expect(sessionRecording['isIdle']).toEqual(false)
expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)

// this triggers idle state and isn't a user interaction so does not take a full snapshot
_emit({
Expand All @@ -959,7 +998,7 @@ describe('SessionRecording', () => {
})
expect(sessionRecording['isIdle']).toEqual(true)
expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)

// this triggers idle state _and_ is a user interaction, so we take a full snapshot
_emit({
Expand All @@ -974,7 +1013,7 @@ describe('SessionRecording', () => {
expect(sessionRecording['_lastActivityTimestamp']).toEqual(
lastActivityTimestamp + RECORDING_IDLE_ACTIVITY_TIMEOUT_MS + 2000
)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2)
})
})
})
Expand Down
40 changes: 22 additions & 18 deletions src/extensions/replay/sessionrecording.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,8 @@ interface SnapshotBuffer {
}

export class SessionRecording {
private _linkedFlagSeen: boolean = false
private instance: PostHog
private _endpoint: string
private windowId: string | null
private sessionId: string | null
private _lastActivityTimestamp: number = Date.now()
private flushBufferTimer?: any
private buffer?: SnapshotBuffer
private mutationRateLimiter?: MutationRateLimiter
Expand All @@ -94,6 +90,11 @@ export class SessionRecording {
private receivedDecide: boolean
private rrwebRecord: rrwebRecord | undefined
private isIdle = false

private _linkedFlagSeen: boolean = false
private _lastActivityTimestamp: number = Date.now()
private windowId: string | null = null
private sessionId: string | null = null
private _linkedFlag: string | null = null
private _sampleRate: number | null = null
private _minimumDuration: number | null = null
Expand Down Expand Up @@ -184,10 +185,6 @@ export class SessionRecording {
throw new Error('Session recording started without valid sessionManager. This is a bug.')
}

const { sessionId, windowId } = this.sessionManager.checkAndGetSessionAndWindowId(true)
this.windowId = windowId
this.sessionId = sessionId

this.buffer = this.clearBuffer()
}

Expand Down Expand Up @@ -383,15 +380,16 @@ export class SessionRecording {

const sessionIdChanged = this.sessionId !== sessionId
const windowIdChanged = this.windowId !== windowId
this.windowId = windowId
this.sessionId = sessionId

if (
[FULL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE].indexOf(event.type) === -1 &&
(windowIdChanged || sessionIdChanged)
) {
this._tryTakeFullSnapshot()
}

this.windowId = windowId
this.sessionId = sessionId
}

private _tryTakeFullSnapshot(): boolean {
Expand Down Expand Up @@ -523,20 +521,20 @@ export class SessionRecording {
const event = truncateLargeConsoleLogs(throttledEvent)
const size = JSON.stringify(event).length

const properties = {
$snapshot_bytes: size,
$snapshot_data: event,
$session_id: this.sessionId,
$window_id: this.windowId,
}

this._updateWindowAndSessionIds(event)

if (this.isIdle) {
// When in an idle state we keep recording, but don't capture the events
return
}

const properties = {
$snapshot_bytes: size,
$snapshot_data: event,
$session_id: this.sessionId,
$window_id: this.windowId,
}

if (this.status !== 'disabled') {
this._captureSnapshotBuffered(properties)
} else {
Expand Down Expand Up @@ -614,11 +612,17 @@ export class SessionRecording {
if (
!this.buffer ||
this.buffer.size + properties.$snapshot_bytes + additionalBytes > RECORDING_MAX_EVENT_SIZE ||
this.buffer.sessionId !== this.sessionId
(!!this.buffer.sessionId && this.buffer.sessionId !== this.sessionId)
) {
this.buffer = this._flushBuffer()
}

if (_isNull(this.buffer.sessionId) && !_isNull(this.sessionId)) {
// session id starts null but has now been assigned, update the buffer
this.buffer.sessionId = this.sessionId
this.buffer.windowId = this.windowId
}

this.buffer.size += properties.$snapshot_bytes
this.buffer.data.push(properties.$snapshot_data)

Expand Down

0 comments on commit 5417b82

Please sign in to comment.