Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(Angular): Add URL Parameterization of Transaction Names#5416
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
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
17190a0
feat(angular): Add URL parameterization of transaction names
Lms24 f9ab851
add unit tests for URL parameterization
Lms24 634e906
use ResolveEnd event instead of ActivationEnd
Lms24 190963d
only update transaction name if its source is a raw URL
Lms24 3ce61c8
add test
Lms24 1395a34
fix naming typo
Lms24 23242aa
apply first batch of review suggestions
Lms24 ca02d49
fix tests
Lms24 9d5b14e
make parameter extraction and parameterization more robust
Lms24 856ba22
fix linter errors
Lms24 be90bdc
revert unintended changes
Lms24 35b9eca
adjust comment
Lms24 195fe33
apply code review suggestions
Lms24 ffaf7f1
Change parameterization approach
Lms24 a8b44b8
small test change
Lms24 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,34 @@ | ||
| import { NavigationStart, Router, RouterEvent } from '@angular/router'; | ||
| import { ActivatedRouteSnapshot, Event, NavigationStart, ResolveEnd, Router } from '@angular/router'; | ||
| import { Hub, Transaction } from '@sentry/types'; | ||
| import { Subject } from 'rxjs'; | ||
| import { instrumentAngularRouting, TraceService } from '../src/index'; | ||
| import { getParameterizedRouteFromSnapshot } from '../src/tracing'; | ||
| let transaction: any; | ||
| let customStartTransaction: (context: any) => Transaction | undefined; | ||
| jest.mock('@sentry/browser', () => { | ||
| const original = jest.requireActual('@sentry/browser'); | ||
| return { | ||
| ...original, | ||
| getCurrentHub: () => { | ||
| return { | ||
| getScope: () => { | ||
| return { | ||
| getTransaction: () => { | ||
| return transaction; | ||
| }, | ||
| }; | ||
| }, | ||
| } as unknown as Hub; | ||
| }, | ||
| }; | ||
| }); | ||
| describe('Angular Tracing', () => { | ||
| const startTransaction = jest.fn(); | ||
| describe('instrumentAngularRouting', () => { | ||
| it('should attach the transaction source on the pageload transaction', () => { | ||
| instrumentAngularRouting(startTransaction); | ||
| @@ -18,15 +42,15 @@ describe('Angular Tracing', () => { | ||
| describe('TraceService', () => { | ||
| let traceService: TraceService; | ||
| const routerEvents$: Subject<RouterEvent> = new Subject(); | ||
| const routerEvents$: Subject<Event> = new Subject(); | ||
| const mockedRouter: Partial<Router> = { | ||
| events: routerEvents$, | ||
| }; | ||
| beforeAll(() => instrumentAngularRouting(startTransaction)); | ||
| beforeEach(() => { | ||
| instrumentAngularRouting(startTransaction); | ||
| jest.resetAllMocks(); | ||
| traceService = new TraceService({ | ||
| events: routerEvents$, | ||
| } as unknown as Router); | ||
| traceService = new TraceService(mockedRouter as Router); | ||
| }); | ||
| afterEach(() => { | ||
| @@ -43,5 +67,154 @@ describe('Angular Tracing', () => { | ||
| metadata: { source: 'url' }, | ||
| }); | ||
| }); | ||
| describe('URL parameterization', () => { | ||
| // TODO: These tests are real unit tests in the sense that they only test TraceService | ||
| // and we essentially just simulate a router navigation by firing the respective | ||
| // routing events and providing the raw URL + the resolved route parameters. | ||
| // In the future we should add more "wholesome" tests that let the Angular router | ||
| // do its thing (e.g. by calling router.navigate) and we check how our service | ||
| // reacts to it. | ||
| // Once we set up Jest for testing Angular, we can use TestBed to inject an actual | ||
| // router instance into TraceService and add more tests. | ||
| beforeEach(() => { | ||
| transaction = undefined; | ||
| customStartTransaction = jest.fn((ctx: any) => { | ||
| transaction = { | ||
| ...ctx, | ||
| setName: jest.fn(name => (transaction.name = name)), | ||
| setMetadata: jest.fn(metadata => (transaction.metadata = metadata)), | ||
| }; | ||
| return transaction; | ||
| }); | ||
| }); | ||
| it.each([ | ||
| [ | ||
| 'handles the root URL correctly', | ||
| '', | ||
| { | ||
| root: { firstChild: { routeConfig: null } }, | ||
| }, | ||
| '/', | ||
| ], | ||
| [ | ||
| 'does not alter static routes', | ||
| '/books/', | ||
| { | ||
| root: { firstChild: { routeConfig: { path: 'books' } } }, | ||
| }, | ||
| '/books/', | ||
| ], | ||
| [ | ||
| 'parameterizes IDs in the URL', | ||
| '/books/1/details', | ||
| { | ||
| root: { firstChild: { routeConfig: { path: 'books/:bookId/details' } } }, | ||
| }, | ||
| '/books/:bookId/details/', | ||
| ], | ||
| [ | ||
| 'parameterizes multiple IDs in the URL', | ||
| '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31', | ||
| { | ||
| root: { firstChild: { routeConfig: { path: 'org/:orgId/projects/:projId/events/:eventId' } } }, | ||
| }, | ||
| '/org/:orgId/projects/:projId/events/:eventId/', | ||
| ], | ||
| [ | ||
| 'parameterizes URLs from route with child routes', | ||
| '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31', | ||
| { | ||
| root: { | ||
| firstChild: { | ||
| routeConfig: { path: 'org/:orgId' }, | ||
| firstChild: { | ||
| routeConfig: { path: 'projects/:projId' }, | ||
| firstChild: { routeConfig: { path: 'events/:eventId' } }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| '/org/:orgId/projects/:projId/events/:eventId/', | ||
| ], | ||
| ])('%s and sets the source to `route`', (_, url, routerState, result) => { | ||
| instrumentAngularRouting(customStartTransaction, false, true); | ||
| // this event starts the transaction | ||
| routerEvents$.next(new NavigationStart(0, url)); | ||
| expect(customStartTransaction).toHaveBeenCalledWith({ | ||
| name: url, | ||
| op: 'navigation', | ||
| metadata: { source: 'url' }, | ||
| }); | ||
| // this event starts the parameterization | ||
| routerEvents$.next(new ResolveEnd(1, url, url, routerState as any)); | ||
| expect(transaction.setName).toHaveBeenCalledWith(result); | ||
| expect(transaction.setMetadata).toHaveBeenCalledWith({ source: 'route' }); | ||
| }); | ||
| it('does not change the transaction name if the source is something other than `url`', () => { | ||
| instrumentAngularRouting(customStartTransaction, false, true); | ||
| const url = '/user/12345/test'; | ||
| routerEvents$.next(new NavigationStart(0, url)); | ||
| expect(customStartTransaction).toHaveBeenCalledWith({ | ||
| name: url, | ||
| op: 'navigation', | ||
| metadata: { source: 'url' }, | ||
| }); | ||
| // Simulate that this transaction has a custom name: | ||
| transaction.metadata.source = 'custom'; | ||
| // this event starts the parameterization | ||
| routerEvents$.next( | ||
| new ResolveEnd(1, url, url, { | ||
| root: { firstChild: { routeConfig: { path: 'org/:orgId/projects/:projId/events/:eventId' } } }, | ||
| } as any), | ||
| ); | ||
| expect(transaction.setName).toHaveBeenCalledTimes(0); | ||
| expect(transaction.setMetadata).toHaveBeenCalledTimes(0); | ||
| expect(transaction.name).toEqual(url); | ||
| }); | ||
| }); | ||
| }); | ||
| describe('getParameterizedRouteFromSnapshot', () => { | ||
| it.each([ | ||
| ['returns `/` empty object if the route no children', {}, '/'], | ||
| [ | ||
| 'returns the route of a snapshot without children', | ||
| { | ||
| firstChild: { routeConfig: { path: 'users/:id' } }, | ||
| }, | ||
| '/users/:id/', | ||
| ], | ||
| [ | ||
| 'returns the complete route of a snapshot with children', | ||
| { | ||
| firstChild: { | ||
| routeConfig: { path: 'orgs/:orgId' }, | ||
| firstChild: { | ||
| routeConfig: { path: 'projects/:projId' }, | ||
Lms24 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| firstChild: { routeConfig: { path: 'overview' } }, | ||
| }, | ||
| }, | ||
| }, | ||
| '/orgs/:orgId/projects/:projId/overview/', | ||
| ], | ||
| ])('%s', (_, routeSnapshot, expectedParams) => { | ||
| expect(getParameterizedRouteFromSnapshot(routeSnapshot as unknown as ActivatedRouteSnapshot)).toEqual( | ||
| expectedParams, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.