feat(angular): Add URL Parameterization of Transaction Names (#5416)

Introduce URL Parameterization to our Angular SDK. With this change, the SDK will update transaction names coming from a URL with a paramterized version of the URL (e.g `GET /users/1235/details` will be parameterized to `GET /users/:userId/details/`).

This is done by listening to the `ResolveEnd` routing event in `TraceService`. When this event is fired, the Angular router has finished resolving the new URL and found the correct route. Besides the url, the event contains a snapshot of the resolved and soon-to-be activated route. This `ActivatedRouteSnapshot` instance is the root instance of the activated route. If the resolved route is something other than `''` or `'/'`, it will also points to its first child. This instance might again point to its (a possibly existing) child but it also holds its part of the resolved and parameterized route path (URL). 
By recursively concatenating the paths, we get a fully parameterized URL. 

The main advantage of this approach vs. a previously tried URL<->parameter interpolation approach is that we do not run into the danger of making interpolation mistakes or leaving parts of the route unparameterized. We now simply take what the Angular router has already resolved.

The potential disadvantage is that we rely on the assumption that there is only one child per route snapshot. While testing, I didn't come across a situation where a parent snapshot had more than one child. I believe that this is because the `ResolveEnd` event contains a snapshot of the newly activated route and not the complete router state. However, if we get reports of incorrect transaction names, we might want to revisit this parameterization approach.  

It should be noted that because there is a gap between transaction creation (where we initially set the unparameterized name) and URL parameterization, it is possible that parameterization might happen after an outgoing Http request is made. In that case, the dynamic sampling context will be populated and frozen without the `transaction` field because at this point the transaction name's source is still `url`. This means that we have a potential timing problem, similar to other routing instrumentations. 
At this point we're not yet sure how often such a timing problem would occur but it seems pretty unlikely for normal usage. For instance, DSC population will happen correctly (with the `transaction` field) when the first outgoing Http request is fired in the constructor of the component that is associated with the new route. This also means that hooks like `ngOnInit` which are frequently used for making requests (e.g. via Service calls) are called long after the `ResolveEnd` routing event.  

Additionally, this add a few unit tests to test this new behaviour. However, these tests are really unit tests, meaning they only test our `TraceService` implementation. We currently simply mock the Angular router and fire the routing events manually. A more "wholesome" approach (e.g. calling `router.navigate` instead of manually firing events) would be better but for this we'd need to inject an actual Angular Router into `TraceService`. This is done best with Angular's `TestBed` feature but it currently doesn't work with our testing setup for the Angular SDK. Changing the setup is out of scope for this PR but we'll revisit this and make the necessary changes to improve the testing setup of our Angular SDK.

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,
+      );
+    });
   });
 });