-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
size-limit report 📦
|
c6a5370
to
21da1b5
Compare
@@ -27,7 +27,7 @@ export function addBreadcrumbEvent(replay: ReplayContainer, breadcrumb: Breadcru | |||
timestamp: (breadcrumb.timestamp || 0) * 1000, | |||
data: { | |||
tag: 'breadcrumb', | |||
payload: normalize(breadcrumb), |
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.
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.
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.
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!
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:
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