ref(v8/angular): Remove instrumentAngularRouting and fix tests (#11021)

ref #10898 ref #10353
getsentry · Mar 12, 2024 · 2274b27 · 2274b27
1 parent 0be633a
commit 2274b27
Show file tree

Hide file tree

Showing 11 changed files with 302 additions and 656 deletions.
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
@@ -1,4 +1,7 @@
 import { Routes } from '@angular/router';
+import { cancelGuard } from './cancel-guard.guard';
+import { CancelComponent } from './cancel/cancel.components';
+import { ComponentTrackingComponent } from './component-tracking/component-tracking.components';
 import { HomeComponent } from './home/home.component';
 import { UserComponent } from './user/user.component';
 
@@ -11,6 +14,15 @@ export const routes: Routes = [
     path: 'home',
     component: HomeComponent,
   },
+  {
+    path: 'cancel',
+    component: CancelComponent,
+    canActivate: [cancelGuard],
+  },
+  {
+    path: 'component-tracking',
+    component: ComponentTrackingComponent,
+  },
   {
     path: 'redirect1',
     redirectTo: '/redirect2',

diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel-guard.guard.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel-guard.guard.ts
@@ -0,0 +1,5 @@
+import { ActivatedRouteSnapshot, CanActivateFn, RouterStateSnapshot } from '@angular/router';
+
+export const cancelGuard: CanActivateFn = (_next: ActivatedRouteSnapshot, _state: RouterStateSnapshot) => {
+  return false;
+};
diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel/cancel.components.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel/cancel.components.ts
@@ -0,0 +1,8 @@
+import { Component } from '@angular/core';
+
+@Component({
+  selector: 'app-cancel',
+  standalone: true,
+  template: `<div></div>`,
+})
+export class CancelComponent {}
diff --git a/.../test-applications/angular-17/src/app/component-tracking/component-tracking.components.ts b/.../test-applications/angular-17/src/app/component-tracking/component-tracking.components.ts
@@ -0,0 +1,18 @@
+import { AfterViewInit, Component, OnInit } from '@angular/core';
+import { TraceClassDecorator, TraceMethodDecorator, TraceModule } from '@sentry/angular-ivy';
+import { SampleComponent } from '../sample-component/sample-component.components';
+
+@Component({
+  selector: 'app-cancel',
+  standalone: true,
+  imports: [TraceModule, SampleComponent],
+  template: `<app-sample-component [trace]="'sample-component'"></app-sample-component>`,
+})
+@TraceClassDecorator()
+export class ComponentTrackingComponent implements OnInit, AfterViewInit {
+  @TraceMethodDecorator()
+  ngOnInit() {}
+
+  @TraceMethodDecorator()
+  ngAfterViewInit() {}
+}
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
@@ -11,6 +11,9 @@ import { RouterLink } from '@angular/router';
     <ul>
       <li> <a id="navLink" [routerLink]="['/users', '123']">Visit User 123</a> </li>
       <li> <a id="redirectLink" [routerLink]="['/redirect1']">Redirect</a> </li>
+      <li> <a id="cancelLink" [routerLink]="['/cancel']">Cancel</a> </li>
+      <li> <a id="nonExistentLink" [routerLink]="['/non-existent']">Error</a> </li>
+      <li> <a id="componentTracking" [routerLink]="['/component-tracking']">Error</a> </li>
     </ul>
     <button id="errorBtn" (click)="throwError()">Throw error</button>
   </main>

diff --git a/...ests/test-applications/angular-17/src/app/sample-component/sample-component.components.ts b/...ests/test-applications/angular-17/src/app/sample-component/sample-component.components.ts
@@ -0,0 +1,12 @@
+import { Component, OnInit } from '@angular/core';
+
+@Component({
+  selector: 'app-sample-component',
+  standalone: true,
+  template: `<div></div>`,
+})
+export class SampleComponent implements OnInit {
+  ngOnInit() {
+    console.log('SampleComponent');
+  }
+}
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
@@ -1,4 +1,5 @@
 import { expect, test } from '@playwright/test';
+import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
 import { waitForTransaction } from '../event-proxy-server';
 
 test('sends a pageload transaction with a parameterized URL', async ({ page }) => {
@@ -126,3 +127,178 @@ test('groups redirects within one navigation root span', async ({ page }) => {
   expect(routingSpan).toBeDefined();
   expect(routingSpan?.description).toBe('/redirect1');
 });
+
+test.describe('finish routing span', () => {
+  test('finishes routing span on navigation cancel', 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('#cancelLink').click(), navigationTxnPromise]);
+
+    expect(navigationTxn).toMatchObject({
+      contexts: {
+        trace: {
+          op: 'navigation',
+          origin: 'auto.navigation.angular',
+        },
+      },
+      transaction: '/cancel',
+      transaction_info: {
+        source: 'url',
+      },
+    });
+
+    const routingSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.routing');
+
+    expect(routingSpan).toBeDefined();
+    expect(routingSpan?.description).toBe('/cancel');
+  });
+
+  test('finishes routing span on navigation error', 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('#nonExistentLink').click(), navigationTxnPromise]);
+
+    const nonExistentRoute = '/non-existent';
+
+    expect(navigationTxn).toMatchObject({
+      contexts: {
+        trace: {
+          op: 'navigation',
+          origin: 'auto.navigation.angular',
+        },
+      },
+      transaction: nonExistentRoute,
+      transaction_info: {
+        source: 'url',
+      },
+    });
+
+    const routingSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.routing');
+
+    expect(routingSpan).toBeDefined();
+    expect(routingSpan?.description).toBe(nonExistentRoute);
+  });
+});
+
+test.describe('TraceDirective', () => {
+  test('creates a child tracingSpan with component name as span name on ngOnInit', 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('#componentTracking').click(), navigationTxnPromise]);
+
+    const traceDirectiveSpan = navigationTxn.spans?.find(
+      span => span?.data && span?.data[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] === 'auto.ui.angular.trace_directive',
+    );
+
+    expect(traceDirectiveSpan).toBeDefined();
+    expect(traceDirectiveSpan).toEqual(
+      expect.objectContaining({
+        data: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'ui.angular.init',
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_directive',
+        },
+        description: '<sample-component>',
+        op: 'ui.angular.init',
+        origin: 'auto.ui.angular.trace_directive',
+        start_timestamp: expect.any(Number),
+        timestamp: expect.any(Number),
+      }),
+    );
+  });
+});
+
+test.describe('TraceClassDecorator', () => {
+  test('adds init span for decorated class', 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('#componentTracking').click(), navigationTxnPromise]);
+
+    const classDecoratorSpan = navigationTxn.spans?.find(
+      span => span?.data && span?.data[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] === 'auto.ui.angular.trace_class_decorator',
+    );
+
+    expect(classDecoratorSpan).toBeDefined();
+    expect(classDecoratorSpan).toEqual(
+      expect.objectContaining({
+        data: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'ui.angular.init',
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_class_decorator',
+        },
+        // todo: right now, it shows the minified version of the component name - we will add a name input to the Decorator
+        description: expect.any(String),
+        op: 'ui.angular.init',
+        origin: 'auto.ui.angular.trace_class_decorator',
+        start_timestamp: expect.any(Number),
+        timestamp: expect.any(Number),
+      }),
+    );
+  });
+});
+
+test.describe('TraceMethodDecorator', () => {
+  test('instruments decorated methods (`ngOnInit` and `ngAfterViewInit`)', 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('#componentTracking').click(), navigationTxnPromise]);
+
+    const ngInitSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.ngOnInit');
+    const ngAfterViewInitSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.ngAfterViewInit');
+
+    expect(ngInitSpan).toBeDefined();
+    expect(ngInitSpan).toEqual(
+      expect.objectContaining({
+        data: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'ui.angular.ngOnInit',
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_method_decorator',
+        },
+        // todo: right now, it shows the minified version of the component name - we will add a name input to the Decorator
+        description: expect.any(String),
+        op: 'ui.angular.ngOnInit',
+        origin: 'auto.ui.angular.trace_method_decorator',
+        start_timestamp: expect.any(Number),
+        timestamp: expect.any(Number),
+      }),
+    );
+
+    expect(ngAfterViewInitSpan).toBeDefined();
+    expect(ngAfterViewInitSpan).toEqual(
+      expect.objectContaining({
+        data: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'ui.angular.ngAfterViewInit',
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_method_decorator',
+        },
+        // todo: right now, it shows the minified version of the component name - we will add a name input to the Decorator
+        description: expect.any(String),
+        op: 'ui.angular.ngAfterViewInit',
+        origin: 'auto.ui.angular.trace_method_decorator',
+        start_timestamp: expect.any(Number),
+        timestamp: expect.any(Number),
+      }),
+    );
+  });
+});
diff --git a/packages/angular/src/index.ts b/packages/angular/src/index.ts
@@ -7,10 +7,6 @@ export { createErrorHandler, SentryErrorHandler } from './errorhandler';
 export {
   // eslint-disable-next-line deprecation/deprecation
   getActiveTransaction,
-  // eslint-disable-next-line deprecation/deprecation
-  instrumentAngularRouting, // new name
-  // eslint-disable-next-line deprecation/deprecation
-  routingInstrumentation, // legacy name
   browserTracingIntegration,
   TraceClassDecorator,
   TraceMethodDecorator,