Navigation Menu

Skip to content
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): Flush immediately on DOM checkouts #6463

Merged
merged 4 commits into from Dec 13, 2022

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Dec 7, 2022

We originally added this delay to flushing to prevent recording short (low value) recordings. However, this causes some issues if the user were to refresh/leave the page before the first recording segment is sent as the backend will consider the replay invalid if it does not have segment id 0 present.

Change to flush immediately to try to reduce the number of replays with missing first segments. Our UI has a default duration filter to hide replays < 5 seconds.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.72 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 61.13 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.51 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 54.66 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.28 KB (0%)
@sentry/browser - Webpack (minified) 66.33 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.3 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.5 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.69 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.14 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 41.7 KB (-0.04% 🔽)
@sentry/replay - Webpack (gzipped + minified) 37.96 KB (-0.04% 🔽)

@billyvg billyvg marked this pull request as ready for review December 7, 2022 18:27
@billyvg
Copy link
Member Author

billyvg commented Dec 8, 2022

Depends on #6467 for some matcher changes.

@billyvg billyvg changed the base branch from master to feat-replay-add-toHaveLastSentReplay December 8, 2022 14:43
@billyvg billyvg force-pushed the feat/replays/flush-immediately-after-checkout branch from a686688 to 3f1aa3c Compare December 8, 2022 22:46
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

I like it, it really simplifies things as well here!

Base automatically changed from feat-replay-add-toHaveLastSentReplay to master December 12, 2022 15:13
@billyvg billyvg force-pushed the feat/replays/flush-immediately-after-checkout branch from 3f1aa3c to 83e9569 Compare December 12, 2022 15:54
We originally added this delay to flushing to prevent recording short (low value) recordings. However, this causes some issues if the user were to refresh/leave the page before the first recording segment is sent as the backend will consider the replay invalid if it does not have segment id 0 present.

Change to flush immediately to try to reduce the number of replays with missing first segments. Our UI has a default duration filter to hide replays < 5 seconds.
@billyvg billyvg force-pushed the feat/replays/flush-immediately-after-checkout branch from a991eba to ac77102 Compare December 13, 2022 18:19
@billyvg billyvg merged commit 5ba721a into master Dec 13, 2022
@billyvg billyvg deleted the feat/replays/flush-immediately-after-checkout branch December 13, 2022 19:01
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.

None yet

3 participants