feat(replay): Keep min. 30 seconds of data in error mode #7025

mydea · 2023-02-01T12:58:29Z

Supersedes #6924

This PR adjusts the event buffer to keep 30-60s of recording data (instead of 0-60s) in error mode.

This is accomplished by keeping the events in a queue that tracks when the last checkout happens, and allows to delete everything up until this event.

In compression mode, this means that we do not stream the events to the compressor, but compress them all at once. That's necessary because it is not really possible to remove parts of the compression stream.

In session mode, we continue to use the streaming compression as we do right now.

github-actions · 2023-02-01T13:06:55Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	20.06 KB (0%)
@sentry/browser - ES5 CDN Bundle (minified)	62.19 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.69 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified)	55.33 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	20.42 KB (0%)
@sentry/browser - Webpack (minified)	66.77 KB (0%)
@sentry/react - Webpack (gzipped + minified)	20.45 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	47.85 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.97 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.23 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	42.57 KB (+0.7% 🔺)
@sentry/replay - Webpack (gzipped + minified)	37 KB (+0.83% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	60.22 KB (+0.48% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	53.77 KB (+0.54% 🔺)

mydea · 2023-02-01T14:40:55Z

packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts


 const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };

 describe('Unit | eventBuffer | EventBufferArray', () => {
-  it('adds events to normal event buffer', async function () {
-    const buffer = createEventBuffer({ useCompression: false });
+  for (let recordingMode of ['session', 'error'] as ReplayRecordingMode[]) {


We just make sure here that this works the same in both error & session mode.

mydea · 2023-02-01T14:42:07Z

packages/replay/test/unit/util/addEvent.test.ts

@@ -29,4 +34,97 @@ describe('Unit | util | addEvent', () => {

    expect(replay.isEnabled()).toEqual(false);
  });
+
+  it.each([


These are the most relevant tests here, making sure it works the same with & without compression for both error & session mode.

billyvg · 2023-02-01T21:57:34Z

packages/replay/src/replay.ts

@@ -182,6 +182,7 @@ export class ReplayContainer implements ReplayContainerInterface {

    this.eventBuffer = createEventBuffer({
      useCompression: this._options.useCompression,
+      recordingMode: this.recordingMode,


Is there gonna be an issue when we switch from error -> session recording mode?

Shouldn't, it only means that (for now) we will continue in partitioned mode for compression - meaning even after the error was sent, we'll still not be using the streaming compression but the "all at once" compression. We may adjust this in the future, but this'll mean to swap out the event buffer at runtime, which I wanted to avoid for this first step.

Maybe it is clearer to rename this, e.g. instead of recordingMode make it rememberLastCheckout: boolean? Then it's a bit clearer what it does and that it is not really directly related to the recording mode?

changed this to keepLastCheckout: boolean, which should hopefully be clearer!

packages/replay/test/unit/eventBuffer/EventBufferPartitionedCompressionWorker.tes.ts

packages/replay/test/utils/setupReplayContainer.ts

billyvg · 2023-02-01T22:45:37Z

packages/replay/src/util/addEvent.ts

+      if (replay.recordingMode === 'error') {
+        // Do not wait on it, just do it
+        // We know in this mode this is actually "sync"
+        void eventBuffer.clear(true);
+
+        // Ensure we have the correct first checkout timestamp when an error occurs
+        if (!session.segmentId) {
+          replay.getContext().earliestEvent = eventBuffer.getEarliestTimestamp();
+        }


Just to clarify for myself, we examine the buffer for the earliest event because we want the replay "start time" to be when the 1st checkout in the buffer happens.

So if session has been running for 2.5 minutes, we would want the replay to "start" at ~2 minutes. And replay.context.earliestEvent (before this block of code), would represent the earliest event of the entire "session".

So an issue I can think of is with the performance observer. I believe we only add them when we flush, so those events will end up becoming the earliest events anyway (which might be fine).

Yeah, to be honest all of this is a bit brittle to me 😅 but this was the best I could come up with to handle this in a somewhat reasonable way. I wonder if it makes sense to maybe compute this on the server? Or generally change how we keep this a bit, not sure...

semi-related to this PR but I was also wondering about that - do we discard events with timestamps before the initial full checkout on the server? Or would sending such events cause problems?

We discard events that are older than SESSION_IDLE_TIMEOUT minutes ago. Because we get buffered data from performance observer, those occur before initial checkout.

I wonder if we need to change/move this logic as well

sentry-javascript/packages/replay/src/replay.ts

Lines 552 to 557 in c3dd355

// See note above re: session start needs to reflect the most recent

// checkout.

if (this.recordingMode === 'error' && this.session && this._context.earliestEvent) {

this.session.started = this._context.earliestEvent;

this._maybeSaveSession();

}

I think this is to meant to update session start time so we know when to correctly timeout a session.

packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts

Lms24

This makes sense to me and I think it's going in a good direction.

mydea · 2023-02-09T09:09:23Z

FYI since main focus right now is on stability, let's probably wait for until GA to merge this in. WDYT @billyvg ?

billyvg · 2023-02-09T14:24:17Z

@mydea agreed!

github-actions · 2023-03-08T00:25:27Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

mydea · 2023-05-02T13:23:13Z

Closing in favor of #7992

mydea added the Package: replay Issues related to the Sentry Replay SDK label Feb 1, 2023

mydea requested review from billyvg and Lms24 February 1, 2023 12:58

mydea self-assigned this Feb 1, 2023

mydea mentioned this pull request Feb 1, 2023

feat(replay): Keep 30-60s instead of 0-60s of recording in replay errror mode #6924

Closed

mydea force-pushed the fn/replay-errors-30s branch 3 times, most recently from f0283ca to 7146b49 Compare February 1, 2023 14:37

mydea commented Feb 1, 2023

View reviewed changes

mydea force-pushed the fn/replay-errors-30s branch from 7146b49 to 07d63b6 Compare February 1, 2023 14:43

mydea marked this pull request as ready for review February 1, 2023 15:07

mydea force-pushed the fn/replay-errors-30s branch from 07d63b6 to 47bdb74 Compare February 1, 2023 15:29

billyvg reviewed Feb 1, 2023

View reviewed changes

mydea force-pushed the fn/replay-errors-30s branch from 47bdb74 to 3602fc4 Compare February 2, 2023 09:44

Lms24 reviewed Feb 2, 2023

View reviewed changes

packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts Show resolved Hide resolved

Lms24 approved these changes Feb 2, 2023

View reviewed changes

mydea force-pushed the fn/replay-errors-30s branch from 3602fc4 to 22275b0 Compare February 9, 2023 15:29

mydea mentioned this pull request Feb 10, 2023

ref(replay): Extract worker into dedicated package #7139

Merged

feat(replay): Keep min. 30 seconds of data in error mode

7ba8c7b

mydea force-pushed the fn/replay-errors-30s branch from 22275b0 to 7ba8c7b Compare February 14, 2023 10:22

bruno-garcia mentioned this pull request Feb 21, 2023

Keep min. 30 seconds for replay in error mode #6908

Open

github-actions bot added the Status: Stale label Mar 8, 2023

mydea added Status: In Progress and removed Status: Stale labels Mar 8, 2023

mydea mentioned this pull request Apr 27, 2023

ref(replay): Move earliest timestamp tracking to eventBuffer #7983

Merged

This was referenced Apr 28, 2023

ref(replay): Explicitly clear eventBuffer for checkout events #7989

Merged

feat(replay): Keep min. 30s of data in buffer & worker mode #7992

Closed

mydea closed this May 2, 2023

AbhiPrasad deleted the fn/replay-errors-30s branch March 27, 2025 15:51

	// See note above re: session start needs to reflect the most recent
	// checkout.
	if (this.recordingMode === 'error' && this.session && this._context.earliestEvent) {
	this.session.started = this._context.earliestEvent;
	this._maybeSaveSession();
	}

Uh oh!

feat(replay): Keep min. 30 seconds of data in error mode #7025

feat(replay): Keep min. 30 seconds of data in error mode #7025

Uh oh!

Conversation

mydea commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billyvg Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lms24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mydea commented Feb 9, 2023

Uh oh!

billyvg commented Feb 9, 2023

Uh oh!

github-actions bot commented Mar 8, 2023

Uh oh!

mydea commented May 2, 2023

Uh oh!

Uh oh!

mydea commented Feb 1, 2023 •

edited

Loading

github-actions bot commented Feb 1, 2023 •

edited

Loading

billyvg Feb 2, 2023 •

edited

Loading

Lms24 left a comment •

edited

Loading