From a2d1b2c37f4f69a1dfc4778b2dbe52643a43e14b Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 10 Sep 2024 15:07:37 +0200 Subject: [PATCH] fix(browser): Ensure Standalone CLS span timestamps are correct (#13649) Fix a bug in the initial experimental CLS standalone span implementation. Previously we'd add the CLS start timestamp value in ms to the performance time origin timestamp which was already converted to seconds. Ensure that we first add time origin and the CLS start timestamp and then convert to seconds --------- Co-authored-by: Abhijeet Prasad --- .../web-vitals-cls-standalone-spans/test.ts | 41 +++++++++++++++++++ packages/browser-utils/src/metrics/cls.ts | 7 ++-- packages/browser-utils/src/metrics/utils.ts | 2 +- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-standalone-spans/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-standalone-spans/test.ts index cdf1e6837ef4..6defe804e665 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-standalone-spans/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls-standalone-spans/test.ts @@ -453,3 +453,44 @@ sentryTest("doesn't send further CLS after the first page hide", async ({ getLoc // a timeout or something similar. await navigationTxnPromise; }); + +sentryTest('CLS span timestamps are set correctly', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.type).toBe('transaction'); + expect(eventData.contexts?.trace?.op).toBe('pageload'); + expect(eventData.timestamp).toBeDefined(); + + const pageloadEndTimestamp = eventData.timestamp!; + + const spanEnvelopePromise = getMultipleSentryEnvelopeRequests( + page, + 1, + { envelopeType: 'span' }, + properFullEnvelopeRequestParser, + ); + + await triggerAndWaitForLayoutShift(page); + + await hidePage(page); + + const spanEnvelope = (await spanEnvelopePromise)[0]; + const spanEnvelopeItem = spanEnvelope[1][0][1]; + + expect(spanEnvelopeItem.start_timestamp).toBeDefined(); + expect(spanEnvelopeItem.timestamp).toBeDefined(); + + const clsSpanStartTimestamp = spanEnvelopeItem.start_timestamp!; + const clsSpanEndTimestamp = spanEnvelopeItem.timestamp!; + + // CLS performance entries have no duration ==> start and end timestamp should be the same + expect(clsSpanStartTimestamp).toEqual(clsSpanEndTimestamp); + + // We don't really care that they are very close together but rather about the order of magnitude + // Previously, we had a bug where the timestamps would be significantly off (by multiple hours) + // so we only ensure that this bug is fixed. 60 seconds should be more than enough. + expect(clsSpanStartTimestamp - pageloadEndTimestamp).toBeLessThan(60); + expect(clsSpanStartTimestamp).toBeGreaterThan(pageloadEndTimestamp); +}); diff --git a/packages/browser-utils/src/metrics/cls.ts b/packages/browser-utils/src/metrics/cls.ts index aa25a54754a1..e1d13286f5f9 100644 --- a/packages/browser-utils/src/metrics/cls.ts +++ b/packages/browser-utils/src/metrics/cls.ts @@ -84,8 +84,7 @@ export function trackClsAsStandaloneSpan(): void { function sendStandaloneClsSpan(clsValue: number, entry: LayoutShift | undefined, pageloadSpanId: string) { DEBUG_BUILD && logger.log(`Sending CLS span (${clsValue})`); - const startTime = msToSec(browserPerformanceTimeOrigin as number) + (entry?.startTime || 0); - const duration = msToSec(entry?.duration || 0); + const startTime = msToSec((browserPerformanceTimeOrigin || 0) + (entry?.startTime || 0)); const routeName = getCurrentScope().getScopeData().transactionName; const name = entry ? htmlTreeAsString(entry.sources[0]?.node) : 'Layout shift'; @@ -110,7 +109,9 @@ function sendStandaloneClsSpan(clsValue: number, entry: LayoutShift | undefined, [SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE]: clsValue, }); - span?.end(startTime + duration); + // LayoutShift performance entries always have a duration of 0, so we don't need to add `entry.duration` here + // see: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry/duration + span?.end(startTime); } function supportsLayoutShift(): boolean { diff --git a/packages/browser-utils/src/metrics/utils.ts b/packages/browser-utils/src/metrics/utils.ts index 5f9d0de4d4ab..70327aeca838 100644 --- a/packages/browser-utils/src/metrics/utils.ts +++ b/packages/browser-utils/src/metrics/utils.ts @@ -86,7 +86,7 @@ export function startStandaloneWebVitalSpan(options: StandaloneWebVitalSpanOptio const user = scope.getUser(); const userDisplay = user !== undefined ? user.email || user.id || user.ip_address : undefined; - let profileId: string | undefined = undefined; + let profileId: string | undefined; try { // @ts-expect-error skip optional chaining to save bundle size with try catch profileId = scope.getScopeData().contexts.profile.profile_id;