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

fix(replay): Fix incorrect uncompressed recording size due to encoding #6740

Merged
merged 8 commits into from Jan 16, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Jan 11, 2023

For uncompressed payloads, we were potentially sending the incorrect size due to encoding issues. Instead encode to UTF8 when determining the payload size. Note that TextEncoder mostly overlaps with MutationObserver support except for IE11. So we will not be supporting IE11.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 11, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.84 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 61.46 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.61 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.37 KB (0%)
@sentry/browser - Webpack (minified) 66.54 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.4 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.67 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.83 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.28 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.43 KB (-0.65% 🔽)
@sentry/replay - Webpack (gzipped + minified) 38.65 KB (-0.79% 🔽)

@billyvg billyvg marked this pull request as ready for review January 11, 2023 21:02
@billyvg billyvg force-pushed the fix-replay-fix-recording-size-uncompressed branch from 1a9a5a7 to bd1bb06 Compare January 11, 2023 22:44
@billyvg billyvg requested review from mydea and Lms24 January 11, 2023 22:50
@@ -17,6 +18,10 @@ useFakeTimers();
let replay: ReplayContainer;

describe('Integration | coreHandlers | handleGlobalEvent', () => {
beforeAll(() => {
(global as any).TextEncoder = TextEncoder;
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this into jest.setup.ts or similar? Just an idea?

Copy link
Member

Choose a reason for hiding this comment

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

+1

@mydea
Copy link
Member

mydea commented Jan 12, 2023

Does this include the pendingLength changes on purpose?

@billyvg billyvg force-pushed the fix-replay-fix-recording-size-uncompressed branch from bd1bb06 to ce59300 Compare January 12, 2023 16:11
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM and +1 on moving this(global as any).TextEncoder = TextEncoder line to somewhere where we don't have to add it to every integration test.

@@ -17,6 +18,10 @@ useFakeTimers();
let replay: ReplayContainer;

describe('Integration | coreHandlers | handleGlobalEvent', () => {
beforeAll(() => {
(global as any).TextEncoder = TextEncoder;
Copy link
Member

Choose a reason for hiding this comment

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

+1

For uncompressed payloads, we were potentially sending the incorrect size due to encoding issues. Instead encode to UTF8 when determining the payload size. Note that `TextEncoder` mostly overlaps with `MutationObserver` support *except* for IE11. So we will not be supporting IE11.
@mydea mydea force-pushed the fix-replay-fix-recording-size-uncompressed branch from 40e3e06 to 129820f Compare January 16, 2023 12:42
@mydea
Copy link
Member

mydea commented Jan 16, 2023

I will go ahead and merge this so we get this into todays release!

@mydea mydea changed the title fix(replay): Fix recording size incorrect due to encoding fix(replay): Fix uncompressed recording size incorrect due to encoding Jan 16, 2023
@mydea mydea changed the title fix(replay): Fix uncompressed recording size incorrect due to encoding fix(replay): Fix incorrect uncompressed recording size due to encoding Jan 16, 2023
@mydea mydea self-assigned this Jan 16, 2023
@mydea mydea enabled auto-merge (squash) January 16, 2023 12:57
@mydea mydea merged commit aed2ce6 into master Jan 16, 2023
@mydea mydea deleted the fix-replay-fix-recording-size-uncompressed branch January 16, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants