-
Notifications
You must be signed in to change notification settings - Fork 11
feat: adding error handler and track directive #1127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
99388e6
76ef6f3
aa91d22
6161655
a8b308d
9ce27b3
1358760
8395555
4db25a3
8abcc0a
adf577b
42bddca
1296635
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { createServiceFactory, mockProvider } from '@ngneat/spectator/jest'; | ||
import { UserTelemetryImplService } from '../user-telemetry-impl.service'; | ||
import { TelemetryGlobalErrorHandler } from './telemetry-global-error-handler'; | ||
|
||
describe('Telemetry Global Error Handler ', () => { | ||
const createService = createServiceFactory({ | ||
service: TelemetryGlobalErrorHandler, | ||
providers: [ | ||
mockProvider(UserTelemetryImplService, { | ||
trackErrorEvent: jest.fn() | ||
}) | ||
] | ||
}); | ||
|
||
test('should delegate to telemetry provider after registration', () => { | ||
const spectator = createService(); | ||
try { | ||
spectator.service.handleError(new Error('Test error')); | ||
} catch (_) { | ||
// NoOP | ||
} | ||
|
||
expect(spectator.inject(UserTelemetryImplService).trackErrorEvent).toHaveBeenCalledWith( | ||
'Test error', | ||
expect.objectContaining({ | ||
message: 'Test error', | ||
name: 'Error', | ||
isError: true | ||
}) | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { LocationStrategy, PathLocationStrategy } from '@angular/common'; | ||
import { ErrorHandler, Injectable, Injector } from '@angular/core'; | ||
import { UserTelemetryImplService } from '../user-telemetry-impl.service'; | ||
|
||
@Injectable() | ||
export class TelemetryGlobalErrorHandler implements ErrorHandler { | ||
public constructor(private readonly injector: Injector) {} | ||
|
||
public handleError(error: Error): Error { | ||
const telemetryService = this.injector.get(UserTelemetryImplService); | ||
|
||
const location = this.injector.get(LocationStrategy); | ||
const message = error.message ?? error.toString(); | ||
const url = location instanceof PathLocationStrategy ? location.path() : ''; | ||
|
||
telemetryService.trackErrorEvent(message, { | ||
message: message, | ||
url: url, | ||
stack: error.stack, | ||
name: error.name, | ||
isError: true | ||
}); | ||
|
||
throw error; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,3 +28,8 @@ export interface UserTraits extends Dictionary<unknown> { | |
name?: string; | ||
displayName?: string; | ||
} | ||
|
||
export const enum TrackUserEventsType { | ||
Click = 'click', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to limit to these two events? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense to keep it flexible (string), but I want to expand this list after I test their behavior, how many events they generate and any perf impact. From Product/Marketing standpoint, the 4 event types we discussed previously are important. Events like MouseOver, MouseOut are not useful. |
||
ContextMenu = 'context-menu' | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import { CommonModule } from '@angular/common'; | ||
import { fakeAsync } from '@angular/core/testing'; | ||
import { createDirectiveFactory, mockProvider, SpectatorDirective } from '@ngneat/spectator/jest'; | ||
import { UserTelemetryImplService } from '../user-telemetry-impl.service'; | ||
import { TrackDirective } from './track.directive'; | ||
|
||
describe('Track directive', () => { | ||
let spectator: SpectatorDirective<TrackDirective>; | ||
|
||
const createDirective = createDirectiveFactory<TrackDirective>({ | ||
directive: TrackDirective, | ||
imports: [CommonModule], | ||
providers: [ | ||
mockProvider(UserTelemetryImplService, { | ||
trackEvent: jest.fn() | ||
}) | ||
] | ||
}); | ||
|
||
test('propagates events with default config', fakeAsync(() => { | ||
spectator = createDirective( | ||
` | ||
<div class="content" [htTrack] [htTrackLabel]="label">Test Content</div> | ||
`, | ||
{ | ||
hostProps: { | ||
events: ['click'], | ||
label: 'Content' | ||
} | ||
} | ||
); | ||
|
||
const telemetryService = spectator.inject(UserTelemetryImplService); | ||
|
||
spectator.click(spectator.element); | ||
spectator.tick(); | ||
|
||
expect(telemetryService.trackEvent).toHaveBeenCalledWith( | ||
'click: Content', | ||
expect.objectContaining({ type: 'click' }) | ||
); | ||
})); | ||
|
||
test('propagates events with custom config', fakeAsync(() => { | ||
spectator = createDirective( | ||
` | ||
<div class="content" [htTrack]="events" [htTrackLabel]="label">Test Content</div> | ||
`, | ||
{ | ||
hostProps: { | ||
events: ['mouseover'], | ||
label: 'Content' | ||
} | ||
} | ||
); | ||
|
||
const telemetryService = spectator.inject(UserTelemetryImplService); | ||
|
||
spectator.dispatchMouseEvent(spectator.element, 'mouseover'); | ||
spectator.tick(); | ||
|
||
expect(telemetryService.trackEvent).toHaveBeenCalledWith( | ||
'mouseover: Content', | ||
expect.objectContaining({ type: 'mouseover' }) | ||
); | ||
})); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import { Directive, ElementRef, Input, OnChanges, OnDestroy, OnInit } from '@angular/core'; | ||
import { fromEvent, Subscription } from 'rxjs'; | ||
import { TypedSimpleChanges } from '../../utilities/types/angular-change-object'; | ||
import { TrackUserEventsType } from '../telemetry'; | ||
import { UserTelemetryImplService } from '../user-telemetry-impl.service'; | ||
|
||
@Directive({ | ||
selector: '[htTrack]' | ||
}) | ||
export class TrackDirective implements OnInit, OnChanges, OnDestroy { | ||
@Input('htTrack') | ||
public userEvents: string[] = [TrackUserEventsType.Click]; | ||
|
||
@Input('htTrackLabel') | ||
public label?: string; | ||
|
||
private activeSubscriptions: Subscription = new Subscription(); | ||
private trackedEventLabel: string = ''; | ||
|
||
public constructor( | ||
private readonly host: ElementRef, | ||
private readonly userTelemetryImplService: UserTelemetryImplService | ||
) {} | ||
|
||
public ngOnInit(): void { | ||
this.setupListeners(); | ||
} | ||
|
||
public ngOnChanges(changes: TypedSimpleChanges<this>): void { | ||
if (changes.userEvents) { | ||
this.setupListeners(); | ||
} | ||
|
||
if (changes.label) { | ||
this.trackedEventLabel = this.label ?? (this.host.nativeElement as HTMLElement)?.tagName; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tag name doesnt seem very useful, maybe we should require label? Could make that the main arg and the event name the addtl arg since that's easier to default. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, the autotrack based telemetry tools extract certain fields from the element. I am yet to figure out the relevant ones. After I do that, I will add those fields here. Imo htTrack= "events" is more readable. You can always miss providing a directive input and angular won't complain. So let's keep the default logic. I will use a better field than just with a later PR. |
||
} | ||
} | ||
|
||
public ngOnDestroy(): void { | ||
this.clearListeners(); | ||
} | ||
|
||
private setupListeners(): void { | ||
this.clearListeners(); | ||
aaron-steinfeld marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.activeSubscriptions = new Subscription(); | ||
|
||
this.activeSubscriptions.add( | ||
...this.userEvents?.map(userEvent => | ||
fromEvent<MouseEvent>(this.host.nativeElement, userEvent).subscribe(eventObj => | ||
this.trackUserEvent(userEvent, eventObj) | ||
) | ||
) | ||
); | ||
} | ||
|
||
private clearListeners(): void { | ||
this.activeSubscriptions.unsubscribe(); | ||
} | ||
|
||
private trackUserEvent(userEvent: string, eventObj: MouseEvent): void { | ||
const targetElement = eventObj.target as HTMLElement; | ||
this.userTelemetryImplService.trackEvent(`${userEvent}: ${this.trackedEventLabel}`, { | ||
tagName: targetElement.tagName, | ||
className: targetElement.className, | ||
type: userEvent | ||
}); | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,7 @@ | ||
import { Injectable } from '@angular/core'; | ||
import { UserTraits } from './telemetry'; | ||
import { UserTelemetryHelperService } from './user-telemetry-helper.service'; | ||
|
||
@Injectable({ providedIn: 'root' }) | ||
export class UserTelemetryService { | ||
public constructor(private readonly userTelemetryHelperService: UserTelemetryHelperService) {} | ||
|
||
public initialize(userTraits: UserTraits): void { | ||
this.userTelemetryHelperService.initialize(); | ||
this.userTelemetryHelperService.identify(userTraits); | ||
} | ||
|
||
public shutdown(): void { | ||
this.userTelemetryHelperService.shutdown(); | ||
} | ||
export abstract class UserTelemetryService { | ||
public abstract initialize(): void; | ||
public abstract identify(userTraits: UserTraits): void; | ||
public abstract shutdown(): void; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be registered as part of the telemetry module, right? or intentionally keeping it as a separate registration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I need to provide it in the module. I missed it in this PR