feat(Angular): Add URL Parameterization of Transaction Names

packages/angular/src/tracing.ts

@@ -1,5 +1,6 @@
+/* eslint-disable max-lines */
 import { AfterViewInit, Directive, Injectable, Input, NgModule, OnDestroy, OnInit } from '@angular/core';
-import { Event, NavigationEnd, NavigationStart, Router } from '@angular/router';
+import { ActivatedRouteSnapshot, Event, NavigationEnd, NavigationStart, ResolveEnd, Router } from '@angular/router';
 import { getCurrentHub } from '@sentry/browser';
 import { Span, Transaction, TransactionContext } from '@sentry/types';
 import { getGlobalObject, logger, stripUrlQueryAndFragment, timestampWithMs } from '@sentry/utils';
@@ -62,15 +63,14 @@ export function getActiveTransaction(): Transaction | undefined {
 @Injectable({ providedIn: 'root' })
 export class TraceService implements OnDestroy {
   public navStart$: Observable<Event> = this._router.events.pipe(
-    filter(event => event instanceof NavigationStart),
-    tap(event => {
+    filter((event): event is NavigationStart => event instanceof NavigationStart),
+    tap(navigationEvent => {
       if (!instrumentationInitialized) {
         IS_DEBUG_BUILD &&
           logger.error('Angular integration has tracing enabled, but Tracing integration is not configured');
         return;
       }
 
-      const navigationEvent = event as NavigationStart;
       const strippedUrl = stripUrlQueryAndFragment(navigationEvent.url);
       let activeTransaction = getActiveTransaction();
 
@@ -101,6 +101,28 @@ export class TraceService implements OnDestroy {
     }),
   );
 
+  // The ResolveEnd event is fired when the Angular router has resolved the URL and
+  // the parameter<->value mapping. It holds the new resolved router state with
+  // the mapping and the new URL.
+  // Only After this event, the route is activated, meaning that the transaction
+  // can be updated with the parameterized route name before e.g. the route's root
+  // component is initialized. This should be early enough before outgoing requests
+  // are made from the new route, with the exceptions of requests being made during
+  // a navigation.
+  public resEnd$: Observable<Event> = this._router.events.pipe(
+    filter((event): event is ResolveEnd => event instanceof ResolveEnd),
+    tap(event => {
+      const route = getParameterizedRouteFromSnapshot(event.state.root);
+
+      const transaction = getActiveTransaction();
+      // TODO (v8 / #5416): revisit the source condition. Do we want to make the parameterized route the default?
+      if (transaction && transaction.metadata.source === 'url') {
+        transaction.setName(route);
+        transaction.setMetadata({ source: 'route' });
+      }
+    }),
+  );
+
   public navEnd$: Observable<Event> = this._router.events.pipe(
     filter(event => event instanceof NavigationEnd),
     tap(() => {
@@ -115,10 +137,12 @@ export class TraceService implements OnDestroy {
   );
 
   private _routingSpan: Span | null = null;
+
   private _subscription: Subscription = new Subscription();
 
   public constructor(private readonly _router: Router) {
     this._subscription.add(this.navStart$.subscribe());
+    this._subscription.add(this.resEnd$.subscribe());
     this._subscription.add(this.navEnd$.subscribe());
   }
 
@@ -241,3 +265,20 @@ export function TraceMethodDecorator(): MethodDecorator {
     return descriptor;
   };
 }
+
+/**
+ * Takes the parameterized route from a given ActivatedRouteSnapshot and concatenates the snapshot's
+ * child route with its parent to produce the complete parameterized URL of the activated route.
+ * This happens recursively until the last child (i.e. the end of the URL) is reached.
+ *
+ * @param route the ActivatedRouteSnapshot of which its path and its child's path is concantenated
+ *
+ * @returns the concatenated parameterzited route string
+ */
+export function getParameterizedRouteFromSnapshot(route?: ActivatedRouteSnapshot | null): string {
+  const path = route && route.firstChild && route.firstChild.routeConfig && route.firstChild.routeConfig.path;
+  if (!path) {
+    return '/';
+  }
+  return `/${path}${getParameterizedRouteFromSnapshot(route && route.firstChild)}`;
+}

packages/angular/test/tracing.test.ts

@@ -1,10 +1,34 @@
-import { NavigationStart, Router, RouterEvent } from '@angular/router';
+import { ActivatedRouteSnapshot, Event, NavigationStart, ResolveEnd, Router } from '@angular/router';
+import { Hub, Transaction } from '@sentry/types';
 import { Subject } from 'rxjs';
 
 import { instrumentAngularRouting, TraceService } from '../src/index';
+import { getParameterizedRouteFromSnapshot } from '../src/tracing';
+
+let transaction: any;
+let customStartTransaction: (context: any) => Transaction | undefined;
+
+jest.mock('@sentry/browser', () => {
+  const original = jest.requireActual('@sentry/browser');
+  return {
+    ...original,
+    getCurrentHub: () => {
+      return {
+        getScope: () => {
+          return {
+            getTransaction: () => {
+              return transaction;
+            },
+          };
+        },
+      } as unknown as Hub;
+    },
+  };
+});
 
 describe('Angular Tracing', () => {
   const startTransaction = jest.fn();
+
   describe('instrumentAngularRouting', () => {
     it('should attach the transaction source on the pageload transaction', () => {
       instrumentAngularRouting(startTransaction);
@@ -18,15 +42,15 @@ describe('Angular Tracing', () => {
 
   describe('TraceService', () => {
     let traceService: TraceService;
-    const routerEvents$: Subject<RouterEvent> = new Subject();
+    const routerEvents$: Subject<Event> = new Subject();
+    const mockedRouter: Partial<Router> = {
+      events: routerEvents$,
+    };
 
-    beforeAll(() => instrumentAngularRouting(startTransaction));
     beforeEach(() => {
+      instrumentAngularRouting(startTransaction);
       jest.resetAllMocks();
-
-      traceService = new TraceService({
-        events: routerEvents$,
-      } as unknown as Router);
+      traceService = new TraceService(mockedRouter as Router);
     });
 
     afterEach(() => {
@@ -43,5 +67,154 @@ describe('Angular Tracing', () => {
         metadata: { source: 'url' },
       });
     });
+
+    describe('URL parameterization', () => {
+      // TODO: These tests are real unit tests in the sense that they only test TraceService
+      //       and we essentially just simulate a router navigation by firing the respective
+      //       routing events and providing the raw URL + the resolved route parameters.
+      //       In the future we should add more "wholesome" tests that let the Angular router
+      //       do its thing (e.g. by calling router.navigate) and we check how our service
+      //       reacts to it.
+      //       Once we set up Jest for testing Angular, we can use TestBed to inject an actual
+      //       router instance into TraceService and add more tests.
+
+      beforeEach(() => {
+        transaction = undefined;
+        customStartTransaction = jest.fn((ctx: any) => {
+          transaction = {
+            ...ctx,
+            setName: jest.fn(name => (transaction.name = name)),
+            setMetadata: jest.fn(metadata => (transaction.metadata = metadata)),
+          };
+          return transaction;
+        });
+      });
+
+      it.each([
+        [
+          'handles the root URL correctly',
+          '',
+          {
+            root: { firstChild: { routeConfig: null } },
+          },
+          '/',
+        ],
+        [
+          'does not alter static routes',
+          '/books/',
+          {
+            root: { firstChild: { routeConfig: { path: 'books' } } },
+          },
+          '/books/',
+        ],
+        [
+          'parameterizes IDs in the URL',
+          '/books/1/details',
+          {
+            root: { firstChild: { routeConfig: { path: 'books/:bookId/details' } } },
+          },
+          '/books/:bookId/details/',
+        ],
+        [
+          'parameterizes multiple IDs in the URL',
+          '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31',
+          {
+            root: { firstChild: { routeConfig: { path: 'org/:orgId/projects/:projId/events/:eventId' } } },
+          },
+          '/org/:orgId/projects/:projId/events/:eventId/',
+        ],
+        [
+          'parameterizes URLs from route with child routes',
+          '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31',
+          {
+            root: {
+              firstChild: {
+                routeConfig: { path: 'org/:orgId' },
+                firstChild: {
+                  routeConfig: { path: 'projects/:projId' },
+                  firstChild: { routeConfig: { path: 'events/:eventId' } },
+                },
+              },
+            },
+          },
+          '/org/:orgId/projects/:projId/events/:eventId/',
+        ],
+      ])('%s and sets the source to `route`', (_, url, routerState, result) => {
+        instrumentAngularRouting(customStartTransaction, false, true);
+
+        // this event starts the transaction
+        routerEvents$.next(new NavigationStart(0, url));
+
+        expect(customStartTransaction).toHaveBeenCalledWith({
+          name: url,
+          op: 'navigation',
+          metadata: { source: 'url' },
+        });
+
+        // this event starts the parameterization
+        routerEvents$.next(new ResolveEnd(1, url, url, routerState as any));
+
+        expect(transaction.setName).toHaveBeenCalledWith(result);
+        expect(transaction.setMetadata).toHaveBeenCalledWith({ source: 'route' });
+      });
+
+      it('does not change the transaction name if the source is something other than `url`', () => {
+        instrumentAngularRouting(customStartTransaction, false, true);
+
+        const url = '/user/12345/test';
+
+        routerEvents$.next(new NavigationStart(0, url));
+
+        expect(customStartTransaction).toHaveBeenCalledWith({
+          name: url,
+          op: 'navigation',
+          metadata: { source: 'url' },
+        });
+
+        // Simulate that this transaction has a custom name:
+        transaction.metadata.source = 'custom';
+
+        // this event starts the parameterization
+        routerEvents$.next(
+          new ResolveEnd(1, url, url, {
+            root: { firstChild: { routeConfig: { path: 'org/:orgId/projects/:projId/events/:eventId' } } },
+          } as any),
+        );
+
+        expect(transaction.setName).toHaveBeenCalledTimes(0);
+        expect(transaction.setMetadata).toHaveBeenCalledTimes(0);
+        expect(transaction.name).toEqual(url);
+      });
+    });
+  });
+
+  describe('getParameterizedRouteFromSnapshot', () => {
+    it.each([
+      ['returns `/` empty object if the route no children', {}, '/'],
+      [
+        'returns the route of a snapshot without children',
+        {
+          firstChild: { routeConfig: { path: 'users/:id' } },
+        },
+        '/users/:id/',
+      ],
+      [
+        'returns the complete route of a snapshot with children',
+        {
+          firstChild: {
+            routeConfig: { path: 'orgs/:orgId' },
+            firstChild: {
+              routeConfig: { path: 'projects/:projId' },
+              firstChild: { routeConfig: { path: 'overview' } },
+            },
+          },
+        },
+        '/orgs/:orgId/projects/:projId/overview/',
+      ],
+    ])('%s', (_, routeSnapshot, expectedParams) => {
+      expect(getParameterizedRouteFromSnapshot(routeSnapshot as unknown as ActivatedRouteSnapshot)).toEqual(
+        expectedParams,
+      );
+    });
   });
 });