fix(replay): Fully stop & restart session when it expires#8834

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

Merged
mydea merged 3 commits into from
Sep 11, 2023
Merged

fix(replay): Fully stop & restart session when it expires #8834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
befae60
fix(replay): Fully stop & restart session when it expires
mydea Aug 18, 2023
7bc5c80
simplify checkAndHandleExpiredSession check
mydea Sep 1, 2023
7b34dd7
PR cleanup
mydea Sep 11, 2023
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
118 changes: 48 additions & 70 deletions packages/replay/src/replay.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -18,8 +18,8 @@ import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
import { createEventBuffer } from './eventBuffer';
import { clearSession } from './session/clearSession';
import { loadOrCreateSession } from './session/loadOrCreateSession';
import { maybeRefreshSession } from './session/maybeRefreshSession';
import { saveSession } from './session/saveSession';
import { shouldRefreshSession } from './session/shouldRefreshSession';
import type {
AddEventResult,
AddUpdateCallback,
Expand DownExpand Up@@ -217,7 +217,7 @@ export class ReplayContainer implements ReplayContainerInterface {
* Initializes the plugin based on sampling configuration. Should not be
* called outside of constructor.
*/
public initializeSampling(): void {
public initializeSampling(previousSessionId?: string): void {

Check failure

Code scanning / CodeQL

Insecure randomness

This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.
const { errorSampleRate, sessionSampleRate } = this._options;

// If neither sample rate is > 0, then do nothing - user will need to call one of
Expand All@@ -228,7 +228,7 @@ export class ReplayContainer implements ReplayContainerInterface {

// Otherwise if there is _any_ sample rate set, try to load an existing
// session, or create a new one.
this._initializeSessionForSampling();
this._initializeSessionForSampling(previousSessionId);

if (!this.session) {
// This should not happen, something wrong has occurred
Expand DownExpand Up@@ -273,7 +273,6 @@ export class ReplayContainer implements ReplayContainerInterface {
logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);

const session = loadOrCreateSession(
this.session,
{
maxReplayDuration: this._options.maxReplayDuration,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
Expand DownExpand Up@@ -304,7 +303,6 @@ export class ReplayContainer implements ReplayContainerInterface {
logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);

const session = loadOrCreateSession(
this.session,
{
sessionIdleExpire: this.timeouts.sessionIdleExpire,
maxReplayDuration: this._options.maxReplayDuration,
Expand DownExpand Up@@ -373,15 +371,16 @@ export class ReplayContainer implements ReplayContainerInterface {
return;
}

// We can't move `_isEnabled` after awaiting a flush, otherwise we can
// enter into an infinite loop when `stop()` is called while flushing.
this._isEnabled = false;

@mydeamydeaAug 17, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔

@billyvgbillyvgAug 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, since try/catch inside of an async function is sugar on top of promises.


try {
logInfo(
`[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
this._options._experiments.traceInternals,
);

// We can't move `_isEnabled` after awaiting a flush, otherwise we can
// enter into an infinite loop when `stop()` is called while flushing.
this._isEnabled = false;
this._removeListeners();
this.stopRecording();

Expand DownExpand Up@@ -475,16 +474,6 @@ 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 beyond max session lifespan (we ignore
// expiration given that `shouldRefresh` is true). Since we flip
// `shouldRefresh`, the session could be considered expired due to
// lifespan, which is not what we want. Update session start date to be
// the current timestamp, so that session is not considered to be
// expired. This means that max replay duration can be MAX_REPLAY_DURATION +
// (length of buffer), which we are ok with.
this._updateUserActivity(activityTime);
this._updateSessionActivity(activityTime);
this._maybeSaveSession();
Expand DownExpand Up@@ -612,8 +601,6 @@ export class ReplayContainer implements ReplayContainerInterface {
* @hidden
*/
public checkAndHandleExpiredSession(): boolean | void {
const oldSessionId = this.getSessionId();

// Prevent starting a new session if the last user activity is older than
// SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
// session+recording. This creates noisy replays that do not have much
Expand All@@ -635,24 +622,11 @@ export class ReplayContainer implements ReplayContainerInterface {
// --- There is recent user activity --- //
// This will create a new session if expired, based on expiry length
if (!this._checkSession()) {
return;
}

// Session was expired if session ids do not match
const expired = oldSessionId !== this.getSessionId();

if (!expired) {
return true;
}

// Session is expired, trigger a full snapshot (which will create a new session)
if (this.isPaused()) {
this.resume();
} else {
this._triggerFullSnapshot();
// Check session handles the refreshing itself
return false;
}

return false;
return true;
}

/**
Expand DownExpand Up@@ -740,6 +714,7 @@ export class ReplayContainer implements ReplayContainerInterface {

// Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
this._isEnabled = true;
this._isPaused = false;

@mydeamydeaAug 17, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we are also unpaused here, when starting new.


this.startRecording();
}
Expand All@@ -756,17 +731,17 @@ export class ReplayContainer implements ReplayContainerInterface {
/**
* Loads (or refreshes) the current session.
*/
private _initializeSessionForSampling(): void {
private _initializeSessionForSampling(previousSessionId?: string): void {
// Whenever there is _any_ error sample rate, we always allow buffering
// Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
const allowBuffering = this._options.errorSampleRate > 0;

const session = loadOrCreateSession(
this.session,
{
sessionIdleExpire: this.timeouts.sessionIdleExpire,
maxReplayDuration: this._options.maxReplayDuration,
traceInternals: this._options._experiments.traceInternals,
previousSessionId,
},
{
stickySession: this._options.stickySession,
Expand All@@ -791,37 +766,32 @@ export class ReplayContainer implements ReplayContainerInterface {

const currentSession = this.session;

const newSession = maybeRefreshSession(
currentSession,
{
if (
shouldRefreshSession(currentSession, {
sessionIdleExpire: this.timeouts.sessionIdleExpire,
traceInternals: this._options._experiments.traceInternals,
maxReplayDuration: this._options.maxReplayDuration,
},
{
stickySession: Boolean(this._options.stickySession),
sessionSampleRate: this._options.sessionSampleRate,
allowBuffering: this._options.errorSampleRate > 0,
},
);

const isNew = newSession.id !== currentSession.id;

// If session was newly created (i.e. was not loaded from storage), then
// enable flag to create the root replay
if (isNew) {
this.setInitialState();
this.session = newSession;
}

if (!this.session.sampled) {
void this.stop({ reason: 'session not refreshed' });
})
) {
void this._refreshSession(currentSession);
return false;
}

return true;
}

/**
* Refresh a session with a new one.
* This stops the current session (without forcing a flush, as that would never work since we are expired),
* and then does a new sampling based on the refreshed session.
*/
private async _refreshSession(session: Session): Promise<void> {
if (!this._isEnabled) {
return;
}
await this.stop({ reason: 'refresh session' });
this.initializeSampling(session.id);
}

/**
* Adds listeners to record events for the replay
*/
Expand DownExpand Up@@ -933,10 +903,14 @@ export class ReplayContainer implements ReplayContainerInterface {

const expired = isSessionExpired(this.session, {
maxReplayDuration: this._options.maxReplayDuration,
...this.timeouts,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
});

if (breadcrumb && !expired) {
if (expired) {
return;
}

if (breadcrumb) {
this._createCustomBreadcrumb(breadcrumb);
}

Expand DownExpand Up@@ -1081,7 +1055,9 @@ export class ReplayContainer implements ReplayContainerInterface {
* Should never be called directly, only by `flush`
*/
private async _runFlush(): Promise<void> {
if (!this.session || !this.eventBuffer) {
const replayId = this.getSessionId();

@mydeamydeaAug 17, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.


if (!this.session || !this.eventBuffer || !replayId) {
__DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
return;
}
Expand All@@ -1101,13 +1077,15 @@ export class ReplayContainer implements ReplayContainerInterface {
return;
}

// if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
if (replayId !== this.getSessionId()) {
return;

@mydeamydeaAug 17, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?

@billyvgbillyvgAug 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here

}

try {
// This uses the data from the eventBuffer, so we need to call this before `finish()
this._updateInitialTimestampFromEventBuffer();

// Note this empties the event buffer regardless of outcome of sending replay
const recordingData = await this.eventBuffer.finish();

const timestamp = Date.now();

// Check total duration again, to avoid sending outdated stuff
Expand All@@ -1117,14 +1095,14 @@ export class ReplayContainer implements ReplayContainerInterface {
throw new Error('Session is too long, not sending replay');
}

// NOTE: Copy values from instance members, as it's possible they could
// change before the flush finishes.
const replayId = this.session.id;
const eventContext = this._popEventContext();
// Always increment segmentId regardless of outcome of sending replay
const segmentId = this.session.segmentId++;
this._maybeSaveSession();

// Note this empties the event buffer regardless of outcome of sending replay
const recordingData = await this.eventBuffer.finish();

@mydeamydeaAug 17, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this down here, the idea is:

  1. We capture the timestamp, event context etc. first
  2. Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.


await sendReplay({
replayId,
recordingData,
Expand Down
2 changes: 0 additions & 2 deletions packages/replay/src/session/Session.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
const lastActivity = session.lastActivity || now;
const segmentId = session.segmentId || 0;
const sampled = session.sampled;
const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
const previousSessionId = session.previousSessionId;

return {
Expand All@@ -22,7 +21,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
lastActivity,
segmentId,
sampled,
shouldRefresh,
previousSessionId,
};
}
19 changes: 12 additions & 7 deletions packages/replay/src/session/loadOrCreateSession.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,33 +2,38 @@ import type { Session, SessionOptions } from '../types';
import { logInfoNextTick } from '../util/log';
import { createSession } from './createSession';
import { fetchSession } from './fetchSession';
import { maybeRefreshSession } from './maybeRefreshSession';
import { shouldRefreshSession } from './shouldRefreshSession';

/**
* Get or create a session, when initializing the replay.
* Returns a session that may be unsampled.
*/
export function loadOrCreateSession(
currentSession: Session | undefined,
{
traceInternals,
sessionIdleExpire,
maxReplayDuration,
previousSessionId,
}: {
sessionIdleExpire: number;
maxReplayDuration: number;
traceInternals?: boolean;
previousSessionId?: string;
},
sessionOptions: SessionOptions,
): Session {
// If session exists and is passed, use it instead of always hitting session storage
const existingSession = currentSession || (sessionOptions.stickySession && fetchSession(traceInternals));
const existingSession = sessionOptions.stickySession && fetchSession(traceInternals);

// No session exists yet, just create a new one
if (!existingSession) {
logInfoNextTick('[Replay] Created new session', traceInternals);
return createSession(sessionOptions);
logInfoNextTick('[Replay] Creating new session', traceInternals);
return createSession(sessionOptions, { previousSessionId });
}

return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions);
if (!shouldRefreshSession(existingSession, { sessionIdleExpire, maxReplayDuration })) {
return existingSession;
}

logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
return createSession(sessionOptions, { previousSessionId: existingSession.id });

Check failure

Code scanning / CodeQL

Insecure randomness

This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.
}
50 changes: 0 additions & 50 deletions packages/replay/src/session/maybeRefreshSession.ts
View file
Open in desktop

This file was deleted.

Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
fix(replay): Fully stop & restart session when it expires#8834

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

 Merged

mydea
merged 3 commits into

from

Sep 11, 2023
 Merged


fix(replay): Fully stop & restart session when it expires
#8834
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
befae60

fix(replay): Fully stop & restart session when it expires


mydea Aug 18, 2023
7bc5c80

simplify checkAndHandleExpiredSession check


mydea Sep 1, 2023
7b34dd7

PR cleanup


mydea Sep 11, 2023

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


118 changes: 48 additions & 70 deletions
packages/replay/src/replay.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,8 +18,8 @@ import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
import { createEventBuffer } from './eventBuffer';
import { clearSession } from './session/clearSession';
import { loadOrCreateSession } from './session/loadOrCreateSession';
import { maybeRefreshSession } from './session/maybeRefreshSession';
import { saveSession } from './session/saveSession';
import { shouldRefreshSession } from './session/shouldRefreshSession';
import type {
 AddEventResult,
 AddUpdateCallback,
Expand DownExpand Up@@ -217,7 +217,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Initializes the plugin based on sampling configuration. Should not be
 * called outside of constructor.
 */
 public initializeSampling(): void {
 public initializeSampling(previousSessionId?: string): void {

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

 const { errorSampleRate, sessionSampleRate } = this._options;

 // If neither sample rate is > 0, then do nothing - user will need to call one of
Expand All@@ -228,7 +228,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Otherwise if there is _any_ sample rate set, try to load an existing
 // session, or create a new one.
 this._initializeSessionForSampling();
 this._initializeSessionForSampling(previousSessionId);

 if (!this.session) {
 // This should not happen, something wrong has occurred
Expand DownExpand Up@@ -273,7 +273,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 maxReplayDuration: this._options.maxReplayDuration,
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
Expand DownExpand Up@@ -304,7 +303,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
Expand DownExpand Up@@ -373,15 +371,16 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, since try/catch inside of an async function is sugar on top of promises.

 try {
 logInfo(
 `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
 this._options._experiments.traceInternals,
 );

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
 this._removeListeners();
 this.stopRecording();

Expand DownExpand Up@@ -475,16 +474,6 @@ 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 beyond max session lifespan (we ignore
 // expiration given that `shouldRefresh` is true). Since we flip
 // `shouldRefresh`, the session could be considered expired due to
 // lifespan, which is not what we want. Update session start date to be
 // the current timestamp, so that session is not considered to be
 // expired. This means that max replay duration can be MAX_REPLAY_DURATION +
 // (length of buffer), which we are ok with.
 this._updateUserActivity(activityTime);
 this._updateSessionActivity(activityTime);
 this._maybeSaveSession();
Expand DownExpand Up@@ -612,8 +601,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 * @hidden
 */
 public checkAndHandleExpiredSession(): boolean | void {
 const oldSessionId = this.getSessionId();

 // Prevent starting a new session if the last user activity is older than
 // SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
 // session+recording. This creates noisy replays that do not have much
Expand All@@ -635,24 +622,11 @@ export class ReplayContainer implements ReplayContainerInterface {
 // --- There is recent user activity --- //
 // This will create a new session if expired, based on expiry length
 if (!this._checkSession()) {
 return;
 }

 // Session was expired if session ids do not match
 const expired = oldSessionId !== this.getSessionId();

 if (!expired) {
 return true;
 }

 // Session is expired, trigger a full snapshot (which will create a new session)
 if (this.isPaused()) {
 this.resume();
 } else {
 this._triggerFullSnapshot();
 // Check session handles the refreshing itself
 return false;
 }

 return false;
 return true;
 }

 /**
Expand DownExpand Up@@ -740,6 +714,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
 this._isEnabled = true;
 this._isPaused = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we are also unpaused here, when starting new.

 this.startRecording();
 }
Expand All@@ -756,17 +731,17 @@ export class ReplayContainer implements ReplayContainerInterface {
 /**
 * Loads (or refreshes) the current session.
 */
 private _initializeSessionForSampling(): void {
 private _initializeSessionForSampling(previousSessionId?: string): void {
 // Whenever there is _any_ error sample rate, we always allow buffering
 // Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
 const allowBuffering = this._options.errorSampleRate > 0;

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
 traceInternals: this._options._experiments.traceInternals,
 previousSessionId,
 },
 {
 stickySession: this._options.stickySession,
Expand All@@ -791,37 +766,32 @@ export class ReplayContainer implements ReplayContainerInterface {

 const currentSession = this.session;

 const newSession = maybeRefreshSession(
 currentSession,
 {
 if (
 shouldRefreshSession(currentSession, {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 traceInternals: this._options._experiments.traceInternals,
 maxReplayDuration: this._options.maxReplayDuration,
 },
 {
 stickySession: Boolean(this._options.stickySession),
 sessionSampleRate: this._options.sessionSampleRate,
 allowBuffering: this._options.errorSampleRate > 0,
 },
 );

 const isNew = newSession.id !== currentSession.id;

 // If session was newly created (i.e. was not loaded from storage), then
 // enable flag to create the root replay
 if (isNew) {
 this.setInitialState();
 this.session = newSession;
 }

 if (!this.session.sampled) {
 void this.stop({ reason: 'session not refreshed' });
 })
 ) {
 void this._refreshSession(currentSession);
 return false;
 }

 return true;
 }

 /**
 * Refresh a session with a new one.
 * This stops the current session (without forcing a flush, as that would never work since we are expired),
 * and then does a new sampling based on the refreshed session.
 */
 private async _refreshSession(session: Session): Promise<void> {
 if (!this._isEnabled) {
 return;
 }
 await this.stop({ reason: 'refresh session' });
 this.initializeSampling(session.id);
 }

 /**
 * Adds listeners to record events for the replay
 */
Expand DownExpand Up@@ -933,10 +903,14 @@ export class ReplayContainer implements ReplayContainerInterface {

 const expired = isSessionExpired(this.session, {
 maxReplayDuration: this._options.maxReplayDuration,
...this.timeouts,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
 });

 if (breadcrumb && !expired) {
 if (expired) {
 return;
 }

 if (breadcrumb) {
 this._createCustomBreadcrumb(breadcrumb);
 }

Expand DownExpand Up@@ -1081,7 +1055,9 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Should never be called directly, only by `flush`
 */
 private async _runFlush(): Promise<void> {
 if (!this.session || !this.eventBuffer) {
 const replayId = this.getSessionId();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.

 if (!this.session || !this.eventBuffer || !replayId) {
 __DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
 return;
 }
Expand All@@ -1101,13 +1077,15 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
 if (replayId !== this.getSessionId()) {
 return;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
 }

 try {
 // This uses the data from the eventBuffer, so we need to call this before `finish()
 this._updateInitialTimestampFromEventBuffer();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();

 const timestamp = Date.now();

 // Check total duration again, to avoid sending outdated stuff
Expand All@@ -1117,14 +1095,14 @@ export class ReplayContainer implements ReplayContainerInterface {
 throw new Error('Session is too long, not sending replay');
 }

 // NOTE: Copy values from instance members, as it's possible they could
 // change before the flush finishes.
 const replayId = this.session.id;
 const eventContext = this._popEventContext();
 // Always increment segmentId regardless of outcome of sending replay
 const segmentId = this.session.segmentId++;
 this._maybeSaveSession();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this down here, the idea is:
  1. We capture the timestamp, event context etc. first
  2. Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.

 await sendReplay({
 replayId,
 recordingData,
Expand Down

2 changes: 0 additions & 2 deletions
packages/replay/src/session/Session.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 const lastActivity = session.lastActivity || now;
 const segmentId = session.segmentId || 0;
 const sampled = session.sampled;
 const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
 const previousSessionId = session.previousSessionId;

 return {
Expand All@@ -22,7 +21,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 lastActivity,
 segmentId,
 sampled,
 shouldRefresh,
 previousSessionId,
 };
}

19 changes: 12 additions & 7 deletions
packages/replay/src/session/loadOrCreateSession.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,33 +2,38 @@ import type { Session, SessionOptions } from '../types';
import { logInfoNextTick } from '../util/log';
import { createSession } from './createSession';
import { fetchSession } from './fetchSession';
import { maybeRefreshSession } from './maybeRefreshSession';
import { shouldRefreshSession } from './shouldRefreshSession';

/**
 * Get or create a session, when initializing the replay.
 * Returns a session that may be unsampled.
 */
export function loadOrCreateSession(
 currentSession: Session | undefined,
 {
 traceInternals,
 sessionIdleExpire,
 maxReplayDuration,
 previousSessionId,
 }: {
 sessionIdleExpire: number;
 maxReplayDuration: number;
 traceInternals?: boolean;
 previousSessionId?: string;
 },
 sessionOptions: SessionOptions,
): Session {
 // If session exists and is passed, use it instead of always hitting session storage
 const existingSession = currentSession || (sessionOptions.stickySession && fetchSession(traceInternals));
 const existingSession = sessionOptions.stickySession && fetchSession(traceInternals);

 // No session exists yet, just create a new one
 if (!existingSession) {
 logInfoNextTick('[Replay] Created new session', traceInternals);
 return createSession(sessionOptions);
 logInfoNextTick('[Replay] Creating new session', traceInternals);
 return createSession(sessionOptions, { previousSessionId });
 }

 return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions);
 if (!shouldRefreshSession(existingSession, { sessionIdleExpire, maxReplayDuration })) {
 return existingSession;
 }

 logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
 return createSession(sessionOptions, { previousSessionId: existingSession.id });

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

}

50 changes: 0 additions & 50 deletions
packages/replay/src/session/maybeRefreshSession.ts

View file


Open in desktop


This file was deleted.

Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix(replay): Fully stop & restart session when it expires#8834

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

 Merged

mydea
merged 3 commits into

from

Sep 11, 2023
 Merged


fix(replay): Fully stop & restart session when it expires
#8834
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
befae60

fix(replay): Fully stop & restart session when it expires


mydea Aug 18, 2023
7bc5c80

simplify checkAndHandleExpiredSession check


mydea Sep 1, 2023
7b34dd7

PR cleanup


mydea Sep 11, 2023

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


118 changes: 48 additions & 70 deletions
packages/replay/src/replay.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,8 +18,8 @@ import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
import { createEventBuffer } from './eventBuffer';
import { clearSession } from './session/clearSession';
import { loadOrCreateSession } from './session/loadOrCreateSession';
import { maybeRefreshSession } from './session/maybeRefreshSession';
import { saveSession } from './session/saveSession';
import { shouldRefreshSession } from './session/shouldRefreshSession';
import type {
 AddEventResult,
 AddUpdateCallback,
Expand DownExpand Up@@ -217,7 +217,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Initializes the plugin based on sampling configuration. Should not be
 * called outside of constructor.
 */
 public initializeSampling(): void {
 public initializeSampling(previousSessionId?: string): void {

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

 const { errorSampleRate, sessionSampleRate } = this._options;

 // If neither sample rate is > 0, then do nothing - user will need to call one of
Expand All@@ -228,7 +228,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Otherwise if there is _any_ sample rate set, try to load an existing
 // session, or create a new one.
 this._initializeSessionForSampling();
 this._initializeSessionForSampling(previousSessionId);

 if (!this.session) {
 // This should not happen, something wrong has occurred
Expand DownExpand Up@@ -273,7 +273,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 maxReplayDuration: this._options.maxReplayDuration,
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
Expand DownExpand Up@@ -304,7 +303,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
Expand DownExpand Up@@ -373,15 +371,16 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, since try/catch inside of an async function is sugar on top of promises.

 try {
 logInfo(
 `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
 this._options._experiments.traceInternals,
 );

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
 this._removeListeners();
 this.stopRecording();

Expand DownExpand Up@@ -475,16 +474,6 @@ 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 beyond max session lifespan (we ignore
 // expiration given that `shouldRefresh` is true). Since we flip
 // `shouldRefresh`, the session could be considered expired due to
 // lifespan, which is not what we want. Update session start date to be
 // the current timestamp, so that session is not considered to be
 // expired. This means that max replay duration can be MAX_REPLAY_DURATION +
 // (length of buffer), which we are ok with.
 this._updateUserActivity(activityTime);
 this._updateSessionActivity(activityTime);
 this._maybeSaveSession();
Expand DownExpand Up@@ -612,8 +601,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 * @hidden
 */
 public checkAndHandleExpiredSession(): boolean | void {
 const oldSessionId = this.getSessionId();

 // Prevent starting a new session if the last user activity is older than
 // SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
 // session+recording. This creates noisy replays that do not have much
Expand All@@ -635,24 +622,11 @@ export class ReplayContainer implements ReplayContainerInterface {
 // --- There is recent user activity --- //
 // This will create a new session if expired, based on expiry length
 if (!this._checkSession()) {
 return;
 }

 // Session was expired if session ids do not match
 const expired = oldSessionId !== this.getSessionId();

 if (!expired) {
 return true;
 }

 // Session is expired, trigger a full snapshot (which will create a new session)
 if (this.isPaused()) {
 this.resume();
 } else {
 this._triggerFullSnapshot();
 // Check session handles the refreshing itself
 return false;
 }

 return false;
 return true;
 }

 /**
Expand DownExpand Up@@ -740,6 +714,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
 this._isEnabled = true;
 this._isPaused = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we are also unpaused here, when starting new.

 this.startRecording();
 }
Expand All@@ -756,17 +731,17 @@ export class ReplayContainer implements ReplayContainerInterface {
 /**
 * Loads (or refreshes) the current session.
 */
 private _initializeSessionForSampling(): void {
 private _initializeSessionForSampling(previousSessionId?: string): void {
 // Whenever there is _any_ error sample rate, we always allow buffering
 // Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
 const allowBuffering = this._options.errorSampleRate > 0;

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
 traceInternals: this._options._experiments.traceInternals,
 previousSessionId,
 },
 {
 stickySession: this._options.stickySession,
Expand All@@ -791,37 +766,32 @@ export class ReplayContainer implements ReplayContainerInterface {

 const currentSession = this.session;

 const newSession = maybeRefreshSession(
 currentSession,
 {
 if (
 shouldRefreshSession(currentSession, {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 traceInternals: this._options._experiments.traceInternals,
 maxReplayDuration: this._options.maxReplayDuration,
 },
 {
 stickySession: Boolean(this._options.stickySession),
 sessionSampleRate: this._options.sessionSampleRate,
 allowBuffering: this._options.errorSampleRate > 0,
 },
 );

 const isNew = newSession.id !== currentSession.id;

 // If session was newly created (i.e. was not loaded from storage), then
 // enable flag to create the root replay
 if (isNew) {
 this.setInitialState();
 this.session = newSession;
 }

 if (!this.session.sampled) {
 void this.stop({ reason: 'session not refreshed' });
 })
 ) {
 void this._refreshSession(currentSession);
 return false;
 }

 return true;
 }

 /**
 * Refresh a session with a new one.
 * This stops the current session (without forcing a flush, as that would never work since we are expired),
 * and then does a new sampling based on the refreshed session.
 */
 private async _refreshSession(session: Session): Promise<void> {
 if (!this._isEnabled) {
 return;
 }
 await this.stop({ reason: 'refresh session' });
 this.initializeSampling(session.id);
 }

 /**
 * Adds listeners to record events for the replay
 */
Expand DownExpand Up@@ -933,10 +903,14 @@ export class ReplayContainer implements ReplayContainerInterface {

 const expired = isSessionExpired(this.session, {
 maxReplayDuration: this._options.maxReplayDuration,
...this.timeouts,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
 });

 if (breadcrumb && !expired) {
 if (expired) {
 return;
 }

 if (breadcrumb) {
 this._createCustomBreadcrumb(breadcrumb);
 }

Expand DownExpand Up@@ -1081,7 +1055,9 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Should never be called directly, only by `flush`
 */
 private async _runFlush(): Promise<void> {
 if (!this.session || !this.eventBuffer) {
 const replayId = this.getSessionId();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.

 if (!this.session || !this.eventBuffer || !replayId) {
 __DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
 return;
 }
Expand All@@ -1101,13 +1077,15 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
 if (replayId !== this.getSessionId()) {
 return;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
 }

 try {
 // This uses the data from the eventBuffer, so we need to call this before `finish()
 this._updateInitialTimestampFromEventBuffer();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();

 const timestamp = Date.now();

 // Check total duration again, to avoid sending outdated stuff
Expand All@@ -1117,14 +1095,14 @@ export class ReplayContainer implements ReplayContainerInterface {
 throw new Error('Session is too long, not sending replay');
 }

 // NOTE: Copy values from instance members, as it's possible they could
 // change before the flush finishes.
 const replayId = this.session.id;
 const eventContext = this._popEventContext();
 // Always increment segmentId regardless of outcome of sending replay
 const segmentId = this.session.segmentId++;
 this._maybeSaveSession();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this down here, the idea is:
  1. We capture the timestamp, event context etc. first
  2. Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.

 await sendReplay({
 replayId,
 recordingData,
Expand Down

2 changes: 0 additions & 2 deletions
packages/replay/src/session/Session.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 const lastActivity = session.lastActivity || now;
 const segmentId = session.segmentId || 0;
 const sampled = session.sampled;
 const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
 const previousSessionId = session.previousSessionId;

 return {
Expand All@@ -22,7 +21,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 lastActivity,
 segmentId,
 sampled,
 shouldRefresh,
 previousSessionId,
 };
}

19 changes: 12 additions & 7 deletions
packages/replay/src/session/loadOrCreateSession.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,33 +2,38 @@ import type { Session, SessionOptions } from '../types';
import { logInfoNextTick } from '../util/log';
import { createSession } from './createSession';
import { fetchSession } from './fetchSession';
import { maybeRefreshSession } from './maybeRefreshSession';
import { shouldRefreshSession } from './shouldRefreshSession';

/**
 * Get or create a session, when initializing the replay.
 * Returns a session that may be unsampled.
 */
export function loadOrCreateSession(
 currentSession: Session | undefined,
 {
 traceInternals,
 sessionIdleExpire,
 maxReplayDuration,
 previousSessionId,
 }: {
 sessionIdleExpire: number;
 maxReplayDuration: number;
 traceInternals?: boolean;
 previousSessionId?: string;
 },
 sessionOptions: SessionOptions,
): Session {
 // If session exists and is passed, use it instead of always hitting session storage
 const existingSession = currentSession || (sessionOptions.stickySession && fetchSession(traceInternals));
 const existingSession = sessionOptions.stickySession && fetchSession(traceInternals);

 // No session exists yet, just create a new one
 if (!existingSession) {
 logInfoNextTick('[Replay] Created new session', traceInternals);
 return createSession(sessionOptions);
 logInfoNextTick('[Replay] Creating new session', traceInternals);
 return createSession(sessionOptions, { previousSessionId });
 }

 return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions);
 if (!shouldRefreshSession(existingSession, { sessionIdleExpire, maxReplayDuration })) {
 return existingSession;
 }

 logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
 return createSession(sessionOptions, { previousSessionId: existingSession.id });

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

}

50 changes: 0 additions & 50 deletions
packages/replay/src/session/maybeRefreshSession.ts

View file


Open in desktop


This file was deleted.

Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix(replay): Fully stop & restart session when it expires#8834

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

 Merged

mydea
merged 3 commits into

from

Sep 11, 2023
 Merged


fix(replay): Fully stop & restart session when it expires
#8834
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
befae60

fix(replay): Fully stop & restart session when it expires


mydea Aug 18, 2023
7bc5c80

simplify checkAndHandleExpiredSession check


mydea Sep 1, 2023
7b34dd7

PR cleanup


mydea Sep 11, 2023

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


118 changes: 48 additions & 70 deletions
packages/replay/src/replay.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,8 +18,8 @@ import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
import { createEventBuffer } from './eventBuffer';
import { clearSession } from './session/clearSession';
import { loadOrCreateSession } from './session/loadOrCreateSession';
import { maybeRefreshSession } from './session/maybeRefreshSession';
import { saveSession } from './session/saveSession';
import { shouldRefreshSession } from './session/shouldRefreshSession';
import type {
 AddEventResult,
 AddUpdateCallback,
Expand DownExpand Up@@ -217,7 +217,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Initializes the plugin based on sampling configuration. Should not be
 * called outside of constructor.
 */
 public initializeSampling(): void {
 public initializeSampling(previousSessionId?: string): void {

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

 const { errorSampleRate, sessionSampleRate } = this._options;

 // If neither sample rate is > 0, then do nothing - user will need to call one of
Expand All@@ -228,7 +228,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Otherwise if there is _any_ sample rate set, try to load an existing
 // session, or create a new one.
 this._initializeSessionForSampling();
 this._initializeSessionForSampling(previousSessionId);

 if (!this.session) {
 // This should not happen, something wrong has occurred
Expand DownExpand Up@@ -273,7 +273,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 maxReplayDuration: this._options.maxReplayDuration,
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
Expand DownExpand Up@@ -304,7 +303,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
Expand DownExpand Up@@ -373,15 +371,16 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, since try/catch inside of an async function is sugar on top of promises.

 try {
 logInfo(
 `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
 this._options._experiments.traceInternals,
 );

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
 this._removeListeners();
 this.stopRecording();

Expand DownExpand Up@@ -475,16 +474,6 @@ 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 beyond max session lifespan (we ignore
 // expiration given that `shouldRefresh` is true). Since we flip
 // `shouldRefresh`, the session could be considered expired due to
 // lifespan, which is not what we want. Update session start date to be
 // the current timestamp, so that session is not considered to be
 // expired. This means that max replay duration can be MAX_REPLAY_DURATION +
 // (length of buffer), which we are ok with.
 this._updateUserActivity(activityTime);
 this._updateSessionActivity(activityTime);
 this._maybeSaveSession();
Expand DownExpand Up@@ -612,8 +601,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 * @hidden
 */
 public checkAndHandleExpiredSession(): boolean | void {
 const oldSessionId = this.getSessionId();

 // Prevent starting a new session if the last user activity is older than
 // SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
 // session+recording. This creates noisy replays that do not have much
Expand All@@ -635,24 +622,11 @@ export class ReplayContainer implements ReplayContainerInterface {
 // --- There is recent user activity --- //
 // This will create a new session if expired, based on expiry length
 if (!this._checkSession()) {
 return;
 }

 // Session was expired if session ids do not match
 const expired = oldSessionId !== this.getSessionId();

 if (!expired) {
 return true;
 }

 // Session is expired, trigger a full snapshot (which will create a new session)
 if (this.isPaused()) {
 this.resume();
 } else {
 this._triggerFullSnapshot();
 // Check session handles the refreshing itself
 return false;
 }

 return false;
 return true;
 }

 /**
Expand DownExpand Up@@ -740,6 +714,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
 this._isEnabled = true;
 this._isPaused = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we are also unpaused here, when starting new.

 this.startRecording();
 }
Expand All@@ -756,17 +731,17 @@ export class ReplayContainer implements ReplayContainerInterface {
 /**
 * Loads (or refreshes) the current session.
 */
 private _initializeSessionForSampling(): void {
 private _initializeSessionForSampling(previousSessionId?: string): void {
 // Whenever there is _any_ error sample rate, we always allow buffering
 // Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
 const allowBuffering = this._options.errorSampleRate > 0;

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
 traceInternals: this._options._experiments.traceInternals,
 previousSessionId,
 },
 {
 stickySession: this._options.stickySession,
Expand All@@ -791,37 +766,32 @@ export class ReplayContainer implements ReplayContainerInterface {

 const currentSession = this.session;

 const newSession = maybeRefreshSession(
 currentSession,
 {
 if (
 shouldRefreshSession(currentSession, {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 traceInternals: this._options._experiments.traceInternals,
 maxReplayDuration: this._options.maxReplayDuration,
 },
 {
 stickySession: Boolean(this._options.stickySession),
 sessionSampleRate: this._options.sessionSampleRate,
 allowBuffering: this._options.errorSampleRate > 0,
 },
 );

 const isNew = newSession.id !== currentSession.id;

 // If session was newly created (i.e. was not loaded from storage), then
 // enable flag to create the root replay
 if (isNew) {
 this.setInitialState();
 this.session = newSession;
 }

 if (!this.session.sampled) {
 void this.stop({ reason: 'session not refreshed' });
 })
 ) {
 void this._refreshSession(currentSession);
 return false;
 }

 return true;
 }

 /**
 * Refresh a session with a new one.
 * This stops the current session (without forcing a flush, as that would never work since we are expired),
 * and then does a new sampling based on the refreshed session.
 */
 private async _refreshSession(session: Session): Promise<void> {
 if (!this._isEnabled) {
 return;
 }
 await this.stop({ reason: 'refresh session' });
 this.initializeSampling(session.id);
 }

 /**
 * Adds listeners to record events for the replay
 */
Expand DownExpand Up@@ -933,10 +903,14 @@ export class ReplayContainer implements ReplayContainerInterface {

 const expired = isSessionExpired(this.session, {
 maxReplayDuration: this._options.maxReplayDuration,
...this.timeouts,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
 });

 if (breadcrumb && !expired) {
 if (expired) {
 return;
 }

 if (breadcrumb) {
 this._createCustomBreadcrumb(breadcrumb);
 }

Expand DownExpand Up@@ -1081,7 +1055,9 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Should never be called directly, only by `flush`
 */
 private async _runFlush(): Promise<void> {
 if (!this.session || !this.eventBuffer) {
 const replayId = this.getSessionId();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.

 if (!this.session || !this.eventBuffer || !replayId) {
 __DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
 return;
 }
Expand All@@ -1101,13 +1077,15 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
 if (replayId !== this.getSessionId()) {
 return;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
 }

 try {
 // This uses the data from the eventBuffer, so we need to call this before `finish()
 this._updateInitialTimestampFromEventBuffer();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();

 const timestamp = Date.now();

 // Check total duration again, to avoid sending outdated stuff
Expand All@@ -1117,14 +1095,14 @@ export class ReplayContainer implements ReplayContainerInterface {
 throw new Error('Session is too long, not sending replay');
 }

 // NOTE: Copy values from instance members, as it's possible they could
 // change before the flush finishes.
 const replayId = this.session.id;
 const eventContext = this._popEventContext();
 // Always increment segmentId regardless of outcome of sending replay
 const segmentId = this.session.segmentId++;
 this._maybeSaveSession();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this down here, the idea is:
  1. We capture the timestamp, event context etc. first
  2. Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.

 await sendReplay({
 replayId,
 recordingData,
Expand Down

2 changes: 0 additions & 2 deletions
packages/replay/src/session/Session.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 const lastActivity = session.lastActivity || now;
 const segmentId = session.segmentId || 0;
 const sampled = session.sampled;
 const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
 const previousSessionId = session.previousSessionId;

 return {
Expand All@@ -22,7 +21,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 lastActivity,
 segmentId,
 sampled,
 shouldRefresh,
 previousSessionId,
 };
}

19 changes: 12 additions & 7 deletions
packages/replay/src/session/loadOrCreateSession.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,33 +2,38 @@ import type { Session, SessionOptions } from '../types';
import { logInfoNextTick } from '../util/log';
import { createSession } from './createSession';
import { fetchSession } from './fetchSession';
import { maybeRefreshSession } from './maybeRefreshSession';
import { shouldRefreshSession } from './shouldRefreshSession';

/**
 * Get or create a session, when initializing the replay.
 * Returns a session that may be unsampled.
 */
export function loadOrCreateSession(
 currentSession: Session | undefined,
 {
 traceInternals,
 sessionIdleExpire,
 maxReplayDuration,
 previousSessionId,
 }: {
 sessionIdleExpire: number;
 maxReplayDuration: number;
 traceInternals?: boolean;
 previousSessionId?: string;
 },
 sessionOptions: SessionOptions,
): Session {
 // If session exists and is passed, use it instead of always hitting session storage
 const existingSession = currentSession || (sessionOptions.stickySession && fetchSession(traceInternals));
 const existingSession = sessionOptions.stickySession && fetchSession(traceInternals);

 // No session exists yet, just create a new one
 if (!existingSession) {
 logInfoNextTick('[Replay] Created new session', traceInternals);
 return createSession(sessionOptions);
 logInfoNextTick('[Replay] Creating new session', traceInternals);
 return createSession(sessionOptions, { previousSessionId });
 }

 return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions);
 if (!shouldRefreshSession(existingSession, { sessionIdleExpire, maxReplayDuration })) {
 return existingSession;
 }

 logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
 return createSession(sessionOptions, { previousSessionId: existingSession.id });

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

}

50 changes: 0 additions & 50 deletions
packages/replay/src/session/maybeRefreshSession.ts

View file


Open in desktop


This file was deleted.

Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
fix(replay): Fully stop & restart session when it expires#8834

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

 Merged

mydea
merged 3 commits into

from

Sep 11, 2023
 Merged


fix(replay): Fully stop & restart session when it expires
#8834
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
befae60

fix(replay): Fully stop & restart session when it expires


mydea Aug 18, 2023
7bc5c80

simplify checkAndHandleExpiredSession check


mydea Sep 1, 2023
7b34dd7

PR cleanup


mydea Sep 11, 2023

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


118 changes: 48 additions & 70 deletions
packages/replay/src/replay.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,8 +18,8 @@ import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
import { createEventBuffer } from './eventBuffer';
import { clearSession } from './session/clearSession';
import { loadOrCreateSession } from './session/loadOrCreateSession';
import { maybeRefreshSession } from './session/maybeRefreshSession';
import { saveSession } from './session/saveSession';
import { shouldRefreshSession } from './session/shouldRefreshSession';
import type {
 AddEventResult,
 AddUpdateCallback,
Expand DownExpand Up@@ -217,7 +217,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Initializes the plugin based on sampling configuration. Should not be
 * called outside of constructor.
 */
 public initializeSampling(): void {
 public initializeSampling(previousSessionId?: string): void {

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

 const { errorSampleRate, sessionSampleRate } = this._options;

 // If neither sample rate is > 0, then do nothing - user will need to call one of
Expand All@@ -228,7 +228,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Otherwise if there is _any_ sample rate set, try to load an existing
 // session, or create a new one.
 this._initializeSessionForSampling();
 this._initializeSessionForSampling(previousSessionId);

 if (!this.session) {
 // This should not happen, something wrong has occurred
Expand DownExpand Up@@ -273,7 +273,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 maxReplayDuration: this._options.maxReplayDuration,
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
Expand DownExpand Up@@ -304,7 +303,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
Expand DownExpand Up@@ -373,15 +371,16 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, since try/catch inside of an async function is sugar on top of promises.

 try {
 logInfo(
 `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
 this._options._experiments.traceInternals,
 );

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
 this._removeListeners();
 this.stopRecording();

Expand DownExpand Up@@ -475,16 +474,6 @@ 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 beyond max session lifespan (we ignore
 // expiration given that `shouldRefresh` is true). Since we flip
 // `shouldRefresh`, the session could be considered expired due to
 // lifespan, which is not what we want. Update session start date to be
 // the current timestamp, so that session is not considered to be
 // expired. This means that max replay duration can be MAX_REPLAY_DURATION +
 // (length of buffer), which we are ok with.
 this._updateUserActivity(activityTime);
 this._updateSessionActivity(activityTime);
 this._maybeSaveSession();
Expand DownExpand Up@@ -612,8 +601,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 * @hidden
 */
 public checkAndHandleExpiredSession(): boolean | void {
 const oldSessionId = this.getSessionId();

 // Prevent starting a new session if the last user activity is older than
 // SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
 // session+recording. This creates noisy replays that do not have much
Expand All@@ -635,24 +622,11 @@ export class ReplayContainer implements ReplayContainerInterface {
 // --- There is recent user activity --- //
 // This will create a new session if expired, based on expiry length
 if (!this._checkSession()) {
 return;
 }

 // Session was expired if session ids do not match
 const expired = oldSessionId !== this.getSessionId();

 if (!expired) {
 return true;
 }

 // Session is expired, trigger a full snapshot (which will create a new session)
 if (this.isPaused()) {
 this.resume();
 } else {
 this._triggerFullSnapshot();
 // Check session handles the refreshing itself
 return false;
 }

 return false;
 return true;
 }

 /**
Expand DownExpand Up@@ -740,6 +714,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
 this._isEnabled = true;
 this._isPaused = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we are also unpaused here, when starting new.

 this.startRecording();
 }
Expand All@@ -756,17 +731,17 @@ export class ReplayContainer implements ReplayContainerInterface {
 /**
 * Loads (or refreshes) the current session.
 */
 private _initializeSessionForSampling(): void {
 private _initializeSessionForSampling(previousSessionId?: string): void {
 // Whenever there is _any_ error sample rate, we always allow buffering
 // Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
 const allowBuffering = this._options.errorSampleRate > 0;

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
 traceInternals: this._options._experiments.traceInternals,
 previousSessionId,
 },
 {
 stickySession: this._options.stickySession,
Expand All@@ -791,37 +766,32 @@ export class ReplayContainer implements ReplayContainerInterface {

 const currentSession = this.session;

 const newSession = maybeRefreshSession(
 currentSession,
 {
 if (
 shouldRefreshSession(currentSession, {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 traceInternals: this._options._experiments.traceInternals,
 maxReplayDuration: this._options.maxReplayDuration,
 },
 {
 stickySession: Boolean(this._options.stickySession),
 sessionSampleRate: this._options.sessionSampleRate,
 allowBuffering: this._options.errorSampleRate > 0,
 },
 );

 const isNew = newSession.id !== currentSession.id;

 // If session was newly created (i.e. was not loaded from storage), then
 // enable flag to create the root replay
 if (isNew) {
 this.setInitialState();
 this.session = newSession;
 }

 if (!this.session.sampled) {
 void this.stop({ reason: 'session not refreshed' });
 })
 ) {
 void this._refreshSession(currentSession);
 return false;
 }

 return true;
 }

 /**
 * Refresh a session with a new one.
 * This stops the current session (without forcing a flush, as that would never work since we are expired),
 * and then does a new sampling based on the refreshed session.
 */
 private async _refreshSession(session: Session): Promise<void> {
 if (!this._isEnabled) {
 return;
 }
 await this.stop({ reason: 'refresh session' });
 this.initializeSampling(session.id);
 }

 /**
 * Adds listeners to record events for the replay
 */
Expand DownExpand Up@@ -933,10 +903,14 @@ export class ReplayContainer implements ReplayContainerInterface {

 const expired = isSessionExpired(this.session, {
 maxReplayDuration: this._options.maxReplayDuration,
...this.timeouts,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
 });

 if (breadcrumb && !expired) {
 if (expired) {
 return;
 }

 if (breadcrumb) {
 this._createCustomBreadcrumb(breadcrumb);
 }

Expand DownExpand Up@@ -1081,7 +1055,9 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Should never be called directly, only by `flush`
 */
 private async _runFlush(): Promise<void> {
 if (!this.session || !this.eventBuffer) {
 const replayId = this.getSessionId();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.

 if (!this.session || !this.eventBuffer || !replayId) {
 __DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
 return;
 }
Expand All@@ -1101,13 +1077,15 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
 if (replayId !== this.getSessionId()) {
 return;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
 }

 try {
 // This uses the data from the eventBuffer, so we need to call this before `finish()
 this._updateInitialTimestampFromEventBuffer();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();

 const timestamp = Date.now();

 // Check total duration again, to avoid sending outdated stuff
Expand All@@ -1117,14 +1095,14 @@ export class ReplayContainer implements ReplayContainerInterface {
 throw new Error('Session is too long, not sending replay');
 }

 // NOTE: Copy values from instance members, as it's possible they could
 // change before the flush finishes.
 const replayId = this.session.id;
 const eventContext = this._popEventContext();
 // Always increment segmentId regardless of outcome of sending replay
 const segmentId = this.session.segmentId++;
 this._maybeSaveSession();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this down here, the idea is:
  1. We capture the timestamp, event context etc. first
  2. Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.

 await sendReplay({
 replayId,
 recordingData,
Expand Down

2 changes: 0 additions & 2 deletions
packages/replay/src/session/Session.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 const lastActivity = session.lastActivity || now;
 const segmentId = session.segmentId || 0;
 const sampled = session.sampled;
 const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
 const previousSessionId = session.previousSessionId;

 return {
Expand All@@ -22,7 +21,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 lastActivity,
 segmentId,
 sampled,
 shouldRefresh,
 previousSessionId,
 };
}

19 changes: 12 additions & 7 deletions
packages/replay/src/session/loadOrCreateSession.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,33 +2,38 @@ import type { Session, SessionOptions } from '../types';
import { logInfoNextTick } from '../util/log';
import { createSession } from './createSession';
import { fetchSession } from './fetchSession';
import { maybeRefreshSession } from './maybeRefreshSession';
import { shouldRefreshSession } from './shouldRefreshSession';

/**
 * Get or create a session, when initializing the replay.
 * Returns a session that may be unsampled.
 */
export function loadOrCreateSession(
 currentSession: Session | undefined,
 {
 traceInternals,
 sessionIdleExpire,
 maxReplayDuration,
 previousSessionId,
 }: {
 sessionIdleExpire: number;
 maxReplayDuration: number;
 traceInternals?: boolean;
 previousSessionId?: string;
 },
 sessionOptions: SessionOptions,
): Session {
 // If session exists and is passed, use it instead of always hitting session storage
 const existingSession = currentSession || (sessionOptions.stickySession && fetchSession(traceInternals));
 const existingSession = sessionOptions.stickySession && fetchSession(traceInternals);

 // No session exists yet, just create a new one
 if (!existingSession) {
 logInfoNextTick('[Replay] Created new session', traceInternals);
 return createSession(sessionOptions);
 logInfoNextTick('[Replay] Creating new session', traceInternals);
 return createSession(sessionOptions, { previousSessionId });
 }

 return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions);
 if (!shouldRefreshSession(existingSession, { sessionIdleExpire, maxReplayDuration })) {
 return existingSession;
 }

 logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
 return createSession(sessionOptions, { previousSessionId: existingSession.id });

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

}

50 changes: 0 additions & 50 deletions
packages/replay/src/session/maybeRefreshSession.ts

View file


Open in desktop


This file was deleted.

Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix(replay): Fully stop & restart session when it expires#8834

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

 Merged

mydea
merged 3 commits into

from

Sep 11, 2023
 Merged


fix(replay): Fully stop & restart session when it expires
#8834
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
befae60

fix(replay): Fully stop & restart session when it expires


mydea Aug 18, 2023
7bc5c80

simplify checkAndHandleExpiredSession check


mydea Sep 1, 2023
7b34dd7

PR cleanup


mydea Sep 11, 2023

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


118 changes: 48 additions & 70 deletions
packages/replay/src/replay.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,8 +18,8 @@ import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
import { createEventBuffer } from './eventBuffer';
import { clearSession } from './session/clearSession';
import { loadOrCreateSession } from './session/loadOrCreateSession';
import { maybeRefreshSession } from './session/maybeRefreshSession';
import { saveSession } from './session/saveSession';
import { shouldRefreshSession } from './session/shouldRefreshSession';
import type {
 AddEventResult,
 AddUpdateCallback,
Expand DownExpand Up@@ -217,7 +217,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Initializes the plugin based on sampling configuration. Should not be
 * called outside of constructor.
 */
 public initializeSampling(): void {
 public initializeSampling(previousSessionId?: string): void {

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

 const { errorSampleRate, sessionSampleRate } = this._options;

 // If neither sample rate is > 0, then do nothing - user will need to call one of
Expand All@@ -228,7 +228,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Otherwise if there is _any_ sample rate set, try to load an existing
 // session, or create a new one.
 this._initializeSessionForSampling();
 this._initializeSessionForSampling(previousSessionId);

 if (!this.session) {
 // This should not happen, something wrong has occurred
Expand DownExpand Up@@ -273,7 +273,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 maxReplayDuration: this._options.maxReplayDuration,
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
Expand DownExpand Up@@ -304,7 +303,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
Expand DownExpand Up@@ -373,15 +371,16 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, since try/catch inside of an async function is sugar on top of promises.

 try {
 logInfo(
 `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
 this._options._experiments.traceInternals,
 );

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
 this._removeListeners();
 this.stopRecording();

Expand DownExpand Up@@ -475,16 +474,6 @@ 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 beyond max session lifespan (we ignore
 // expiration given that `shouldRefresh` is true). Since we flip
 // `shouldRefresh`, the session could be considered expired due to
 // lifespan, which is not what we want. Update session start date to be
 // the current timestamp, so that session is not considered to be
 // expired. This means that max replay duration can be MAX_REPLAY_DURATION +
 // (length of buffer), which we are ok with.
 this._updateUserActivity(activityTime);
 this._updateSessionActivity(activityTime);
 this._maybeSaveSession();
Expand DownExpand Up@@ -612,8 +601,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 * @hidden
 */
 public checkAndHandleExpiredSession(): boolean | void {
 const oldSessionId = this.getSessionId();

 // Prevent starting a new session if the last user activity is older than
 // SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
 // session+recording. This creates noisy replays that do not have much
Expand All@@ -635,24 +622,11 @@ export class ReplayContainer implements ReplayContainerInterface {
 // --- There is recent user activity --- //
 // This will create a new session if expired, based on expiry length
 if (!this._checkSession()) {
 return;
 }

 // Session was expired if session ids do not match
 const expired = oldSessionId !== this.getSessionId();

 if (!expired) {
 return true;
 }

 // Session is expired, trigger a full snapshot (which will create a new session)
 if (this.isPaused()) {
 this.resume();
 } else {
 this._triggerFullSnapshot();
 // Check session handles the refreshing itself
 return false;
 }

 return false;
 return true;
 }

 /**
Expand DownExpand Up@@ -740,6 +714,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
 this._isEnabled = true;
 this._isPaused = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we are also unpaused here, when starting new.

 this.startRecording();
 }
Expand All@@ -756,17 +731,17 @@ export class ReplayContainer implements ReplayContainerInterface {
 /**
 * Loads (or refreshes) the current session.
 */
 private _initializeSessionForSampling(): void {
 private _initializeSessionForSampling(previousSessionId?: string): void {
 // Whenever there is _any_ error sample rate, we always allow buffering
 // Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
 const allowBuffering = this._options.errorSampleRate > 0;

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
 traceInternals: this._options._experiments.traceInternals,
 previousSessionId,
 },
 {
 stickySession: this._options.stickySession,
Expand All@@ -791,37 +766,32 @@ export class ReplayContainer implements ReplayContainerInterface {

 const currentSession = this.session;

 const newSession = maybeRefreshSession(
 currentSession,
 {
 if (
 shouldRefreshSession(currentSession, {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 traceInternals: this._options._experiments.traceInternals,
 maxReplayDuration: this._options.maxReplayDuration,
 },
 {
 stickySession: Boolean(this._options.stickySession),
 sessionSampleRate: this._options.sessionSampleRate,
 allowBuffering: this._options.errorSampleRate > 0,
 },
 );

 const isNew = newSession.id !== currentSession.id;

 // If session was newly created (i.e. was not loaded from storage), then
 // enable flag to create the root replay
 if (isNew) {
 this.setInitialState();
 this.session = newSession;
 }

 if (!this.session.sampled) {
 void this.stop({ reason: 'session not refreshed' });
 })
 ) {
 void this._refreshSession(currentSession);
 return false;
 }

 return true;
 }

 /**
 * Refresh a session with a new one.
 * This stops the current session (without forcing a flush, as that would never work since we are expired),
 * and then does a new sampling based on the refreshed session.
 */
 private async _refreshSession(session: Session): Promise<void> {
 if (!this._isEnabled) {
 return;
 }
 await this.stop({ reason: 'refresh session' });
 this.initializeSampling(session.id);
 }

 /**
 * Adds listeners to record events for the replay
 */
Expand DownExpand Up@@ -933,10 +903,14 @@ export class ReplayContainer implements ReplayContainerInterface {

 const expired = isSessionExpired(this.session, {
 maxReplayDuration: this._options.maxReplayDuration,
...this.timeouts,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
 });

 if (breadcrumb && !expired) {
 if (expired) {
 return;
 }

 if (breadcrumb) {
 this._createCustomBreadcrumb(breadcrumb);
 }

Expand DownExpand Up@@ -1081,7 +1055,9 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Should never be called directly, only by `flush`
 */
 private async _runFlush(): Promise<void> {
 if (!this.session || !this.eventBuffer) {
 const replayId = this.getSessionId();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.

 if (!this.session || !this.eventBuffer || !replayId) {
 __DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
 return;
 }
Expand All@@ -1101,13 +1077,15 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
 if (replayId !== this.getSessionId()) {
 return;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
 }

 try {
 // This uses the data from the eventBuffer, so we need to call this before `finish()
 this._updateInitialTimestampFromEventBuffer();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();

 const timestamp = Date.now();

 // Check total duration again, to avoid sending outdated stuff
Expand All@@ -1117,14 +1095,14 @@ export class ReplayContainer implements ReplayContainerInterface {
 throw new Error('Session is too long, not sending replay');
 }

 // NOTE: Copy values from instance members, as it's possible they could
 // change before the flush finishes.
 const replayId = this.session.id;
 const eventContext = this._popEventContext();
 // Always increment segmentId regardless of outcome of sending replay
 const segmentId = this.session.segmentId++;
 this._maybeSaveSession();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this down here, the idea is:
  1. We capture the timestamp, event context etc. first
  2. Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.

 await sendReplay({
 replayId,
 recordingData,
Expand Down

2 changes: 0 additions & 2 deletions
packages/replay/src/session/Session.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 const lastActivity = session.lastActivity || now;
 const segmentId = session.segmentId || 0;
 const sampled = session.sampled;
 const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
 const previousSessionId = session.previousSessionId;

 return {
Expand All@@ -22,7 +21,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 lastActivity,
 segmentId,
 sampled,
 shouldRefresh,
 previousSessionId,
 };
}

19 changes: 12 additions & 7 deletions
packages/replay/src/session/loadOrCreateSession.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,33 +2,38 @@ import type { Session, SessionOptions } from '../types';
import { logInfoNextTick } from '../util/log';
import { createSession } from './createSession';
import { fetchSession } from './fetchSession';
import { maybeRefreshSession } from './maybeRefreshSession';
import { shouldRefreshSession } from './shouldRefreshSession';

/**
 * Get or create a session, when initializing the replay.
 * Returns a session that may be unsampled.
 */
export function loadOrCreateSession(
 currentSession: Session | undefined,
 {
 traceInternals,
 sessionIdleExpire,
 maxReplayDuration,
 previousSessionId,
 }: {
 sessionIdleExpire: number;
 maxReplayDuration: number;
 traceInternals?: boolean;
 previousSessionId?: string;
 },
 sessionOptions: SessionOptions,
): Session {
 // If session exists and is passed, use it instead of always hitting session storage
 const existingSession = currentSession || (sessionOptions.stickySession && fetchSession(traceInternals));
 const existingSession = sessionOptions.stickySession && fetchSession(traceInternals);

 // No session exists yet, just create a new one
 if (!existingSession) {
 logInfoNextTick('[Replay] Created new session', traceInternals);
 return createSession(sessionOptions);
 logInfoNextTick('[Replay] Creating new session', traceInternals);
 return createSession(sessionOptions, { previousSessionId });
 }

 return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions);
 if (!shouldRefreshSession(existingSession, { sessionIdleExpire, maxReplayDuration })) {
 return existingSession;
 }

 logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
 return createSession(sessionOptions, { previousSessionId: existingSession.id });

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

}

50 changes: 0 additions & 50 deletions
packages/replay/src/session/maybeRefreshSession.ts

View file


Open in desktop


This file was deleted.

Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix(replay): Fully stop & restart session when it expires#8834

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

 Merged

mydea
merged 3 commits into

from

Sep 11, 2023
 Merged


fix(replay): Fully stop & restart session when it expires
#8834
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
befae60

fix(replay): Fully stop & restart session when it expires


mydea Aug 18, 2023
7bc5c80

simplify checkAndHandleExpiredSession check


mydea Sep 1, 2023
7b34dd7

PR cleanup


mydea Sep 11, 2023

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


118 changes: 48 additions & 70 deletions
packages/replay/src/replay.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,8 +18,8 @@ import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
import { createEventBuffer } from './eventBuffer';
import { clearSession } from './session/clearSession';
import { loadOrCreateSession } from './session/loadOrCreateSession';
import { maybeRefreshSession } from './session/maybeRefreshSession';
import { saveSession } from './session/saveSession';
import { shouldRefreshSession } from './session/shouldRefreshSession';
import type {
 AddEventResult,
 AddUpdateCallback,
Expand DownExpand Up@@ -217,7 +217,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Initializes the plugin based on sampling configuration. Should not be
 * called outside of constructor.
 */
 public initializeSampling(): void {
 public initializeSampling(previousSessionId?: string): void {

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

 const { errorSampleRate, sessionSampleRate } = this._options;

 // If neither sample rate is > 0, then do nothing - user will need to call one of
Expand All@@ -228,7 +228,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Otherwise if there is _any_ sample rate set, try to load an existing
 // session, or create a new one.
 this._initializeSessionForSampling();
 this._initializeSessionForSampling(previousSessionId);

 if (!this.session) {
 // This should not happen, something wrong has occurred
Expand DownExpand Up@@ -273,7 +273,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 maxReplayDuration: this._options.maxReplayDuration,
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
Expand DownExpand Up@@ -304,7 +303,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
Expand DownExpand Up@@ -373,15 +371,16 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, since try/catch inside of an async function is sugar on top of promises.

 try {
 logInfo(
 `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
 this._options._experiments.traceInternals,
 );

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
 this._removeListeners();
 this.stopRecording();

Expand DownExpand Up@@ -475,16 +474,6 @@ 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 beyond max session lifespan (we ignore
 // expiration given that `shouldRefresh` is true). Since we flip
 // `shouldRefresh`, the session could be considered expired due to
 // lifespan, which is not what we want. Update session start date to be
 // the current timestamp, so that session is not considered to be
 // expired. This means that max replay duration can be MAX_REPLAY_DURATION +
 // (length of buffer), which we are ok with.
 this._updateUserActivity(activityTime);
 this._updateSessionActivity(activityTime);
 this._maybeSaveSession();
Expand DownExpand Up@@ -612,8 +601,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 * @hidden
 */
 public checkAndHandleExpiredSession(): boolean | void {
 const oldSessionId = this.getSessionId();

 // Prevent starting a new session if the last user activity is older than
 // SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
 // session+recording. This creates noisy replays that do not have much
Expand All@@ -635,24 +622,11 @@ export class ReplayContainer implements ReplayContainerInterface {
 // --- There is recent user activity --- //
 // This will create a new session if expired, based on expiry length
 if (!this._checkSession()) {
 return;
 }

 // Session was expired if session ids do not match
 const expired = oldSessionId !== this.getSessionId();

 if (!expired) {
 return true;
 }

 // Session is expired, trigger a full snapshot (which will create a new session)
 if (this.isPaused()) {
 this.resume();
 } else {
 this._triggerFullSnapshot();
 // Check session handles the refreshing itself
 return false;
 }

 return false;
 return true;
 }

 /**
Expand DownExpand Up@@ -740,6 +714,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
 this._isEnabled = true;
 this._isPaused = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we are also unpaused here, when starting new.

 this.startRecording();
 }
Expand All@@ -756,17 +731,17 @@ export class ReplayContainer implements ReplayContainerInterface {
 /**
 * Loads (or refreshes) the current session.
 */
 private _initializeSessionForSampling(): void {
 private _initializeSessionForSampling(previousSessionId?: string): void {
 // Whenever there is _any_ error sample rate, we always allow buffering
 // Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
 const allowBuffering = this._options.errorSampleRate > 0;

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
 traceInternals: this._options._experiments.traceInternals,
 previousSessionId,
 },
 {
 stickySession: this._options.stickySession,
Expand All@@ -791,37 +766,32 @@ export class ReplayContainer implements ReplayContainerInterface {

 const currentSession = this.session;

 const newSession = maybeRefreshSession(
 currentSession,
 {
 if (
 shouldRefreshSession(currentSession, {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 traceInternals: this._options._experiments.traceInternals,
 maxReplayDuration: this._options.maxReplayDuration,
 },
 {
 stickySession: Boolean(this._options.stickySession),
 sessionSampleRate: this._options.sessionSampleRate,
 allowBuffering: this._options.errorSampleRate > 0,
 },
 );

 const isNew = newSession.id !== currentSession.id;

 // If session was newly created (i.e. was not loaded from storage), then
 // enable flag to create the root replay
 if (isNew) {
 this.setInitialState();
 this.session = newSession;
 }

 if (!this.session.sampled) {
 void this.stop({ reason: 'session not refreshed' });
 })
 ) {
 void this._refreshSession(currentSession);
 return false;
 }

 return true;
 }

 /**
 * Refresh a session with a new one.
 * This stops the current session (without forcing a flush, as that would never work since we are expired),
 * and then does a new sampling based on the refreshed session.
 */
 private async _refreshSession(session: Session): Promise<void> {
 if (!this._isEnabled) {
 return;
 }
 await this.stop({ reason: 'refresh session' });
 this.initializeSampling(session.id);
 }

 /**
 * Adds listeners to record events for the replay
 */
Expand DownExpand Up@@ -933,10 +903,14 @@ export class ReplayContainer implements ReplayContainerInterface {

 const expired = isSessionExpired(this.session, {
 maxReplayDuration: this._options.maxReplayDuration,
...this.timeouts,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
 });

 if (breadcrumb && !expired) {
 if (expired) {
 return;
 }

 if (breadcrumb) {
 this._createCustomBreadcrumb(breadcrumb);
 }

Expand DownExpand Up@@ -1081,7 +1055,9 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Should never be called directly, only by `flush`
 */
 private async _runFlush(): Promise<void> {
 if (!this.session || !this.eventBuffer) {
 const replayId = this.getSessionId();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.

 if (!this.session || !this.eventBuffer || !replayId) {
 __DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
 return;
 }
Expand All@@ -1101,13 +1077,15 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
 if (replayId !== this.getSessionId()) {
 return;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
 }

 try {
 // This uses the data from the eventBuffer, so we need to call this before `finish()
 this._updateInitialTimestampFromEventBuffer();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();

 const timestamp = Date.now();

 // Check total duration again, to avoid sending outdated stuff
Expand All@@ -1117,14 +1095,14 @@ export class ReplayContainer implements ReplayContainerInterface {
 throw new Error('Session is too long, not sending replay');
 }

 // NOTE: Copy values from instance members, as it's possible they could
 // change before the flush finishes.
 const replayId = this.session.id;
 const eventContext = this._popEventContext();
 // Always increment segmentId regardless of outcome of sending replay
 const segmentId = this.session.segmentId++;
 this._maybeSaveSession();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this down here, the idea is:
  1. We capture the timestamp, event context etc. first
  2. Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.

 await sendReplay({
 replayId,
 recordingData,
Expand Down

2 changes: 0 additions & 2 deletions
packages/replay/src/session/Session.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 const lastActivity = session.lastActivity || now;
 const segmentId = session.segmentId || 0;
 const sampled = session.sampled;
 const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
 const previousSessionId = session.previousSessionId;

 return {
Expand All@@ -22,7 +21,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 lastActivity,
 segmentId,
 sampled,
 shouldRefresh,
 previousSessionId,
 };
}

19 changes: 12 additions & 7 deletions
packages/replay/src/session/loadOrCreateSession.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,33 +2,38 @@ import type { Session, SessionOptions } from '../types';
import { logInfoNextTick } from '../util/log';
import { createSession } from './createSession';
import { fetchSession } from './fetchSession';
import { maybeRefreshSession } from './maybeRefreshSession';
import { shouldRefreshSession } from './shouldRefreshSession';

/**
 * Get or create a session, when initializing the replay.
 * Returns a session that may be unsampled.
 */
export function loadOrCreateSession(
 currentSession: Session | undefined,
 {
 traceInternals,
 sessionIdleExpire,
 maxReplayDuration,
 previousSessionId,
 }: {
 sessionIdleExpire: number;
 maxReplayDuration: number;
 traceInternals?: boolean;
 previousSessionId?: string;
 },
 sessionOptions: SessionOptions,
): Session {
 // If session exists and is passed, use it instead of always hitting session storage
 const existingSession = currentSession || (sessionOptions.stickySession && fetchSession(traceInternals));
 const existingSession = sessionOptions.stickySession && fetchSession(traceInternals);

 // No session exists yet, just create a new one
 if (!existingSession) {
 logInfoNextTick('[Replay] Created new session', traceInternals);
 return createSession(sessionOptions);
 logInfoNextTick('[Replay] Creating new session', traceInternals);
 return createSession(sessionOptions, { previousSessionId });
 }

 return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions);
 if (!shouldRefreshSession(existingSession, { sessionIdleExpire, maxReplayDuration })) {
 return existingSession;
 }

 logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
 return createSession(sessionOptions, { previousSessionId: existingSession.id });

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

}

50 changes: 0 additions & 50 deletions
packages/replay/src/session/maybeRefreshSession.ts

View file


Open in desktop


This file was deleted.

Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
fix(replay): Fully stop & restart session when it expires#8834

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

 Merged

mydea
merged 3 commits into

from

Sep 11, 2023
 Merged


fix(replay): Fully stop & restart session when it expires
#8834
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
befae60

fix(replay): Fully stop & restart session when it expires


mydea Aug 18, 2023
7bc5c80

simplify checkAndHandleExpiredSession check


mydea Sep 1, 2023
7b34dd7

PR cleanup


mydea Sep 11, 2023

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


118 changes: 48 additions & 70 deletions
packages/replay/src/replay.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,8 +18,8 @@ import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
import { createEventBuffer } from './eventBuffer';
import { clearSession } from './session/clearSession';
import { loadOrCreateSession } from './session/loadOrCreateSession';
import { maybeRefreshSession } from './session/maybeRefreshSession';
import { saveSession } from './session/saveSession';
import { shouldRefreshSession } from './session/shouldRefreshSession';
import type {
 AddEventResult,
 AddUpdateCallback,
Expand DownExpand Up@@ -217,7 +217,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Initializes the plugin based on sampling configuration. Should not be
 * called outside of constructor.
 */
 public initializeSampling(): void {
 public initializeSampling(previousSessionId?: string): void {

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

 const { errorSampleRate, sessionSampleRate } = this._options;

 // If neither sample rate is > 0, then do nothing - user will need to call one of
Expand All@@ -228,7 +228,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Otherwise if there is _any_ sample rate set, try to load an existing
 // session, or create a new one.
 this._initializeSessionForSampling();
 this._initializeSessionForSampling(previousSessionId);

 if (!this.session) {
 // This should not happen, something wrong has occurred
Expand DownExpand Up@@ -273,7 +273,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 maxReplayDuration: this._options.maxReplayDuration,
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
Expand DownExpand Up@@ -304,7 +303,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
Expand DownExpand Up@@ -373,15 +371,16 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, since try/catch inside of an async function is sugar on top of promises.

 try {
 logInfo(
 `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
 this._options._experiments.traceInternals,
 );

 // We can't move `_isEnabled` after awaiting a flush, otherwise we can
 // enter into an infinite loop when `stop()` is called while flushing.
 this._isEnabled = false;
 this._removeListeners();
 this.stopRecording();

Expand DownExpand Up@@ -475,16 +474,6 @@ 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 beyond max session lifespan (we ignore
 // expiration given that `shouldRefresh` is true). Since we flip
 // `shouldRefresh`, the session could be considered expired due to
 // lifespan, which is not what we want. Update session start date to be
 // the current timestamp, so that session is not considered to be
 // expired. This means that max replay duration can be MAX_REPLAY_DURATION +
 // (length of buffer), which we are ok with.
 this._updateUserActivity(activityTime);
 this._updateSessionActivity(activityTime);
 this._maybeSaveSession();
Expand DownExpand Up@@ -612,8 +601,6 @@ export class ReplayContainer implements ReplayContainerInterface {
 * @hidden
 */
 public checkAndHandleExpiredSession(): boolean | void {
 const oldSessionId = this.getSessionId();

 // Prevent starting a new session if the last user activity is older than
 // SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
 // session+recording. This creates noisy replays that do not have much
Expand All@@ -635,24 +622,11 @@ export class ReplayContainer implements ReplayContainerInterface {
 // --- There is recent user activity --- //
 // This will create a new session if expired, based on expiry length
 if (!this._checkSession()) {
 return;
 }

 // Session was expired if session ids do not match
 const expired = oldSessionId !== this.getSessionId();

 if (!expired) {
 return true;
 }

 // Session is expired, trigger a full snapshot (which will create a new session)
 if (this.isPaused()) {
 this.resume();
 } else {
 this._triggerFullSnapshot();
 // Check session handles the refreshing itself
 return false;
 }

 return false;
 return true;
 }

 /**
Expand DownExpand Up@@ -740,6 +714,7 @@ export class ReplayContainer implements ReplayContainerInterface {

 // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
 this._isEnabled = true;
 this._isPaused = false;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure we are also unpaused here, when starting new.

 this.startRecording();
 }
Expand All@@ -756,17 +731,17 @@ export class ReplayContainer implements ReplayContainerInterface {
 /**
 * Loads (or refreshes) the current session.
 */
 private _initializeSessionForSampling(): void {
 private _initializeSessionForSampling(previousSessionId?: string): void {
 // Whenever there is _any_ error sample rate, we always allow buffering
 // Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
 const allowBuffering = this._options.errorSampleRate > 0;

 const session = loadOrCreateSession(
 this.session,
 {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 maxReplayDuration: this._options.maxReplayDuration,
 traceInternals: this._options._experiments.traceInternals,
 previousSessionId,
 },
 {
 stickySession: this._options.stickySession,
Expand All@@ -791,37 +766,32 @@ export class ReplayContainer implements ReplayContainerInterface {

 const currentSession = this.session;

 const newSession = maybeRefreshSession(
 currentSession,
 {
 if (
 shouldRefreshSession(currentSession, {
 sessionIdleExpire: this.timeouts.sessionIdleExpire,
 traceInternals: this._options._experiments.traceInternals,
 maxReplayDuration: this._options.maxReplayDuration,
 },
 {
 stickySession: Boolean(this._options.stickySession),
 sessionSampleRate: this._options.sessionSampleRate,
 allowBuffering: this._options.errorSampleRate > 0,
 },
 );

 const isNew = newSession.id !== currentSession.id;

 // If session was newly created (i.e. was not loaded from storage), then
 // enable flag to create the root replay
 if (isNew) {
 this.setInitialState();
 this.session = newSession;
 }

 if (!this.session.sampled) {
 void this.stop({ reason: 'session not refreshed' });
 })
 ) {
 void this._refreshSession(currentSession);
 return false;
 }

 return true;
 }

 /**
 * Refresh a session with a new one.
 * This stops the current session (without forcing a flush, as that would never work since we are expired),
 * and then does a new sampling based on the refreshed session.
 */
 private async _refreshSession(session: Session): Promise<void> {
 if (!this._isEnabled) {
 return;
 }
 await this.stop({ reason: 'refresh session' });
 this.initializeSampling(session.id);
 }

 /**
 * Adds listeners to record events for the replay
 */
Expand DownExpand Up@@ -933,10 +903,14 @@ export class ReplayContainer implements ReplayContainerInterface {

 const expired = isSessionExpired(this.session, {
 maxReplayDuration: this._options.maxReplayDuration,
...this.timeouts,
sessionIdleExpire: this.timeouts.sessionIdleExpire,
 });

 if (breadcrumb && !expired) {
 if (expired) {
 return;
 }

 if (breadcrumb) {
 this._createCustomBreadcrumb(breadcrumb);
 }

Expand DownExpand Up@@ -1081,7 +1055,9 @@ export class ReplayContainer implements ReplayContainerInterface {
 * Should never be called directly, only by `flush`
 */
 private async _runFlush(): Promise<void> {
 if (!this.session || !this.eventBuffer) {
 const replayId = this.getSessionId();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.

 if (!this.session || !this.eventBuffer || !replayId) {
 __DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
 return;
 }
Expand All@@ -1101,13 +1077,15 @@ export class ReplayContainer implements ReplayContainerInterface {
 return;
 }

 // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
 if (replayId !== this.getSessionId()) {
 return;
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?
@billyvgbillyvgAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
 }

 try {
 // This uses the data from the eventBuffer, so we need to call this before `finish()
 this._updateInitialTimestampFromEventBuffer();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();

 const timestamp = Date.now();

 // Check total duration again, to avoid sending outdated stuff
Expand All@@ -1117,14 +1095,14 @@ export class ReplayContainer implements ReplayContainerInterface {
 throw new Error('Session is too long, not sending replay');
 }

 // NOTE: Copy values from instance members, as it's possible they could
 // change before the flush finishes.
 const replayId = this.session.id;
 const eventContext = this._popEventContext();
 // Always increment segmentId regardless of outcome of sending replay
 const segmentId = this.session.segmentId++;
 this._maybeSaveSession();

 // Note this empties the event buffer regardless of outcome of sending replay
 const recordingData = await this.eventBuffer.finish();
@mydeamydeaAug 17, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this down here, the idea is:
  1. We capture the timestamp, event context etc. first
  2. Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.

 await sendReplay({
 replayId,
 recordingData,
Expand Down

2 changes: 0 additions & 2 deletions
packages/replay/src/session/Session.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 const lastActivity = session.lastActivity || now;
 const segmentId = session.segmentId || 0;
 const sampled = session.sampled;
 const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
 const previousSessionId = session.previousSessionId;

 return {
Expand All@@ -22,7 +21,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
 lastActivity,
 segmentId,
 sampled,
 shouldRefresh,
 previousSessionId,
 };
}

19 changes: 12 additions & 7 deletions
packages/replay/src/session/loadOrCreateSession.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,33 +2,38 @@ import type { Session, SessionOptions } from '../types';
import { logInfoNextTick } from '../util/log';
import { createSession } from './createSession';
import { fetchSession } from './fetchSession';
import { maybeRefreshSession } from './maybeRefreshSession';
import { shouldRefreshSession } from './shouldRefreshSession';

/**
 * Get or create a session, when initializing the replay.
 * Returns a session that may be unsampled.
 */
export function loadOrCreateSession(
 currentSession: Session | undefined,
 {
 traceInternals,
 sessionIdleExpire,
 maxReplayDuration,
 previousSessionId,
 }: {
 sessionIdleExpire: number;
 maxReplayDuration: number;
 traceInternals?: boolean;
 previousSessionId?: string;
 },
 sessionOptions: SessionOptions,
): Session {
 // If session exists and is passed, use it instead of always hitting session storage
 const existingSession = currentSession || (sessionOptions.stickySession && fetchSession(traceInternals));
 const existingSession = sessionOptions.stickySession && fetchSession(traceInternals);

 // No session exists yet, just create a new one
 if (!existingSession) {
 logInfoNextTick('[Replay] Created new session', traceInternals);
 return createSession(sessionOptions);
 logInfoNextTick('[Replay] Creating new session', traceInternals);
 return createSession(sessionOptions, { previousSessionId });
 }

 return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions);
 if (!shouldRefreshSession(existingSession, { sessionIdleExpire, maxReplayDuration })) {
 return existingSession;
 }

 logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
 return createSession(sessionOptions, { previousSessionId: existingSession.id });

Check failure

Code scanning / CodeQL


Insecure randomness


This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.

}

50 changes: 0 additions & 50 deletions
packages/replay/src/session/maybeRefreshSession.ts

View file


Open in desktop


This file was deleted.

Loading