fix(browser): Avoid recording long animation frame spans starting bef…

…ore their parent span (#14186) - Check for start time of parent navigation span and don't start long animation frame span if its start timestamp is earlier than the navigation start time stamp - Refactor span starting logic to use common helper function to compensate the bundle size increase - Add regression test that failed previously - Improve regression test from #14183 to avoid flakes and improve the in-test navigation
getsentry · Nov 6, 2024 · fa684ab · fa684ab
1 parent 7c7272b
commit fa684ab
Show file tree

Hide file tree

Showing 8 changed files with 97 additions and 9 deletions.
diff --git a/...s/suites/tracing/browserTracingIntegration/long-animation-frame-before-navigation/init.js b/...s/suites/tracing/browserTracingIntegration/long-animation-frame-before-navigation/init.js
@@ -0,0 +1,17 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  integrations: [
+    Sentry.browserTracingIntegration({
+      idleTimeout: 9000,
+      enableLongTask: false,
+      enableLongAnimationFrame: true,
+      instrumentPageLoad: false,
+      enableInp: false,
+    }),
+  ],
+  tracesSampleRate: 1,
+});
diff --git a/...uites/tracing/browserTracingIntegration/long-animation-frame-before-navigation/subject.js b/...uites/tracing/browserTracingIntegration/long-animation-frame-before-navigation/subject.js
@@ -0,0 +1,18 @@
+function getElapsed(startTime) {
+  const time = Date.now();
+  return time - startTime;
+}
+
+function handleClick() {
+  const startTime = Date.now();
+  while (getElapsed(startTime) < 105) {
+    //
+  }
+  window.history.pushState({}, '', `#myHeading`);
+}
+
+const button = document.getElementById('clickme');
+
+console.log('button', button);
+
+button.addEventListener('click', handleClick);
diff --git a/...es/tracing/browserTracingIntegration/long-animation-frame-before-navigation/template.html b/...es/tracing/browserTracingIntegration/long-animation-frame-before-navigation/template.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <body>
+    <button id="clickme">
+      click me to start the long animation!
+    </button>
+
+    <h1 id="myHeading">My Heading</h1>
+  </body>
+</html>
diff --git a/...s/suites/tracing/browserTracingIntegration/long-animation-frame-before-navigation/test.ts b/...s/suites/tracing/browserTracingIntegration/long-animation-frame-before-navigation/test.ts
@@ -0,0 +1,31 @@
+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 animation frame that starts before a navigation.",
+  async ({ browserName, getLocalTestPath, page }) => {
+    // Long animation frames only work on chrome
+    if (shouldSkipTracingTest() || browserName !== 'chromium') {
+      sentryTest.skip();
+    }
+
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    await page.goto(url);
+
+    const navigationTransactionEventPromise = getFirstSentryEnvelopeRequest<Event>(page);
+
+    await page.locator('#clickme').click();
+
+    const navigationTransactionEvent = await navigationTransactionEventPromise;
+
+    expect(navigationTransactionEvent.contexts?.trace?.op).toBe('navigation');
+
+    const loafSpans = navigationTransactionEvent.spans?.filter(s => s.op?.startsWith('ui.long-animation-frame'));
+
+    expect(loafSpans?.length).toEqual(0);
+  },
+);
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
@@ -15,5 +15,4 @@ Sentry.init({
     }),
   ],
   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
@@ -13,5 +13,5 @@ longTaskButton?.addEventListener('click', () => {
   }
 
   // trigger a navigation in the same event loop tick
-  window.history.pushState({}, '', '/#myHeading');
+  window.history.pushState({}, '', '#myHeading');
 });
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
@@ -15,9 +15,11 @@ sentryTest(
 
     await page.goto(url);
 
+    const navigationTransactionEventPromise = getFirstSentryEnvelopeRequest<Event>(page);
+
     await page.locator('#myButton').click();
 
-    const navigationTransactionEvent = await getFirstSentryEnvelopeRequest<Event>(page, url);
+    const navigationTransactionEvent = await navigationTransactionEventPromise;
 
     expect(navigationTransactionEvent.contexts?.trace?.op).toBe('navigation');
 

diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts
@@ -143,7 +143,8 @@ export function startTrackingLongAnimationFrames(): void {
   // we directly observe `long-animation-frame` events instead of through the web-vitals
   // `observe` helper function.
   const observer = new PerformanceObserver(list => {
-    if (!getActiveSpan()) {
+    const parent = getActiveSpan();
+    if (!parent) {
       return;
     }
     for (const entry of list.getEntries() as PerformanceLongAnimationFrameTiming[]) {
@@ -152,6 +153,17 @@ export function startTrackingLongAnimationFrames(): void {
       }
 
       const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime);
+
+      const { start_timestamp: parentStartTimestamp, op: parentOp } = spanToJSON(parent);
+
+      if (parentOp === 'navigation' && parentStartTimestamp && startTime < parentStartTimestamp) {
+        // Skip adding the span if the long animation frame 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;
+      }
+
       const duration = msToSec(entry.duration);
 
       const attributes: SpanAttributes = {
@@ -172,15 +184,11 @@ export function startTrackingLongAnimationFrames(): void {
         attributes['browser.script.source_char_position'] = sourceCharPosition;
       }
 
-      const span = startInactiveSpan({
+      startAndEndSpan(parent, startTime, startTime + duration, {
         name: 'Main UI thread blocked',
         op: 'ui.long-animation-frame',
-        startTime,
         attributes,
       });
-      if (span) {
-        span.end(startTime + duration);
-      }
     }
   });
-Original file line number
+Diff line change
@@ Expand Up / @@ -15,5 +15,4 @@ Sentry.init({ @@
         }),
       ],
       tracesSampleRate: 1,
-      debug: true,
     });