fix(angular): Ensure navigations always create a transaction (#10646)

This PR addresses the problematic check of `getActiveSpan()` in our
Angular SDK's `TraceService` raised in #10634.

In certain situations (e.g. very quick navigations after the initial
pageload) the check of `getActiveSpan()` could lead to navigations not
getting their own transaction, as the pageload transaction was still
active at that time.

ref #10634

Author: Lms24

dev-packages/e2e-tests/test-applications/angular-17/package.json

@@ -4,7 +4,8 @@
   "scripts": {
     "ng": "ng",
     "dev": "ng serve",
-    "preview": "http-server dist/angular-17/browser",
+    "proxy": "ts-node-script start-event-proxy.ts",
+    "preview": "http-server dist/angular-17/browser --port 8080",
     "build": "ng build",
     "watch": "ng build --watch --configuration development",
     "test": "playwright test",
@@ -22,6 +23,7 @@
     "@angular/platform-browser": "^17.1.0",
     "@angular/platform-browser-dynamic": "^17.1.0",
     "@angular/router": "^17.1.0",
+    "@sentry/angular-ivy": "* || latest",
     "rxjs": "~7.8.0",
     "tslib": "^2.3.0",
     "zone.js": "~0.14.3"
@@ -31,7 +33,6 @@
     "@angular/cli": "^17.1.1",
     "@angular/compiler-cli": "^17.1.0",
     "@playwright/test": "^1.41.1",
-    "@sentry/angular-ivy": "latest || *",
     "@types/jasmine": "~5.1.0",
     "http-server": "^14.1.1",
     "jasmine-core": "~5.1.0",
@@ -40,11 +41,18 @@
     "karma-coverage": "~2.2.0",
     "karma-jasmine": "~5.1.0",
     "karma-jasmine-html-reporter": "~2.1.0",
-    "typescript": "~5.3.2",
     "ts-node": "10.9.1",
+    "typescript": "~5.3.2",
     "wait-port": "1.0.4"
   },
   "volta": {
     "extends": "../../package.json"
+  },
+  "pnpm": {
+    "overrides": {
+      "@sentry/browser": "latest || *",
+      "@sentry/core": "latest || *",
+      "@sentry/utils": "latest || *"
+    }
   }
 }

dev-packages/e2e-tests/test-applications/angular-17/playwright.config.ts

@@ -29,8 +29,8 @@ const config: PlaywrightTestConfig = {
      */
     timeout: 10000,
   },
-  /* Run tests in files in parallel */
-  fullyParallel: true,
+  fullyParallel: false,
+  workers: 1,
   /* Fail the build on CI if you accidentally left test.only in the source code. */
   forbidOnly: !!process.env.CI,
   /* `next dev` is incredibly buggy with the app dir */

dev-packages/e2e-tests/test-applications/angular-17/src/app/app.routes.ts

@@ -11,6 +11,18 @@ export const routes: Routes = [
     path: 'home',
     component: HomeComponent,
   },
+  {
+    path: 'redirect1',
+    redirectTo: '/redirect2',
+  },
+  {
+    path: 'redirect2',
+    redirectTo: '/redirect3',
+  },
+  {
+    path: 'redirect3',
+    redirectTo: '/users/456',
+  },
   {
     path: '**',
     redirectTo: 'home',

dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts

@@ -10,6 +10,7 @@ import { RouterLink } from '@angular/router';
     <h1>Welcome to Sentry's Angular 17 E2E test app</h1>
     <ul>
       <li> <a id="navLink" [routerLink]="['/users', '123']">Visit User 123</a> </li>
+      <li> <a id="redirectLink" [routerLink]="['/redirect1']">Redirect</a> </li>
     </ul>
     <button id="errorBtn" (click)="throwError()">Throw error</button>
   </main>

dev-packages/e2e-tests/test-applications/angular-17/tests/performance.test.ts

@@ -52,3 +52,77 @@ test('sends a navigation transaction with a parameterized URL', async ({ page })
     },
   });
 });
+
+test('sends a navigation transaction even if the pageload span is still active', async ({ page }) => {
+  const pageloadTxnPromise = waitForTransaction('angular-17', async transactionEvent => {
+    return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
+  });
+
+  const navigationTxnPromise = waitForTransaction('angular-17', async transactionEvent => {
+    return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation';
+  });
+
+  await page.goto(`/`);
+
+  // immediately navigate to a different route
+  const [_, pageloadTxn, navigationTxn] = await Promise.all([
+    page.locator('#navLink').click(),
+    pageloadTxnPromise,
+    navigationTxnPromise,
+  ]);
+
+  expect(pageloadTxn).toMatchObject({
+    contexts: {
+      trace: {
+        op: 'pageload',
+        origin: 'auto.pageload.angular',
+      },
+    },
+    transaction: '/home/',
+    transaction_info: {
+      source: 'route',
+    },
+  });
+
+  expect(navigationTxn).toMatchObject({
+    contexts: {
+      trace: {
+        op: 'navigation',
+        origin: 'auto.navigation.angular',
+      },
+    },
+    transaction: '/users/:id/',
+    transaction_info: {
+      source: 'route',
+    },
+  });
+});
+
+test('groups redirects within one navigation root span', async ({ page }) => {
+  const navigationTxnPromise = waitForTransaction('angular-17', async transactionEvent => {
+    return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation';
+  });
+
+  await page.goto(`/`);
+
+  // immediately navigate to a different route
+  const [_, navigationTxn] = await Promise.all([page.locator('#redirectLink').click(), navigationTxnPromise]);
+
+  expect(navigationTxn).toMatchObject({
+    contexts: {
+      trace: {
+        op: 'navigation',
+        origin: 'auto.navigation.angular',
+      },
+    },
+    transaction: '/users/:id/',
+    transaction_info: {
+      source: 'route',
+    },
+  });
+
+  const routingSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.routing');
+
+  expect(routingSpan).toBeDefined();
+  expect(routingSpan?.description).toBe('/redirect1');
+});

packages/angular/src/tracing.ts

@@ -8,19 +8,14 @@ import type { ActivatedRouteSnapshot, Event, RouterState } from '@angular/router
 import { NavigationCancel, NavigationError, Router } from '@angular/router';
 import { NavigationEnd, NavigationStart, ResolveEnd } from '@angular/router';
 import {
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
   WINDOW,
   browserTracingIntegration as originalBrowserTracingIntegration,
   getCurrentScope,
   startBrowserTracingNavigationSpan,
 } from '@sentry/browser';
-import {
-  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
-  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
-  getActiveSpan,
-  getClient,
-  spanToJSON,
-  startInactiveSpan,
-} from '@sentry/core';
+import { getActiveSpan, getClient, getRootSpan, spanToJSON, startInactiveSpan } from '@sentry/core';
 import type { Integration, Span, Transaction, TransactionContext } from '@sentry/types';
 import { logger, stripUrlQueryAndFragment, timestampInSeconds } from '@sentry/utils';
 import type { Observable } from 'rxjs';
@@ -126,24 +121,31 @@ export class TraceService implements OnDestroy {
       const strippedUrl = stripUrlQueryAndFragment(navigationEvent.url);
 
       if (client && hooksBasedInstrumentation) {
-        if (!getActiveSpan()) {
+        // see comment in `_isPageloadOngoing` for rationale
+        if (!this._isPageloadOngoing()) {
           startBrowserTracingNavigationSpan(client, {
             name: strippedUrl,
-            origin: 'auto.navigation.angular',
             attributes: {
+              [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.angular',
               [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
             },
           });
+        } else {
+          // The first time we end up here, we set the pageload flag to false
+          // Subsequent navigations are going to get their own navigation root span
+          // even if the pageload root span is still ongoing.
+          this._pageloadOngoing = false;
         }
 
-        // eslint-disable-next-line deprecation/deprecation
         this._routingSpan =
           startInactiveSpan({
             name: `${navigationEvent.url}`,
             op: ANGULAR_ROUTING_OP,
-            origin: 'auto.ui.angular',
+            attributes: {
+              [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular',
+              [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+            },
             tags: {
-              'routing.instrumentation': '@sentry/angular',
               url: strippedUrl,
               ...(navigationEvent.navigationTrigger && {
                 navigationTrigger: navigationEvent.navigationTrigger,
@@ -232,8 +234,15 @@ export class TraceService implements OnDestroy {
 
   private _subscription: Subscription;
 
+  /**
+   * @see _isPageloadOngoing()
+   */
+  private _pageloadOngoing: boolean;
+
   public constructor(private readonly _router: Router) {
     this._routingSpan = null;
+    this._pageloadOngoing = true;
+
     this._subscription = new Subscription();
 
     this._subscription.add(this.navStart$.subscribe());
@@ -248,6 +257,49 @@ export class TraceService implements OnDestroy {
   public ngOnDestroy(): void {
     this._subscription.unsubscribe();
   }
+
+  /**
+   * We only _avoid_ creating a navigation root span in one case:
+   *
+   * There is an ongoing pageload span AND the router didn't yet emit the first navigation start event
+   *
+   * The first navigation start event will create the child routing span
+   * and update the pageload root span name on ResolveEnd.
+   *
+   * There's an edge case we need to avoid here: If the router fires the first navigation start event
+   * _after_ the pageload root span finished. This is why we check for the pageload root span.
+   * Possible real-world scenario: Angular application and/or router is bootstrapped after the pageload
+   * idle root span finished
+   *
+   * The overall rationale is:
+   * - if we already avoided creating a navigation root span once, we don't avoid it again
+   *   (i.e. set `_pageloadOngoing` to `false`)
+   * - if `_pageloadOngoing` is already `false`, create a navigation root span
+   * - if there's no active/pageload root span, create a navigation root span
+   * - only if there's an ongoing pageload root span AND `_pageloadOngoing` is still `true,
+   *   con't create a navigation root span
+   */
+  private _isPageloadOngoing(): boolean {
+    if (!this._pageloadOngoing) {
+      // pageload is already finished, no need to update
+      return false;
+    }
+
+    const activeSpan = getActiveSpan();
+    if (!activeSpan) {
+      this._pageloadOngoing = false;
+      return false;
+    }
+
+    const rootSpan = getRootSpan(activeSpan);
+    if (!rootSpan) {
+      this._pageloadOngoing = false;
+      return false;
+    }
+
+    this._pageloadOngoing = spanToJSON(rootSpan).op === 'pageload';
+    return this._pageloadOngoing;
+  }
 }
 
 const UNKNOWN_COMPONENT = 'unknown';