From 2c4c631dd944f1841d377a671234e682e5432cec Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 19 Jul 2024 15:57:26 +0200 Subject: [PATCH 1/4] fix(browser): Set artificial zero-value CLS measurement to report no layout shift --- .../template.html | 11 +++++ .../web-vitals-cls-no-layout-shift/test.ts | 41 +++++++++++++++++++ .../src/metrics/browserMetrics.ts | 37 ++++++++++++++++- 3 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-no-layout-shift/template.html create mode 100644 dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-no-layout-shift/test.ts diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-no-layout-shift/template.html b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-no-layout-shift/template.html new file mode 100644 index 000000000000..4245edf74abb --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-no-layout-shift/template.html @@ -0,0 +1,11 @@ + + + + + + +
+ Some content +
+ + diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-no-layout-shift/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-no-layout-shift/test.ts new file mode 100644 index 000000000000..c1b83ce6e447 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-no-layout-shift/test.ts @@ -0,0 +1,41 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; + +sentryTest.beforeEach(async ({ page }) => { + await page.setViewportSize({ width: 800, height: 1200 }); +}); + +sentryTest('captures 0 CLS if the browser supports reporting CLS', async ({ getLocalTestPath, page, browserName }) => { + if (shouldSkipTracingTest() || browserName !== 'chromium') { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + const transactionEvent = await getFirstSentryEnvelopeRequest(page, url); + + expect(transactionEvent.measurements).toBeDefined(); + expect(transactionEvent.measurements?.cls?.value).toBe(0); + + // but no source entry (no source if there is no layout shift) + expect(transactionEvent.contexts?.trace?.data?.['cls.source.1']).toBeUndefined(); +}); + +sentryTest( + "doesn't capture 0 CLS if the browser doesn't support reporting CLS", + async ({ getLocalTestPath, page, browserName }) => { + if (shouldSkipTracingTest() || browserName === 'chromium') { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + const transactionEvent = await getFirstSentryEnvelopeRequest(page, `${url}#no-cls`); + + expect(transactionEvent.measurements).toBeDefined(); + expect(transactionEvent.measurements?.cls).toBeUndefined(); + + expect(transactionEvent.contexts?.trace?.data?.['cls.source.1']).toBeUndefined(); + }, +); diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index 02c044322bd3..af51d038028e 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -2,7 +2,14 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, getActiveSpan, startInactiveSpan } from '@sentry/core'; import { setMeasurement } from '@sentry/core'; import type { Measurements, Span, SpanAttributes, StartSpanOptions } from '@sentry/types'; -import { browserPerformanceTimeOrigin, getComponentName, htmlTreeAsString, logger, parseUrl } from '@sentry/utils'; +import { + browserPerformanceTimeOrigin, + consoleSandbox, + getComponentName, + htmlTreeAsString, + logger, + parseUrl, +} from '@sentry/utils'; import { spanToJSON } from '@sentry/core'; import { DEBUG_BUILD } from '../debug-build'; @@ -215,6 +222,8 @@ export { startTrackingINP, registerInpInteractionListener } from './inp'; /** Starts tracking the Cumulative Layout Shift on the current page. */ function _trackCLS(): () => void { + trySetZeroClsValue(); + return addClsInstrumentationHandler(({ metric }) => { const entry = metric.entries[metric.entries.length - 1]; if (!entry) { @@ -227,6 +236,32 @@ function _trackCLS(): () => void { }, true); } +/** + * Why does this function exist? A very good question! + * + * The `PerformanceObserver` emits `LayoutShift` entries whenever a layout shift occurs. + * If none occurs (which is great!), the observer will never emit any entries. Makes sense so far! + * + * This is problematic for the Sentry product though. We can't differentiate between a CLS of 0 and not having received + * CLS data at all. So in both cases, we'd show users that the CLS score simply is not available. When in fact, it can + * be 0, which is a very good score. This function is a workaround to emit a CLS of 0 right at the start of + * listening to CLS events. This way, we can differentiate between a CLS of 0 and no CLS at all. If a layout shift + * occurs later, the real CLS value will be emitted and the 0 value will be ignored. + * We also only send this artificial 0 value if the browser supports reporting the `layout-shift` entry type. + */ +function trySetZeroClsValue(): void { + try { + const canReportLayoutShift = PerformanceObserver.supportedEntryTypes.includes('layout-shift'); + if (canReportLayoutShift) { + DEBUG_BUILD && logger.log('[Measurements] Adding CLS 0'); + _measurements['cls'] = { value: 0, unit: '' }; + // TODO: Do we have to set _clsEntry here as well? If so, what attribution should we give it? + } + } catch { + // catching and ignoring access errors for bundle size reduction + } +} + /** Starts tracking the Largest Contentful Paint on the current page. */ function _trackLCP(): () => void { return addLcpInstrumentationHandler(({ metric }) => { From 0ded2651f0e1a91913ac5cec8ceeb7940af340e4 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 19 Jul 2024 16:08:21 +0200 Subject: [PATCH 2/4] formatting --- packages/browser-utils/src/metrics/browserMetrics.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index af51d038028e..b950985adfb6 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -2,14 +2,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, getActiveSpan, startInactiveSpan } from '@sentry/core'; import { setMeasurement } from '@sentry/core'; import type { Measurements, Span, SpanAttributes, StartSpanOptions } from '@sentry/types'; -import { - browserPerformanceTimeOrigin, - consoleSandbox, - getComponentName, - htmlTreeAsString, - logger, - parseUrl, -} from '@sentry/utils'; +import { browserPerformanceTimeOrigin, getComponentName, htmlTreeAsString, logger, parseUrl } from '@sentry/utils'; import { spanToJSON } from '@sentry/core'; import { DEBUG_BUILD } from '../debug-build'; From 9bcc7144761fbaae06f7afd3bafe025ce730a6dd Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 22 Jul 2024 10:10:23 +0200 Subject: [PATCH 3/4] cleanup --- packages/browser-utils/src/metrics/browserMetrics.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index b950985adfb6..fb183dc7a637 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -244,14 +244,12 @@ function _trackCLS(): () => void { */ function trySetZeroClsValue(): void { try { - const canReportLayoutShift = PerformanceObserver.supportedEntryTypes.includes('layout-shift'); - if (canReportLayoutShift) { + if (PerformanceObserver.supportedEntryTypes.includes('layout-shift')) { DEBUG_BUILD && logger.log('[Measurements] Adding CLS 0'); _measurements['cls'] = { value: 0, unit: '' }; - // TODO: Do we have to set _clsEntry here as well? If so, what attribution should we give it? } } catch { - // catching and ignoring access errors for bundle size reduction + // catching and ignoring access errors for bundle size minimization. } } From 9b7499678dc3bec92b1a86ee80c356845d239858 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 22 Jul 2024 10:10:28 +0200 Subject: [PATCH 4/4] size-limit --- .size-limit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.size-limit.js b/.size-limit.js index 2e7899cb934a..5a2ff44cc32d 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -48,7 +48,7 @@ module.exports = [ path: 'packages/browser/build/npm/esm/index.js', import: createImport('init', 'browserTracingIntegration', 'replayIntegration', 'replayCanvasIntegration'), gzip: true, - limit: '76 KB', + limit: '77 KB', }, { name: '@sentry/browser (incl. Tracing, Replay, Feedback)',