diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index e2a1b46f8b2e..751d196dff78 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -1,5 +1,8 @@ import { captureException } from '@sentry/browser'; import { Transaction, TransactionContext, TransactionSource } from '@sentry/types'; +import { WINDOW } from '@sentry/utils'; + +import { getActiveTransaction } from './tracing'; export type VueRouterInstrumentation = ( startTransaction: (context: TransactionContext) => T | undefined, @@ -31,7 +34,7 @@ interface VueRouter { } /** - * Creates routing instrumentation for Vue Router v2 + * Creates routing instrumentation for Vue Router v2, v3 and v4 * * @param router The Vue Router instance that is used */ @@ -41,6 +44,23 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument startTransactionOnPageLoad: boolean = true, startTransactionOnLocationChange: boolean = true, ) => { + const tags = { + 'routing.instrumentation': 'vue-router', + }; + + // We have to start the pageload transaction as early as possible (before the router's `beforeEach` hook + // is called) to not miss child spans of the pageload. + if (startTransactionOnPageLoad) { + startTransaction({ + name: WINDOW.location.pathname, + op: 'pageload', + tags, + metadata: { + source: 'url', + }, + }); + } + router.onError(error => captureException(error)); router.beforeEach((to, from, next) => { @@ -54,9 +74,6 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument // hence only '==' instead of '===', because `undefined == null` evaluates to `true` const isPageLoadNavigation = from.name == null && from.matched.length === 0; - const tags = { - 'routing.instrumentation': 'vue-router', - }; const data = { params: to.params, query: to.query, @@ -74,15 +91,12 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument } if (startTransactionOnPageLoad && isPageLoadNavigation) { - startTransaction({ - name: transactionName, - op: 'pageload', - tags, - data, - metadata: { - source: transactionSource, - }, - }); + const pageloadTransaction = getActiveTransaction(); + if (pageloadTransaction) { + pageloadTransaction.setName(transactionName, transactionSource); + pageloadTransaction.setData('params', data.params); + pageloadTransaction.setData('query', data.query); + } } if (startTransactionOnLocationChange && !isPageLoadNavigation) { diff --git a/packages/vue/src/tracing.ts b/packages/vue/src/tracing.ts index e4b0b493f80a..889cbe30dca6 100644 --- a/packages/vue/src/tracing.ts +++ b/packages/vue/src/tracing.ts @@ -29,7 +29,7 @@ const HOOKS: { [key in Operation]: Hook[] } = { }; /** Grabs active transaction off scope, if any */ -function getActiveTransaction(): Transaction | undefined { +export function getActiveTransaction(): Transaction | undefined { return getCurrentHub().getScope()?.getTransaction(); } @@ -117,8 +117,8 @@ export const createTracingMixins = (options: TracingOptions): Mixins => { // The before hook did not start the tracking span, so the span was not added. // This is probably because it happened before there is an active transaction if (!span) return; - span.finish(); + finishRootSpan(this, timestampInSeconds(), options.timeout); } }; diff --git a/packages/vue/test/router.test.ts b/packages/vue/test/router.test.ts index c29aa571dae4..d2e9248452a5 100644 --- a/packages/vue/test/router.test.ts +++ b/packages/vue/test/router.test.ts @@ -1,7 +1,9 @@ import * as SentryBrowser from '@sentry/browser'; +import { Transaction } from '@sentry/types'; import { vueRouterInstrumentation } from '../src'; import { Route } from '../src/router'; +import * as vueTracing from '../src/tracing'; const captureExceptionSpy = jest.spyOn(SentryBrowser, 'captureException'); @@ -74,13 +76,12 @@ describe('vueRouterInstrumentation()', () => { }); 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'], + ['normalRoute1', 'normalRoute2', '/accounts/:accountId', 'route'], + ['normalRoute2', 'namedRoute', 'login-screen', 'custom'], + ['normalRoute2', 'unmatchedRoute', '/e8733846-20ac-488c-9871-a5cbcb647294', 'url'], ])( - 'should return instrumentation that instruments VueRouter.beforeEach(%s, %s)', - (fromKey, toKey, op, transactionName, transactionSource) => { + 'should return instrumentation that instruments VueRouter.beforeEach(%s, %s) for navigations', + (fromKey, toKey, transactionName, transactionSource) => { // create instrumentation const instrument = vueRouterInstrumentation(mockVueRouter); @@ -95,7 +96,8 @@ describe('vueRouterInstrumentation()', () => { const to = testRoutes[toKey]; beforeEachCallback(to, from, mockNext); - expect(mockStartTransaction).toHaveBeenCalledTimes(1); + // first startTx call happens when the instrumentation is initialized (for pageloads) + expect(mockStartTransaction).toHaveBeenCalledTimes(2); expect(mockStartTransaction).toHaveBeenCalledWith({ name: transactionName, metadata: { @@ -105,7 +107,7 @@ describe('vueRouterInstrumentation()', () => { params: to.params, query: to.query, }, - op: op, + op: 'navigation', tags: { 'routing.instrumentation': 'vue-router', }, @@ -115,6 +117,57 @@ describe('vueRouterInstrumentation()', () => { }, ); + it.each([ + ['initialPageloadRoute', 'normalRoute1', '/books/:bookId/chapter/:chapterId', 'route'], + ['initialPageloadRoute', 'namedRoute', 'login-screen', 'custom'], + ['initialPageloadRoute', 'unmatchedRoute', '/e8733846-20ac-488c-9871-a5cbcb647294', 'url'], + ])( + 'should return instrumentation that instruments VueRouter.beforeEach(%s, %s) for pageloads', + (fromKey, toKey, _transactionName, _transactionSource) => { + const mockedTxn = { + setName: jest.fn(), + setData: jest.fn(), + }; + const customMockStartTxn = { ...mockStartTransaction }.mockImplementation(_ => { + return mockedTxn; + }); + jest.spyOn(vueTracing, 'getActiveTransaction').mockImplementation(() => mockedTxn as unknown as Transaction); + + // create instrumentation + const instrument = vueRouterInstrumentation(mockVueRouter); + + // instrument + instrument(customMockStartTxn, true, true); + + // check for transaction start + expect(customMockStartTxn).toHaveBeenCalledTimes(1); + expect(customMockStartTxn).toHaveBeenCalledWith({ + name: '/', + metadata: { + source: 'url', + }, + op: 'pageload', + tags: { + 'routing.instrumentation': 'vue-router', + }, + }); + + const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0]; + + const from = testRoutes[fromKey]; + const to = testRoutes[toKey]; + + beforeEachCallback(to, from, mockNext); + expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1); + + expect(mockedTxn.setName).toHaveBeenCalledWith(_transactionName, _transactionSource); + expect(mockedTxn.setData).toHaveBeenNthCalledWith(1, 'params', to.params); + expect(mockedTxn.setData).toHaveBeenNthCalledWith(2, 'query', to.query); + + expect(mockNext).toHaveBeenCalledTimes(1); + }, + ); + test.each([ [undefined, 1], [false, 0], @@ -148,7 +201,7 @@ describe('vueRouterInstrumentation()', () => { // create instrumentation const instrument = vueRouterInstrumentation(mockVueRouter); - // instrument + // instrument (this will call startTrransaction once for pageloads but we can ignore that) instrument(mockStartTransaction, true, startTransactionOnLocationChange); // check @@ -157,7 +210,7 @@ describe('vueRouterInstrumentation()', () => { const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0]; beforeEachCallback(testRoutes['normalRoute2'], testRoutes['normalRoute1'], mockNext); - expect(mockStartTransaction).toHaveBeenCalledTimes(expectedCallsAmount); + expect(mockStartTransaction).toHaveBeenCalledTimes(expectedCallsAmount + 1); }, ); });