Skip to content

Commit

Permalink
fix(replay): Ignore max session life for buffered sessions
Browse files Browse the repository at this point in the history 
Theres an edge case where a buffered session becomes expired, an error comes in, the link from error to replay is lost because a new session is created due to the session being expired.

We should either ignore the max session life for a buffered session, or possibly check/refresh session when an error comes in.
  • Loading branch information
@billyvg
billyvg committed May 30, 2023
1 parent e95e574 commit 88781b1
Show file tree
Hide file tree
Showing 5 changed files with 159 additions and 38 deletions.
11 changes: 10 additions & 1 deletion packages/replay/src/replay.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,15 @@ export class ReplayContainer implements ReplayContainerInterface {
// Once this session ends, we do not want to refresh it
if (this.session) {
this.session.shouldRefresh = false;

// It's possible that the session lifespan is > max session lifespan
// because we have been buffering (which ignores expiration given that
// `shouldRefresh` is true). Since we flip `shouldRefresh`, the session
// could be considered expired due to lifespan. Update session start date
// to the earliest event in buffer, or current timestamp.
this._updateUserActivity();
this._updateSessionActivity();
this.session.started = Date.now();
this._maybeSaveSession();
}

Expand Down Expand Up @@ -657,7 +666,7 @@ export class ReplayContainer implements ReplayContainerInterface {
stickySession: Boolean(this._options.stickySession),
currentSession: this.session,
sessionSampleRate: this._options.sessionSampleRate,
allowBuffering: this._options.errorSampleRate > 0,
allowBuffering: this._options.errorSampleRate > 0 || this.recordingMode === 'buffer',
});

// If session was newly created (i.e. was not loaded from storage), then
Expand Down
8 changes: 5 additions & 3 deletions packages/replay/src/session/getSession.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,13 @@ export function getSession({
// within "max session time").
const isExpired = isSessionExpired(session, timeouts);

if (!isExpired) {
if (!isExpired || (allowBuffering && session.shouldRefresh)) {
return { type: 'saved', session };
} else if (!session.shouldRefresh) {
// In this case, stop
// This is the case if we have an error session that is completed (=triggered an error)
// This is the case if we have an error session that is completed
// (=triggered an error). Session will continue as session-based replay,
// and when this session is expired, it will not be renewed until user
// reloads.
const discardedSession = makeSession({ sampled: false });
return { type: 'new', session: discardedSession };
} else {
Expand Down
52 changes: 36 additions & 16 deletions packages/replay/test/integration/errorSampleRate-delayFlush.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ describe('Integration | errorSampleRate with delayed flush', () => {

await waitForBufferFlush();

expect(replay.session?.started).toBe(BASE_TIMESTAMP + ELAPSED + 20);
expect(replay.session?.started).toBe(BASE_TIMESTAMP + ELAPSED + DEFAULT_FLUSH_MIN_DELAY + 80);

// Does not capture mouse click
expect(replay).toHaveSentReplay({
Expand Down Expand Up @@ -662,7 +662,8 @@ describe('Integration | errorSampleRate with delayed flush', () => {
expect(replay.isEnabled()).toBe(false);
});

it('stops replay when session exceeds max length', async () => {
it('stops replay when session exceeds max length after latest captured error', async () => {
const sessionId = replay.session?.id;
jest.setSystemTime(BASE_TIMESTAMP);

const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
Expand All @@ -674,38 +675,57 @@ describe('Integration | errorSampleRate with delayed flush', () => {
jest.runAllTimers();
await new Promise(process.nextTick);

captureException(new Error('testing'));
jest.advanceTimersByTime(2 * MAX_SESSION_LIFE);

jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
await new Promise(process.nextTick);

expect(replay).not.toHaveLastSentReplay();
captureException(new Error('testing'));

// Wait a bit, shortly before session expires
jest.advanceTimersByTime(MAX_SESSION_LIFE - 1000);
// Flush due to exception
await new Promise(process.nextTick);
await waitForFlush();

mockRecord._emitter(TEST_EVENT);
replay.triggerUserActivity();
expect(replay.session?.id).toBe(sessionId);
expect(replay).toHaveLastSentReplay({
recordingPayloadHeader: { segment_id: 0 },
});

expect(replay).toHaveLastSentReplay();
// This comes from `startRecording()` in `sendBufferedReplayOrFlush()`
await waitForFlush();
expect(replay).toHaveLastSentReplay({
recordingPayloadHeader: { segment_id: 1 },
recordingData: JSON.stringify([
{
data: {
isCheckout: true,
},
timestamp: BASE_TIMESTAMP + 2 * MAX_SESSION_LIFE + DEFAULT_FLUSH_MIN_DELAY + 40,
type: 2,
},
]),
});

// Now wait after session expires - should stop recording
mockRecord.takeFullSnapshot.mockClear();
(getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>).mockClear();

jest.advanceTimersByTime(10_000);
jest.advanceTimersByTime(MAX_SESSION_LIFE);
await new Promise(process.nextTick);

mockRecord._emitter(TEST_EVENT);
replay.triggerUserActivity();

jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
jest.runAllTimers();
await new Promise(process.nextTick);

expect(replay).not.toHaveLastSentReplay();
expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
expect(replay.isEnabled()).toBe(false);

// Once the session is stopped after capturing a replay already
// (buffer-mode), another error will not trigger a new replay
captureException(new Error('testing'));

await new Promise(process.nextTick);
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
await new Promise(process.nextTick);
expect(replay).not.toHaveLastSentReplay();
});
});

Expand Down
67 changes: 49 additions & 18 deletions packages/replay/test/integration/errorSampleRate.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,9 @@ describe('Integration | errorSampleRate', () => {
['MAX_SESSION_LIFE', MAX_SESSION_LIFE],
['SESSION_IDLE_EXPIRE_DURATION', SESSION_IDLE_EXPIRE_DURATION],
])('continues buffering replay if session had no error and exceeds %s', async (_label, waitTime) => {
const oldSessionId = replay.session?.id;
expect(oldSessionId).toBeDefined();

expect(replay).not.toHaveLastSentReplay();

// Idle for given time
Expand Down Expand Up @@ -475,13 +478,24 @@ describe('Integration | errorSampleRate', () => {
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
await new Promise(process.nextTick);

expect(replay).toHaveLastSentReplay({
expect(replay.session?.id).toBe(oldSessionId);

// Flush of buffered events
expect(replay).toHaveSentReplay({
recordingPayloadHeader: { segment_id: 0 },
replayEventPayload: expect.objectContaining({
replay_type: 'buffer',
}),
});

// Checkout from `startRecording`
expect(replay).toHaveLastSentReplay({
recordingPayloadHeader: { segment_id: 1 },
replayEventPayload: expect.objectContaining({
replay_type: 'buffer',
}),
});

expect(replay.isEnabled()).toBe(true);
expect(replay.isPaused()).toBe(false);
expect(replay.recordingMode).toBe('session');
Expand All @@ -491,6 +505,9 @@ describe('Integration | errorSampleRate', () => {

// Should behave the same as above test
it('stops replay if user has been idle for more than SESSION_IDLE_EXPIRE_DURATION and does not start a new session thereafter', async () => {
const oldSessionId = replay.session?.id;
expect(oldSessionId).toBeDefined();

// Idle for 15 minutes
jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION + 1);

Expand All @@ -517,14 +534,24 @@ describe('Integration | errorSampleRate', () => {
await new Promise(process.nextTick);
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
await new Promise(process.nextTick);
expect(replay.session?.id).toBe(oldSessionId);

expect(replay).toHaveLastSentReplay({
// buffered events
expect(replay).toHaveSentReplay({
recordingPayloadHeader: { segment_id: 0 },
replayEventPayload: expect.objectContaining({
replay_type: 'buffer',
}),
});

// `startRecording` full checkout
expect(replay).toHaveLastSentReplay({
recordingPayloadHeader: { segment_id: 1 },
replayEventPayload: expect.objectContaining({
replay_type: 'buffer',
}),
});

expect(replay.isEnabled()).toBe(true);
expect(replay.isPaused()).toBe(false);
expect(replay.recordingMode).toBe('session');
Expand Down Expand Up @@ -605,7 +632,7 @@ describe('Integration | errorSampleRate', () => {
jest.advanceTimersByTime(20);
await new Promise(process.nextTick);

expect(replay.session?.started).toBe(BASE_TIMESTAMP + ELAPSED + 20);
expect(replay.session?.started).toBe(BASE_TIMESTAMP + ELAPSED + 100);

// Does not capture mouse click
expect(replay).toHaveSentReplay({
Expand Down Expand Up @@ -667,7 +694,8 @@ describe('Integration | errorSampleRate', () => {
expect(replay.isEnabled()).toBe(false);
});

it('stops replay when session exceeds max length', async () => {
it('stops replay when session exceeds max length after latest captured error', async () => {
const sessionId = replay.session?.id;
jest.setSystemTime(BASE_TIMESTAMP);

const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
Expand All @@ -679,37 +707,40 @@ describe('Integration | errorSampleRate', () => {
jest.runAllTimers();
await new Promise(process.nextTick);

jest.advanceTimersByTime(2 * MAX_SESSION_LIFE);

captureException(new Error('testing'));

jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
// Flush due to exception
await new Promise(process.nextTick);
expect(replay).not.toHaveLastSentReplay();

// Wait a bit, shortly before session expires
jest.advanceTimersByTime(MAX_SESSION_LIFE - 1000);
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
await new Promise(process.nextTick);

mockRecord._emitter(TEST_EVENT);
replay.triggerUserActivity();

expect(replay.session?.id).toBe(sessionId);
expect(replay).toHaveLastSentReplay();

// Now wait after session expires - should stop recording
// Now wait after session expires - should re-start into buffering mode
mockRecord.takeFullSnapshot.mockClear();
(getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>).mockClear();

jest.advanceTimersByTime(10_000);
jest.advanceTimersByTime(MAX_SESSION_LIFE);
await new Promise(process.nextTick);

mockRecord._emitter(TEST_EVENT);
replay.triggerUserActivity();

jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
jest.runAllTimers();
await new Promise(process.nextTick);

expect(replay).not.toHaveLastSentReplay();
expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
expect(replay.isEnabled()).toBe(false);

// Once the session is stopped after capturing a replay already
// (buffer-mode), another error should trigger a new replay
captureException(new Error('testing'));

await new Promise(process.nextTick);
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
await new Promise(process.nextTick);
expect(replay).not.toHaveLastSentReplay();
});
});

Expand Down
59 changes: 59 additions & 0 deletions packages/replay/test/unit/session/getSession.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,4 +229,63 @@ describe('Unit | session | getSession', () => {
expect(session.id).toBe('test_session_uuid_2');
expect(session.segmentId).toBe(0);
});

it('re-uses the same "buffer" session if it is expired and has never sent a buffered replay', function () {
const { type, session } = getSession({
timeouts: {
sessionIdlePause: SESSION_IDLE_PAUSE_DURATION,
sessionIdleExpire: 1000,
maxSessionLife: MAX_SESSION_LIFE,
},
stickySession: false,
...SAMPLE_OPTIONS,
currentSession: makeSession({
id: 'test_session_uuid_2',
lastActivity: +new Date() - MAX_SESSION_LIFE - 1,
started: +new Date() - MAX_SESSION_LIFE - 1,
segmentId: 0,
sampled: 'buffer',
}),
allowBuffering: true,
});

expect(FetchSession.fetchSession).not.toHaveBeenCalled();
expect(CreateSession.createSession).not.toHaveBeenCalled();

expect(type).toBe('saved');
expect(session.id).toBe('test_session_uuid_2');
expect(session.sampled).toBe('buffer');
expect(session.segmentId).toBe(0);
});

it('creates a new session if it is expired and it was a "buffer" session that has sent a replay', function () {
const currentSession = makeSession({
id: 'test_session_uuid_2',
lastActivity: +new Date() - MAX_SESSION_LIFE - 1,
started: +new Date() - MAX_SESSION_LIFE - 1,
segmentId: 0,
sampled: 'buffer',
});
currentSession.shouldRefresh = false;

const { type, session } = getSession({
timeouts: {
sessionIdlePause: SESSION_IDLE_PAUSE_DURATION,
sessionIdleExpire: 1000,
maxSessionLife: MAX_SESSION_LIFE,
},
stickySession: false,
...SAMPLE_OPTIONS,
currentSession,
allowBuffering: true,
});

expect(FetchSession.fetchSession).not.toHaveBeenCalled();
expect(CreateSession.createSession).not.toHaveBeenCalled();

expect(type).toBe('new');
expect(session.id).not.toBe('test_session_uuid_2');
expect(session.sampled).toBe(false);
expect(session.segmentId).toBe(0);
});
});

0 comments on commit 88781b1

Please sign in to comment.