From f2276e2486ac6cb0bc55f4374bcb81418a1b83a4 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 20 Jun 2023 14:06:12 +0200 Subject: [PATCH 1/3] fix(angular): Stop routing spans on navigation cancel and error events --- packages/angular/src/tracing.ts | 6 ++- packages/angular/test/tracing.test.ts | 63 ++++++++++++++++++++++++++- packages/angular/test/utils/index.ts | 22 +++++++--- 3 files changed, 82 insertions(+), 9 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index b206d7fe429d..f2e79adfe9b0 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -5,7 +5,7 @@ import type { ActivatedRouteSnapshot, Event, RouterState } from '@angular/router // Duplicated import to work around a TypeScript bug where it'd complain that `Router` isn't imported as a type. // We need to import it as a value to satisfy Angular dependency injection. So: // eslint-disable-next-line @typescript-eslint/consistent-type-imports, import/no-duplicates -import { Router } from '@angular/router'; +import { NavigationCancel, NavigationError, Router } from '@angular/router'; // eslint-disable-next-line import/no-duplicates import { NavigationEnd, NavigationStart, ResolveEnd } from '@angular/router'; import { getCurrentHub, WINDOW } from '@sentry/browser'; @@ -131,7 +131,9 @@ export class TraceService implements OnDestroy { ); public navEnd$: Observable = this._router.events.pipe( - filter(event => event instanceof NavigationEnd), + filter( + event => event instanceof NavigationEnd || event instanceof NavigationCancel || event instanceof NavigationError, + ), tap(() => { if (this._routingSpan) { runOutsideAngular(() => { diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 0afef2771add..531f5f4bf929 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -1,6 +1,7 @@ import { Component } from '@angular/core'; -import type { ActivatedRouteSnapshot } from '@angular/router'; +import type { ActivatedRouteSnapshot, CanActivate, RouterStateSnapshot } from '@angular/router'; import type { Hub } from '@sentry/types'; +import { of } from 'rxjs'; import { instrumentAngularRouting, TraceClassDecorator, TraceDirective, TraceMethodDecorator } from '../src'; import { getParameterizedRouteFromSnapshot } from '../src/tracing'; @@ -185,6 +186,66 @@ describe('Angular Tracing', () => { env.destroy(); }); + it('finishes routing span on navigation error', async () => { + const customStartTransaction = jest.fn(defaultStartTransaction); + + const env = await TestEnv.setup({ + customStartTransaction, + routes: [ + { + path: '', + component: AppComponent, + }, + ], + useTraceService: true, + }); + + const finishMock = jest.fn(); + transaction.startChild = jest.fn(() => ({ + finish: finishMock, + })); + + await env.navigateInAngular('/somewhere'); + + expect(finishMock).toHaveBeenCalledTimes(1); + + env.destroy(); + }); + + it('finishes routing span on navigation cancel', async () => { + const customStartTransaction = jest.fn(defaultStartTransaction); + + class CanActivateGuard implements CanActivate { + canActivate(_route: ActivatedRouteSnapshot, _state: RouterStateSnapshot): boolean { + return false; + } + } + + const env = await TestEnv.setup({ + customStartTransaction, + routes: [ + { + path: 'cancel', + component: AppComponent, + canActivate: [CanActivateGuard], + }, + ], + useTraceService: true, + additionalProviders: [{ provide: CanActivateGuard, useClass: CanActivateGuard }], + }); + + const finishMock = jest.fn(); + transaction.startChild = jest.fn(() => ({ + finish: finishMock, + })); + + await env.navigateInAngular('/cancel'); + + expect(finishMock).toHaveBeenCalledTimes(1); + + env.destroy(); + }); + describe('URL parameterization', () => { it.each([ [ diff --git a/packages/angular/test/utils/index.ts b/packages/angular/test/utils/index.ts index b15ad2028560..217734cacdbc 100644 --- a/packages/angular/test/utils/index.ts +++ b/packages/angular/test/utils/index.ts @@ -1,4 +1,4 @@ -import { Component, NgModule } from '@angular/core'; +import { Component, NgModule, Provider } from '@angular/core'; import type { ComponentFixture } from '@angular/core/testing'; import { TestBed } from '@angular/core/testing'; import type { Routes } from '@angular/router'; @@ -47,6 +47,7 @@ export class TestEnv { startTransactionOnPageLoad?: boolean; startTransactionOnNavigation?: boolean; useTraceService?: boolean; + additionalProviders?: Provider[]; }): Promise { instrumentAngularRouting( conf.customStartTransaction || jest.fn(), @@ -66,8 +67,9 @@ export class TestEnv { provide: TraceService, deps: [Router], }, + ...(conf.additionalProviders || []), ] - : [], + : [...(conf.additionalProviders || [])], }); const router: Router = TestBed.inject(Router); @@ -80,10 +82,18 @@ export class TestEnv { public async navigateInAngular(url: string): Promise { return new Promise(resolve => { return this.fixture.ngZone?.run(() => { - void this.router.navigateByUrl(url).then(() => { - this.fixture.detectChanges(); - resolve(); - }); + void this.router + .navigateByUrl(url) + .then(() => { + this.fixture.detectChanges(); + resolve(); + }) + .catch(err => { + console.log({ err }); + + this.fixture.detectChanges(); + resolve(); + }); }); }); } From d5d418e8c27352a4c6e3396fd7cd9869f7c75775 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 20 Jun 2023 14:21:44 +0200 Subject: [PATCH 2/3] cleanup test setup --- packages/angular/test/utils/index.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/angular/test/utils/index.ts b/packages/angular/test/utils/index.ts index 217734cacdbc..c5ed4b1518a0 100644 --- a/packages/angular/test/utils/index.ts +++ b/packages/angular/test/utils/index.ts @@ -61,7 +61,7 @@ export class TestEnv { TestBed.configureTestingModule({ imports: [AppModule, RouterTestingModule.withRoutes(routes)], declarations: [...(conf.components || []), AppComponent], - providers: useTraceService + providers: (useTraceService ? [ { provide: TraceService, @@ -69,7 +69,8 @@ export class TestEnv { }, ...(conf.additionalProviders || []), ] - : [...(conf.additionalProviders || [])], + : [] + ).concat(...(conf.additionalProviders || [])), }); const router: Router = TestBed.inject(Router); @@ -88,9 +89,7 @@ export class TestEnv { this.fixture.detectChanges(); resolve(); }) - .catch(err => { - console.log({ err }); - + .catch(() => { this.fixture.detectChanges(); resolve(); }); From 9adca3eb75f9dce1521f884e7c195e7891fbf56b Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 20 Jun 2023 14:48:03 +0200 Subject: [PATCH 3/3] linter --- packages/angular/test/tracing.test.ts | 1 - packages/angular/test/utils/index.ts | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 531f5f4bf929..a3375518466a 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -1,7 +1,6 @@ import { Component } from '@angular/core'; import type { ActivatedRouteSnapshot, CanActivate, RouterStateSnapshot } from '@angular/router'; import type { Hub } from '@sentry/types'; -import { of } from 'rxjs'; import { instrumentAngularRouting, TraceClassDecorator, TraceDirective, TraceMethodDecorator } from '../src'; import { getParameterizedRouteFromSnapshot } from '../src/tracing'; diff --git a/packages/angular/test/utils/index.ts b/packages/angular/test/utils/index.ts index c5ed4b1518a0..daa23155d931 100644 --- a/packages/angular/test/utils/index.ts +++ b/packages/angular/test/utils/index.ts @@ -1,4 +1,5 @@ -import { Component, NgModule, Provider } from '@angular/core'; +import type { Provider } from '@angular/core'; +import { Component, NgModule } from '@angular/core'; import type { ComponentFixture } from '@angular/core/testing'; import { TestBed } from '@angular/core/testing'; import type { Routes } from '@angular/router';