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
fix(replay): Handle compression failures more robustly #6988
Conversation
this._ensureWorkerIsLoadedPromise = this._ensureWorkerIsLoaded().catch(() => { | ||
// Ignore errors here | ||
}); | ||
this._ensureWorkerIsLoadedPromise = this._ensureWorkerIsLoaded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to catch this, as we catch everything in this method now.
@@ -772,7 +772,7 @@ export class ReplayContainer implements ReplayContainerInterface { | |||
await this._addPerformanceEntries(); | |||
|
|||
// Check eventBuffer again, as it could have been stopped in the meanwhile | |||
if (!this.eventBuffer || !this.eventBuffer.pendingLength) { | |||
if (!this.eventBuffer || !this.eventBuffer.hasEvents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this was actually not really correct before? as pendingLength
could be 0 in a compression worker if everything was correctly compressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is hasEvents
different from pendingLength
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you're right of course, I misunderstood this. I guess it's fine anyhow 👍 sorry about the confusion!
@@ -39,5 +41,10 @@ export async function addEvent( | |||
replay.getContext().earliestEvent = timestampInMs; | |||
} | |||
|
|||
return replay.eventBuffer.addEvent(event, isCheckout); | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the "actual" change. Note that for finish
we already try-catch this (when flushing).
size-limit report 📦
|
27439bb
to
4f7e6d5
Compare
@@ -0,0 +1,106 @@ | |||
import { logger } from '@sentry/utils'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from a few questions
public get pendingEvents(): RecordingEvent[] { | ||
return this._pendingEvents; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
if (!data) { | ||
throw new Error('Adding invalid event'); | ||
} | ||
// If the event is not the first event, we need to prefix it with a `,` so | ||
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this now 😅
@@ -772,7 +772,7 @@ export class ReplayContainer implements ReplayContainerInterface { | |||
await this._addPerformanceEntries(); | |||
|
|||
// Check eventBuffer again, as it could have been stopped in the meanwhile | |||
if (!this.eventBuffer || !this.eventBuffer.pendingLength) { | |||
if (!this.eventBuffer || !this.eventBuffer.hasEvents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is hasEvents
different from pendingLength
?
__DEBUG_BUILD__ && logger.error(error); | ||
replay.stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this gets caught and doesn't bubble up, the callers will continue to run -- is that going to cause issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be, I guess in the worst case we call addEvent
again somewhere, which will no-op because it is stopped.
// TODO: We may want Z_SYNC_FLUSH or Z_FULL_FLUSH (not sure the difference) | ||
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
4f7e6d5
to
0f115ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
0f115ab
to
39e7908
Compare
We now stop the recording when either `addEvent` or `finish` fail.
39e7908
to
5b53ef6
Compare
We now stop the recording when either
addEvent
orfinish
fail.Other semi-related changes:
pendingEvents
on the buffer, as well aspendingLength
. We really only need to know if any event has been added (I think?).init
toclear
, as that is IMHO clearer.Closes #6923