Skip to content

Commit ac57e53

Browse files
Lms24Lms24
authored
fix(browser): Avoid recording long task spans starting before their parent 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
1 parent ff18dfd commit ac57e53

File tree

7 files changed

+95
-15
lines changed

7 files changed

+95
-15
lines changed

‎.size-limit.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ module.exports = [
4040
path: 'packages/browser/build/npm/esm/index.js',
4141
import: createImport('init', 'browserTracingIntegration'),
4242
gzip: true,
43-
limit: '36 KB',
43+
limit: '36.5 KB',
4444
},
4545
{
4646
name: '@sentry/browser (incl. Tracing, Replay)',
@@ -124,7 +124,7 @@ module.exports = [
124124
import: createImport('init', 'ErrorBoundary', 'reactRouterV6BrowserTracingIntegration'),
125125
ignore: ['react/jsx-runtime'],
126126
gzip: true,
127-
limit: '39.05 KB',
127+
limit: '39.5 KB',
128128
},
129129
// Vue SDK (ESM)
130130
{

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/init.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
7+
integrations: [
8+
Sentry.browserTracingIntegration({
9+
idleTimeout: 9000,
10+
enableLongAnimationFrame: false,
11+
instrumentPageLoad: false,
12+
instrumentNavigation: true,
13+
enableInp: false,
14+
enableLongTask: true,
15+
}),
16+
],
17+
tracesSampleRate: 1,
18+
debug: true,
19+
});

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/subject.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
const longTaskButton = document.getElementById('myButton');
2+
3+
longTaskButton?.addEventListener('click', () => {
4+
const startTime = Date.now();
5+
6+
function getElapsed() {
7+
const time = Date.now();
8+
return time - startTime;
9+
}
10+
11+
while (getElapsed() < 500) {
12+
//
13+
}
14+
15+
// trigger a navigation in the same event loop tick
16+
window.history.pushState({}, '', '/#myHeading');
17+
});

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/template.html‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<div>Rendered Before Long Task</div>
8+
<script src="https://example.com/path/to/script.js"></script>
9+
10+
<button id="myButton">Start long task</button>
11+
<h1 id="myHeading">Heading</h1>
12+
</body>
13+
</html>

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/long-tasks-before-navigation/test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { expect } from '@playwright/test';
2+
import type { Event } from '@sentry/types';
3+
4+
import { sentryTest } from '../../../../utils/fixtures';
5+
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
6+
7+
sentryTest(
8+
"doesn't capture long task spans starting before a navigation in the navigation transaction",
9+
async ({ browserName, getLocalTestPath, page }) => {
10+
// Long tasks only work on chrome
11+
if (shouldSkipTracingTest() || browserName !== 'chromium') {
12+
sentryTest.skip();
13+
}
14+
const url = await getLocalTestPath({ testDir: __dirname });
15+
16+
await page.goto(url);
17+
18+
await page.locator('#myButton').click();
19+
20+
const navigationTransactionEvent = await getFirstSentryEnvelopeRequest<Event>(page, url);
21+
22+
expect(navigationTransactionEvent.contexts?.trace?.op).toBe('navigation');
23+
24+
const longTaskSpans = navigationTransactionEvent?.spans?.filter(span => span.op === 'ui.long-task');
25+
expect(longTaskSpans).toHaveLength(0);
26+
},
27+
);

‎packages/browser-utils/src/metrics/browserMetrics.ts‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -105,24 +105,32 @@ export function startTrackingWebVitals({ recordClsStandaloneSpans }: StartTracki
105105
*/
106106
export function startTrackingLongTasks(): void {
107107
addPerformanceInstrumentationHandler('longtask', ({ entries }) => {
108-
if (!getActiveSpan()) {
108+
const parent = getActiveSpan();
109+
if (!parent) {
109110
return;
110111
}
112+
113+
const { op: parentOp, start_timestamp: parentStartTimestamp } = spanToJSON(parent);
114+
111115
for (const entry of entries) {
112116
const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime);
113117
const duration = msToSec(entry.duration);
114118

115-
const span = startInactiveSpan({
119+
if (parentOp === 'navigation' && parentStartTimestamp && startTime < parentStartTimestamp) {
120+
// Skip adding a span if the long task started before the navigation started.
121+
// `startAndEndSpan` will otherwise adjust the parent's start time to the span's start
122+
// time, potentially skewing the duration of the actual navigation as reported via our
123+
// routing instrumentations
124+
continue;
125+
}
126+
127+
startAndEndSpan(parent, startTime, startTime + duration, {
116128
name: 'Main UI thread blocked',
117129
op: 'ui.long-task',
118-
startTime,
119130
attributes: {
120131
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.browser.metrics',
121132
},
122133
});
123-
if (span) {
124-
span.end(startTime + duration);
125-
}
126134
}
127135
});
128136
}

‎packages/browser/package.json‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,7 @@
99
"engines": {
1010
"node": ">=14.18"
1111
},
12-
"files": [
13-
"/build/npm"
14-
],
12+
"files": ["/build/npm"],
1513
"main": "build/npm/cjs/index.js",
1614
"module": "build/npm/esm/index.js",
1715
"types": "build/npm/types/index.d.ts",
@@ -30,9 +28,7 @@
3028
},
3129
"typesVersions": {
3230
"<4.9": {
33-
"build/npm/types/index.d.ts": [
34-
"build/npm/types-ts3.8/index.d.ts"
35-
]
31+
"build/npm/types/index.d.ts": ["build/npm/types-ts3.8/index.d.ts"]
3632
}
3733
},
3834
"publishConfig": {
@@ -53,7 +49,7 @@
5349
},
5450
"scripts": {
5551
"build": "run-p build:transpile build:bundle build:types",
56-
"build:dev": "yarn build",
52+
"build:dev": "run-p build:transpile build:types",
5753
"build:bundle": "rollup -c rollup.bundle.config.mjs",
5854
"build:transpile": "rollup -c rollup.npm.config.mjs",
5955
"build:types": "run-s build:types:core build:types:downlevel",

0 commit comments

Comments
 (0)