Skip to content

fix(replay): Handle compression worker errors more gracefully #6936

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

Merged
merged 7 commits into from
Jan 26, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Jan 25, 2023

partially extracted from #6924

This PR improves the event buffer to handle compression worker errors more gracefully.

  1. Split up the event buffers into separate files
  2. Ensure the initial event is compressed
  3. Fall back to using the uncompressed payload when worker compression fails

@mydea mydea added the Package: replay Issues related to the Sentry Replay SDK label Jan 25, 2023
@mydea mydea requested review from billyvg and Lms24 January 25, 2023 19:07
@mydea mydea self-assigned this Jan 25, 2023
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

github-actions bot commented Jan 25, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.83 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 61.48 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.5 KB (-0.02% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 54.79 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.22 KB (0%)
@sentry/browser - Webpack (minified) 66.19 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.25 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.54 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.75 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.03 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.65 KB (+0.2% 🔺)
@sentry/replay - Webpack (gzipped + minified) 38.42 KB (+0.27% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.98 KB (+0.12% 🔺)

@@ -323,9 +330,18 @@ export class ReplayContainer implements ReplayContainerInterface {
* from calling both `flush` and `_debouncedFlush`. Otherwise, there could be
* cases of mulitple flushes happening closely together.
*/
public flushImmediate(): Promise<void> {
public async flushImmediate(waitForCompression?: boolean): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

Should this default true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I guess we could also remove this option and just always use this behavior 🤔 WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

That'd be even better

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 actually moved this into the worker proxy itself, which will now always wait for this when calling finish().

@mydea mydea force-pushed the fn/replay-eventBuffer branch from a6977c8 to 84a73d6 Compare January 26, 2023 15:12
@mydea
Copy link
Member Author

mydea commented Jan 26, 2023

@billyvg added a try-catch around the worker creation!

@mydea mydea force-pushed the fn/replay-eventBuffer branch from 5d7b5f7 to a6f11c8 Compare January 26, 2023 16:00
@mydea mydea merged commit b86ac10 into master Jan 26, 2023
@mydea mydea deleted the fn/replay-eventBuffer branch January 26, 2023 17:39
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.

3 participants