Skip to content
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

feat(vue): Add transaction source to VueRouter instrumentation #5381

Merged
merged 4 commits into from
Jul 8, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 40 additions & 14 deletions packages/vue/src/router.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import { captureException } from '@sentry/browser';
import { Transaction, TransactionContext } from '@sentry/types';

Expand All @@ -8,14 +7,24 @@ export type VueRouterInstrumentation = <T extends Transaction>(
startTransactionOnLocationChange?: boolean,
) => void;

// This is not great, but kinda necessary to make it work with VueRouter@3 and VueRouter@4 at the same time.
type Route = {
params: any;
query: any;
name?: any;
path: any;
matched: any[];
// The following type is an intersection of the Route type from VueRouter v2, v3, and v4.
// This is not great, but kinda necessary to make it work with all versions at the same time.
export type Route = {
/** Unparameterized URL */
path: string;
/**
* Query params (keys map to null when there is no value associated, e.g. "?foo" and to an array when there are
* multiple query params that have the same key, e.g. "?foo&foo=bar")
*/
query: Record<string, string | null | (string | null)[]>;
/** Route name (VueRouter provides a way to give routes individual names) */
name?: string | symbol | null | undefined;
/** Evaluated parameters */
params: Record<string, string | string[]>;
/** All the matched route objects as defined in VueRouter constructor */
matched: { path: string }[];
};

interface VueRouter {
onError: (fn: (err: Error) => void) => void;
beforeEach: (fn: (to: Route, from: Route, next: () => void) => void) => void;
Expand All @@ -39,8 +48,10 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument
// https://router.vuejs.org/api/#router-start-location
// https://next.router.vuejs.org/api/#start-location

// Vue2 - null
// Vue3 - undefined
// from.name:
// - Vue 2: null
// - Vue 3: undefined
// hence only '==' instead of '===', because `undefined == null` evaluates to `true`
const isPageLoadNavigation = from.name == null && from.matched.length === 0;

const tags = {
Expand All @@ -51,23 +62,38 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument
query: to.query,
};

// Determine a name for the routing transaction and where that name came from
let transactionName: string = to.path;
let transactionSource: 'url' | 'custom' | 'route' = 'url';
Copy link
Member

@Lms24 Lms24 Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally optional but wdyt about extracting'url' | 'custom' | 'route' to a type like in the ReactRouterV3 implementation? Just for consistency...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, at the time of opening this I didn't see that we exported the TransactionSource type. Thanks!

if (to.name) {
transactionName = to.name.toString();
transactionSource = 'custom';
} else if (to.matched[0] && to.matched[0].path) {
transactionName = to.matched[0].path;
transactionSource = 'route';
}

if (startTransactionOnPageLoad && isPageLoadNavigation) {
startTransaction({
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
name: (to.name && to.name.toString()) || to.path,
name: transactionName,
op: 'pageload',
tags,
data,
metadata: {
source: transactionSource,
},
});
}

if (startTransactionOnLocationChange && !isPageLoadNavigation) {
startTransaction({
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
name: (to.name && to.name.toString()) || (to.matched[0] && to.matched[0].path) || to.path,
name: transactionName,
op: 'navigation',
tags,
data,
metadata: {
source: transactionSource,
},
});
}

Expand Down
2 changes: 1 addition & 1 deletion packages/vue/src/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export const createTracingMixins = (options: TracingOptions): Mixins => {
// Skip components that we don't want to track to minimize the noise and give a more granular control to the user
const name = formatComponentName(this, false);
const shouldTrack = Array.isArray(options.trackComponents)
? options.trackComponents.includes(name)
? options.trackComponents.indexOf(name) > -1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't get tests to work because there was a type error here. I could have messed with tsconfigs but there is an upside of doing indexOf() instead of includes() which is backwards compatibility. I think includes is ES6 only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, coincidence? #5389

: options.trackComponents;

// We always want to track root component
Expand Down
163 changes: 163 additions & 0 deletions packages/vue/test/router.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
import * as SentryBrowser from '@sentry/browser';

import { vueRouterInstrumentation } from '../src';
import { Route } from '../src/router';

const captureExceptionSpy = jest.spyOn(SentryBrowser, 'captureException');

const mockVueRouter = {
onError: jest.fn<void, [(error: Error) => void]>(),
beforeEach: jest.fn<void, [(from: Route, to: Route, next: () => void) => void]>(),
};

const mockStartTransaction = jest.fn();
const mockNext = jest.fn();

const testRoutes: Record<string, Route> = {
initialPageloadRoute: { matched: [], params: {}, path: '', query: {} },
normalRoute1: {
matched: [{ path: '/books/:bookId/chapter/:chapterId' }],
params: {
bookId: '12',
chapterId: '3',
},
path: '/books/12/chapter/3',
query: {
utm_source: 'google',
},
},
normalRoute2: {
matched: [{ path: '/accounts/:accountId' }],
params: {
accountId: '4',
},
path: '/accounts/4',
query: {},
},
namedRoute: {
matched: [{ path: '/login' }],
name: 'login-screen',
params: {},
path: '/login',
query: {},
},
unmatchedRoute: {
matched: [],
params: {},
path: '/e8733846-20ac-488c-9871-a5cbcb647294',
query: {},
},
};

describe('vueRouterInstrumentation()', () => {
afterEach(() => {
jest.clearAllMocks();
});

it('should return instrumentation that instruments VueRouter.onError', () => {
// create instrumentation
const instrument = vueRouterInstrumentation(mockVueRouter);

// instrument
instrument(mockStartTransaction);

// check
expect(mockVueRouter.onError).toHaveBeenCalledTimes(1);

const onErrorCallback = mockVueRouter.onError.mock.calls[0][0];

const testError = new Error();
onErrorCallback(testError);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith(testError);
});

it.each([
['initialPageloadRoute', 'normalRoute1', 'pageload', '/books/:bookId/chapter/:chapterId', 'route'],
['normalRoute1', 'normalRoute2', 'navigation', '/accounts/:accountId', 'route'],
['normalRoute2', 'namedRoute', 'navigation', 'login-screen', 'custom'],
['normalRoute2', 'unmatchedRoute', 'navigation', '/e8733846-20ac-488c-9871-a5cbcb647294', 'url'],
])(
'should return instrumentation that instruments VueRouter.beforeEach(%s, %s)',
(fromKey, toKey, op, transactionName, transactionSource) => {
// create instrumentation
const instrument = vueRouterInstrumentation(mockVueRouter);

// instrument
instrument(mockStartTransaction, true, true);

// check
expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1);
const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0];

const from = testRoutes[fromKey];
const to = testRoutes[toKey];
beforeEachCallback(to, from, mockNext);

expect(mockStartTransaction).toHaveBeenCalledTimes(1);
expect(mockStartTransaction).toHaveBeenCalledWith({
name: transactionName,
metadata: {
source: transactionSource,
},
data: {
params: to.params,
query: to.query,
},
op: op,
tags: {
'routing.instrumentation': 'vue-router',
},
});

expect(mockNext).toHaveBeenCalledTimes(1);
},
);

test.each([
[undefined, 1],
[false, 0],
[true, 1],
])(
'should return instrumentation that considers the startTransactionOnPageLoad option = %p',
(startTransactionOnPageLoad, expectedCallsAmount) => {
// create instrumentation
const instrument = vueRouterInstrumentation(mockVueRouter);

// instrument
instrument(mockStartTransaction, startTransactionOnPageLoad, true);

// check
expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1);

const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0];
beforeEachCallback(testRoutes['normalRoute1'], testRoutes['initialPageloadRoute'], mockNext);

expect(mockStartTransaction).toHaveBeenCalledTimes(expectedCallsAmount);
},
);

test.each([
[undefined, 1],
[false, 0],
[true, 1],
])(
'should return instrumentation that considers the startTransactionOnLocationChange option = %p',
(startTransactionOnLocationChange, expectedCallsAmount) => {
// create instrumentation
const instrument = vueRouterInstrumentation(mockVueRouter);

// instrument
instrument(mockStartTransaction, true, startTransactionOnLocationChange);

// check
expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1);

const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0];
beforeEachCallback(testRoutes['normalRoute2'], testRoutes['normalRoute1'], mockNext);

expect(mockStartTransaction).toHaveBeenCalledTimes(expectedCallsAmount);
},
);
});