fix(replay): Handle compression failures more robustly by mydea · Pull Request #6988 · getsentry/sentry-javascript

mydea · 2023-01-31T13:13:43Z

We now stop the recording when either addEvent or finish fail.

Other semi-related changes:

We send the event as a string to the worker now. Currently, we serialize/deserialize/serialize it, we really only need the string form on the worker anyhow, that should save a bit of processing.
We do not really need the pendingEvents on the buffer, as well as pendingLength. We really only need to know if any event has been added (I think?).
I renamend the worker method init to clear, as that is IMHO clearer.
I split the event buffer tests up into separate files per module

mydea · 2023-01-31T13:14:18Z

packages/replay/src/eventBuffer/EventBufferProxy.ts

-    this._ensureWorkerIsLoadedPromise = this._ensureWorkerIsLoaded().catch(() => {
-      // Ignore errors here
-    });
+    this._ensureWorkerIsLoadedPromise = this._ensureWorkerIsLoaded();


No need to catch this, as we catch everything in this method now.

mydea · 2023-01-31T13:14:46Z

packages/replay/src/replay.ts


    // Check eventBuffer again, as it could have been stopped in the meanwhile
-    if (!this.eventBuffer || !this.eventBuffer.pendingLength) {
+    if (!this.eventBuffer || !this.eventBuffer.hasEvents) {


I believe this was actually not really correct before? as pendingLength could be 0 in a compression worker if everything was correctly compressed?

How is hasEvents different from pendingLength?

pendingLength in the compression worker was subtracted when an event was successfully compressed (=sent to the worker). So if there would have been 10 events in the worker, but nothing pending to be sent, this would have been 0.

Ah it doesn't matter now, but pendingLength was never subtracted, it only got reset when we finish the compression payload, so I think these two are the same

Ah you're right of course, I misunderstood this. I guess it's fine anyhow 👍 sorry about the confusion!

mydea · 2023-01-31T13:15:07Z

packages/replay/src/util/addEvent.ts

  }

-  return replay.eventBuffer.addEvent(event, isCheckout);
+  try {


This is the "actual" change. Note that for finish we already try-catch this (when flushing).

github-actions · 2023-01-31T13:21:53Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.87 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.54 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	20.29 KB (0%)
@sentry/browser - Webpack (minified)	66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)	20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.79 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.08 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	44.16 KB (-0.1% 🔽)
@sentry/replay - Webpack (gzipped + minified)	38.89 KB (-0.17% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	61.5 KB (-0.09% 🔽)

mydea · 2023-01-31T16:09:21Z

packages/replay/src/eventBuffer/WorkerHandler.ts

@@ -0,0 +1,106 @@
+import { logger } from '@sentry/utils';


Note: I extracted this out of the EventBufferCompressionWorker, first because we may need it in follow ups (for error handling etc.), second I think it makes sense architecturally to have this separated - no need to leak this through the buffer implementation! Also allows us to easier encapsulate the id handling stuff etc.

billyvg

LGTM aside from a few questions

billyvg · 2023-01-31T23:02:01Z

packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts

-  public get pendingEvents(): RecordingEvent[] {
-    return this._pendingEvents;


pendingEvents was added to attempt to send a segment on unload (without compression), but since we have decided to pause on that, I think it's good to remove this 👍

billyvg · 2023-01-31T23:06:59Z

packages/replay/worker/src/Compressor.ts

    // that we end up with a list of events
-    const prefix = this.added > 0 ? ',' : '';
+    const prefix = this._hasEvents ? ',' : '';
    // TODO: We may want Z_SYNC_FLUSH or Z_FULL_FLUSH (not sure the difference)


We can remove this now 😅

billyvg · 2023-01-31T23:56:47Z

packages/replay/src/replay.ts


    // Check eventBuffer again, as it could have been stopped in the meanwhile
-    if (!this.eventBuffer || !this.eventBuffer.pendingLength) {
+    if (!this.eventBuffer || !this.eventBuffer.hasEvents) {


How is hasEvents different from pendingLength?

billyvg · 2023-02-01T00:05:56Z

packages/replay/src/util/addEvent.ts

+    __DEBUG_BUILD__ && logger.error(error);
+    replay.stop();


Since this gets caught and doesn't bubble up, the callers will continue to run -- is that going to cause issues?

It should be, I guess in the worst case we call addEvent again somewhere, which will no-op because it is stopped.

billyvg · 2023-02-01T00:06:23Z

packages/replay/worker/src/Compressor.ts

    // Using NO_FLUSH here for now as we can create many attachments that our
    // web UI will get API rate limited.
-    this.deflate.push(prefix + JSON.stringify(data), constants.Z_SYNC_FLUSH);
+    this.deflate.push(prefix + data, constants.Z_SYNC_FLUSH);


Lms24

LGTM

We now stop the recording when either `addEvent` or `finish` fail.

mydea added the Package: replay Issues related to the Sentry Replay SDK label Jan 31, 2023

mydea requested review from Lms24 and billyvg January 31, 2023 13:13

mydea self-assigned this Jan 31, 2023

mydea commented Jan 31, 2023

View reviewed changes

mydea changed the base branch from master to develop January 31, 2023 15:38

mydea force-pushed the fn/replay-handle-worker-errors branch from 27439bb to 4f7e6d5 Compare January 31, 2023 16:06

mydea commented Jan 31, 2023

View reviewed changes

mydea mentioned this pull request Jan 31, 2023

Error in compression worker being spammed after adding replay integration #6923

Closed

3 tasks

billyvg reviewed Feb 1, 2023

View reviewed changes

mydea force-pushed the fn/replay-handle-worker-errors branch from 4f7e6d5 to 0f115ab Compare February 1, 2023 08:55

Lms24 approved these changes Feb 1, 2023

View reviewed changes

mydea force-pushed the fn/replay-handle-worker-errors branch from 0f115ab to 39e7908 Compare February 1, 2023 09:34

mydea enabled auto-merge (squash) February 1, 2023 09:35

mydea added 2 commits February 1, 2023 10:09

fix(replay): Handle compression failures more robustly

8f5b997

We now stop the recording when either `addEvent` or `finish` fail.

ref(replay): Extract worker code into class

5b53ef6

mydea force-pushed the fn/replay-handle-worker-errors branch from 39e7908 to 5b53ef6 Compare February 1, 2023 10:14

mydea merged commit 5c41ab7 into develop Feb 1, 2023

mydea deleted the fn/replay-handle-worker-errors branch February 1, 2023 10:31

		public get pendingEvents(): RecordingEvent[] {
		return this._pendingEvents;

Uh oh!

Conversation

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

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!

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!

github-actions bot commented Jan 31, 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!

billyvg left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mydea commented Jan 31, 2023 •

edited

Loading

github-actions bot commented Jan 31, 2023 •

edited

Loading