Skip to content

feat(browser): Send additional LCP timing info #14372

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 21, 2024
Merged

feat(browser): Send additional LCP timing info #14372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f11c040
feat(browser): Send additional LCP timing info
Lms24 Nov 19, 2024
437d21a
add additional test
Lms24 Nov 19, 2024
3b24913
fix definedness check
Lms24 Nov 20, 2024
78f8ff6
fix test
Lms24 Nov 20, 2024
917d09e
maybe fix test flakiness?
Lms24 Nov 20, 2024
a90c628
fix e2e test
Lms24 Nov 20, 2024
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
1 change: 0 additions & 1 deletion dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,5 @@
<body>
<div id="content"></div>
<img src="https://example.com/my/image.png" />
<button type="button">Test button</button>
</body>
</html>
56 changes: 48 additions & 8 deletions dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ import type { Event } from '@sentry/types';
import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest('should capture a LCP vital with element details.', async ({ browserName, getLocalTestUrl, page }) => {
/*
Because we "serve" the html test page as a static file, all requests for the image
are considered 3rd party requests. So the LCP value we obtain for the image is also
considered a 3rd party LCP value, meaning `renderTime` is only set if we also
return the `Timing-Allow-Origin` header.
*/

sentryTest('captures LCP vitals with element details.', async ({ browserName, getLocalTestUrl, page }) => {
if (shouldSkipTracingTest() || browserName !== 'chromium') {
sentryTest.skip();
}
Expand All @@ -16,17 +23,50 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN
});

const url = await getLocalTestUrl({ testDir: __dirname });
const [eventData] = await Promise.all([
getFirstSentryEnvelopeRequest<Event>(page),
page.goto(url),
page.locator('button').click(),
]);
const [eventData] = await Promise.all([getFirstSentryEnvelopeRequest<Event>(page), page.goto(url)]);

expect(eventData.measurements).toBeDefined();
expect(eventData.measurements?.lcp?.value).toBeDefined();

// XXX: This should be body > img, but it can be flakey as sometimes it will report
// the button as LCP.
expect(eventData.contexts?.trace?.data?.['lcp.element'].startsWith('body >')).toBe(true);
expect(eventData.contexts?.trace?.data?.['lcp.size']).toBeGreaterThan(0);
expect(eventData.contexts?.trace?.data?.['lcp.loadTime']).toBeGreaterThan(0);

// renderTime is 0 because we don't return the `Timing-Allow-Origin` header
// and the image is loaded from a 3rd party origin
expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBe(0);

// The LCP value should be the loadTime because the renderTime is not set
expect(eventData.measurements?.lcp?.value).toBeCloseTo(eventData.contexts?.trace?.data?.['lcp.loadTime']);
});

sentryTest(
'captures LCP renderTime when returning Timing-Allow-Origin header.',
async ({ browserName, getLocalTestUrl, page }) => {
if (shouldSkipTracingTest() || browserName !== 'chromium') {
sentryTest.skip();
}

page.route('**', route => route.continue());
page.route('**/my/image.png', async (route: Route) => {
return route.fulfill({
path: `${__dirname}/assets/sentry-logo-600x179.png`,
headers: { 'Timing-Allow-Origin': '*' },
});
});

const url = await getLocalTestUrl({ testDir: __dirname });
const [eventData] = await Promise.all([getFirstSentryEnvelopeRequest<Event>(page), page.goto(url)]);

expect(eventData.measurements).toBeDefined();
expect(eventData.measurements?.lcp?.value).toBeDefined();

expect(eventData.contexts?.trace?.data?.['lcp.element'].startsWith('body >')).toBe(true);
expect(eventData.contexts?.trace?.data?.['lcp.size']).toBeGreaterThan(0);
expect(eventData.contexts?.trace?.data?.['lcp.loadTime']).toBeGreaterThan(0);
expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBeGreaterThan(0);

// The LCP value should be the renderTime because the renderTime is set
expect(eventData.measurements?.lcp?.value).toBeCloseTo(eventData.contexts?.trace?.data?.['lcp.renderTime']);
},
);
2 changes: 2 additions & 0 deletions dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ test('Captures a pageload transaction', async ({ page }) => {
'sentry.source': 'route',
'performance.timeOrigin': expect.any(Number),
'performance.activationStart': expect.any(Number),
'lcp.renderTime': expect.any(Number),
'lcp.loadTime': expect.any(Number),
},
op: 'pageload',
span_id: expect.any(String),
Expand Down
12 changes: 12 additions & 0 deletions packages/browser-utils/src/metrics/browserMetrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,18 @@ function _setWebVitalAttributes(span: Span): void {
span.setAttribute('lcp.url', _lcpEntry.url.trim().slice(0, 200));
}

if (_lcpEntry.loadTime != null) {
Copy link
Member

@s1gr1d s1gr1d Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is != on purpose or do we want !== here? (also for renderTime)

Copy link
Member Author

@Lms24 Lms24 Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I only want to add the attribute it loadTime is not null or undefined. != null is a shorthand way of checking for that (and a bit more size efficient than !== undefined)

// loadTime is the time of LCP that's related to receiving the LCP element response..
span.setAttribute('lcp.loadTime', _lcpEntry.loadTime);
}

if (_lcpEntry.renderTime != null) {
// renderTime is loadTime + rendering time
// it's 0 if the LCP element is loaded from a 3rd party origin that doesn't send the
// `Timing-Allow-Origin` header.
span.setAttribute('lcp.renderTime', _lcpEntry.renderTime);
}

span.setAttribute('lcp.size', _lcpEntry.size);
}

Expand Down
Loading