From ecd6e1ab85b5d573dc8a5f2ca51c230bb0db9e41 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 17 Nov 2022 12:58:07 +0100 Subject: [PATCH 1/3] feat(angular): Auto-detect component name in `TraceDirective` --- packages/angular/src/tracing.ts | 39 ++++++++++++++++++++-- packages/angular/test/tracing.test.ts | 48 ++++++++++++++++++++++++--- 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index d2bf67900f3f..9647fd70fc63 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -1,5 +1,15 @@ /* eslint-disable max-lines */ -import { AfterViewInit, Directive, Injectable, Input, NgModule, OnDestroy, OnInit } from '@angular/core'; +import { + AfterViewInit, + Directive, + Injectable, + Input, + NgModule, + OnDestroy, + OnInit, + Optional, + ViewContainerRef, +} from '@angular/core'; import { ActivatedRouteSnapshot, Event, NavigationEnd, NavigationStart, ResolveEnd, Router } from '@angular/router'; import { getCurrentHub, WINDOW } from '@sentry/browser'; import { Span, Transaction, TransactionContext } from '@sentry/types'; @@ -163,13 +173,15 @@ export class TraceDirective implements OnInit, AfterViewInit { private _tracingSpan?: Span; + public constructor(@Optional() private readonly _vcRef: ViewContainerRef) {} + /** * Implementation of OnInit lifecycle method * @inheritdoc */ public ngOnInit(): void { if (!this.componentName) { - this.componentName = UNKNOWN_COMPONENT; + this.componentName = detectComponentName(this._vcRef as EnhancedVieContainerRef); } const activeTransaction = getActiveTransaction(); @@ -192,6 +204,29 @@ export class TraceDirective implements OnInit, AfterViewInit { } } +type EnhancedVieContainerRef = ViewContainerRef & { + _lContainer?: { + localName?: string; + }[][]; +}; + +/** + * Detects the angular component name from the passed ViewContainerReference. + * Specifically, it looks up the selector of the component (e.g. `app-my-component`) or + * falls back to the default component name if the selector is not available. + */ +function detectComponentName(vcRef: EnhancedVieContainerRef | undefined): string { + if (vcRef && vcRef._lContainer && vcRef._lContainer[0] && vcRef._lContainer[0][0]) { + // TODO: is this preferrable? + // Alternative: We could get the class name like so: + // const className = Object.getPrototypeOf(vcRef._lContainer[0][8]).constructor.name; + + const selectorName = vcRef._lContainer[0][0].localName; + return selectorName || UNKNOWN_COMPONENT; + } + return UNKNOWN_COMPONENT; +} + /** * A module serves as a single compilation unit for the `TraceDirective` and can be re-used by any other module. */ diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index d5a88c9bb082..d626acaadde9 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -256,12 +256,50 @@ describe('Angular Tracing', () => { describe('TraceDirective', () => { it('should create an instance', () => { - const directive = new TraceDirective(); + const directive = new TraceDirective({} as unknown as any); expect(directive).toBeTruthy(); }); + it('should create a child tracingSpan on init (WIP)', async () => { + const customStartTransaction = jest.fn(defaultStartTransaction); + + @Component({ + selector: 'app-component', + template: ``, + }) + class AppComponent { + public constructor() {} + } + + @Component({ + selector: 'app-child', + template: `

Hi

`, + }) + class ChildComponent { + public constructor() {} + } + + const env = await TestEnv.setup({ + components: [AppComponent, ChildComponent, TraceDirective], + defaultComponent: AppComponent, + customStartTransaction, + useTraceService: false, + }); + + transaction.startChild = jest.fn(); + + //directive.ngOnInit(); + + expect(transaction.startChild).toHaveBeenCalledWith({ + op: 'ui.angular.init', + description: '', + }); + + env.destroy(); + }); + it('should create a child tracingSpan on init', async () => { - const directive = new TraceDirective(); + //const directive = new TraceDirective({} as unknown as any); const customStartTransaction = jest.fn(defaultStartTransaction); const env = await TestEnv.setup({ @@ -272,7 +310,7 @@ describe('Angular Tracing', () => { transaction.startChild = jest.fn(); - directive.ngOnInit(); + //directive.ngOnInit(); expect(transaction.startChild).toHaveBeenCalledWith({ op: 'ui.angular.init', @@ -283,7 +321,7 @@ describe('Angular Tracing', () => { }); it('should use component name as span description', async () => { - const directive = new TraceDirective(); + const directive = new TraceDirective({} as unknown as any); const finishMock = jest.fn(); const customStartTransaction = jest.fn(defaultStartTransaction); @@ -309,7 +347,7 @@ describe('Angular Tracing', () => { }); it('should finish tracingSpan after view init', async () => { - const directive = new TraceDirective(); + const directive = new TraceDirective({} as unknown as any); const finishMock = jest.fn(); const customStartTransaction = jest.fn(defaultStartTransaction); From b7c87900d33bdc1a5de0d15ab17930b7e43c736e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 17 Nov 2022 14:26:20 +0100 Subject: [PATCH 2/3] wip tests (no real progress :( ) --- packages/angular/test/tracing.test.ts | 15 ++++--- packages/angular/test/utils/index.ts | 62 ++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index d626acaadde9..268b1532a081 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -4,7 +4,7 @@ import { Hub } from '@sentry/types'; import { instrumentAngularRouting, TraceClassDecorator, TraceDirective, TraceMethodDecorator } from '../src'; import { getParameterizedRouteFromSnapshot } from '../src/tracing'; -import { AppComponent, TestEnv } from './utils/index'; +import { AppComponent, TestEnv, TestViewContainerRef } from './utils/index'; let transaction: any; @@ -265,7 +265,7 @@ describe('Angular Tracing', () => { @Component({ selector: 'app-component', - template: ``, + template: '', }) class AppComponent { public constructor() {} @@ -273,7 +273,7 @@ describe('Angular Tracing', () => { @Component({ selector: 'app-child', - template: `

Hi

`, + template: '

Hi

', }) class ChildComponent { public constructor() {} @@ -288,7 +288,7 @@ describe('Angular Tracing', () => { transaction.startChild = jest.fn(); - //directive.ngOnInit(); + // directive.ngOnInit(); expect(transaction.startChild).toHaveBeenCalledWith({ op: 'ui.angular.init', @@ -299,7 +299,7 @@ describe('Angular Tracing', () => { }); it('should create a child tracingSpan on init', async () => { - //const directive = new TraceDirective({} as unknown as any); + // const directive = new TraceDirective({} as unknown as any); const customStartTransaction = jest.fn(defaultStartTransaction); const env = await TestEnv.setup({ @@ -310,7 +310,7 @@ describe('Angular Tracing', () => { transaction.startChild = jest.fn(); - //directive.ngOnInit(); + // directive.ngOnInit(); expect(transaction.startChild).toHaveBeenCalledWith({ op: 'ui.angular.init', @@ -347,7 +347,8 @@ describe('Angular Tracing', () => { }); it('should finish tracingSpan after view init', async () => { - const directive = new TraceDirective({} as unknown as any); + // @ts-ignore - we don't need to pass a param here + const directive = new TraceDirective(new TestViewContainerRef()); const finishMock = jest.fn(); const customStartTransaction = jest.fn(defaultStartTransaction); diff --git a/packages/angular/test/utils/index.ts b/packages/angular/test/utils/index.ts index 0f4ff55b0b23..cc9ecaa02f55 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, ViewContainerRef } from '@angular/core'; import { ComponentFixture, TestBed } from '@angular/core/testing'; import { Router, Routes } from '@angular/router'; import { RouterTestingModule } from '@angular/router/testing'; @@ -65,7 +65,12 @@ export class TestEnv { deps: [Router], }, ] - : [], + : [ + { + provide: ViewContainerRef, + useValue: new TestViewContainerRef(), + }, + ], }); const router: Router = TestBed.inject(Router); @@ -94,3 +99,56 @@ export class TestEnv { jest.clearAllMocks(); } } + +// create the test class +export class TestViewContainerRef extends ViewContainerRef { + get element(): import('@angular/core').ElementRef { + throw new Error('Method not implemented.'); + } + get injector(): import('@angular/core').Injector { + throw new Error('Method not implemented.'); + } + get parentInjector(): import('@angular/core').Injector { + throw new Error('Method not implemented.'); + } + clear(): void { + throw new Error('Method not implemented.'); + } + get(_index: number): import('@angular/core').ViewRef { + throw new Error('Method not implemented.'); + } + get length(): number { + throw new Error('Method not implemented.'); + } + createEmbeddedView( + _templateRef: import('@angular/core').TemplateRef, + _context?: C, + _index?: number, + ): import('@angular/core').EmbeddedViewRef { + throw new Error('Method not implemented.'); + } + createComponent( + _componentFactory: import('@angular/core').ComponentFactory, + _index?: number, + _injector?: import('@angular/core').Injector, + _projectableNodes?: any[][], + _ngModule?: import('@angular/core').NgModuleRef, + ): import('@angular/core').ComponentRef { + throw new Error('Method not implemented.'); + } + insert(_viewRef: import('@angular/core').ViewRef, _index?: number): import('@angular/core').ViewRef { + throw new Error('Method not implemented.'); + } + move(_viewRef: import('@angular/core').ViewRef, _currentIndex: number): import('@angular/core').ViewRef { + throw new Error('Method not implemented.'); + } + indexOf(_viewRef: import('@angular/core').ViewRef): number { + throw new Error('Method not implemented.'); + } + remove(_index?: number): void { + throw new Error('Method not implemented.'); + } + detach(_index?: number): import('@angular/core').ViewRef { + throw new Error('Method not implemented.'); + } +} From 6bd5dae226e8aa2975c932fb6117a9c6e4b181ab Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 22 Dec 2022 15:57:22 +0100 Subject: [PATCH 3/3] fix and adjust tests --- packages/angular/src/tracing.ts | 12 ++-- packages/angular/test/tracing.test.ts | 91 +++++++++++++++------------ packages/angular/test/utils/index.ts | 62 +----------------- 3 files changed, 58 insertions(+), 107 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 9647fd70fc63..7b9cc43818d0 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -2,6 +2,7 @@ import { AfterViewInit, Directive, + Inject, Injectable, Input, NgModule, @@ -173,7 +174,7 @@ export class TraceDirective implements OnInit, AfterViewInit { private _tracingSpan?: Span; - public constructor(@Optional() private readonly _vcRef: ViewContainerRef) {} + public constructor(@Optional() @Inject(ViewContainerRef) private readonly _vcRef: ViewContainerRef) {} /** * Implementation of OnInit lifecycle method @@ -181,7 +182,7 @@ export class TraceDirective implements OnInit, AfterViewInit { */ public ngOnInit(): void { if (!this.componentName) { - this.componentName = detectComponentName(this._vcRef as EnhancedVieContainerRef); + this.componentName = detectComponentName(this._vcRef as EnhancedViewContainerRef); } const activeTransaction = getActiveTransaction(); @@ -204,7 +205,7 @@ export class TraceDirective implements OnInit, AfterViewInit { } } -type EnhancedVieContainerRef = ViewContainerRef & { +type EnhancedViewContainerRef = ViewContainerRef & { _lContainer?: { localName?: string; }[][]; @@ -215,10 +216,9 @@ type EnhancedVieContainerRef = ViewContainerRef & { * Specifically, it looks up the selector of the component (e.g. `app-my-component`) or * falls back to the default component name if the selector is not available. */ -function detectComponentName(vcRef: EnhancedVieContainerRef | undefined): string { +function detectComponentName(vcRef: EnhancedViewContainerRef | undefined): string { if (vcRef && vcRef._lContainer && vcRef._lContainer[0] && vcRef._lContainer[0][0]) { - // TODO: is this preferrable? - // Alternative: We could get the class name like so: + // Alternatively, we could get the class name like so: // const className = Object.getPrototypeOf(vcRef._lContainer[0][8]).constructor.name; const selectorName = vcRef._lContainer[0][0].localName; diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 268b1532a081..96e580323bab 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -4,7 +4,7 @@ import { Hub } from '@sentry/types'; import { instrumentAngularRouting, TraceClassDecorator, TraceDirective, TraceMethodDecorator } from '../src'; import { getParameterizedRouteFromSnapshot } from '../src/tracing'; -import { AppComponent, TestEnv, TestViewContainerRef } from './utils/index'; +import { AppComponent, TestEnv } from './utils/index'; let transaction: any; @@ -12,6 +12,7 @@ const defaultStartTransaction = (ctx: any) => { transaction = { ...ctx, setName: jest.fn(name => (transaction.name = name)), + startChild: jest.fn(), }; return transaction; @@ -260,67 +261,76 @@ describe('Angular Tracing', () => { expect(directive).toBeTruthy(); }); - it('should create a child tracingSpan on init (WIP)', async () => { - const customStartTransaction = jest.fn(defaultStartTransaction); - - @Component({ - selector: 'app-component', - template: '', - }) - class AppComponent { - public constructor() {} - } + it("should auto detect the component's selector.", async () => { + const directive = new TraceDirective({ + _lContainer: [[{ localName: 'app-test' }]], + } as unknown as any); - @Component({ - selector: 'app-child', - template: '

Hi

', - }) - class ChildComponent { - public constructor() {} - } + const customStartTransaction = jest.fn(defaultStartTransaction); const env = await TestEnv.setup({ - components: [AppComponent, ChildComponent, TraceDirective], - defaultComponent: AppComponent, + components: [TraceDirective], customStartTransaction, useTraceService: false, }); transaction.startChild = jest.fn(); - // directive.ngOnInit(); + directive.ngOnInit(); expect(transaction.startChild).toHaveBeenCalledWith({ op: 'ui.angular.init', - description: '', + description: '', }); env.destroy(); }); - it('should create a child tracingSpan on init', async () => { - // const directive = new TraceDirective({} as unknown as any); - const customStartTransaction = jest.fn(defaultStartTransaction); + it.each([ + {}, + { + _lContainer: [], + }, + { + _lContainer: [[]], + }, + { + _lContainer: [[{}]], + }, + { + _lContainer: [[{ localName: undefined }]], + }, + ])( + "should fall back to the default component name if auto-detection doesn't work and no custom name is given", + async (containerViewRef: any) => { + const directive = new TraceDirective(containerViewRef); + const customStartTransaction = jest.fn(defaultStartTransaction); - const env = await TestEnv.setup({ - components: [TraceDirective], - customStartTransaction, - useTraceService: false, - }); + const env = await TestEnv.setup({ + components: [TraceDirective], + customStartTransaction, + useTraceService: false, + }); - transaction.startChild = jest.fn(); + const finishSpan = jest.fn(); + transaction.startChild = jest.fn().mockReturnValue({ finish: finishSpan }); + transaction.finish = jest.fn(); - // directive.ngOnInit(); + directive.ngOnInit(); - expect(transaction.startChild).toHaveBeenCalledWith({ - op: 'ui.angular.init', - description: '', - }); + expect(transaction.startChild).toHaveBeenCalledWith({ + op: 'ui.angular.init', + description: '', + }); - env.destroy(); - }); + directive.ngAfterViewInit(); + expect(finishSpan).toHaveBeenCalledTimes(1); - it('should use component name as span description', async () => { + env.destroy(); + }, + ); + + it('should use the custom component name as span description if one is passed', async () => { const directive = new TraceDirective({} as unknown as any); const finishMock = jest.fn(); const customStartTransaction = jest.fn(defaultStartTransaction); @@ -347,8 +357,7 @@ describe('Angular Tracing', () => { }); it('should finish tracingSpan after view init', async () => { - // @ts-ignore - we don't need to pass a param here - const directive = new TraceDirective(new TestViewContainerRef()); + const directive = new TraceDirective({} as unknown as any); const finishMock = jest.fn(); const customStartTransaction = jest.fn(defaultStartTransaction); diff --git a/packages/angular/test/utils/index.ts b/packages/angular/test/utils/index.ts index cc9ecaa02f55..0f4ff55b0b23 100644 --- a/packages/angular/test/utils/index.ts +++ b/packages/angular/test/utils/index.ts @@ -1,4 +1,4 @@ -import { Component, NgModule, ViewContainerRef } from '@angular/core'; +import { Component, NgModule } from '@angular/core'; import { ComponentFixture, TestBed } from '@angular/core/testing'; import { Router, Routes } from '@angular/router'; import { RouterTestingModule } from '@angular/router/testing'; @@ -65,12 +65,7 @@ export class TestEnv { deps: [Router], }, ] - : [ - { - provide: ViewContainerRef, - useValue: new TestViewContainerRef(), - }, - ], + : [], }); const router: Router = TestBed.inject(Router); @@ -99,56 +94,3 @@ export class TestEnv { jest.clearAllMocks(); } } - -// create the test class -export class TestViewContainerRef extends ViewContainerRef { - get element(): import('@angular/core').ElementRef { - throw new Error('Method not implemented.'); - } - get injector(): import('@angular/core').Injector { - throw new Error('Method not implemented.'); - } - get parentInjector(): import('@angular/core').Injector { - throw new Error('Method not implemented.'); - } - clear(): void { - throw new Error('Method not implemented.'); - } - get(_index: number): import('@angular/core').ViewRef { - throw new Error('Method not implemented.'); - } - get length(): number { - throw new Error('Method not implemented.'); - } - createEmbeddedView( - _templateRef: import('@angular/core').TemplateRef, - _context?: C, - _index?: number, - ): import('@angular/core').EmbeddedViewRef { - throw new Error('Method not implemented.'); - } - createComponent( - _componentFactory: import('@angular/core').ComponentFactory, - _index?: number, - _injector?: import('@angular/core').Injector, - _projectableNodes?: any[][], - _ngModule?: import('@angular/core').NgModuleRef, - ): import('@angular/core').ComponentRef { - throw new Error('Method not implemented.'); - } - insert(_viewRef: import('@angular/core').ViewRef, _index?: number): import('@angular/core').ViewRef { - throw new Error('Method not implemented.'); - } - move(_viewRef: import('@angular/core').ViewRef, _currentIndex: number): import('@angular/core').ViewRef { - throw new Error('Method not implemented.'); - } - indexOf(_viewRef: import('@angular/core').ViewRef): number { - throw new Error('Method not implemented.'); - } - remove(_index?: number): void { - throw new Error('Method not implemented.'); - } - detach(_index?: number): import('@angular/core').ViewRef { - throw new Error('Method not implemented.'); - } -}