-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Detect huge MutationObserver triggers #55
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
| const observer = new mutationObserverCtor( | ||
| callbackWrapper(mutationBuffer.processMutations.bind(mutationBuffer)), | ||
| callbackWrapper((mutations) => { | ||
| if (mutations.length > 500) { |
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.
Note: 500 is completely arbitrary here as of now. We've seen issues with this being > 2000, but hard to say what makes most sense here.
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.
- Should we make this a configurable option instead?
- I wonder if we can setup a custom repro for this easily and test some different breakpoints. I'd opt for something higher if we are just blindly adding this in
- Should we add a callback here to give control back to SDK? This way we could potentially add metrics in our SDK. I would want the callback separate from the length check as we probably want to avoid calling the cb too much.
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.
Yeah, we could add this as a config option to record I guess 🤔 It's really hard to test this though, I tried with a custom app that adds 2k items at once and it seemed to handle it fine, so you really need something where a lot is going on, apparently, or goes on in a specific way.
What about: We start creating a breadcrumb (type other) for this when we discover "a lot" of records, e.g.:
if (mutations.length > 500) {
emit({
type: 5 // custom
timestamp: now,
data: {
tag: 'breadcrumb',
payload: { type: 'metrics', data: { mutationObserverCount: mutations.length } },
},
})Something along these lines...?
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.
What about: We set it to 1000 for the time being and add an _experiments option in the Replay options to configure the value. Then we go back to getsentry/sentry-javascript#6946 and asks folks to play around with the number if the default doesn't work for them. If we get to a number that works for most people, we can still remove the experiments option or promote it to an actual option if otherwise necessary.
I like the emit-breadcrumb approach as it would at least let us know in the SDK that this happened and we could still decide on how we handle it. Client reports would be great ... if we could actually use them for something else than dropped events, which right now we can't 😅.
The other option would be event.contexts.replay but we still don't have a way to query that data properly other than looking at individual events. This might be fine though if there's a way to look at replay events from a customer's replay.
Regardless, a console logger output would definitely be the first step so that we could ask users to set debug: true and report if they see this message (including the actual number of mutations).
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.
Or, a variation of this:
- We make this configurable in rrweb, defaulting to 0 === never do this.
- Add an _experiments option in replay to allow to overwrite this
- Tell users that have this problem to play around with this value
On the other hand, the downside is that users probably don't really know what to set this to, and may just set it to something small to get it work 🤔
Or, we allow to pass an optional function in that returns true/false, then we can write the logging code in replay (which is probably better suited for this than rrweb). I'll give it a try!
1228005 to
0052649
Compare
Lms24
left a comment
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.
I think this is worth a try.
|
Let's close this in favor of #58 |
This is another stab at getsentry/sentry-javascript#7211.
Here, we actually check if a mutation observer triggers with a massive amount of changes at once.
If that happens, we instead trigger a full snapshot.
I've tested this in our reproduction, and it seemed to drastically improve perf. (Still slightly noticeable, but it doesn't freeze up anymore).
See https://sentry-sdks.sentry.io/replays/javascript-vue-replays:fa48453747814b5cb67cd7b4e68881da/?query=&referrer=%2Freplays%2F&statsPeriod=14d&yAxis=count%28%29 for a recording with this.