diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 73bf94c89f73..56dc1cb60525 100644 --- a/packages/angular/src/tracing.ts +++ b/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 = 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 = 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 = 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)}`; +} diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 403c9b7c3cf1..ad4b67f2ecc9 100644 --- a/packages/angular/test/tracing.test.ts +++ b/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 = new Subject(); + const routerEvents$: Subject = new Subject(); + const mockedRouter: Partial = { + 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, + ); + }); }); });