-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(replay): Stop replay when event buffer exceeds max. size #8315
Conversation
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.
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?
There was a problem hiding this comment.
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).
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 |
Last updated: Mon, 19 Jun 2023 11:08:32 GMT
There was a problem hiding this comment.
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?
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 |
When the buffer exceeds 20MB, stop the replay.
4726940
to
5a5b0bf
Compare
When the buffer exceeds ~20MB, stop the replay.
Closes #7657
Closes https://github.com/getsentry/team-replay/issues/94