Skip to content

fix(replay): Ensure we normalize scope breadcrumbs to max. depth to avoid circular ref #7915

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

Merged
merged 2 commits into from
Apr 20, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Apr 20, 2023

After some in-depth debugging, I figured out that the replay may lock up if we do not normalize the scope breadcrumbs to a given depth.

This seems to fix the provided reproduction for me, and seems like a reasonable change overall.
10 is more or less a random value, but seems reasonable for me for this case?

it gets slower and slower with an increasing value there, becoming basically unusable around ~18-20.

I did some further digging, and the problem is that around a normalization of 20, it becomes crazy large:

console.log(JSON.stringify(normalize(breadcrumb, 20)));

results in a 60MB string 🤯

This is a simple first step to ensure we normalize all breadcrumbs to a max. depth. This applies to scope & dom breadcrumbs from Sentry itself.

This is not really a fix per se, as it could still be massive in shallow size. But it should help generally, and is perf-free (or even positive), as we are already walking this in normalize anyhow.

Ref https://github.com/getsentry/team-replay/issues/67

@mydea mydea added Type: Bug Package: replay Issues related to the Sentry Replay SDK labels Apr 20, 2023
@mydea mydea requested review from billyvg and Lms24 April 20, 2023 09:34
@mydea mydea self-assigned this Apr 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 20, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.98 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 65.49 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 19.52 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 57.95 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 21.12 KB (0%)
@sentry/browser - Webpack (minified) 68.9 KB (0%)
@sentry/react - Webpack (gzipped + minified) 21.14 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.99 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 28.55 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 26.78 KB (+0.01% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 46.01 KB (+1.45% 🔺)
@sentry/replay - Webpack (gzipped + minified) 39.92 KB (+1.64% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 64.87 KB (+1% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 57.85 KB (+1.14% 🔺)

@mydea mydea force-pushed the fn/fix-replay-infinite-loop branch from c6a5370 to 21da1b5 Compare April 20, 2023 09:47
@mydea mydea changed the title fix(replay): Ensure we normalize scope breadcrumbs to avoid circular ref fix(replay): Ensure we normalize scope breadcrumbs to max. depth to avoid circular ref Apr 20, 2023
@@ -27,7 +27,7 @@ export function addBreadcrumbEvent(replay: ReplayContainer, breadcrumb: Breadcru
timestamp: (breadcrumb.timestamp || 0) * 1000,
data: {
tag: 'breadcrumb',
payload: normalize(breadcrumb),
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use the normalizeDepth client option and set the max breadth to smth like 1_000 - which I find a sane number that will prevent an insane amount of data from being generated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think we may want to have a dedicated setting for the depth here eventually, as we may not need as much depth here as in other places, for example. But a good point, will think about it. Will also add 1_000 as max properties, sounds like a good additional check!

@mydea mydea merged commit 6e5cb41 into develop Apr 20, 2023
@mydea mydea deleted the fn/fix-replay-infinite-loop branch April 20, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants