8000 fix(vue): Start pageload transaction earlier to capture missing spans… · vaind/sentry-javascript@4080ee9 · GitHub
[go: up one dir, main page]
Skip to content

Commit 4080ee9

Browse files
Lms24Lms24
authored
fix(vue): Start pageload transaction earlier to capture missing spans (getsentry#5983)
Make the `pageload` transaction start a little earlier in the Vue routing instrumentation, allowing our SDK to add the root `ui.vue.render` span and a few missed child component spans to the now available and active transaction. Previously, in our Vue routing instrumentation, the `pageload` transaction would only be created when the `beforeEach` hook of the Vue router was called for the first time. This worked reasonably well in Vue 2 apps where this hook was called before any components were rendered. However, in Vue 3 (Vue router v3 and v4) it seems like the routing/rendering process was changed so that the root component would be rendered before the initial `beforeEach` router hook call. To be clear, this change makes the `pageload` transaction always start with a `url` transaction source (because we don't yet have a matched route at this stage). However, it is updated to a more high-quality source later on - exactly at the time where we previously used to start the transaction.
1 parent 21020d9 commit 4080ee9

File tree

3 files changed

+92
-25
lines changed

3 files changed

+92
-25
lines changed

‎packages/vue/src/router.ts

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import { captureException } from '@sentry/browser';
22
import { Transaction, TransactionContext, TransactionSource } from '@sentry/types';
3+
import { WINDOW } from '@sentry/utils';
4+
5+
import { getActiveTransaction } from './tracing';
36

47
export type VueRouterInstrumentation = <T extends Transaction>(
58
startTransaction: (context: TransactionContext) => T | undefined,
@@ -31,7 +34,7 @@ interface VueRouter {
3134
}
3235

3336
/**
34-
* Creates routing instrumentation for Vue Router v2
37+
* Creates routing instrumentation for Vue Router v2, v3 and v4
3538
*
3639
* @param router The Vue Router instance that is used
3740
*/
@@ -41,6 +44,23 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument
4144
startTransactionOnPageLoad: boolean = true,
4245
startTransactionOnLocationChange: boolean = true,
4346
) => {
47+
const tags = {
48+
'routing.instrumentation': 'vue-router',
49+
};
50+
51+
// We have to start the pageload transaction as early as possible (before the router's `beforeEach` hook
52+
// is called) to not miss child spans of the pageload.
53+
if (startTransactionOnPageLoad) {
54+
startTransaction({
55+
name: WINDOW.location.pathname,
56+
op: 'pageload',
57+
tags,
58+
metadata: {
59+
source: 'url',
60+
},
61+
});
62+
}
63+
4464
router.onError(error => captureException(error));
4565

4666
router.beforeEach((to, from, next) => {
@@ -54,9 +74,6 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument
5474
// hence only '==' instead of '===', because `undefined == null` evaluates to `true`
5575
const isPageLoadNavigation = from.name == null && from.matched.length === 0;
5676

57-
const tags = {
58-
'routing.instrumentation': 'vue-router',
59-
};
6077
const data = {
6178
params: to.params,
6279
query: to.query,
@@ -74,15 +91,12 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument
7491
}
7592

7693
if (startTransactionOnPageLoad && isPageLoadNavigation) {
77-
startTransaction({
78-
name: transactionName,
79-
op: 'pageload',
80-
tags,
81-
data,
82-
metadata: {
83-
source: transactionSource,
84-
},
85-
});
94+
const pageloadTransaction = getActiveTransaction();
95+
if (pageloadTransaction) {
96+
pageloadTransaction.setName(transactionName, transactionSource);
97+
pageloadTransaction.setData('params', data.params);
98+
pageloadTransaction.setData('query', data.query);
99+
}
86100
}
87101

88102
if (startTransactionOnLocationChange && !isPageLoadNavigation) {

‎packages/vue/src/tracing.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ const HOOKS: { [key in Operation]: Hook[] } = {
2929
};
3030

3131
/** Grabs active transaction off scope, if any */
32-
function getActiveTransaction(): Transaction | undefined {
32+
export function getActiveTransaction(): Transaction | undefined {
3333
return getCurrentHub().getScope()?.getTransaction();
3434
}
3535

@@ -117,8 +117,8 @@ export const createTracingMixins = (options: TracingOptions): Mixins => {
117117
// The before hook did not start the tracking span, so the span was not added.
118118
// This is probably because it happened before there is an active transaction
119119
if (!span) return;
120-
121120
span.finish();
121+
122122
finishRootSpan(this, timestampInSeconds(), options.timeout);
123123
}
124124
};

‎packages/vue/test/router.test.ts

Lines changed: 63 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import * as SentryBrowser from '@sentry/browser';
2+
import { Transaction } from '@sentry/types';
23

34
import { vueRouterInstrumentation } from '../src';
45
import { Route } from '../src/router';
6+
import * as vueTracing from '../src/tracing';
57

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

@@ -74,13 +76,12 @@ describe('vueRouterInstrumentation()', () => {
7476
});
7577

7678
it.each([
77-
['initialPageloadRoute', 'normalRoute1', 'pageload', '/books/:bookId/chapter/:chapterId', 'route'],
78-
['normalRoute1', 'normalRoute2', 'navigation', '/accounts/:accountId', 'route'],
79-
['normalRoute2', 'namedRoute', 'navigation', 'login-screen', 'custom'],
80-
['normalRoute2', 'unmatchedRoute', 'navigation', '/e8733846-20ac-488c-9871-a5cbcb647294', 'url'],
79+
['normalRoute1', 'normalRoute2', '/accounts/:accountId', 'route'],
80+
['normalRoute2', 'namedRoute', 'login-screen', 'custom'],
81+
['normalRoute2', 'unmatchedRoute', '/e8733846-20ac-488c-9871-a5cbcb647294', 'url'],
8182
])(
82-
'should return instrumentation that instruments VueRouter.beforeEach(%s, %s)',
83-
(fromKey, toKey, op, transactionName, transactionSource) => {
83+
'should return instrumentation that instruments VueRouter.beforeEach(%s, %s) for navigations',
84+
(fromKey, toKey, transactionName, transactionSource) => {
8485
// create instrumentation
8586
const instrument = vueRouterInstrumentation(mockVueRouter);
8687

@@ -95,7 +96,8 @@ describe('vueRouterInstrumentation()', () => {
9596
const to = testRoutes[toKey];
9697
beforeEachCallback(to, from, mockNext);
9798

98-
expect(mockStartTransaction).toHaveBeenCalledTimes(1);
99+
// first startTx call happens when the instrumentation is initialized (for pageloads)
100+
expect(mockStartTransaction).toHaveBeenCalledTimes(2);
99101
expect(mockStartTransaction).toHaveBeenCalledWith({
100102
name: transactionName,
101103
metadata: {
@@ -105,7 +107,7 @@ describe('vueRouterInstrumentation()', () => {
105107
params: to.params,
106108
query: to.query,
107109
},
108-
op: op,
110+
op: 'navigation',
109111
tags: {
110112
'routing.instrumentation': 'vue-router',
111113
},
@@ -115,6 +117,57 @@ describe('vueRouterInstrumentation()', () => {
115117
},
116118
);
117119

120+
it.each([
121+
['initialPageloadRoute', 'normalRoute1', '/books/:bookId/chapter/:chapterId', 'route'],
122+
['initialPageloadRoute', 'namedRoute', 'login-screen', 'custom'],
123+
['initialPageloadRoute', 'unmatchedRoute', '/e8733846-20ac-488c-9871-a5cbcb647294', 'url'],
124+
])(
125+
'should return instrumentation that instruments VueRouter.beforeEach(%s, %s) for pageloads',
126+
(fromKey, toKey, _transactionName, _transactionSource) => {
127+
const mockedTxn = {
128+
setName: jest.fn(),
129+
setData: jest.fn(),
130+
};
131+
const customMockStartTxn = { ...mockStartTransaction }.mockImplementation(_ => {
132+
return mockedTxn;
133+
});
134+
jest.spyOn(vueTracing, 'getActiveTransaction').mockImplementation(() => mockedTxn as unknown as Transaction);
135+
136+
// create instrumentation
137+
const instrument = vueRouterInstrumentation(mockVueRouter);
138+
139+
// instrument
140+
instrument(customMockStartTxn, true, true);
141+
142+
// check for transaction start
143+
expect(customMockStartTxn).toHaveBeenCalledTimes(1);
144+
expect(customMockStartTxn).toHaveBeenCalledWith({
145+
name: '/',
146+
metadata: {
147+
source: 'url',
148+
},
149+
op: 'pageload',
150+
tags: {
151+
'routing.instrumentation': 'vue-router',
152+
},
153+
});
154+
155+
const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0];
156+
157+
const from = testRoutes[fromKey];
158+
const to = testRoutes[toKey];
159+
160+
beforeEachCallback(to, from, mockNext);
161+
expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1);
162+
163+
expect(mockedTxn.setName).toHaveBeenCalledWith(_transactionName, _transactionSource);
164+
expect(mockedTxn.setData).toHaveBeenNthCalledWith(1, 'params', to.params);
165+
expect(mockedTxn.setData).toHaveBeenNthCalledWith(2, 'query', to.query);
166+
167+
expect(mockNext).toHaveBeenCalledTimes(1);
168+
},
169+
);
170+
118171
test.each([
119172
[undefined, 1],
120173
[false, 0],
@@ -148,7 +201,7 @@ describe('vueRouterInstrumentation()', () => {
148201
// create instrumentation
149202
const instrument = vueRouterInstrumentation(mockVueRouter);
150203

151-
// instrument
204+
// instrument (this will call startTrransaction once for pageloads but we can ignore that)
152205
instrument(mockStartTransaction, true, startTransactionOnLocationChange);
153206

154207
// check
@@ -157,7 +210,7 @@ describe('vueRouterInstrumentation()', () => {
157210
const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0];
158211
beforeEachCallback(testRoutes['normalRoute2'], testRoutes['normalRoute1'], mockNext);
159212

160-
expect(mockStartTransaction).toHaveBeenCalledTimes(expectedCallsAmount);
213+
expect(mockStartTransaction).toHaveBeenCalledTimes(expectedCallsAmount + 1);
161214
},
162215
);
163216
});

0 commit comments

Comments
 (0)
0