From 2274b276ec70fb82242d6602c422f05e22117f27 Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Tue, 12 Mar 2024 11:25:08 +0100 Subject: [PATCH] ref(v8/angular): Remove instrumentAngularRouting and fix tests (#11021) ref https://github.com/getsentry/sentry-javascript/issues/10898 ref https://github.com/getsentry/sentry-javascript/pull/10353 --- .../angular-17/src/app/app.routes.ts | 12 + .../angular-17/src/app/cancel-guard.guard.ts | 5 + .../src/app/cancel/cancel.components.ts | 8 + .../component-tracking.components.ts | 18 + .../angular-17/src/app/home/home.component.ts | 3 + .../sample-component.components.ts | 12 + .../angular-17/tests/performance.test.ts | 176 ++++++ packages/angular/src/index.ts | 4 - packages/angular/src/tracing.ts | 110 +--- packages/angular/test/tracing.test.ts | 501 ++---------------- packages/angular/test/utils/index.ts | 109 ---- 11 files changed, 302 insertions(+), 656 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel-guard.guard.ts create mode 100644 dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel/cancel.components.ts create mode 100644 dev-packages/e2e-tests/test-applications/angular-17/src/app/component-tracking/component-tracking.components.ts create mode 100644 dev-packages/e2e-tests/test-applications/angular-17/src/app/sample-component/sample-component.components.ts delete mode 100644 packages/angular/test/utils/index.ts diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/app.routes.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/app.routes.ts index d4d9a04f6c99..24bf8b769051 100644 --- a/dev-packages/e2e-tests/test-applications/angular-17/src/app/app.routes.ts +++ b/dev-packages/e2e-tests/test-applications/angular-17/src/app/app.routes.ts @@ -1,4 +1,7 @@ import { Routes } from '@angular/router'; +import { cancelGuard } from './cancel-guard.guard'; +import { CancelComponent } from './cancel/cancel.components'; +import { ComponentTrackingComponent } from './component-tracking/component-tracking.components'; import { HomeComponent } from './home/home.component'; import { UserComponent } from './user/user.component'; @@ -11,6 +14,15 @@ export const routes: Routes = [ path: 'home', component: HomeComponent, }, + { + path: 'cancel', + component: CancelComponent, + canActivate: [cancelGuard], + }, + { + path: 'component-tracking', + component: ComponentTrackingComponent, + }, { path: 'redirect1', redirectTo: '/redirect2', diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel-guard.guard.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel-guard.guard.ts new file mode 100644 index 000000000000..16ec4a2ab164 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel-guard.guard.ts @@ -0,0 +1,5 @@ +import { ActivatedRouteSnapshot, CanActivateFn, RouterStateSnapshot } from '@angular/router'; + +export const cancelGuard: CanActivateFn = (_next: ActivatedRouteSnapshot, _state: RouterStateSnapshot) => { + return false; +}; diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel/cancel.components.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel/cancel.components.ts new file mode 100644 index 000000000000..b6ee1876e035 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/angular-17/src/app/cancel/cancel.components.ts @@ -0,0 +1,8 @@ +import { Component } from '@angular/core'; + +@Component({ + selector: 'app-cancel', + standalone: true, + template: `
`, +}) +export class CancelComponent {} diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/component-tracking/component-tracking.components.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/component-tracking/component-tracking.components.ts new file mode 100644 index 000000000000..b82924ab0602 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/angular-17/src/app/component-tracking/component-tracking.components.ts @@ -0,0 +1,18 @@ +import { AfterViewInit, Component, OnInit } from '@angular/core'; +import { TraceClassDecorator, TraceMethodDecorator, TraceModule } from '@sentry/angular-ivy'; +import { SampleComponent } from '../sample-component/sample-component.components'; + +@Component({ + selector: 'app-cancel', + standalone: true, + imports: [TraceModule, SampleComponent], + template: ``, +}) +@TraceClassDecorator() +export class ComponentTrackingComponent implements OnInit, AfterViewInit { + @TraceMethodDecorator() + ngOnInit() {} + + @TraceMethodDecorator() + ngAfterViewInit() {} +} diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts index 9179f5d79638..298d7f7d54cd 100644 --- a/dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts +++ b/dev-packages/e2e-tests/test-applications/angular-17/src/app/home/home.component.ts @@ -11,6 +11,9 @@ import { RouterLink } from '@angular/router'; diff --git a/dev-packages/e2e-tests/test-applications/angular-17/src/app/sample-component/sample-component.components.ts b/dev-packages/e2e-tests/test-applications/angular-17/src/app/sample-component/sample-component.components.ts new file mode 100644 index 000000000000..bd331a9dbff0 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/angular-17/src/app/sample-component/sample-component.components.ts @@ -0,0 +1,12 @@ +import { Component, OnInit } from '@angular/core'; + +@Component({ + selector: 'app-sample-component', + standalone: true, + template: `
`, +}) +export class SampleComponent implements OnInit { + ngOnInit() { + console.log('SampleComponent'); + } +} diff --git a/dev-packages/e2e-tests/test-applications/angular-17/tests/performance.test.ts b/dev-packages/e2e-tests/test-applications/angular-17/tests/performance.test.ts index 9b5f1b08e9d2..ae6241016e00 100644 --- a/dev-packages/e2e-tests/test-applications/angular-17/tests/performance.test.ts +++ b/dev-packages/e2e-tests/test-applications/angular-17/tests/performance.test.ts @@ -1,4 +1,5 @@ import { expect, test } from '@playwright/test'; +import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import { waitForTransaction } from '../event-proxy-server'; test('sends a pageload transaction with a parameterized URL', async ({ page }) => { @@ -126,3 +127,178 @@ test('groups redirects within one navigation root span', async ({ page }) => { expect(routingSpan).toBeDefined(); expect(routingSpan?.description).toBe('/redirect1'); }); + +test.describe('finish routing span', () => { + test('finishes routing span on navigation cancel', async ({ page }) => { + const navigationTxnPromise = waitForTransaction('angular-17', async transactionEvent => { + return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation'; + }); + + await page.goto(`/`); + + // immediately navigate to a different route + const [_, navigationTxn] = await Promise.all([page.locator('#cancelLink').click(), navigationTxnPromise]); + + expect(navigationTxn).toMatchObject({ + contexts: { + trace: { + op: 'navigation', + origin: 'auto.navigation.angular', + }, + }, + transaction: '/cancel', + transaction_info: { + source: 'url', + }, + }); + + const routingSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.routing'); + + expect(routingSpan).toBeDefined(); + expect(routingSpan?.description).toBe('/cancel'); + }); + + test('finishes routing span on navigation error', async ({ page }) => { + const navigationTxnPromise = waitForTransaction('angular-17', async transactionEvent => { + return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation'; + }); + + await page.goto(`/`); + + // immediately navigate to a different route + const [_, navigationTxn] = await Promise.all([page.locator('#nonExistentLink').click(), navigationTxnPromise]); + + const nonExistentRoute = '/non-existent'; + + expect(navigationTxn).toMatchObject({ + contexts: { + trace: { + op: 'navigation', + origin: 'auto.navigation.angular', + }, + }, + transaction: nonExistentRoute, + transaction_info: { + source: 'url', + }, + }); + + const routingSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.routing'); + + expect(routingSpan).toBeDefined(); + expect(routingSpan?.description).toBe(nonExistentRoute); + }); +}); + +test.describe('TraceDirective', () => { + test('creates a child tracingSpan with component name as span name on ngOnInit', async ({ page }) => { + const navigationTxnPromise = waitForTransaction('angular-17', async transactionEvent => { + return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation'; + }); + + await page.goto(`/`); + + // immediately navigate to a different route + const [_, navigationTxn] = await Promise.all([page.locator('#componentTracking').click(), navigationTxnPromise]); + + const traceDirectiveSpan = navigationTxn.spans?.find( + span => span?.data && span?.data[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] === 'auto.ui.angular.trace_directive', + ); + + expect(traceDirectiveSpan).toBeDefined(); + expect(traceDirectiveSpan).toEqual( + expect.objectContaining({ + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'ui.angular.init', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_directive', + }, + description: '', + op: 'ui.angular.init', + origin: 'auto.ui.angular.trace_directive', + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + }), + ); + }); +}); + +test.describe('TraceClassDecorator', () => { + test('adds init span for decorated class', async ({ page }) => { + const navigationTxnPromise = waitForTransaction('angular-17', async transactionEvent => { + return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation'; + }); + + await page.goto(`/`); + + // immediately navigate to a different route + const [_, navigationTxn] = await Promise.all([page.locator('#componentTracking').click(), navigationTxnPromise]); + + const classDecoratorSpan = navigationTxn.spans?.find( + span => span?.data && span?.data[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] === 'auto.ui.angular.trace_class_decorator', + ); + + expect(classDecoratorSpan).toBeDefined(); + expect(classDecoratorSpan).toEqual( + expect.objectContaining({ + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'ui.angular.init', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_class_decorator', + }, + // todo: right now, it shows the minified version of the component name - we will add a name input to the Decorator + description: expect.any(String), + op: 'ui.angular.init', + origin: 'auto.ui.angular.trace_class_decorator', + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + }), + ); + }); +}); + +test.describe('TraceMethodDecorator', () => { + test('instruments decorated methods (`ngOnInit` and `ngAfterViewInit`)', async ({ page }) => { + const navigationTxnPromise = waitForTransaction('angular-17', async transactionEvent => { + return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation'; + }); + + await page.goto(`/`); + + // immediately navigate to a different route + const [_, navigationTxn] = await Promise.all([page.locator('#componentTracking').click(), navigationTxnPromise]); + + const ngInitSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.ngOnInit'); + const ngAfterViewInitSpan = navigationTxn.spans?.find(span => span.op === 'ui.angular.ngAfterViewInit'); + + expect(ngInitSpan).toBeDefined(); + expect(ngInitSpan).toEqual( + expect.objectContaining({ + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'ui.angular.ngOnInit', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_method_decorator', + }, + // todo: right now, it shows the minified version of the component name - we will add a name input to the Decorator + description: expect.any(String), + op: 'ui.angular.ngOnInit', + origin: 'auto.ui.angular.trace_method_decorator', + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + }), + ); + + expect(ngAfterViewInitSpan).toBeDefined(); + expect(ngAfterViewInitSpan).toEqual( + expect.objectContaining({ + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'ui.angular.ngAfterViewInit', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_method_decorator', + }, + // todo: right now, it shows the minified version of the component name - we will add a name input to the Decorator + description: expect.any(String), + op: 'ui.angular.ngAfterViewInit', + origin: 'auto.ui.angular.trace_method_decorator', + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + }), + ); + }); +}); diff --git a/packages/angular/src/index.ts b/packages/angular/src/index.ts index a2b1195c4e3c..a51191a443a3 100644 --- a/packages/angular/src/index.ts +++ b/packages/angular/src/index.ts @@ -7,10 +7,6 @@ export { createErrorHandler, SentryErrorHandler } from './errorhandler'; export { // eslint-disable-next-line deprecation/deprecation getActiveTransaction, - // eslint-disable-next-line deprecation/deprecation - instrumentAngularRouting, // new name - // eslint-disable-next-line deprecation/deprecation - routingInstrumentation, // legacy name browserTracingIntegration, TraceClassDecorator, TraceMethodDecorator, diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index f18295e049a2..bea704930d43 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -1,4 +1,3 @@ -/* eslint-disable max-lines */ import type { AfterViewInit, OnDestroy, OnInit } from '@angular/core'; import { Directive, Injectable, Input, NgModule } from '@angular/core'; import type { ActivatedRouteSnapshot, Event, RouterState } from '@angular/router'; @@ -10,13 +9,12 @@ import { NavigationEnd, NavigationStart, ResolveEnd } from '@angular/router'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - WINDOW, browserTracingIntegration as originalBrowserTracingIntegration, getCurrentScope, startBrowserTracingNavigationSpan, } from '@sentry/browser'; import { getActiveSpan, getClient, getRootSpan, spanToJSON, startInactiveSpan } from '@sentry/core'; -import type { Integration, Span, Transaction, TransactionContext } from '@sentry/types'; +import type { Integration, Span, Transaction } from '@sentry/types'; import { logger, stripUrlQueryAndFragment, timestampInSeconds } from '@sentry/utils'; import type { Observable } from 'rxjs'; import { Subscription } from 'rxjs'; @@ -27,44 +25,6 @@ import { IS_DEBUG_BUILD } from './flags'; import { runOutsideAngular } from './zone'; let instrumentationInitialized: boolean; -let stashedStartTransaction: (context: TransactionContext) => Transaction | undefined; -let stashedStartTransactionOnLocationChange: boolean; - -let hooksBasedInstrumentation = false; - -/** - * Creates routing instrumentation for Angular Router. - * - * @deprecated Use `browserTracingIntegration()` instead, which includes Angular-specific instrumentation out of the box. - */ -export function routingInstrumentation( - customStartTransaction: (context: TransactionContext) => Transaction | undefined, - startTransactionOnPageLoad: boolean = true, - startTransactionOnLocationChange: boolean = true, -): void { - instrumentationInitialized = true; - stashedStartTransaction = customStartTransaction; - stashedStartTransactionOnLocationChange = startTransactionOnLocationChange; - - if (startTransactionOnPageLoad && WINDOW && WINDOW.location) { - customStartTransaction({ - name: WINDOW.location.pathname, - op: 'pageload', - origin: 'auto.pageload.angular', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - }, - }); - } -} - -/** - * Creates routing instrumentation for Angular Router. - * - * @deprecated Use `browserTracingIntegration()` instead, which includes Angular-specific instrumentation out of the box. - */ -// eslint-disable-next-line deprecation/deprecation -export const instrumentAngularRouting = routingInstrumentation; /** * A custom browser tracing integration for Angular. @@ -78,7 +38,6 @@ export function browserTracingIntegration( // That way, the TraceService will not actually do anything, functionally disabling this. if (options.instrumentNavigation !== false) { instrumentationInitialized = true; - hooksBasedInstrumentation = true; } return originalBrowserTracingIntegration({ @@ -87,6 +46,19 @@ export function browserTracingIntegration( }); } +/** + * This function is extracted to make unit testing easier. + */ +export function _updateSpanAttributesForParametrizedUrl(route: string, span?: Span): void { + const attributes = (span && spanToJSON(span).data) || {}; + + if (span && attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'url') { + span.updateName(route); + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, `auto.${spanToJSON(span).op}.angular`); + } +} + /** * Grabs active transaction off scope. * @@ -120,7 +92,7 @@ export class TraceService implements OnDestroy { const client = getClient(); const strippedUrl = stripUrlQueryAndFragment(navigationEvent.url); - if (client && hooksBasedInstrumentation) { + if (client) { // see comment in `_isPageloadOngoing` for rationale if (!this._isPageloadOngoing()) { startBrowserTracingNavigationSpan(client, { @@ -153,35 +125,6 @@ export class TraceService implements OnDestroy { return; } - - // eslint-disable-next-line deprecation/deprecation - let activeTransaction = getActiveTransaction(); - - if (!activeTransaction && stashedStartTransactionOnLocationChange) { - activeTransaction = stashedStartTransaction({ - name: strippedUrl, - op: 'navigation', - origin: 'auto.navigation.angular', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - }, - }); - } - - if (activeTransaction) { - // eslint-disable-next-line deprecation/deprecation - this._routingSpan = activeTransaction.startChild({ - name: `${navigationEvent.url}`, - op: ANGULAR_ROUTING_OP, - origin: 'auto.ui.angular', - attributes: { - url: strippedUrl, - ...(navigationEvent.navigationTrigger && { - navigationTrigger: navigationEvent.navigationTrigger, - }), - }, - }); - } }), ); @@ -200,15 +143,11 @@ export class TraceService implements OnDestroy { (event.state as unknown as RouterState & { root: ActivatedRouteSnapshot }).root, ); - // eslint-disable-next-line deprecation/deprecation - const transaction = getActiveTransaction(); + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan && getRootSpan(activeSpan); + // TODO (v8 / #5416): revisit the source condition. Do we want to make the parameterized route the default? - const attributes = (transaction && spanToJSON(transaction).data) || {}; - if (transaction && attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'url') { - transaction.updateName(route); - transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); - transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, `auto.${spanToJSON(transaction).op}.angular`); - } + _updateSpanAttributesForParametrizedUrl(route, rootSpan); }), ); @@ -274,7 +213,7 @@ export class TraceService implements OnDestroy { * - if `_pageloadOngoing` is already `false`, create a navigation root span * - if there's no active/pageload root span, create a navigation root span * - only if there's an ongoing pageload root span AND `_pageloadOngoing` is still `true, - * con't create a navigation root span + * don't create a navigation root span */ private _isPageloadOngoing(): boolean { if (!this._pageloadOngoing) { @@ -315,14 +254,11 @@ export class TraceDirective implements OnInit, AfterViewInit { this.componentName = UNKNOWN_COMPONENT; } - // eslint-disable-next-line deprecation/deprecation - const activeTransaction = getActiveTransaction(); - if (activeTransaction) { - // eslint-disable-next-line deprecation/deprecation - this._tracingSpan = activeTransaction.startChild({ + if (getActiveSpan()) { + this._tracingSpan = startInactiveSpan({ name: `<${this.componentName}>`, op: ANGULAR_INIT_OP, - origin: 'auto.ui.angular.trace_directive', + attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.angular.trace_directive' }, }); } } diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index f48de0043fc6..f41b40750b81 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -1,30 +1,15 @@ -import { Component } from '@angular/core'; -import type { ActivatedRouteSnapshot, CanActivate, RouterStateSnapshot } from '@angular/router'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core'; - -import { TraceClassDecorator, TraceDirective, TraceMethodDecorator, instrumentAngularRouting } from '../src'; -import { getParameterizedRouteFromSnapshot } from '../src/tracing'; -import { AppComponent, TestEnv } from './utils/index'; +import type { ActivatedRouteSnapshot } from '@angular/router'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, + SentrySpan, + spanToJSON, +} from '@sentry/core'; +import { TraceDirective, browserTracingIntegration, init } from '../src/index'; +import { _updateSpanAttributesForParametrizedUrl, getParameterizedRouteFromSnapshot } from '../src/tracing'; let transaction: any; -const defaultStartTransaction = (ctx: any) => { - transaction = { - ...ctx, - updateName: jest.fn(name => (transaction.name = name)), - setAttribute: jest.fn(), - getSpanJSON: () => ({ - data: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', - ...ctx.data, - ...ctx.attributes, - }, - }), - }; - - return transaction; -}; - jest.mock('@sentry/browser', () => { const original = jest.requireActual('@sentry/browser'); return { @@ -39,27 +24,18 @@ jest.mock('@sentry/browser', () => { }; }); +describe('browserTracingIntegration', () => { + it('implements required hooks', () => { + const integration = browserTracingIntegration(); + expect(integration.name).toEqual('BrowserTracing'); + }); +}); + describe('Angular Tracing', () => { beforeEach(() => { transaction = undefined; }); - /* eslint-disable deprecation/deprecation */ - describe('instrumentAngularRouting', () => { - it('should attach the transaction source on the pageload transaction', () => { - const startTransaction = jest.fn(); - instrumentAngularRouting(startTransaction); - - expect(startTransaction).toHaveBeenCalledWith({ - name: '/', - op: 'pageload', - origin: 'auto.pageload.angular', - attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' }, - }); - }); - }); - /* eslint-enable deprecation/deprecation */ - describe('getParameterizedRouteFromSnapshot', () => { it.each([ ['returns `/` if the route has no children', {}, '/'], @@ -113,240 +89,44 @@ describe('Angular Tracing', () => { }); describe('TraceService', () => { - it('does not change the transaction name if the source is something other than `url`', async () => { - const customStartTransaction = jest.fn((ctx: any) => { - transaction = { - ...ctx, - getSpanJSON: () => ({ - data: { - ...ctx.data, - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', - }, - }), - metadata: ctx.metadata, - updateName: jest.fn(name => (transaction.name = name)), - setAttribute: jest.fn(), - }; - - return transaction; - }); - - const env = await TestEnv.setup({ - customStartTransaction, - routes: [ - { - path: '', - component: AppComponent, - }, - ], - }); - - const url = '/'; - - await env.navigateInAngular(url); - - expect(customStartTransaction).toHaveBeenCalledWith({ - name: url, - op: 'pageload', - origin: 'auto.pageload.angular', - attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' }, - }); - - expect(transaction.updateName).toHaveBeenCalledTimes(0); - expect(transaction.name).toEqual(url); - expect(spanToJSON(transaction).data).toEqual({ [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' }); - - env.destroy(); - }); - - it('re-assigns routing span on navigation start with active transaction.', async () => { - const customStartTransaction = jest.fn(defaultStartTransaction); - - const env = await TestEnv.setup({ - customStartTransaction, - }); - - const finishMock = jest.fn(); - transaction.startChild = jest.fn(() => ({ - end: finishMock, - })); - - await env.navigateInAngular('/'); - - expect(finishMock).toHaveBeenCalledTimes(1); + it('change the span name to route name if the the source is `url`', async () => { + init({ integrations: [browserTracingIntegration()] }); - env.destroy(); - }); - - it('finishes routing span on navigation end', async () => { - const customStartTransaction = jest.fn(defaultStartTransaction); - - const env = await TestEnv.setup({ - customStartTransaction, - }); - - const finishMock = jest.fn(); - transaction.startChild = jest.fn(() => ({ - end: finishMock, - })); - - await env.navigateInAngular('/'); - - expect(finishMock).toHaveBeenCalledTimes(1); - - 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(() => ({ - end: finishMock, - })); - - await env.navigateInAngular('/somewhere'); + const route = 'sample-route'; + const span = new SentrySpan({ name: 'initial-span-name' }); + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url'); - expect(finishMock).toHaveBeenCalledTimes(1); + _updateSpanAttributesForParametrizedUrl(route, span); - 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], + expect(spanToJSON(span)).toEqual( + expect.objectContaining({ + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.undefined.angular', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', }, - ], - useTraceService: true, - additionalProviders: [{ provide: CanActivateGuard, useClass: CanActivateGuard }], - }); - - const finishMock = jest.fn(); - transaction.startChild = jest.fn(() => ({ - end: finishMock, - })); - - await env.navigateInAngular('/cancel'); - - expect(finishMock).toHaveBeenCalledTimes(1); - - env.destroy(); + description: route, + }), + ); }); - describe('URL parameterization', () => { - it.each([ - [ - 'handles the root URL correctly', - '/', - '/', - [ - { - path: '', - component: AppComponent, - }, - ], - ], - [ - 'does not alter static routes', - '/books', - '/books/', - [ - { - path: 'books', - component: AppComponent, - }, - ], - ], - [ - 'parameterizes IDs in the URL', - '/books/1/details', - '/books/:bookId/details/', - [ - { - path: 'books/:bookId/details', - component: AppComponent, - }, - ], - ], - [ - 'parameterizes multiple IDs in the URL', - '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31', - '/org/:orgId/projects/:projId/events/:eventId/', - [ - { - path: 'org/:orgId/projects/:projId/events/:eventId', - component: AppComponent, - }, - ], - ], - [ - 'parameterizes URLs from route with child routes', - '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31', - '/org/:orgId/projects/:projId/events/:eventId/', - [ - { - path: 'org/:orgId', - component: AppComponent, - children: [ - { - path: 'projects/:projId', - component: AppComponent, - children: [ - { - path: 'events/:eventId', - component: AppComponent, - }, - ], - }, - ], - }, - ], - ], - ])('%s and sets the source to `route`', async (_, url, result, routes) => { - const customStartTransaction = jest.fn(defaultStartTransaction); - const env = await TestEnv.setup({ - customStartTransaction, - routes, - startTransactionOnPageLoad: false, - }); + it('does not change the span name if the source is something other than `url`', async () => { + init({ integrations: [browserTracingIntegration()] }); - await env.navigateInAngular(url); + const route = 'sample-route'; + const span = new SentrySpan({ name: 'initial-span-name' }); + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'sample-source'); - expect(customStartTransaction).toHaveBeenCalledWith({ - name: url, - op: 'navigation', - origin: 'auto.navigation.angular', - attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' }, - }); - expect(transaction.updateName).toHaveBeenCalledWith(result); - expect(transaction.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); + _updateSpanAttributesForParametrizedUrl(route, span); - env.destroy(); - }); + expect(spanToJSON(span)).toEqual( + expect.objectContaining({ + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'sample-source', + }, + description: 'initial-span-name', + }), + ); }); }); @@ -355,196 +135,5 @@ describe('Angular Tracing', () => { const directive = new TraceDirective(); expect(directive).toBeTruthy(); }); - - it('should create a child tracingSpan on init', async () => { - const directive = new TraceDirective(); - const customStartTransaction = jest.fn(defaultStartTransaction); - - const env = await TestEnv.setup({ - components: [TraceDirective], - customStartTransaction, - useTraceService: false, - }); - - transaction.startChild = jest.fn(); - - directive.ngOnInit(); - - expect(transaction.startChild).toHaveBeenCalledWith({ - op: 'ui.angular.init', - origin: 'auto.ui.angular.trace_directive', - name: '', - }); - - env.destroy(); - }); - - it('should use component name as span name', async () => { - const directive = new TraceDirective(); - const finishMock = jest.fn(); - const customStartTransaction = jest.fn(defaultStartTransaction); - - const env = await TestEnv.setup({ - components: [TraceDirective], - customStartTransaction, - useTraceService: false, - }); - - transaction.startChild = jest.fn(() => ({ - end: finishMock, - })); - - directive.componentName = 'test-component'; - directive.ngOnInit(); - - expect(transaction.startChild).toHaveBeenCalledWith({ - op: 'ui.angular.init', - origin: 'auto.ui.angular.trace_directive', - name: '', - }); - - env.destroy(); - }); - - it('should finish tracingSpan after view init', async () => { - const directive = new TraceDirective(); - const finishMock = jest.fn(); - const customStartTransaction = jest.fn(defaultStartTransaction); - - const env = await TestEnv.setup({ - components: [TraceDirective], - customStartTransaction, - useTraceService: false, - }); - - transaction.startChild = jest.fn(() => ({ - end: finishMock, - })); - - directive.ngOnInit(); - directive.ngAfterViewInit(); - - expect(finishMock).toHaveBeenCalledTimes(1); - - env.destroy(); - }); - }); - - describe('TraceClassDecorator', () => { - const origNgOnInitMock = jest.fn(); - const origNgAfterViewInitMock = jest.fn(); - - @Component({ - selector: 'layout-header', - template: '', - }) - @TraceClassDecorator() - class DecoratedComponent { - public ngOnInit() { - origNgOnInitMock(); - } - - public ngAfterViewInit() { - origNgAfterViewInitMock(); - } - } - - it('Instruments `ngOnInit` and `ngAfterViewInit` methods of the decorated class', async () => { - const finishMock = jest.fn(); - const startChildMock = jest.fn(() => ({ - end: finishMock, - })); - - const customStartTransaction = jest.fn((ctx: any) => { - transaction = { - ...ctx, - startChild: startChildMock, - }; - - return transaction; - }); - - const env = await TestEnv.setup({ - customStartTransaction, - components: [DecoratedComponent], - defaultComponent: DecoratedComponent, - useTraceService: false, - }); - - expect(transaction.startChild).toHaveBeenCalledWith({ - name: '', - op: 'ui.angular.init', - origin: 'auto.ui.angular.trace_class_decorator', - }); - - expect(origNgOnInitMock).toHaveBeenCalledTimes(1); - expect(origNgAfterViewInitMock).toHaveBeenCalledTimes(1); - expect(finishMock).toHaveBeenCalledTimes(1); - - env.destroy(); - }); - }); - - describe('TraceMethodDecorator', () => { - const origNgOnInitMock = jest.fn(); - const origNgAfterViewInitMock = jest.fn(); - - @Component({ - selector: 'layout-header', - template: '', - }) - class DecoratedComponent { - @TraceMethodDecorator() - public ngOnInit() { - origNgOnInitMock(); - } - - @TraceMethodDecorator() - public ngAfterViewInit() { - origNgAfterViewInitMock(); - } - } - - it('Instruments `ngOnInit` and `ngAfterViewInit` methods of the decorated class', async () => { - const startChildMock = jest.fn(); - - const customStartTransaction = jest.fn((ctx: any) => { - transaction = { - ...ctx, - startChild: startChildMock, - }; - - return transaction; - }); - - const env = await TestEnv.setup({ - customStartTransaction, - components: [DecoratedComponent], - defaultComponent: DecoratedComponent, - useTraceService: false, - }); - - expect(transaction.startChild).toHaveBeenCalledTimes(2); - expect(transaction.startChild.mock.calls[0][0]).toEqual({ - name: '', - op: 'ui.angular.ngOnInit', - origin: 'auto.ui.angular.trace_method_decorator', - startTimestamp: expect.any(Number), - endTimestamp: expect.any(Number), - }); - - expect(transaction.startChild.mock.calls[1][0]).toEqual({ - name: '', - op: 'ui.angular.ngAfterViewInit', - origin: 'auto.ui.angular.trace_method_decorator', - startTimestamp: expect.any(Number), - endTimestamp: expect.any(Number), - }); - - expect(origNgOnInitMock).toHaveBeenCalledTimes(1); - expect(origNgAfterViewInitMock).toHaveBeenCalledTimes(1); - - env.destroy(); - }); }); }); diff --git a/packages/angular/test/utils/index.ts b/packages/angular/test/utils/index.ts deleted file mode 100644 index 390d7fbe14ac..000000000000 --- a/packages/angular/test/utils/index.ts +++ /dev/null @@ -1,109 +0,0 @@ -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'; -import { Router } from '@angular/router'; -import { RouterTestingModule } from '@angular/router/testing'; -import type { Transaction } from '@sentry/types'; - -import { TraceService, instrumentAngularRouting } from '../../src'; - -@Component({ - template: '', -}) -export class AppComponent {} - -@NgModule({ - providers: [ - { - provide: TraceService, - deps: [Router], - }, - ], -}) -export class AppModule {} - -const defaultRoutes = [ - { - path: '', - component: AppComponent, - }, -]; - -export class TestEnv { - constructor( - public router: Router, - public fixture: ComponentFixture, - public traceService: TraceService | null, - ) { - fixture.detectChanges(); - } - - public static async setup(conf: { - routes?: Routes; - components?: any[]; - defaultComponent?: any; - customStartTransaction?: (context: any) => Transaction | undefined; - startTransactionOnPageLoad?: boolean; - startTransactionOnNavigation?: boolean; - useTraceService?: boolean; - additionalProviders?: Provider[]; - }): Promise { - // eslint-disable-next-line deprecation/deprecation - instrumentAngularRouting( - conf.customStartTransaction || jest.fn(), - conf.startTransactionOnPageLoad !== undefined ? conf.startTransactionOnPageLoad : true, - conf.startTransactionOnNavigation !== undefined ? conf.startTransactionOnNavigation : true, - ); - - const useTraceService = conf.useTraceService !== undefined ? conf.useTraceService : true; - const routes = conf.routes === undefined ? defaultRoutes : conf.routes; - - TestBed.configureTestingModule({ - imports: [AppModule, RouterTestingModule.withRoutes(routes)], - declarations: [...(conf.components || []), AppComponent], - providers: (useTraceService - ? [ - { - provide: TraceService, - deps: [Router], - }, - ...(conf.additionalProviders || []), - ] - : [] - ).concat(...(conf.additionalProviders || [])), - }); - - const router: Router = TestBed.inject(Router); - const traceService = useTraceService ? new TraceService(router) : null; - const fixture = TestBed.createComponent(conf.defaultComponent || AppComponent); - - return new TestEnv(router, fixture, traceService); - } - - 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(); - }) - .catch(() => { - this.fixture.detectChanges(); - resolve(); - }); - }); - }); - } - - public destroy(): void { - if (this.traceService) { - this.traceService.ngOnDestroy(); - } - - jest.clearAllMocks(); - } -}