Skip to content

Commit a87c0c6

Browse files
Lms24Lms24
authored
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.
1 parent c39e640 commit a87c0c6

File tree

2 files changed

+225
-11
lines changed

2 files changed

+225
-11
lines changed

‎packages/angular/src/tracing.ts

+45-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1+
/* eslint-disable max-lines */
12
import { AfterViewInit, Directive, Injectable, Input, NgModule, OnDestroy, OnInit } from '@angular/core';
2-
import { Event, NavigationEnd, NavigationStart, Router } from '@angular/router';
3+
import { ActivatedRouteSnapshot, Event, NavigationEnd, NavigationStart, ResolveEnd, Router } from '@angular/router';
34
import { getCurrentHub } from '@sentry/browser';
45
import { Span, Transaction, TransactionContext } from '@sentry/types';
56
import { getGlobalObject, logger, stripUrlQueryAndFragment, timestampWithMs } from '@sentry/utils';
@@ -62,15 +63,14 @@ export function getActiveTransaction(): Transaction | undefined {
6263
@Injectable({ providedIn: 'root' })
6364
export class TraceService implements OnDestroy {
6465
public navStart$: Observable<Event> = this._router.events.pipe(
65-
filter(event => event instanceof NavigationStart),
66-
tap(event => {
66+
filter((event): event is NavigationStart => event instanceof NavigationStart),
67+
tap(navigationEvent => {
6768
if (!instrumentationInitialized) {
6869
IS_DEBUG_BUILD &&
6970
logger.error('Angular integration has tracing enabled, but Tracing integration is not configured');
7071
return;
7172
}
7273

73-
const navigationEvent = event as NavigationStart;
7474
const strippedUrl = stripUrlQueryAndFragment(navigationEvent.url);
7575
let activeTransaction = getActiveTransaction();
7676

@@ -101,6 +101,28 @@ export class TraceService implements OnDestroy {
101101
}),
102102
);
103103

104+
// The ResolveEnd event is fired when the Angular router has resolved the URL and
105+
// the parameter<->value mapping. It holds the new resolved router state with
106+
// the mapping and the new URL.
107+
// Only After this event, the route is activated, meaning that the transaction
108+
// can be updated with the parameterized route name before e.g. the route's root
109+
// component is initialized. This should be early enough before outgoing requests
110+
// are made from the new route, with the exceptions of requests being made during
111+
// a navigation.
112+
public resEnd$: Observable<Event> = this._router.events.pipe(
113+
filter((event): event is ResolveEnd => event instanceof ResolveEnd),
114+
tap(event => {
115+
const route = getParameterizedRouteFromSnapshot(event.state.root);
116+
117+
const transaction = getActiveTransaction();
118+
// TODO (v8 / #5416): revisit the source condition. Do we want to make the parameterized route the default?
119+
if (transaction && transaction.metadata.source === 'url') {
120+
transaction.setName(route);
121+
transaction.setMetadata({ source: 'route' });
122+
}
123+
}),
124+
);
125+
104126
public navEnd$: Observable<Event> = this._router.events.pipe(
105127
filter(event => event instanceof NavigationEnd),
106128
tap(() => {
@@ -115,10 +137,12 @@ export class TraceService implements OnDestroy {
115137
);
116138

117139
private _routingSpan: Span | null = null;
140+
118141
private _subscription: Subscription = new Subscription();
119142

120143
public constructor(private readonly _router: Router) {
121144
this._subscription.add(this.navStart$.subscribe());
145+
this._subscription.add(this.resEnd$.subscribe());
122146
this._subscription.add(this.navEnd$.subscribe());
123147
}
124148

@@ -241,3 +265,20 @@ export function TraceMethodDecorator(): MethodDecorator {
241265
return descriptor;
242266
};
243267
}
268+
269+
/**
270+
* Takes the parameterized route from a given ActivatedRouteSnapshot and concatenates the snapshot's
271+
* child route with its parent to produce the complete parameterized URL of the activated route.
272+
* This happens recursively until the last child (i.e. the end of the URL) is reached.
273+
*
274+
* @param route the ActivatedRouteSnapshot of which its path and its child's path is concantenated
275+
*
276+
* @returns the concatenated parameterzited route string
277+
*/
278+
export function getParameterizedRouteFromSnapshot(route?: ActivatedRouteSnapshot | null): string {
279+
const path = route && route.firstChild && route.firstChild.routeConfig && route.firstChild.routeConfig.path;
280+
if (!path) {
281+
return '/';
282+
}
283+
return `/${path}${getParameterizedRouteFromSnapshot(route && route.firstChild)}`;
284+
}

‎packages/angular/test/tracing.test.ts

+180-7
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,34 @@
1-
import { NavigationStart, Router, RouterEvent } from '@angular/router';
1+
import { ActivatedRouteSnapshot, Event, NavigationStart, ResolveEnd, Router } from '@angular/router';
2+
import { Hub, Transaction } from '@sentry/types';
23
import { Subject } from 'rxjs';
34

45
import { instrumentAngularRouting, TraceService } from '../src/index';
6+
import { getParameterizedRouteFromSnapshot } from '../src/tracing';
7+
8+
let transaction: any;
9+
let customStartTransaction: (context: any) => Transaction | undefined;
10+
11+
jest.mock('@sentry/browser', () => {
12+
const original = jest.requireActual('@sentry/browser');
13+
return {
14+
...original,
15+
getCurrentHub: () => {
16+
return {
17+
getScope: () => {
18+
return {
19+
getTransaction: () => {
20+
return transaction;
21+
},
22+
};
23+
},
24+
} as unknown as Hub;
25+
},
26+
};
27+
});
528

629
describe('Angular Tracing', () => {
730
const startTransaction = jest.fn();
31+
832
describe('instrumentAngularRouting', () => {
933
it('should attach the transaction source on the pageload transaction', () => {
1034
instrumentAngularRouting(startTransaction);
@@ -18,15 +42,15 @@ describe('Angular Tracing', () => {
1842

1943
describe('TraceService', () => {
2044
let traceService: TraceService;
21-
const routerEvents$: Subject<RouterEvent> = new Subject();
45+
const routerEvents$: Subject<Event> = new Subject();
46+
const mockedRouter: Partial<Router> = {
47+
events: routerEvents$,
48+
};
2249

23-
beforeAll(() => instrumentAngularRouting(startTransaction));
2450
beforeEach(() => {
51+
instrumentAngularRouting(startTransaction);
2552
jest.resetAllMocks();
26-
27-
traceService = new TraceService({
28-
events: routerEvents$,
29-
} as unknown as Router);
53+
traceService = new TraceService(mockedRouter as Router);
3054
});
3155

3256
afterEach(() => {
@@ -43,5 +67,154 @@ describe('Angular Tracing', () => {
4367
metadata: { source: 'url' },
4468
});
4569
});
70+
71+
describe('URL parameterization', () => {
72+
// TODO: These tests are real unit tests in the sense that they only test TraceService
73+
// and we essentially just simulate a router navigation by firing the respective
74+
// routing events and providing the raw URL + the resolved route parameters.
75+
// In the future we should add more "wholesome" tests that let the Angular router
76+
// do its thing (e.g. by calling router.navigate) and we check how our service
77+
// reacts to it.
78+
// Once we set up Jest for testing Angular, we can use TestBed to inject an actual
79+
// router instance into TraceService and add more tests.
80+
81+
beforeEach(() => {
82+
transaction = undefined;
83+
customStartTransaction = jest.fn((ctx: any) => {
84+
transaction = {
85+
...ctx,
86+
setName: jest.fn(name => (transaction.name = name)),
87+
setMetadata: jest.fn(metadata => (transaction.metadata = metadata)),
88+
};
89+
return transaction;
90+
});
91+
});
92+
93+
it.each([
94+
[
95+
'handles the root URL correctly',
96+
'',
97+
{
98+
root: { firstChild: { routeConfig: null } },
99+
},
100+
'/',
101+
],
102+
[
103+
'does not alter static routes',
104+
'/books/',
105+
{
106+
root: { firstChild: { routeConfig: { path: 'books' } } },
107+
},
108+
'/books/',
109+
],
110+
[
111+
'parameterizes IDs in the URL',
112+
'/books/1/details',
113+
{
114+
root: { firstChild: { routeConfig: { path: 'books/:bookId/details' } } },
115+
},
116+
'/books/:bookId/details/',
117+
],
118+
[
119+
'parameterizes multiple IDs in the URL',
120+
'/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31',
121+
{
122+
root: { firstChild: { routeConfig: { path: 'org/:orgId/projects/:projId/events/:eventId' } } },
123+
},
124+
'/org/:orgId/projects/:projId/events/:eventId/',
125+
],
126+
[
127+
'parameterizes URLs from route with child routes',
128+
'/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31',
129+
{
130+
root: {
131+
firstChild: {
132+
routeConfig: { path: 'org/:orgId' },
133+
firstChild: {
134+
routeConfig: { path: 'projects/:projId' },
135+
firstChild: { routeConfig: { path: 'events/:eventId' } },
136+
},
137+
},
138+
},
139+
},
140+
'/org/:orgId/projects/:projId/events/:eventId/',
141+
],
142+
])('%s and sets the source to `route`', (_, url, routerState, result) => {
143+
instrumentAngularRouting(customStartTransaction, false, true);
144+
145+
// this event starts the transaction
146+
routerEvents$.next(new NavigationStart(0, url));
147+
148+
expect(customStartTransaction).toHaveBeenCalledWith({
149+
name: url,
150+
op: 'navigation',
151+
metadata: { source: 'url' },
152+
});
153+
154+
// this event starts the parameterization
155+
routerEvents$.next(new ResolveEnd(1, url, url, routerState as any));
156+
157+
expect(transaction.setName).toHaveBeenCalledWith(result);
158+
expect(transaction.setMetadata).toHaveBeenCalledWith({ source: 'route' });
159+
});
160+
161+
it('does not change the transaction name if the source is something other than `url`', () => {
162+
instrumentAngularRouting(customStartTransaction, false, true);
163+
164+
const url = '/user/12345/test';
165+
166+
routerEvents$.next(new NavigationStart(0, url));
167+
168+
expect(customStartTransaction).toHaveBeenCalledWith({
169+
name: url,
170+
op: 'navigation',
171+
metadata: { source: 'url' },
172+
});
173+
174+
// Simulate that this transaction has a custom name:
175+
transaction.metadata.source = 'custom';
176+
177+
// this event starts the parameterization
178+
routerEvents$.next(
179+
new ResolveEnd(1, url, url, {
180+
root: { firstChild: { routeConfig: { path: 'org/:orgId/projects/:projId/events/:eventId' } } },
181+
} as any),
182+
);
183+
184+
expect(transaction.setName).toHaveBeenCalledTimes(0);
185+
expect(transaction.setMetadata).toHaveBeenCalledTimes(0);
186+
expect(transaction.name).toEqual(url);
187+
});
188+
});
189+
});
190+
191+
describe('getParameterizedRouteFromSnapshot', () => {
192+
it.each([
193+
['returns `/` empty object if the route no children', {}, '/'],
194+
[
195+
'returns the route of a snapshot without children',
196+
{
197+
firstChild: { routeConfig: { path: 'users/:id' } },
198+
},
199+
'/users/:id/',
200+
],
201+
[
202+
'returns the complete route of a snapshot with children',
203+
{
204+
firstChild: {
205+
routeConfig: { path: 'orgs/:orgId' },
206+
firstChild: {
207+
routeConfig: { path: 'projects/:projId' },
208+
firstChild: { routeConfig: { path: 'overview' } },
209+
},
210+
},
211+
},
212+
'/orgs/:orgId/projects/:projId/overview/',
213+
],
214+
])('%s', (_, routeSnapshot, expectedParams) => {
215+
expect(getParameterizedRouteFromSnapshot(routeSnapshot as unknown as ActivatedRouteSnapshot)).toEqual(
216+
expectedParams,
217+
);
218+
});
46219
});
47220
});

0 commit comments

Comments
 (0)