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

Tracing finalTimeout blocks Angular Zone stabilisation #8983

Closed
3 tasks done
henry-alakazhang opened this issue Sep 8, 2023 · 6 comments
Closed
3 tasks done

Tracing finalTimeout blocks Angular Zone stabilisation #8983

henry-alakazhang opened this issue Sep 8, 2023 · 6 comments
Assignees
Labels

Comments

@henry-alakazhang
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/angular-ivy

SDK Version

7.57.0

Framework Version

Angular 15.2.0

Link to Sentry event

No response

SDK Setup

this.ngZone.runOutsideAngular(() => {
  const tracingIntegrations =
    tracesSampleRate !== undefined
      ? [
          new Sentry.BrowserTracing({
            tracePropagationTargets: [
              'localhost',
              this.httpConfig.baseUrl,
              /^\//,
            ],
          }),
        ]
      : [];

  Sentry.init({
    dsn: this.config.sentryDsn,
    release: "...",
    environment: this.environment,
    ignoreErrors: [
      // <snip>
    ],
    allowUrls: !this.pwa.isNative() ? [/\/assets\//] : undefined,
    denyUrls: [/^file:\/\//, /^moz-extension:\/\//],
    integrations: integrations =>
      // integrations will be all default integrations
      [
        ...integrations.filter(
          integration =>
            !['TryCatch', 'GlobalHandlers'].includes(integration.name),
        ),
        new SentryIntegrations.ExtraErrorData(),
        new SentryIntegrations.ReportingObserver(),
        ...tracingIntegrations,
      ],
    tracesSampler: context => {
      if (context.location?.search.includes('sentry-tracing=true')) {
        return 1;
      }
      return tracesSampleRate ?? 0;
    },
  });
});        

Steps to Reproduce

  1. Trigger a pageload or navigation operation

Expected Result

Angular's zone should stabilize and appRef.isStable/testability.whenStable should emit after network requests have finished and app is stable.

eg. code like this should log:

this.testability.whenStable(() => {
  console.log("App is stable.");
});

Actual Result

NgZone doesn't stabilise because of a pending timeout from finalTimeout. It only stabilises after 30 seconds (the default value of the finalTimeout), even if the idle timeout and heartbeat interval have already completed.

We utilise the stability API for a number of things (e2e tests, performance tracking) which have been affected by upgrading from Sentry 6 to 7. Our e2e tests against our dev environment take longer to run, and long task tracking is less accurate.

We've worked around it partially by running Sentry.init outside of the Angular zone (see snippet above) but this only affects the pageload event and not future navigations.

I've tested this using @sentry/browser, @sentry/angular and @sentry/angular-ivy and the behaviour is the same.

Thanks.

@github-actions github-actions bot added the Package: angular Issues related to the Sentry Angular SDK label Sep 8, 2023
@Lms24
Copy link
Member

Lms24 commented Sep 8, 2023

Hi @henry-alakazhang thanks for writing in!

The reason why we need this timeout (finalTimeout) is because our BrowserTracing integration starts IdleTransactions on navigations and pageloads that finish themselvse (as opposed to anyone calling transaction.finish() after a period of inactivity.

A question: Is there a reason why you're not using our Angular routing instrumentation as shown in our docs? Similarly, are you using TraceService? Both things are required to get proper transactions for pageloads and navigations in Angular.

I'm curious if using routing instrumentation and TraceService change anything w.r.t to this behaviour.

Otherwise, I don't think we can get rid of the finalTimeout from our idletransactions.

Do you think starting the idle transactions outside the NgZone would make a difference here?

@henry-alakazhang
Copy link
Author

Thanks for the response @Lms24.

A question: Is there a reason why you're not using our Angular routing instrumentation as shown in our docs? Similarly, are you using TraceService? Both things are required to get proper transactions for pageloads and navigations in Angular.

The Sentry.init I copied is from our current codebase which hasn't been migrated to @sentry/angular yet, but I did test swapping to @sentry/angular, including the routing instrumentation and TraceService, and it still had the same result. I'm pretty sure I set it up correctly as I was seeing the full navigation event in the transaction and the routes were pulled from the router correctly.

Do you think starting the idle transactions outside the NgZone would make a difference here?

Yeah, that'd be one way to fix the issue.

I'm curious though if there's a reason the finalTimeout continues to run even when the IdleTransaction is done. Before v7 there was still a max duration but it didn't seem to add a pending timer to the zone.

@Lms24
Copy link
Member

Lms24 commented Sep 8, 2023

I'm curious though if there's a reason the finalTimeout continues to run even when the IdleTransaction is done. Before v7 there was still a max duration but it didn't seem to add a pending timer to the zone.

That's an interesting observation, thanks! We'll take a look at this in greater detail next week. Maybe there's something we can fix more generally here; otherwise I'll go with running outside the ngzone.

@Lms24 Lms24 self-assigned this Sep 8, 2023
@AbhiPrasad
Copy link
Member

Before v7 there was still a max duration but it didn't seem to add a pending timer to the zone

Yeah this was done on purpose because we didn't want there to be a infinitely running transaction, so we used a timeout to hard cut it off.

I wonder if we should use appRef.isStable to end transactions in angular instead of relying on timeouts 🤔. We can then create regular transactions instead of idle ones.

@arturovt
Copy link
Contributor

Considering the changes made in #11703 and #11748 to the Angular tracer and error handler, it should now be possible to fix all of the issues you may have been facing previously. All of the Sentry functionality is now running outside of the Angular zone, meaning Angular is unaware of any tasks being scheduled within its context. As a result, server-side rendering, hydration, and early stabilization would now only rely on tasks being scheduled explicitly by the consumer.

If you're still facing any issues, I can guide you through the process of identifying which tasks are preventing the application from becoming stable and determining if Sentry is the cause of them.

@Lms24
Copy link
Member

Lms24 commented Jul 5, 2024

I'm gonna close this issue as to the best of my knowledge this was addressed in the PRs mentioned in the comment above. Please let me know if it should be reopened.

@Lms24 Lms24 closed this as completed Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Archived in project
Development

No branches or pull requests

5 participants