fix(vue): Make pageload span handling more reliable (#16799)

We used to rely on a somewhat complex heuristic to determine if a router
change is a pageload or not. This somehow did not work anymore here:
#16783 in nuxt-3-min.
Likely some vue router difference... However, I think this can be
simplified anyhow, by just checking if we have an active pageload span.
That seems to work reliably enough.

Author: mydea

dev-packages/e2e-tests/test-applications/nuxt-3-min/tests/tracing.client.test.ts

@@ -1,10 +1,11 @@
 import { expect, test } from '@playwright/test';
 import { waitForTransaction } from '@sentry-internal/test-utils';
-import type { Span } from '@sentry/nuxt';
 
 test('sends a pageload root span with a parameterized URL', async ({ page }) => {
   const transactionPromise = waitForTransaction('nuxt-3-min', async transactionEvent => {
-    return transactionEvent.transaction === '/test-param/:param()';
+    return (
+      transactionEvent.contexts?.trace?.op === 'pageload' && transactionEvent.transaction === '/test-param/:param()'
+    );
   });
 
   await page.goto(`/test-param/1234`);
@@ -39,7 +40,7 @@ test('sends component tracking spans when `trackComponents` is enabled', async (
   await page.goto(`/client-error`);
 
   const rootSpan = await transactionPromise;
-  const errorButtonSpan = rootSpan.spans.find((span: Span) => span.description === 'Vue <ErrorButton>');
+  const errorButtonSpan = rootSpan.spans.find(span => span.description === 'Vue <ErrorButton>');
 
   const expected = {
     data: { 'sentry.origin': 'auto.ui.vue', 'sentry.op': 'ui.vue.mount' },

packages/vue/src/router.ts

@@ -50,32 +50,15 @@ export function instrumentVueRouter(
   },
   startNavigationSpanFn: (context: StartSpanOptions) => void,
 ): void {
-  let isFirstPageLoad = true;
+  let hasHandledFirstPageLoad = false;
 
   router.onError(error => captureException(error, { mechanism: { handled: false } }));
 
-  router.beforeEach((to, from, next) => {
-    // According to docs we could use `from === VueRouter.START_LOCATION` but I couldn't get it working for Vue 2
-    // https://router.vuejs.org/api/#router-start-location
-    // https://next.router.vuejs.org/api/#start-location
-    // Additionally, Nuxt does not provide the possibility to check for `from.matched.length === 0` (this is never 0).
-    // Therefore, a flag was added to track the page-load: isFirstPageLoad
-
-    // from.name:
-    // - Vue 2: null
-    // - Vue 3: undefined
-    // - Nuxt: undefined
-    // hence only '==' instead of '===', because `undefined == null` evaluates to `true`
-    const isPageLoadNavigation =
-      (from.name == null && from.matched.length === 0) || (from.name === undefined && isFirstPageLoad);
-
-    if (isFirstPageLoad) {
-      isFirstPageLoad = false;
-    }
+  router.beforeEach((to, _from, next) => {
+    // We avoid trying to re-fetch the page load span when we know we already handled it the first time
+    const activePageLoadSpan = !hasHandledFirstPageLoad ? getActivePageLoadSpan() : undefined;
 
-    const attributes: SpanAttributes = {
-      [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.vue',
-    };
+    const attributes: SpanAttributes = {};
 
     for (const key of Object.keys(to.params)) {
       attributes[`params.${key}`] = to.params[key];
@@ -102,30 +85,33 @@ export function instrumentVueRouter(
 
     getCurrentScope().setTransactionName(spanName);
 
-    if (options.instrumentPageLoad && isPageLoadNavigation) {
-      const activeRootSpan = getActiveRootSpan();
-      if (activeRootSpan) {
-        const existingAttributes = spanToJSON(activeRootSpan).data;
-        if (existingAttributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] !== 'custom') {
-          activeRootSpan.updateName(spanName);
-          activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, transactionSource);
-        }
-        // Set router attributes on the existing pageload transaction
-        // This will override the origin, and add params & query attributes
-        activeRootSpan.setAttributes({
-          ...attributes,
-          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.vue',
-        });
+    // Update the existing page load span with parametrized route information
+    if (options.instrumentPageLoad && activePageLoadSpan) {
+      const existingAttributes = spanToJSON(activePageLoadSpan).data;
+      if (existingAttributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] !== 'custom') {
+        activePageLoadSpan.updateName(spanName);
+        activePageLoadSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, transactionSource);
       }
+
+      // Set router attributes on the existing pageload transaction
+      // This will override the origin, and add params & query attributes
+      activePageLoadSpan.setAttributes({
+        ...attributes,
+        [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.vue',
+      });
+
+      hasHandledFirstPageLoad = true;
     }
 
-    if (options.instrumentNavigation && !isPageLoadNavigation) {
-      attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] = transactionSource;
-      attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] = 'auto.navigation.vue';
+    if (options.instrumentNavigation && !activePageLoadSpan) {
       startNavigationSpanFn({
         name: spanName,
         op: 'navigation',
-        attributes,
+        attributes: {
+          ...attributes,
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.vue',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: transactionSource,
+        },
       });
     }
 
@@ -138,7 +124,7 @@ export function instrumentVueRouter(
   });
 }
 
-function getActiveRootSpan(): Span | undefined {
+function getActivePageLoadSpan(): Span | undefined {
   const span = getActiveSpan();
   const rootSpan = span && getRootSpan(span);
 
@@ -148,6 +134,5 @@ function getActiveRootSpan(): Span | undefined {
 
   const op = spanToJSON(rootSpan).op;
 
-  // Only use this root span if it is a pageload or navigation span
-  return op === 'navigation' || op === 'pageload' ? rootSpan : undefined;
+  return op === 'pageload' ? rootSpan : undefined;
 }

packages/vue/test/router.test.ts

@@ -121,8 +121,8 @@ describe('instrumentVueRouter()', () => {
       beforeEachCallback(to, testRoutes['initialPageloadRoute']!, mockNext); // fake initial pageload
       beforeEachCallback(to, from, mockNext);
 
-      expect(mockStartSpan).toHaveBeenCalledTimes(1);
-      expect(mockStartSpan).toHaveBeenCalledWith({
+      expect(mockStartSpan).toHaveBeenCalledTimes(2);
+      expect(mockStartSpan).toHaveBeenLastCalledWith({
         name: transactionName,
         attributes: {
           [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.vue',