Skip to content

Commit fde653e

Browse files
Lms24Lms24
committed
fix(browser): Remove faulty LCP, FCP and FP normalization logic
1 parent 3a1417f commit fde653e

File tree

4 files changed

+124
-21
lines changed

4 files changed

+124
-21
lines changed

‎dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-relations/assets/sentry-logo-600x179.png‎

15.7 KB
Loading

‎dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-relations/template.html‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<div id="content"></div>
8+
<img src="https://example.com/path/to/image.png" />
9+
<button type="button">Test button</button>
10+
</body>
11+
</html>

‎dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-relations/test.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
import type { Route } from '@playwright/test';
2+
import { expect } from '@playwright/test';
3+
import type { Event } from '@sentry/types';
4+
5+
import { sentryTest } from '../../../../utils/fixtures';
6+
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
7+
8+
/**
9+
* Bit of an odd test but we previously ran into cases where we would report TTFB > (LCP, FCP, FP)
10+
* This should never happen and this test serves as a regression test for that.
11+
*
12+
* The problem is: We don't always get valid TTFB from the web-vitals library, so we skip the test if that's the case.
13+
* Note: There is another test that covers that we actually report TTFB if it is valid (@see ../web-vitals-lcp/test.ts).
14+
*/
15+
sentryTest('paint web vitals values are greater than TTFB', async ({ browserName, getLocalTestPath, page }) => {
16+
// Only run in chromium to ensure all vitals are present
17+
if (shouldSkipTracingTest() || browserName !== 'chromium') {
18+
sentryTest.skip();
19+
}
20+
21+
page.route('**', route => route.continue());
22+
page.route('**/path/to/image.png', async (route: Route) => {
23+
return route.fulfill({ path: `${__dirname}/assets/sentry-logo-600x179.png` });
24+
});
25+
26+
const url = await getLocalTestPath({ testDir: __dirname });
27+
const [eventData] = await Promise.all([
28+
getFirstSentryEnvelopeRequest<Event>(page),
29+
page.goto(url),
30+
page.locator('button').click(),
31+
page.waitForTimeout(2000),
32+
// page.waitForFunction(() => {}),
33+
]);
34+
35+
expect(eventData.measurements).toBeDefined();
36+
37+
const ttfbValue = eventData.measurements?.ttfb?.value;
38+
39+
if (!ttfbValue) {
40+
// TTFB is unfortunately quite flaky. Sometimes, the web-vitals library doesn't report TTFB because
41+
// responseStart is 0. This seems to happen somewhat randomly, so we just ignore this in that case.
42+
// @see packages/browser-utils/src/metrics/web-vitals/onTTFB
43+
44+
// logging the skip reason so that we at least can check for that in CI logs
45+
// eslint-disable-next-line no-console
46+
console.log('SKIPPING: TTFB is not reported');
47+
sentryTest.skip();
48+
}
49+
50+
const lcpValue = eventData.measurements?.lcp?.value;
51+
const fcpValue = eventData.measurements?.fcp?.value;
52+
const fpValue = eventData.measurements?.fp?.value;
53+
54+
expect(lcpValue).toBeDefined();
55+
expect(fcpValue).toBeDefined();
56+
expect(fpValue).toBeDefined();
57+
58+
// (LCP, FCP, FP) >= TTFB
59+
expect(lcpValue).toBeGreaterThanOrEqual(ttfbValue!);
60+
expect(fcpValue).toBeGreaterThanOrEqual(ttfbValue!);
61+
expect(fpValue).toBeGreaterThanOrEqual(ttfbValue!);
62+
});
63+
64+
/**
65+
* Continuing the theme of odd tests, in this one, we check that LCP is greater or equal to FCP and FP.
66+
*
67+
* The problem: There are cases where for _some reason_ the browser reports lower LCP than FCP/FP values :(
68+
* This might have to do with timing inaccuracies in the browser or with some weird bug in the PerformanceObserver
69+
* or Web vitals library. While this shouldn't happen, checking that they're not _vastly_ off is at least better
70+
* than not checking at all, so we factor in a margin of error.
71+
*/
72+
sentryTest('LCP >= (FCP, FP)', async ({ browserName, getLocalTestPath, page }) => {
73+
// Only run in chromium to ensure all vitals are present
74+
if (shouldSkipTracingTest() || browserName !== 'chromium') {
75+
sentryTest.skip();
76+
}
77+
78+
page.route('**', route => route.continue());
79+
page.route('**/path/to/image.png', async (route: Route) => {
80+
return route.fulfill({ path: `${__dirname}/assets/sentry-logo-600x179.png` });
81+
});
82+
83+
const url = await getLocalTestPath({ testDir: __dirname });
84+
const [eventData] = await Promise.all([
85+
getFirstSentryEnvelopeRequest<Event>(page),
86+
page.goto(url),
87+
page.locator('button').click(),
88+
page.waitForTimeout(2000),
89+
]);
90+
91+
expect(eventData.measurements).toBeDefined();
92+
93+
const lcpValue = eventData.measurements?.lcp?.value;
94+
const fcpValue = eventData.measurements?.fcp?.value;
95+
const fpValue = eventData.measurements?.fp?.value;
96+
97+
expect(lcpValue).toBeDefined();
98+
expect(fcpValue).toBeDefined();
99+
expect(fpValue).toBeDefined();
100+
101+
// Assumption: The browser can render at 60FPS which equals 1 frame every 16.6ms.
102+
// Rounded up, 20ms seems like a reasonable margin of error.
103+
const epsilon = 20;
104+
105+
// LCP >= (FCP, FP)
106+
expect(lcpValue).toBeGreaterThanOrEqual(fcpValue! - epsilon);
107+
expect(lcpValue).toBeGreaterThanOrEqual(fpValue! - epsilon);
108+
});

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

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -354,25 +354,6 @@ export function addPerformanceEntries(span: Span, options: AddPerformanceEntries
354354
if (op === 'pageload') {
355355
_addTtfbRequestTimeToMeasurements(_measurements);
356356

357-
['fcp', 'fp', 'lcp'].forEach(name => {
358-
const measurement = _measurements[name];
359-
if (!measurement || !transactionStartTime || timeOrigin >= transactionStartTime) {
360-
return;
361-
}
362-
// The web vitals, fcp, fp, lcp, and ttfb, all measure relative to timeOrigin.
363-
// Unfortunately, timeOrigin is not captured within the span span data, so these web vitals will need
364-
// to be adjusted to be relative to span.startTimestamp.
365-
const oldValue = measurement.value;
366-
const measurementTimestamp = timeOrigin + msToSec(oldValue);
367-
368-
// normalizedValue should be in milliseconds
369-
const normalizedValue = Math.abs((measurementTimestamp - transactionStartTime) * 1000);
370-
const delta = normalizedValue - oldValue;
371-
372-
DEBUG_BUILD && logger.log(`[Measurements] Normalized ${name} from ${oldValue} to ${normalizedValue} (${delta})`);
373-
measurement.value = normalizedValue;
374-
});
375-
376357
const fidMark = _measurements['mark.fid'];
377358
if (fidMark && _measurements['fid']) {
378359
// create span for FID
@@ -399,7 +380,10 @@ export function addPerformanceEntries(span: Span, options: AddPerformanceEntries
399380
setMeasurement(measurementName, measurement.value, measurement.unit);
400381
});
401382

402-
_tagMetricInfo(span);
383+
// Set timeOrigin which denotes the timestamp which to base the LCP/FCP/FP/TTFB measurements on
384+
span.setAttribute('web-vitals.timeOrigin', timeOrigin);
385+
386+
_setWebVitalAttributes(span);
403387
}
404388

405389
_lcpEntry = undefined;
@@ -604,7 +588,7 @@ function _trackNavigator(span: Span): void {
604588
}
605589

606590
/** Add LCP / CLS data to span to allow debugging */
607-
function _tagMetricInfo(span: Span): void {
591+
function _setWebVitalAttributes(span: Span): void {
608592
if (_lcpEntry) {
609593
DEBUG_BUILD && logger.log('[Measurements] Adding LCP Data');
610594

0 commit comments

Comments
 (0)