Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(browser): Avoid recording long animation frame spans starting before their parent span #14186

Merged
merged 2 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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,
});
Original file line number Diff line number Diff line change
@@ -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);
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -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);
},
);
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,4 @@ Sentry.init({
}),
],
tracesSampleRate: 1,
debug: true,
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ longTaskButton?.addEventListener('click', () => {
}

// trigger a navigation in the same event loop tick
window.history.pushState({}, '', '/#myHeading');
window.history.pushState({}, '', '#myHeading');
});
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down
20 changes: 14 additions & 6 deletions packages/browser-utils/src/metrics/browserMetrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[]) {
Expand All @@ -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 = {
Expand All @@ -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);
}
}
});

Expand Down
Loading