Skip to content

feat(replay): Stop replay when event buffer exceeds max. size - #8315

Merged
mydea merged 1 commit into
developfrom
fn/drop-replay-large-segment
Jun 19, 2023
Merged

feat(replay): Stop replay when event buffer exceeds max. size#8315
mydea merged 1 commit into
developfrom
fn/drop-replay-large-segment

Conversation

@mydea

@mydea mydea commented Jun 12, 2023

Copy link
Copy Markdown
Member

When the buffer exceeds ~20MB, stop the replay.

Closes #7657
Closes https://github.com/getsentry/team-replay/issues/94

@mydea
mydea requested a review from billyvg June 12, 2023 08:08
@mydea mydea self-assigned this Jun 12, 2023
@mydea mydea mentioned this pull request Jun 12, 2023
Closed
@github-actions

github-actions Bot commented Jun 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 21.18 KB (0%)
@sentry/browser - ES5 CDN Bundle (minified) 66.13 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 19.71 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 58.6 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 21.33 KB (0%)
@sentry/browser - Webpack (minified) 69.52 KB (0%)
@sentry/react - Webpack (gzipped + minified) 21.36 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 49.28 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 28.79 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 27.03 KB (+0.01% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 49.29 KB (+0.24% 🔺)
@sentry/replay - Webpack (gzipped + minified) 43.01 KB (+0.25% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 68.33 KB (+0.19% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 61.22 KB (+0.21% 🔺)

bruno-garcia
bruno-garcia reviewed Jun 12, 2023
View reviewed changes
Comment thread packages/replay/src/eventBuffer/EventBufferArray.ts

/** @inheritdoc */
public async addEvent(event: RecordingEvent): Promise<AddEventResult> {
const eventSize = JSON.stringify(event).length;

@bruno-garcia bruno-garcia Jun 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems expensive to be running on each event. This is running before we pass the event to the web worker?
Do we stringify this in the transport? Or elsewhere so we can measure at that point?

@mydea mydea Jun 13, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, in the non-worker based event buffer it adds overhead as we need to stringify as we go. In the buffer-based worker we already do that, so there it adds no overhead. If we want to keep track in any way, we will have to add overhead to the non-worker based buffer (currently, we only stringify on finish(), which is the most performant - but gives us no idea how large we are as we go).

Comment thread packages/replay/src/constants.ts
@mydea
mydea force-pushed the fn/drop-replay-large-segment branch from 92faa6b to 4726940 Compare June 12, 2023 15:01
@github-actions

github-actions Bot commented Jun 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Replay SDK metrics 🚀

    Plain +Sentry +Replay
Revision Value Value Diff Ratio Value Diff Ratio
LCP This PR 3030643 69.98 ms 63.14 ms -6.84 ms -9.77 % 66.51 ms -3.47 ms -4.96 %
Baseline a4c858f 65.10 ms 72.63 ms +7.53 ms +11.57 % 78.75 ms +13.65 ms +20.97 %
CLS This PR 3030643 0.00 ms 0.00 ms 0.00 ms 0.00 % 0.00 ms 0.00 ms 0.00 %
Baseline a4c858f 0.00 ms 0.00 ms 0.00 ms 0.00 % 0.00 ms 0.00 ms 0.00 %
CPU This PR 3030643 19.24 % 18.73 % -0.51 pp -2.63 % 36.36 % +17.12 pp +89.00 %
Baseline a4c858f 20.14 % 20.67 % +0.53 pp +2.62 % 38.71 % +18.57 pp +92.19 %
JS heap avg This PR 3030643 3.52 MB 7.13 MB +3.61 MB +102.59 % 11.1 MB +7.58 MB +215.29 %
Baseline a4c858f 3.53 MB 7.14 MB +3.61 MB +102.38 % 10.83 MB +7.3 MB +206.78 %
JS heap max This PR 3030643 3.86 MB 8.62 MB +4.76 MB +123.18 % 14.3 MB +10.43 MB +270.03 %
Baseline a4c858f 3.87 MB 8.5 MB +4.63 MB +119.41 % 14.91 MB +11.03 MB +284.74 %
netTx This PR 3030643 0 B 360.1 kB +360.1 kB n/a 106.7 kB +106.7 kB n/a
Baseline a4c858f 0 B 360.1 kB +360.1 kB n/a 106.48 kB +106.48 kB n/a
netRx This PR 3030643 0 B 41 B +41 B n/a 164 B +164 B n/a
Baseline a4c858f 0 B 41 B +41 B n/a 164 B +164 B n/a
netCount This PR 3030643 0 1 +1 n/a 4 +4 n/a
Baseline a4c858f 0 1 +1 n/a 4 +4 n/a
netTime This PR 3030643 0.00 ms 133.90 ms +133.90 ms n/a 273.29 ms +273.29 ms n/a
Baseline a4c858f 0.00 ms 131.03 ms +131.03 ms n/a 299.73 ms +299.73 ms n/a

Baseline results on branch: develop
RevisionLCPCLSCPUJS heap avgJS heap maxnetTxnetRxnetCountnetTime
a4c858f-12.66 ms0.00 ms+21.87 pp+7.38 MB+10.21 MB+106.55 kB+164 B+4+320.66 ms
db8421c-9.73 ms0.00 ms+18.33 pp+7.22 MB+10.66 MB+106.51 kB+164 B+4+276.01 ms
d04488e-23.76 ms0.00 ms+22.85 pp+7.02 MB+10.73 MB+106.62 kB+164 B+4+357.55 ms
7563d56+5.62 ms0.00 ms+20.84 pp+7.28 MB+10.11 MB+106.48 kB+164 B+4+355.29 ms
2868626-2.55 ms0.00 ms+24.04 pp+7.17 MB+10.22 MB+106.61 kB+164 B+4+327.35 ms
435847d+4.35 ms0.00 ms+18.09 pp+7.5 MB+10.58 MB+118.07 kB+164 B+4+294.38 ms
8934ccc-3.00 ms0.00 ms+21.24 pp+7.12 MB+10.26 MB+106.52 kB+164 B+4+295.94 ms
8934ccc-8.76 ms0.00 ms+20.52 pp+7.14 MB+10.12 MB+106.55 kB+164 B+4+248.77 ms
8934ccc+9.46 ms0.00 ms+20.29 pp+7.39 MB+10.28 MB+106.63 kB+164 B+4+289.35 ms

Previous results on branch: fn/drop-replay-large-segment
RevisionLCPCLSCPUJS heap avgJS heap maxnetTxnetRxnetCountnetTime
a4c858f+13.65 ms0.00 ms+18.57 pp+7.3 MB+11.03 MB+106.48 kB+164 B+4+299.73 ms
a4c858f-12.66 ms0.00 ms+21.87 pp+7.38 MB+10.21 MB+106.55 kB+164 B+4+320.66 ms
db8421c-9.73 ms0.00 ms+18.33 pp+7.22 MB+10.66 MB+106.51 kB+164 B+4+276.01 ms
d04488e-23.76 ms0.00 ms+22.85 pp+7.02 MB+10.73 MB+106.62 kB+164 B+4+357.55 ms
7563d56+5.62 ms0.00 ms+20.84 pp+7.28 MB+10.11 MB+106.48 kB+164 B+4+355.29 ms
2868626-2.55 ms0.00 ms+24.04 pp+7.17 MB+10.22 MB+106.61 kB+164 B+4+327.35 ms
435847d+4.35 ms0.00 ms+18.09 pp+7.5 MB+10.58 MB+118.07 kB+164 B+4+294.38 ms
8934ccc-3.00 ms0.00 ms+21.24 pp+7.12 MB+10.26 MB+106.52 kB+164 B+4+295.94 ms
8934ccc-8.76 ms0.00 ms+20.52 pp+7.14 MB+10.12 MB+106.55 kB+164 B+4+248.77 ms
8934ccc+9.46 ms0.00 ms+20.29 pp+7.39 MB+10.28 MB+106.63 kB+164 B+4+289.35 ms
*) pp - percentage points - an absolute difference between two percentages.
Last updated: Mon, 19 Jun 2023 11:08:32 GMT

billyvg
billyvg reviewed Jun 13, 2023
View reviewed changes

@billyvg billyvg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For compression worker, we're looking at the size before compression, should we use the compressed size?

@mydea

mydea commented Jun 13, 2023

Copy link
Copy Markdown
Member Author

For compression worker, we're looking at the size before compression, should we use the compressed size?

Hmm, I don't think we have easy access to that from pako?

@mydea

mydea commented Jun 16, 2023

Copy link
Copy Markdown
Member Author

So based on a comment by @JoshFerge, we should look at the uncompressed size anyhow (as it can still lead to problems on the UI/render side later on even if it is well compressed). Considering this, are we fine merging this? cc @billyvg & @bruno-garcia

@mydea
feat(replay): Stop replay when event buffer exceeds max. size
5a5b0bf 
When the buffer exceeds 20MB, stop the replay.
@mydea
mydea force-pushed the fn/drop-replay-large-segment branch from 4726940 to 5a5b0bf Compare June 19, 2023 10:54
@mydea
mydea merged commit 34bb403 into develop Jun 19, 2023
@mydea
mydea deleted the fn/drop-replay-large-segment branch June 19, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bruno-garcia bruno-garcia bruno-garcia left review comments
@billyvg billyvg billyvg approved these changes

Assignees

@mydea mydea

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Replay: Flush based on buffer size

3 participants

@mydea @billyvg @bruno-garcia