diff --git a/dev-packages/e2e-tests/test-applications/angular-17/package.json b/dev-packages/e2e-tests/test-applications/angular-17/package.json index c81a2d19c8c4..75ccc8f3e091 100644 --- a/dev-packages/e2e-tests/test-applications/angular-17/package.json +++ b/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 || *" + } } } diff --git a/dev-packages/e2e-tests/test-applications/angular-17/playwright.config.ts b/dev-packages/e2e-tests/test-applications/angular-17/playwright.config.ts index 967aad98df5e..3b0d25b80d09 100644 --- a/dev-packages/e2e-tests/test-applications/angular-17/playwright.config.ts +++ b/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 */ diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/app.routes.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/app.routes.ts index 0b44bc341a9b..d4d9a04f6c99 100644 --- a/dev-packages/e2e-tests/test-applications/angular-17/src/app/app.routes.ts +++ b/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', diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts index 58a375be1a2d..9179f5d79638 100644 --- a/dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts +++ b/dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts @@ -10,6 +10,7 @@ import { RouterLink } from '@angular/router';

Welcome to Sentry's Angular 17 E2E test app

diff --git a/dev-packages/e2e-tests/test-applications/angular-17/tests/performance.test.ts b/dev-packages/e2e-tests/test-applications/angular-17/tests/performance.test.ts index 8cda20ec3853..9b5f1b08e9d2 100644 --- a/dev-packages/e2e-tests/test-applications/angular-17/tests/performance.test.ts +++ b/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'); +}); diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 43c6227b0f8e..bbfe3afd3b67 100644 --- a/packages/angular/src/tracing.ts +++ b/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';