Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(browser): Ensure Standalone CLS span timestamps are correct #13649

Merged
merged 2 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 `enntry.duration` here
Lms24 marked this conversation as resolved.
Show resolved Hide resolved
// 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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a tiny bundle size improvement :)

try {
// @ts-expect-error skip optional chaining to save bundle size with try catch
profileId = scope.getScopeData().contexts.profile.profile_id;
Expand Down
Loading