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';
@@ -10,6 +11,8 @@ import { ANGULAR_INIT_OP, ANGULAR_OP, ANGULAR_ROUTING_OP } from './constants';
 import { IS_DEBUG_BUILD } from './flags';
 import { runOutsideAngular } from './zone';
 
+type ParamMap = { [key: string]: string[] };
+
 let instrumentationInitialized: boolean;
 let stashedStartTransaction: (context: TransactionContext) => Transaction | undefined;
 let stashedStartTransactionOnLocationChange: boolean;
@@ -101,6 +104,40 @@ export class TraceService implements OnDestroy {
     }),
   );
 
+  // The ResolveEnd event is fired when the Angular router has resolved the URL
+  // and activated the route. It holds the new resolved router state 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.
+  // In any case, this is the earliest stage where we can reliably get a resolved
+  //
+  public resEnd$: Observable<Event> = this._router.events.pipe(
+    filter(event => event instanceof ResolveEnd),
+    tap(event => {
+      const ev = event as ResolveEnd;
+
+      const params = getParamsOfRoute(ev.state.root);
+
+      // ev.urlAfterRedirects is the one we prefer because it should hold the most recent
+      // one that holds information about a redirect to another route if this was specified
+      // in the Angular router config. In case this doesn't exist (for whatever reason),
+      // we fall back to ev.url which holds the primarily resolved URL before a potential
+      // redirect.
+      const url = ev.urlAfterRedirects || ev.url;
+
+      const route = getParameterizedRouteFromUrlAndParams(url, params);
+
+      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 +152,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 +280,44 @@ export function TraceMethodDecorator(): MethodDecorator {
     return descriptor;
   };
 }
+
+/**
+ * Recursively traverses the routerstate and its children to collect a map of parameters on the entire route.
+ *
+ * Because Angular supports child routes (e.g. when creating nested routes or lazy loaded routes), we have
+ * to not only visit the root router snapshot but also its children to get all parameters of the entire route.
+ *
+ * @param activatedRouteSnapshot the root router snapshot
+ * @returns a map of params, mapping from a key to an array of values
+ */
+export function getParamsOfRoute(activatedRouteSnapshot: ActivatedRouteSnapshot): ParamMap {
+  function getParamsOfRouteH(routeSnapshot: ActivatedRouteSnapshot, accParams: ParamMap): ParamMap {
+    Object.keys(routeSnapshot.params).forEach(key => {
+      accParams[key] = [...(accParams[key] || []), routeSnapshot.params[key]];
+    });
+    routeSnapshot.children.forEach(child => getParamsOfRouteH(child, accParams));
+    return accParams;
+  }
+
+  return getParamsOfRouteH(activatedRouteSnapshot, {});
+}
+
+/**
+ * Takes a raw URl and a map of params and replaces each values occuring in the raw URL with
+ * the name of the parameter.
+ *
+ * @param url raw URL (e.g. /user/1234/details)
+ * @param params a map of type ParamMap
+ * @returns the parameterized URL (e.g. /user/:userId/details)
+ */
+export function getParameterizedRouteFromUrlAndParams(url: string, params: ParamMap): string {
+  if (params) {
+    return Object.keys(params).reduce((prevUrl: string, paramName: string) => {
+      return prevUrl
+        .split('/')
+        .map(segment => (params[paramName].find(p => p === segment) ? `:${paramName}` : segment))
+        .join('/');
+    }, url);
+  }
+  return url;
+}

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 { getParameterizedRouteFromUrlAndParams, getParamsOfRoute } 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,159 @@ 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([
+        ['does not alter static routes', '/books/', {}, '/books/'],
+        ['parameterizes number IDs in the URL', '/books/1/details', { bookId: '1' }, '/books/:bookId/details'],
+        [
+          'parameterizes string IDs in the URL',
+          '/books/asd123/details',
+          { bookId: 'asd123' },
+          '/books/:bookId/details',
+        ],
+        [
+          'parameterizes UUID4 IDs in the URL',
+          '/books/04bc6846-4a1e-4af5-984a-003258f33e31/details',
+          { bookId: '04bc6846-4a1e-4af5-984a-003258f33e31' },
+          '/books/:bookId/details',
+        ],
+        [
+          'parameterizes multiple IDs in the URL',
+          '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31',
+          { orgId: 'sentry', projId: '1234', eventId: '04bc6846-4a1e-4af5-984a-003258f33e31' },
+          '/org/:orgId/projects/:projId/events/:eventId',
+        ],
+      ])('%s and sets the source to `route`', (_, url, params, 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, { root: { params, children: [] } } 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: { params: { userId: '12345' }, children: [] } } as any));
+
+        expect(transaction.setName).toHaveBeenCalledTimes(0);
+        expect(transaction.setMetadata).toHaveBeenCalledTimes(0);
+        expect(transaction.name).toEqual(url);
+      });
+    });
+  });
+
+  describe('getParamsOfRoute', () => {
+    it.each([
+      ['returns an empty object if the route has no params', { params: {}, children: [] }, {}],
+      [
+        'returns an params of a route without children',
+        { params: { bookId: '1234' }, children: [] },
+        { bookId: ['1234'] },
+      ],
+      [
+        'returns an params of a route with children',
+        {
+          params: { bookId: '1234' },
+          children: [
+            { params: { authorId: 'abc' }, children: [] },
+            { params: { pageNum: '66' }, children: [{ params: { userId: 'xyz' }, children: [] }] },
+          ],
+        },
+        { bookId: ['1234'], authorId: ['abc'], pageNum: ['66'], userId: ['xyz'] },
+      ],
+      [
+        'returns an params of a route with children where param names are identical',
+        {
+          params: { id: '1234' },
+          children: [
+            { params: { id: 'abc' }, children: [] },
+            { params: { pageNum: '66' }, children: [{ params: { id: 'xyz', somethingElse: '789' }, children: [] }] },
+          ],
+        },
+        { id: ['1234', 'abc', 'xyz'], pageNum: ['66'], somethingElse: ['789'] },
+      ],
+    ])('%s', (_, routeSnapshot, expectedParams) => {
+      expect(getParamsOfRoute(routeSnapshot as unknown as ActivatedRouteSnapshot)).toEqual(expectedParams);
+    });
+  });
+
+  describe('getParameterizedRouteFromUrlAndParams', () => {
+    it.each([
+      ['returns untouched URL if no params are passed', '/static/route', {}, '/static/route'],
+      [
+        'returns parameterized URL if params are passed',
+        '/books/1234/read/0',
+        { bookId: ['1234'], pageNum: ['0'] },
+        '/books/:bookId/read/:pageNum',
+      ],
+      [
+        'does not replace partially matching URL segments',
+        '/books/1234/read/0',
+        { wrongId: ['12'] },
+        '/books/1234/read/0',
+      ],
+      [
+        'does not replace partially matching URL segments',
+        '/books/1234/read/04',
+        { wrongId: ['12'], bookId: ['1234'], pageNum: ['4'] },
+        '/books/:bookId/read/04',
+      ],
+      [
+        'correctly matches URLs containing multiple identical param names',
+        '/books/1234/read/4/abc/test/123',
+        { id: ['1234', '123', 'abc'], pageNum: ['4'] },
+        '/books/:id/read/:pageNum/:id/test/:id',
+      ],
+    ])('%s', (_, url, params, expectedRoute) => {
+      expect(getParameterizedRouteFromUrlAndParams(url, params)).toEqual(expectedRoute);
+    });
   });
 });