Skip to content

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 Lms24 and billyvg 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% 🔺)

* 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.

4 participants