fix(browser): Avoid recording long task spans starting before their p…

…arent span (#14183) - check for the start time stamp and drops long task spans starting before a _navigation_ spans started. - refactor the start and end logic a bit to compensate for bundle size increase (see comment) - add a regression test that previously would fail
getsentry · Nov 5, 2024 · ac57e53 · ac57e53
1 parent ff18dfd
commit ac57e53
Show file tree

Hide file tree

Showing 7 changed files with 95 additions and 15 deletions.
diff --git a/.size-limit.js b/.size-limit.js
@@ -40,7 +40,7 @@ module.exports = [
     path: 'packages/browser/build/npm/esm/index.js',
     import: createImport('init', 'browserTracingIntegration'),
     gzip: true,
-    limit: '36 KB',
+    limit: '36.5 KB',
   },
   {
     name: '@sentry/browser (incl. Tracing, Replay)',
@@ -124,7 +124,7 @@ module.exports = [
     import: createImport('init', 'ErrorBoundary', 'reactRouterV6BrowserTracingIntegration'),
     ignore: ['react/jsx-runtime'],
     gzip: true,
-    limit: '39.05 KB',
+    limit: '39.5 KB',
   },
   // Vue SDK (ESM)
   {

diff --git a/...ation-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/init.js b/...ation-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/init.js
@@ -0,0 +1,19 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  integrations: [
+    Sentry.browserTracingIntegration({
+      idleTimeout: 9000,
+      enableLongAnimationFrame: false,
+      instrumentPageLoad: false,
+      instrumentNavigation: true,
+      enableInp: false,
+      enableLongTask: true,
+    }),
+  ],
+  tracesSampleRate: 1,
+  debug: true,
+});
diff --git a/...on-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/subject.js b/...on-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/subject.js
@@ -0,0 +1,17 @@
+const longTaskButton = document.getElementById('myButton');
+
+longTaskButton?.addEventListener('click', () => {
+  const startTime = Date.now();
+
+  function getElapsed() {
+    const time = Date.now();
+    return time - startTime;
+  }
+
+  while (getElapsed() < 500) {
+    //
+  }
+
+  // trigger a navigation in the same event loop tick
+  window.history.pushState({}, '', '/#myHeading');
+});
diff --git a/...tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/template.html b/...tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/template.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <body>
+    <div>Rendered Before Long Task</div>
+    <script src="https://example.com/path/to/script.js"></script>
+
+    <button id="myButton">Start long task</button>
+    <h1 id="myHeading">Heading</h1>
+  </body>
+</html>
diff --git a/...ation-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/test.ts b/...ation-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/test.ts
@@ -0,0 +1,27 @@
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
+
+sentryTest(
+  "doesn't capture long task spans starting before a navigation in the navigation transaction",
+  async ({ browserName, getLocalTestPath, page }) => {
+    // Long tasks only work on chrome
+    if (shouldSkipTracingTest() || browserName !== 'chromium') {
+      sentryTest.skip();
+    }
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    await page.goto(url);
+
+    await page.locator('#myButton').click();
+
+    const navigationTransactionEvent = await getFirstSentryEnvelopeRequest<Event>(page, url);
+
+    expect(navigationTransactionEvent.contexts?.trace?.op).toBe('navigation');
+
+    const longTaskSpans = navigationTransactionEvent?.spans?.filter(span => span.op === 'ui.long-task');
+    expect(longTaskSpans).toHaveLength(0);
+  },
+);
diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts
@@ -105,24 +105,32 @@ export function startTrackingWebVitals({ recordClsStandaloneSpans }: StartTracki
  */
 export function startTrackingLongTasks(): void {
   addPerformanceInstrumentationHandler('longtask', ({ entries }) => {
-    if (!getActiveSpan()) {
+    const parent = getActiveSpan();
+    if (!parent) {
       return;
     }
+
+    const { op: parentOp, start_timestamp: parentStartTimestamp } = spanToJSON(parent);
+
     for (const entry of entries) {
       const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime);
       const duration = msToSec(entry.duration);
 
-      const span = startInactiveSpan({
+      if (parentOp === 'navigation' && parentStartTimestamp && startTime < parentStartTimestamp) {
+        // Skip adding a span if the long task started before the navigation started.
+        // `startAndEndSpan` will otherwise adjust the parent's start time to the span's start
+        // time, potentially skewing the duration of the actual navigation as reported via our
+        // routing instrumentations
+        continue;
+      }
+
+      startAndEndSpan(parent, startTime, startTime + duration, {
         name: 'Main UI thread blocked',
         op: 'ui.long-task',
-        startTime,
         attributes: {
           [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.browser.metrics',
         },
       });
-      if (span) {
-        span.end(startTime + duration);
-      }
     }
   });
 }

diff --git a/packages/browser/package.json b/packages/browser/package.json
@@ -9,9 +9,7 @@
   "engines": {
     "node": ">=14.18"
   },
-  "files": [
-    "/build/npm"
-  ],
+  "files": ["/build/npm"],
   "main": "build/npm/cjs/index.js",
   "module": "build/npm/esm/index.js",
   "types": "build/npm/types/index.d.ts",
@@ -30,9 +28,7 @@
   },
   "typesVersions": {
     "<4.9": {
-      "build/npm/types/index.d.ts": [
-        "build/npm/types-ts3.8/index.d.ts"
-      ]
+      "build/npm/types/index.d.ts": ["build/npm/types-ts3.8/index.d.ts"]
     }
   },
   "publishConfig": {
@@ -53,7 +49,7 @@
   },
   "scripts": {
     "build": "run-p build:transpile build:bundle build:types",
-    "build:dev": "yarn build",
+    "build:dev": "run-p build:transpile build:types",
     "build:bundle": "rollup -c rollup.bundle.config.mjs",
     "build:transpile": "rollup -c rollup.npm.config.mjs",
     "build:types": "run-s build:types:core build:types:downlevel",