feat(replay): Stop replay when event buffer exceeds max. size #8315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
| @@ -30,6 +33,12 @@ export class EventBufferArray implements EventBuffer { | |||
|
|
|||
| /** @inheritdoc */ | |||
| public async addEvent(event: RecordingEvent): Promise<AddEventResult> { | |||
| const eventSize = JSON.stringify(event).length; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
mydea
added
the
CI-Overhead-Measurements
mydea
force-pushed
the
fn/drop-replay-large-segment
branch
from
92faa6b
to
4726940
Compare
Replay SDK metrics 🚀
develop |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Revision | LCP | CLS | CPU | JS heap avg | JS heap max | netTx | netRx | netCount | netTime |
|---|---|---|---|---|---|---|---|---|---|
| a4c858f | -12.66 ms | 0.00 ms | +21.87 pp | +7.38 MB | +10.21 MB | +106.55 kB | +164 B | +4 | +320.66 ms |
| db8421c | -9.73 ms | 0.00 ms | +18.33 pp | +7.22 MB | +10.66 MB | +106.51 kB | +164 B | +4 | +276.01 ms |
| d04488e | -23.76 ms | 0.00 ms | +22.85 pp | +7.02 MB | +10.73 MB | +106.62 kB | +164 B | +4 | +357.55 ms |
| 7563d56 | +5.62 ms | 0.00 ms | +20.84 pp | +7.28 MB | +10.11 MB | +106.48 kB | +164 B | +4 | +355.29 ms |
| 2868626 | -2.55 ms | 0.00 ms | +24.04 pp | +7.17 MB | +10.22 MB | +106.61 kB | +164 B | +4 | +327.35 ms |
| 435847d | +4.35 ms | 0.00 ms | +18.09 pp | +7.5 MB | +10.58 MB | +118.07 kB | +164 B | +4 | +294.38 ms |
| 8934ccc | -3.00 ms | 0.00 ms | +21.24 pp | +7.12 MB | +10.26 MB | +106.52 kB | +164 B | +4 | +295.94 ms |
| 8934ccc | -8.76 ms | 0.00 ms | +20.52 pp | +7.14 MB | +10.12 MB | +106.55 kB | +164 B | +4 | +248.77 ms |
| 8934ccc | +9.46 ms | 0.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
fn/drop-replay-large-segment| Revision | LCP | CLS | CPU | JS heap avg | JS heap max | netTx | netRx | netCount | netTime |
|---|---|---|---|---|---|---|---|---|---|
| a4c858f | +13.65 ms | 0.00 ms | +18.57 pp | +7.3 MB | +11.03 MB | +106.48 kB | +164 B | +4 | +299.73 ms |
| a4c858f | -12.66 ms | 0.00 ms | +21.87 pp | +7.38 MB | +10.21 MB | +106.55 kB | +164 B | +4 | +320.66 ms |
| db8421c | -9.73 ms | 0.00 ms | +18.33 pp | +7.22 MB | +10.66 MB | +106.51 kB | +164 B | +4 | +276.01 ms |
| d04488e | -23.76 ms | 0.00 ms | +22.85 pp | +7.02 MB | +10.73 MB | +106.62 kB | +164 B | +4 | +357.55 ms |
| 7563d56 | +5.62 ms | 0.00 ms | +20.84 pp | +7.28 MB | +10.11 MB | +106.48 kB | +164 B | +4 | +355.29 ms |
| 2868626 | -2.55 ms | 0.00 ms | +24.04 pp | +7.17 MB | +10.22 MB | +106.61 kB | +164 B | +4 | +327.35 ms |
| 435847d | +4.35 ms | 0.00 ms | +18.09 pp | +7.5 MB | +10.58 MB | +118.07 kB | +164 B | +4 | +294.38 ms |
| 8934ccc | -3.00 ms | 0.00 ms | +21.24 pp | +7.12 MB | +10.26 MB | +106.52 kB | +164 B | +4 | +295.94 ms |
| 8934ccc | -8.76 ms | 0.00 ms | +20.52 pp | +7.14 MB | +10.12 MB | +106.55 kB | +164 B | +4 | +248.77 ms |
| 8934ccc | +9.46 ms | 0.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
Last updated: Mon, 19 Jun 2023 11:08:32 GMT
billyvg
reviewed
Jun 13, 2023
Hmm, I don't think we have easy access to that from pako? |
|
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 |
billyvg
approved these changes
Jun 16, 2023
When the buffer exceeds 20MB, stop the replay.
mydea
force-pushed
the
fn/drop-replay-large-segment
branch
from
4726940
to
5a5b0bf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the buffer exceeds ~20MB, stop the replay.
Closes #7657
Closes https://github.com/getsentry/team-replay/issues/94