I wonder how frequent the mutation observer fires - we may want to split this into 2 configurations so that we're not calling a function for every mutation callback and only call it when it hits the mutation limit.

I think we should also only call the callback when limit is hit and not perform any side-effects, let the callback define the behavior when limit is hit, and then use the return value of the callback to determine if we should continue to process mutations or not.

I see the API as something like this:

record({
 maxMutations: 1000,
 onMaxMutations: (mutations) => {
   createBreadcrumb(...);
   record.takeFullSnapshot(false);
   return true; // skip processing mutations
 },
});

(ugh, API would be cleaner with just a callback though)

I see what you mean, but I think the impact wouldn't really be different, would it? As at run time you'd either do:

// How expensive this is depends on what `myCallback` does
// if you just do `mutations.length > 500` it should be pretty cheap
if( myCallback()) {
  record.takeFullSnapshot();
  return;
}

vs.

if (mutations.length > maxMutations) {
  const res = onMaxMutations();
  if(res) {
    return;
  }
}

--> as long as triggerFullSnapshotOnMutation is just (mutations) => mutations.length > 500, IMHO the frist will be "cheaper" or basically the same. Of course, if we put more logic in there, not necessarily 😅

Overall, considering how many things are happening in this mutation handler in rrweb already, I think whatever we're adding here is probably rather insignificant (e.g. one or two checks vs. iterating over 10s or thousands of mutations etc).

Mostly I try to think about this in the way of "what is most likely to be accepted if we upstream this", and honestly I'm not quite sure 😅 I think the callback only adds a smaller API surface, and is a bit more generic. What you suggested can maybe be a bit better optimized. 🤔

The difference is the extra function call, which is likely neglible and why I'm wondering generally how often the mutation observer callback gets called. But since by default callback will be undefined, this shouldn't matter for most users. And for us, like you said, this will not matter compared to the rest of rrweb. So let's do the onMutation callback approach imo

👍 so we're good to go with this PR from your perspective?

@mydea I think we should only add a generic callback and not have rrweb trigger fullsnapshot (let the SDK decide that).

ah, ok, I see - so just a callback to opt out of processing the mutations overall. I'll adjust this!