Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(replay): Reduce time limit before pausing a recording #7356

Merged
merged 7 commits into from Mar 13, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Mar 7, 2023

Reduce the time limit before pausing a recording from MAX_SESSION_LIFE (1 hour) to SESSION_IDLE_DURATION (5 minutes). This reduces the amount of empty replays that are caused by a DOM mutation on a timer. An example of this is on sentry.io where there is a time component that updates every x seconds.

As a reminder, there are three events that will break the idle timeout:

  • mouse click
  • user input in a form field
  • a navigation

Closes #7352

Reduce the time limit before pausing a recording from `MAX_SESSION_LIFE` (1 hour) to `SESSION_IDLE_DURATION` (5 minutes). This reduces the amount of empty replays that are caused by a DOM mutation on a timer. An example of this is on sentry.io where there is a time component that updates every x seconds.

As a reminder, there are three events that will break the idle timeout:

* mouse click
* user input in a form field
* a navigation

Closes #7352
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

this sounds reasonable to me! 👍

@billyvg billyvg marked this pull request as ready for review March 8, 2023 20:33
expect(replay).not.toHaveSameSession(initialSession);
});

it('does not create a new session if user hides the tab and comes back within [SESSION_IDLE_DURATION] seconds', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests here and below are actually duplicated

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.21 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 62.89 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.85 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 55.8 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.6 KB (0%)
@sentry/browser - Webpack (minified) 67.32 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.62 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.51 KB (+0.11% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.49 KB (+0.15% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.74 KB (+0.14% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.17 KB (+0.02% 🔺)
@sentry/replay - Webpack (gzipped + minified) 37.19 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 61.17 KB (+0.07% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 54.34 KB (+0.01% 🔺)

// session+recording. This creates noisy replays that do not have much
// content in them.
if (this._lastActivity && isExpired(this._lastActivity, this.timeouts.maxSessionLife)) {
// Pause recording
if (this._lastActivity && isExpired(this._lastActivity, this.timeouts.sessionIdle) && this.session?.sampled === 'session') {
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to only pause for session based replays. Error based replays that idle should immediately end (as any action will start a new session which isn't what we want).

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so what would happen to an error session in that case? If my session is error-sampled, I leave the tab for an hour and come back, recording should stop?

Would it not work to pause/resume an error session? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@mydea In that case the session completely stops, the only way is to get resampled (e.g. reload SDK).

For all sessions, pause is effectively ending the current replay/session, but for session sampled, we want to continue listening via core SDK for a user interaction to start a new session.

For error sessions we want this to end so they get resampled otherwise it leads to a new session with 0 errors. (I suppose we could restart as error sampled session).

expect(replay.session).toBe(undefined);
});

it('creates a new session if current session exceeds MAX_SESSION_LIFE', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

New test to ensure we start a new session if we are active and exceed MAX_SESSION_LIFE

@billyvg billyvg merged commit a70376e into develop Mar 13, 2023
48 checks passed
@billyvg billyvg deleted the feat-replay-reduce-idle-limit-before-pausing branch March 13, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix "empty" replays due to idle sessions + non-user initiated events
2 participants