Angular change detection problems in Replay

billyvg commented 2 weeks ago

Angular monkeypatches browser APIs so that when they are called, will trigger Angular's change detection. This means that our SDK can end up causing customer applications to unnecessarily re-render, which in turn causes our Replay SDK to perform more work and can even cause performance regressions.

Some example areas where this happens:

In the rrweb-snapshot package, snapshot.ts is using the global setTimeout and clearTimeout functions, which are monkeypatched by Angular and triggers their change detection.
Promise
Any EventTarget (e.g. window, Performance, Worker)

billyvg commented 5 days ago

This should help on the rrweb side of things: https://github.com/rrweb-io/rrweb/pull/1509/files

billyvg commented 4 days ago

I think there are a few potential solutions:

Continue using `getNativeImplementation()`

This has slight overhead for all browser packages as we need to check if the fn we're using is a native fn. There is also a slight hit as we have to use DOM APIs to create an iframe to get the native implementation (this happens per first function call, so we could try to pre-load this in the Angular SDK as an optimization).

Run outside of Angular Zone

I'm not super familiar with Angular, but I think this could work if new async calls made outside of the zone continue to be outside. Then we could have the Angular SDK wrap browser/init() with runOutsideAngular(). However, this wouldn't cover the case where users call Sentry APIs directly, as well as other edge cases. It will be difficult to target specific methods to override from the Angular SDK.

Set APIs at run-time

Another option would be having the Angular SDK set the APIs at run-time before we initialize replayIntegration. This could mean we could avoid calling getNativeImplementation for all non-angular SDKs at the expense of always calling a getter (as well as some developer ergonomics). This would look something like the following:

1) a setter function to set a map of the clean APIs. This would be stored at the module level. By default this will use the current global APIs, but Angular would call it with the cleaned versions. e.g.

setApis({
  setTimeout: getNativeImplementation('setTimeout'),
  ...
});

2) an exported map of getters. Otherwise, the imports would reference the value of the export at the time of import, rather than when the exported property is accessed.

export const apis = {
  get setTimeout() { return _foo.setTimeout; }
  ...
};

...

import {apis} from '@sentry-internal/browser-utils';

apis.setTimeout(() => ...);

All three solutions are easy to regress as it's normal for a developer to call the globals directly, but this can likely be mitigated by a linter.

cc @Lms24 when you're back as you're probably the only one with any Angular experience :P

getsentry / sentry-javascript