Skip to content

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.

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

Closed
wants to merge 2 commits into from
Closed

feat: Detect huge MutationObserver triggers #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f97b4a8
feat: Detect huge mutation observers
mydea Feb 20, 2023
0052649
add test
mydea Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/rrweb/src/record/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ function record<T = eventWithTime>(
mutationCb: wrappedMutationEmit,
scrollCb: wrappedScrollEmit,
bypassOptions: {
takeFullSnapshot,
blockClass,
blockSelector,
unblockSelector,
Expand Down Expand Up @@ -351,6 +352,7 @@ function record<T = eventWithTime>(
const observe = (doc: Document) => {
return callbackWrapper(initObservers)(
{
takeFullSnapshot,
mutationCb: wrappedMutationEmit,
mousemoveCb: (positions, source) =>
wrappedEmit(
Expand Down
12 changes: 11 additions & 1 deletion packages/rrweb/src/record/observer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,17 @@ export function initMutationObserver(
}

const observer = new mutationObserverCtor(
callbackWrapper(mutationBuffer.processMutations.bind(mutationBuffer)),
callbackWrapper((mutations) => {
if (mutations.length > 500) {
Copy link
Member Author

@mydea mydea Feb 21, 2023

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.

Copy link
Member

@billyvg billyvg Feb 21, 2023

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.

Copy link
Member Author

@mydea mydea Feb 22, 2023

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...?

Copy link
Member

@Lms24 Lms24 Feb 22, 2023

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).

Copy link
Member Author

@mydea mydea Feb 22, 2023

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:

  1. We make this configurable in rrweb, defaulting to 0 === never do this.
  2. Add an _experiments option in replay to allow to overwrite this
  3. 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!

// too many mutations, just do a full snapshot
// There are scenarios where this triggers with e.g. thousands of mutations
// This can lead to the browser freezing up
// In such a case, it is more efficient for the browser to just do a full snapshot instead
options.takeFullSnapshot(false);
return;
}
mutationBuffer.processMutations(mutations);
}),
);

observer.observe(rootEl, {
Expand Down
2 changes: 2 additions & 0 deletions packages/rrweb/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ export type recordOptions<T> = {
};

export type observerParam = {
takeFullSnapshot: (isCheckout?: boolean | undefined) => void;
mutationCb: mutationCallBack;
mousemoveCb: mousemoveCallBack;
mouseInteractionCb: mouseInteractionCallBack;
Expand Down Expand Up @@ -293,6 +294,7 @@ export type observerParam = {

export type MutationBufferParam = Pick<
observerParam,
| 'takeFullSnapshot'
| 'mutationCb'
| 'blockClass'
| 'blockSelector'
Expand Down
Loading