fix(vue): Start pageload transaction earlier to capture missing spans (…

…#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.

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 = <T extends Transaction>(
   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) {

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);
         }
       };

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);
     },
   );
 });