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.

Sign up for GitHub

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

Jump to bottom

feat(replay): Upgrade to rrweb 2.0 #8760

Merged
merged 26 commits into from
Sep 27, 2023
Merged

feat(replay): Upgrade to rrweb 2.0 #8760

merged 26 commits into from
Sep 27, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Aug 9, 2023 •
edited
Loading

This is fully backwards compatible with prior versions of the Replay SDK. The only breaking change that we will making is to not be masking aria-label by default. The reason for this change is to align with our core SDK which also does not mask aria-label. This change also enables better support of searching by clicks.

Another change that needs to be highlighted is the 13% bundle size increase. This bundle size increase is necessary to bring improved recording performance and improved replay fidelity, especially in regards to web components and iframes. We will be investigating the reduction of the bundle size in this PR.

Here are benchmarks comparing the version 1 of rrweb to version 2

metric v1 v2
lcp 1486.06 ms 1529.11 ms
cls 0.40 ms 0.40 ms
fid 1.53 ms 1.50 ms
tbt 3207.22 ms 3036.80 ms
memoryAvg 131.83 MB 124.84 MB
memoryMax 324.8 MB 339.03 MB
netTx 282.67 KB 272.51 KB
netRx 8.02 MB 8.07 MB

@billyvg billyvg changed the title !!!! export some types feat(replay): Upgrade to rrweb 2.0 Aug 9, 2023
@billyvg billyvg requested a review from mydea August 9, 2023 12:50
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023 •
edited
Loading

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 84.25 KB (+11.48% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.46 KB (0%)
@sentry/browser - Webpack (gzipped) 22.06 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 78.7 KB (+11.98% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.57 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.65 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 254.13 KB (+14.37% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 86.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 61.42 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.43 KB (+0.01% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 84.28 KB (+11.48% 🔺)
@sentry/react - Webpack (gzipped) 22.09 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 102.22 KB (+9.35% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 51.04 KB (0%)

billyvg
billyvg commented Aug 10, 2023
View reviewed changes
packages/replay/src/util/getPrivacyOptions.ts Outdated
Comment on lines 81 to 82
// maskInputSelector: maskSelector,
// unmaskInputSelector: unmaskSelector,
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't needed since it's just duplicate of maskSelector/unmaskSelector

mydea
mydea reviewed Aug 11, 2023
View reviewed changes
...gration-tests/suites/replay/multiple-pages/test.ts-snapshots/seg-5-snap-incremental-chromium
@@ -5,8 +5,8 @@
"source": 2,
"type": 1,
"id": 12,
"x": 41,
"y": 90
"x": 41.810001373291016,
Copy link
Member

Choose a reason for hiding this comment

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

are we sure these do not flake 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Will double check where these are even used, does seem sus

Copy link
Member Author

@billyvg billyvg Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ok I assume rrweb updated something so we get more precision from a quick look at the code, the positions should be coming from the mouse event itself

hasn't flaked so 🤞

@mydea
Copy link
Member

mydea commented Aug 11, 2023

Overall looks great, left a few small comments. Bundle size increases are sad but I guess it is what it is - eventually we can take a look at potential savings by shaking out stuff (?) we don't need.

I didn't look over the test changes in detail, I trust you that they make sense xD

@AbhiPrasad
Copy link
Member

Bundle size increases are sad but I guess it is what it is - eventually we can take a look at potential savings by shaking out stuff (?) we don't need

I skimmed over the rrweb codebase, there's some improvements that can be upstreamed. Looking at https://unpkg.com/[email protected]/dist/rrweb.min.js some easy ones are to use terser and minify private class properties and convert some classes to objects.

@billyvg
Copy link
Member Author

billyvg commented Aug 11, 2023

sessionInactive test is a bit flakey for webkit -- looks like the ids are off by 15, which is the number of nodes in the snapshot. https://github.com/getsentry/sentry-javascript/actions/runs/5834071494/job/15822830869?pr=8760#step:13:884

@AbhiPrasad AbhiPrasad mentioned this pull request Aug 14, 2023
Closed
@billyvg billyvg force-pushed the feat-rrweb2-alpha branch 2 times, most recently from 5d58230 to 238e185 Compare August 16, 2023 19:40
@billyvg
Copy link
Member Author

billyvg commented Aug 17, 2023

Node IDs are flaking for this test: 1) [webkit] › suites/replay/sessionExpiry/test.ts:17:11 › handles an expired session

@billyvg billyvg force-pushed the feat-rrweb2-alpha branch 2 times, most recently from cf1c93a to b4b4a2b Compare August 30, 2023 22:16
@billyvg billyvg force-pushed the feat-rrweb2-alpha branch 4 times, most recently from be81bb0 to 53e33e0 Compare September 13, 2023 13:47
@billyvg billyvg force-pushed the feat-rrweb2-alpha branch 4 times, most recently from be58ba9 to 76c513a Compare September 21, 2023 19:06
mydea
mydea approved these changes Sep 27, 2023
View reviewed changes
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.

:shipit:

@billyvg billyvg merged commit 398c6ba into develop Sep 27, 2023
83 checks passed
@billyvg billyvg deleted the feat-rrweb2-alpha branch September 27, 2023 15:09
@AbhiPrasad
Copy link
Member

Congrats on the ship @billyvg!

@bruno-garcia bruno-garcia mentioned this pull request Sep 27, 2023
Closed
c298lee pushed a commit that referenced this pull request Sep 27, 2023
@billyvg @c298lee
feat(replay): Upgrade to rrweb 2.0 (#8760)
bab1849 
This is fully backwards compatible with prior versions of the Replay
SDK. The only breaking change that we will making is to not be masking
`aria-label` by default. The reason for this change is to align with our
core SDK which also does not mask `aria-label`. This change also enables
better support of searching by clicks.

Another change that needs to be highlighted is the 13% bundle size
increase. This bundle size increase is necessary to bring improved
recording performance and improved replay fidelity, especially in
regards to web components and iframes. We will be investigating the
reduction of the bundle size in [this
PR](#8815).

Here are benchmarks comparing the version 1 of rrweb to version 2

| metric    | v1         | v2         |
| --------- | ---------- | ---------- |
| lcp       | 1486.06 ms | 1529.11 ms |
| cls       | 0.40 ms    | 0.40 ms    |
| fid       | 1.53 ms    | 1.50 ms    |
| tbt       | 3207.22 ms | 3036.80 ms |
| memoryAvg | 131.83 MB  | 124.84 MB  |
| memoryMax | 324.8 MB   | 339.03 MB  |
| netTx     | 282.67 KB  | 272.51 KB  |
| netRx     | 8.02 MB    | 8.07 MB    |
c298lee added a commit that referenced this pull request Sep 27, 2023
@c298lee
Revert "feat(replay): Upgrade to rrweb 2.0 (#8760)"
37d759e 
This reverts commit bab1849.
c298lee added a commit that referenced this pull request Sep 29, 2023
@c298lee
Revert "feat(replay): Upgrade to rrweb 2.0 (#8760)"
7bd9e41 
This reverts commit bab1849.
billyvg pushed a commit that referenced this pull request Sep 29, 2023
@c298lee @billyvg
Revert "feat(replay): Upgrade to rrweb 2.0 (#8760)"
165f9f4 
This reverts commit bab1849.
billyvg added a commit that referenced this pull request Sep 29, 2023
@billyvg
Revert "Revert "feat(replay): Upgrade to rrweb 2.0 (#8760)""
3063b07 
This reverts commit 165f9f4.
@mydea mydea mentioned this pull request Oct 16, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mydea mydea mydea approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@billyvg @mydea @AbhiPrasad