Skip to content

feat(replay): Handle large amounts of consecutive events #7211

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

Closed
wants to merge 1 commit into from

Conversation

mydea
Copy link
Member

@mydea mydea commented Feb 17, 2023

We've seen problems when a lot of events are happening at the same time. E.g. when thousands of mutation observer events are triggered at the same time, this can lead to very poor performance.

With this change, we detect if more than 2000 events happen in a rolling time window of 100ms. If so, we pause the replay, wait for 100ms, and resume it (which will trigger a full snapshot).

Note: I've selected the values 2000 & 100ms completely random. We can think about what reasonable amounts are here.
This is not a perfect fix, but IMHO it is a better experience to have a 100ms gap in your replay (and afterwards it should continue normally), than to freeze your page.

@billyvg can you try this with the repro app you've got?

We've seen problems when a lot of events are happening at the same time. E.g. when thousands of mutation observer events are triggered at the same time, this can lead to very poor performance.

With this change, we detect if more than 2000 events happen in a rolling time window of 100ms. If so, we pause the replay, wait for 100ms, and resume it (which will trigger a full snapshot).
@mydea mydea requested review from billyvg and Lms24 February 17, 2023 08:53
@mydea mydea self-assigned this Feb 17, 2023
@github-actions
Copy link
Contributor

Replay SDK metrics 🚀

    Plain +Sentry +Replay
Revision Value Value Diff Ratio Value Diff Ratio
LCP This PR aacdf2b 93.66 ms 123.32 ms +29.66 ms +31.66 % 157.38 ms +63.72 ms +68.03 %
Previous 1cf8988 70.81 ms 94.27 ms +23.46 ms +33.13 % 124.62 ms +53.81 ms +75.99 %
CLS This PR aacdf2b 0.06 ms 0.06 ms +0.00 ms +0.07 % 0.06 ms +0.00 ms +0.20 %
Previous 1cf8988 0.06 ms 0.06 ms -0.00 ms -0.39 % 0.06 ms -0.00 ms -0.46 %
CPU This PR aacdf2b 21.67 % 24.57 % +2.90 pp +13.37 % 30.16 % +8.49 pp +39.17 %
Previous 1cf8988 12.30 % 12.01 % -0.29 pp -2.37 % 17.10 % +4.80 pp +39.02 %
JS heap avg This PR aacdf2b 1.94 MB 2 MB +65.34 kB +3.37 % 2.86 MB +926.13 kB +47.78 %
Previous 1cf8988 1.94 MB 1.99 MB +48.93 kB +2.52 % 2.87 MB +929.88 kB +47.85 %
JS heap max This PR aacdf2b 2.3 MB 2.57 MB +264.75 kB +11.50 % 3.36 MB +1.06 MB +46.08 %
Previous 1cf8988 2.3 MB 2.57 MB +267.17 kB +11.60 % 3.35 MB +1.05 MB +45.56 %
netTx This PR aacdf2b 0 B 0 B 0 B n/a 2.23 kB +2.23 kB n/a
Previous 1cf8988 0 B 0 B 0 B n/a 2.22 kB +2.22 kB n/a
netRx This PR aacdf2b 0 B 0 B 0 B n/a 41 B +41 B n/a
Previous 1cf8988 0 B 0 B 0 B n/a 41 B +41 B n/a
netCount This PR aacdf2b 0 0 0 n/a 1 +1 n/a
Previous 1cf8988 0 0 0 n/a 1 +1 n/a
netTime This PR aacdf2b 0.00 ms 0.00 ms 0.00 ms n/a 120.01 ms +120.01 ms n/a
Previous 1cf8988 0.00 ms 0.00 ms 0.00 ms n/a 91.07 ms +91.07 ms n/a

Previous results on branch: develop

RevisionLCPCLSCPUJS heap avgJS heap maxnetTxnetRxnetCountnetTime
1cf8988+53.81 ms-0.00 ms+4.80 pp+929.88 kB+1.05 MB+2.22 kB+41 B+1+91.07 ms
68655e3+72.60 ms+0.00 ms+7.90 pp+922.72 kB+1.04 MB+2.22 kB+41 B+1+109.40 ms
a8449de+58.27 ms-0.00 ms+7.12 pp+927.42 kB+1.05 MB+2.2 kB+41 B+1+98.31 ms
79babe9+58.69 ms-0.00 ms+4.40 pp+927.46 kB+1.06 MB+2.23 kB+41 B+1+103.20 ms
5359ba9+55.62 ms-0.00 ms+4.29 pp+935.26 kB+1.05 MB+2.2 kB+41 B+1+79.05 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Fri, 17 Feb 2023 08:57:44 GMT

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 - this might improve the problems described in #6946

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.05 KB (0%)
@sentry/browser - ES5 CDN Bundle (minified) 62.14 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.68 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 55.29 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.41 KB (0%)
@sentry/browser - Webpack (minified) 66.73 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.44 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.78 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.93 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.2 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.68 KB (+0.37% 🔺)
@sentry/replay - Webpack (gzipped + minified) 36.88 KB (+0.44% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.31 KB (+0.27% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 53.91 KB (+0.3% 🔺)

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

This is not going to help in our particular case because the issue is in the rrweb library before an event is even emitted/processed.

@mydea
Copy link
Member Author

mydea commented Mar 2, 2023

Closing this in favor of #7314 and other approaches following this.

@mydea mydea closed this Mar 2, 2023
@mydea mydea deleted the fn/replay-detect-many-changes branch March 2, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants