Skip to content

Commit

Permalink
fix(browser): Ensure Standalone CLS span timestamps are correct (#13649)
Browse files Browse the repository at this point in the history
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 <aprasad@sentry.io>
  • Loading branch information
Lms24 and AbhiPrasad authored Sep 10, 2024
1 parent 1285e4b commit a2d1b2c
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<SentryEvent>(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<SpanEnvelope>(
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);
});
7 changes: 4 additions & 3 deletions packages/browser-utils/src/metrics/cls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion packages/browser-utils/src/metrics/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit a2d1b2c

Please sign in to comment.