Skip to content
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

Replay is making my Angular app super slow #6946

Closed
3 tasks done
mebibou opened this issue Jan 26, 2023 · 38 comments · Fixed by #10072
Closed
3 tasks done

Replay is making my Angular app super slow #6946

mebibou opened this issue Jan 26, 2023 · 38 comments · Fixed by #10072
Labels
Package: angular Issues related to the Sentry Angular SDK Package: replay Issues related to the Sentry Replay SDK

Comments

@mebibou
Copy link

mebibou commented Jan 26, 2023

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/angular

SDK Version

7.31.1

Framework Version

Angular 14.2.8

Link to Sentry event

No response

SDK Setup

Sentry.init({
  ...environment.sentryApi,
  replaysSessionSampleRate: 0.5,
  replaysOnErrorSampleRate: 1.0,
  integrations: [
    new Sentry.Replay(),
    new Sentry.Integrations.TryCatch({
      XMLHttpRequest: false
    }),
    new BrowserTracing({
      tracingOrigins: ['apiUrl']
    })
  ],
  tracesSampleRate: 0.5
});

Steps to Reproduce

Add the Replay integration to the Angular app. Without it the website is smooth, as soon as I add it it gets super slow continuously

Expected Result

Add Replay shouldn't impact performance so much, or it's basically becomes unusable

Actual Result

Everything is slow

@Lms24
Copy link
Member

Lms24 commented Jan 26, 2023

Hi @mebibou, thanks for writing in! I'm afraid, Replay is always going to add overhead to your app, as rrweb (the underlying library we use to record everything that's happening) just needs to listen and react to so many events, DOM mutations, style changes, etc.

We're right now working on performance overhead measurements, to better understand the overall impact. For instance, #6611 introduced the first version of them. We're still trying to improve here, as we've noticed that results seem to fluctuate and depend a lot on the individual apps.

It's interesting that the impact is so noticeable in your app. Would you mind sharing a little information about it? For instance:

  • Do you have a lot of frequent DOM mutations? Or a bunch of animations?
  • How do users interact with your app? E.g. a lot of input/clicking vs. more reading
  • How complex is your app? For instance, in terms of number of components or bundle size
  • Any chance that you're running into very frequent (maybe unintentional) component updates? We've observerd that high-frequency DOM updates have significant impact on Replay playback performance. My assumption is that the same thing holds true for recording.
  • Did you perform any measurements? e.g. in terms of CPU or web vitals (e.g. LCP) difference of your plain app vs. your app with Replay recording?

Lastly, I guess the thin you can try at the moment to mitigate the overhead would be to lower the sample rates of errors and sessions. If a session is not sampled (both entire sessions and errors), our integration doesn't do anything and there should be no noticable performance impact.

Hope this helps a little. LMK if you have more questions :)

@Lms24 Lms24 added Status: Needs Information Package: replay Issues related to the Sentry Replay SDK and removed Type: Bug Status: Untriaged labels Jan 26, 2023
@mebibou
Copy link
Author

mebibou commented Jan 26, 2023

@Lms24 thanks for the reply

Our app does have a lot of interaction, and one part of it where the lag is very noticeable is a place where we have a dynamic form, i.e. the user can add more components, add some text, sometimes clicking on a button will open a dialogue, etc. so based on ReactiveForms.

But even on simple pages where we just display a list of search results, the results took about 2-5 seconds to display compared to without the Replay you would barely see the loader.

I didn't really perform any measurements as users started to complain the website was too slow to use so I simply disabled it. I don't think sampling would help, as it would still fall randomly on some users to have a horrible experience?

An idea that I had though: is there any way to enable the integration, but have some way to turn it on/off at runtime? I am thinking in cases where the user has a bug, and it could "turn on recording" (with a disclaimer that everything will be slower when recording is on)?

@Lms24
Copy link
Member

Lms24 commented Jan 26, 2023

Our app does have a lot of interaction, and one part of it where the lag is very noticeable is a place where we have a dynamic form, i.e. the user can add more components, add some text, sometimes clicking on a button will open a dialogue, etc. so based on ReactiveForms.

A wild guess here would be that maybe some validation logic is constantly updating the UI after an input? It's been a while since I played with Angular Reactive Forms but maybe the debounceTime operator for observing user input helps?

An idea that I had though: is there any way to enable the integration, but have some way to turn it on/off at runtime? I am thinking in cases where the user has a bug, and it could "turn on recording" (with a disclaimer that everything will be slower when recording is on)?

Yes, you can start and stop recording manually. For instance, it might be possible to stop recording on these more complex pages.

Furthermore, only recording when errors happen is supported ootb by the Replay integration:

  replaysSessionSampleRate: 0,
  replaysOnErrorSampleRate: 1.0, // or a smaller value, depending on what you perefer:

Note though, at the beginning of a session, we make a sampling decision based on the sample rates. If the session is sampled for errors, we'll still record all the time but only keep the last 60s of recording in the buffer. Hence, performance degredation is probably still noticeable.

@mebibou
Copy link
Author

mebibou commented Jan 26, 2023

sorry missed that section in the readme, I will have a play with it when I can spend more time on this.

For instance, it might be possible to stop recording on these more complex pages.

Yes but unfortunately, the more complex the page, the more the need to record the sessions!

We do use debounceTime and other performance related tricks, but when you've got hundreds of inputs and buttons etc on a page, eventually the memory usage increases. I imagined this would impact performances, but not by that much (most of the time the page was unresponsive after a few minutes)

@billyvg
Copy link
Member

billyvg commented Jan 26, 2023

@mebibou do you have a publicly accessible URL where this happens that we can use to test ourselves? I'm curious if it's form input related and if ignoring events on the inputs would help perf.

@Lms24
Copy link
Member

Lms24 commented Jan 26, 2023

One more question: What kind of change detection strategy are you using?

@woodchuck
Copy link

We use Angular Material. In a tab group you can wrap a tab with matTabContent to prevent it from initializing until you actually click on the tab. For tabs where we've done that, it appears Sentry replay causes it to hang badly. For example, the initial Audits tab here loads fine, clicking to Pending MR Review works fine, but clicking Incomplete/Approval needed hangs to the point the app is unusable. This worked fine before enabling replay, and adding/removing matTabContent toggles the problem on and off.

<mat-tab-group (selectedTabChange)="onTabChanged($event);">
  <mat-tab label="Audits">
    <app-audit-list [dateRange]=dateRange></app-audit-list>
  </mat-tab>
  <mat-tab label="Pending MR Review">
      <app-result-list [data]="mrPending"></app-result-list>
  </mat-tab>
  <mat-tab label="Incomplete/Approval Needed">
    <ng-template matTabContent>
      <app-result-list [data]="mrIncomplete"></app-result-list>
    </ng-template>
  </mat-tab>
</mat-tab-group>

We are using the default change detection strategy.

I'd also like to say replay is amazing. It's been a long time since software has delighted me this much. Kudos!

@mebibou
Copy link
Author

mebibou commented Jan 31, 2023

One more question: What kind of change detection strategy are you using?

We use the default in most places. We tried to use "OnPush" but we had many problems with components no re-rendering anything and as the application is quite complex, it would take too long to debug/rewrite everything so OnPush is applicable.
Is that something that doesn't bode well with Replay?

@Lms24
Copy link
Member

Lms24 commented Jan 31, 2023

Is that something that doesn't bode well with Replay?

Potentially, yes. Probably not the default CD strategy per sé but the combination of it with complex apps, is my best guess. Generally, because rrweb records every single DOM mutation, style change, etc, my guess is that the default CD strategy causes quite a lot of these (including unnecessary ones) which might slow things down.

@bruno-garcia bruno-garcia added the Package: angular Issues related to the Sentry Angular SDK label Feb 10, 2023
@Lms24 Lms24 self-assigned this Feb 13, 2023
@ginagr
Copy link

ginagr commented Feb 14, 2023

I'm seeing something similar in my Angular app where I'm using Bootstrap instead of Angular Material for my tabs. I have a tab that has a decently large table of data (~500+ rows) and when Replay is enabled and I scroll down/up quickly it completely breaks the UI (text/buttons/content randomly disappear) and the page won't respond to the point of chrome eventually crashing. When I turn replay off, the table works fine.

I've played around with the amount of rows that will break the UI and it looks like it starts to become unresponsive at around 200 rows

@jpike88
Copy link

jpike88 commented Mar 3, 2023

Replay is always going to add overhead to your app

Our angular app has dynamic forms and other complex UI/DOM stuff going on as well.

However, in the past we have used other replay-like products in the past including hotjar, logrocket not to mention a few more. This is the only one that is producing serious performance penalties to our angular app to the point of us having to disable it. So there is some sort of extra inefficiency built into it whether by design or not that is producing this overhead, and until such overhead is brought down to a manageable level we will not be able to use replay going forward.

@Lms24
Copy link
Member

Lms24 commented Mar 3, 2023

Hi @jpike88, I'm sorry that you're experiencing this performance impact. I understand that this is a serious problem for you and we're working on improving performance. Our running theory is that the slowdown is caused by a spike in DOM mutations within a very short time period. We'll soon be able to detect these spikes to confirm our theory. Angular's default change detection might contribute to this spike, given that it is likely to update DOM elements that necessarily don't need to be updated.

We'll keep you posted on further improvements and developments in this area :)

@github-actions
Copy link
Contributor

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry
Copy link

getsantry bot commented Dec 12, 2023

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Dec 12, 2023
@getsantry getsantry bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2023
@duncan-c
Copy link

duncan-c commented Jan 3, 2024

I have also been seeing this issue as I've been integrating Sentry into our Angular app. I've done some digging into it and I believe I've found the main cause of the performance hit (at least in our case).

It appears that rrweb is running a call to requestAnimationFrame on a loop (in a method called periodicallyClear), and in Angular requestAnimationFrame is patched so it triggers a change detection whenever it is invoked. This means change detections are happening repeatedly, which slows the app down massively.

I've managed to restore the performance of our app by disabling the patching of requestAnimationFrame, and I've shared the results below. But ideally the rrweb library should be using the non-patched version of RAF (as well as other patched methods like setTimeout, etc) to avoid unnecessary change detection.


Here's a screenshot of a profile from our app, when it's just sitting idly on a simple page for 4 seconds:

image

And here's a profile of the exact same page sitting for 4 seconds, but with the requestAnimationFrame NgZone patch disabled:

image


@mydea
Copy link
Member

mydea commented Jan 4, 2024

Hey,
thank you for the investigative work! That sounds like it could very well be the culprit.

For reference, could you share how you disabled this patching in your app?
Now, I wonder if there is a way to get the "unpatched" requestAnimationFrame method 🤔 I fear we are not gonna be able to put angular-specific code everywhere in rrweb to avoid this 🤔 Do you by chance know if there is a way to access the unpatched version? If so, we could think about adding a way to pass a "custom" requestAnimationFrame to rrweb to use instead of the global one, I guess 🤔

@mydea mydea reopened this Jan 4, 2024
@mydea mydea removed the Stale label Jan 4, 2024
mydea added a commit to getsentry/rrweb that referenced this issue Jan 4, 2024
As explained here: getsentry/sentry-javascript#6946 (comment) the usage of `requestAnimationFrame` can lead to issues when working with Zone.js.
@mydea
Copy link
Member

mydea commented Jan 4, 2024

I opened a PR to hopefully fix this on rrweb side: getsentry/rrweb#150

mydea added a commit to getsentry/rrweb that referenced this issue Jan 4, 2024
As explained here:
getsentry/sentry-javascript#6946 (comment)
the usage of `requestAnimationFrame` can lead to issues when working
with Zone.js.

With this fix, where possible we should now use the
`requestAnimationFrame` implementation from an iframe, if possible, and
else fall back on just using `window.requestAnimationFrame` as before.

We already do something similar in sentry-javascript to get an unpatched
`fetch`:
https://github.com/getsentry/sentry-javascript/blob/23ef22b115c8868861896cc9003bd4bb6afb0690/packages/browser/src/transports/utils.ts#L65-L71
so this should hopefully work fine!
@duncan-c
Copy link

duncan-c commented Jan 5, 2024

Great work with the fix @mydea!

Sorry I didn't reply in time. The way I disabled it was to globally disable the monkey-patching of the function by Angular, because we didn't need change detection to automatically run whenever RAF runs - but obviously that's not a feasible solution for many people.

The iframe technique is clever, and will also mean that it'll work for other libraries that override the native implementation 💪

Sadly the performance is still not great on our application, but I did realise that we were using an outdated version of the JS SDK because we use the Capacitor plugin which is locked to v7.81 - and when I manually overrode this to get the latest version things did improve a bit, because of the rrweb improvements.
We're seeing a lot of mutation limit warnings in the breadcrumbs - so I guess the main issue is that Angular is pretty heavy on the dom mutations, unless you're really careful with change detection, etc.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Jan 5, 2024
@mydea
Copy link
Member

mydea commented Jan 5, 2024

Thanks for the info! yeah, updating to a more recent version of replay should improve performance a bit. we will work on further improvements down the line too! But it will be great to unblock this angular specific issue here 🙏

@getsantry getsantry bot removed the status in GitHub Issues with 👀 2 Jan 5, 2024
mydea added a commit that referenced this issue Jan 10, 2024
This bump contains the following changes:

- fix(rrweb): Use unpatched requestAnimationFrame when possible
[#150](getsentry/rrweb#150)
- ref: Avoid async in canvas
[#143](getsentry/rrweb#143)
- feat: Bundle canvas worker manually
[#144](getsentry/rrweb#144)
- build: Build for ES2020
[#142](getsentry/rrweb#142)

Extracted out from
#9826

Closes #6946
@AbhiPrasad
Copy link
Member

The fix for this was released with https://github.com/getsentry/sentry-javascript/releases/tag/7.93.0 - please give it a try!

bruno-garcia added a commit to getsentry/sentry-docs that referenced this issue Jan 10, 2024
bruno-garcia added a commit to getsentry/sentry-docs that referenced this issue Jan 10, 2024
billyvg pushed a commit to getsentry/rrweb that referenced this issue Apr 26, 2024
As explained here:
getsentry/sentry-javascript#6946 (comment)
the usage of `requestAnimationFrame` can lead to issues when working
with Zone.js.

With this fix, where possible we should now use the
`requestAnimationFrame` implementation from an iframe, if possible, and
else fall back on just using `window.requestAnimationFrame` as before.

We already do something similar in sentry-javascript to get an unpatched
`fetch`:
https://github.com/getsentry/sentry-javascript/blob/23ef22b115c8868861896cc9003bd4bb6afb0690/packages/browser/src/transports/utils.ts#L65-L71
so this should hopefully work fine!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: angular Issues related to the Sentry Angular SDK Package: replay Issues related to the Sentry Replay SDK
Projects
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.