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): Ensure min/max duration when flushing #8596

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Jul 20, 2023

This PR adds a safeguard to ensure we do not flush (=send) a replay that is either too short or too long.

We allow to configure a minReplayDuration, which defaults to 5s and maxes out at 15s. Whenever we try to flush and the duration is shorter than this, we'll just skip flushing.

Additionally, we also skip flushing if the replay is longer than MAX_SESSION_LIFE + 5s (=60min + 5s). This should not happen, technically, but apparently it still does. So while we figure out the root cause of this, we can at least avoid sending stuff in that case.

@mydea mydea added the Package: replay Issues related to the Sentry Replay SDK label Jul 20, 2023
@mydea mydea requested review from billyvg and Lms24 July 20, 2023 11:46
@mydea mydea self-assigned this Jul 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 21.95 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 69.13 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 20.28 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 60.38 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 21.91 KB (0%)
@sentry/browser - Webpack (minified) 71.51 KB (0%)
@sentry/react - Webpack (gzipped + minified) 21.92 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 50.76 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 30.33 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 28.2 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 49.54 KB (+0.21% 🔺)
@sentry/replay - Webpack (gzipped + minified) 43.31 KB (+0.35% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 69.67 KB (+0.15% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 61.94 KB (+0.17% 🔺)

@@ -45,3 +45,8 @@ export const SLOW_CLICK_SCROLL_TIMEOUT = 300;

/** When encountering a total segment size exceeding this size, stop the replay (as we cannot properly ingest it). */
export const REPLAY_MAX_EVENT_BUFFER_SIZE = 20_000_000; // ~20MB

/** Replays must be min. 4s long before we send them. */
export const MIN_REPLAY_DURATION = 4_000;
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this 5

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe let's make it 4_999, wanted to ensure we avoid race conditions with the 5s flush delay...?

packages/replay/src/constants.ts Outdated Show resolved Hide resolved
@@ -51,6 +56,7 @@ export class Replay implements Integration {
public constructor({
flushMinDelay = DEFAULT_FLUSH_MIN_DELAY,
flushMaxDelay = DEFAULT_FLUSH_MAX_DELAY,
minReplayDuration = MIN_REPLAY_DURATION,
Copy link
Member

Choose a reason for hiding this comment

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

should DEFAULT_FLUSH_MIN_DELAY = MIN_REPLAY_DURATION?

Copy link
Member

Choose a reason for hiding this comment

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

Or probably MIN_REPLAY_DURATION + small delay?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd set them up for 5_000 flush delay, 4_999 min duration..?

@@ -1100,6 +1101,23 @@ export class ReplayContainer implements ReplayContainerInterface {
return;
}

const start = this._context.initialTimestamp;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is reliable to use as the start of the session? We should trace where this and the session start gets updated

Copy link
Member Author

Choose a reason for hiding this comment

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

we cannot use session.start here as that may be outdated for e.g. buffer session etc. What we care about (?? I think at least?) is the first event that may actually be sent, which should be this timestamp?

packages/replay/test/integration/flush.test.ts Outdated Show resolved Hide resolved
packages/replay/test/integration/flush.test.ts Outdated Show resolved Hide resolved

// If session is too short, or too long (allow some wiggle room over maxSessionLife), do not send it
// This _should_ not happen, but it may happen if flush is triggered due to a page activity change or similar
if (duration < this._options.minReplayDuration || duration > this.timeouts.maxSessionLife + 5_000) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we have different logic if replay is too long? e.g. it needs to start a new session (though that logic could be where the bug is).

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, we have a bunch of logic for this (and tests...), so ideally it should really not happen (but it is 😬 ). IMHO I'd get out this fix, I also added some logic to log out when this happens, so maybe we can look at this then to help figure out why this even happens.

Copy link
Member

Choose a reason for hiding this comment

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

Should we log out when we update initialTimestamp and session start?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add another log for this! 👍

@@ -981,6 +982,9 @@ export class ReplayContainer implements ReplayContainerInterface {

const earliestEvent = eventBuffer.getEarliestTimestamp();
if (earliestEvent && earliestEvent < this._context.initialTimestamp) {
// eslint-disable-next-line no-console
const log = this.getOptions()._experiments.traceInternals ? console.info : logger.info;
__DEBUG_BUILD__ && log(`[Replay] Updating initial timestamp to ${earliestEvent}`);
Copy link
Member Author

Choose a reason for hiding this comment

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

@billyvg added this log here, that's the only place that updates this (apart from initial setup, which sets it to Date.now() by default).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants