From f11c040868f2b0efbd5e3d796a763c7b19bf5a3a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 19 Nov 2024 16:40:08 -0500 Subject: [PATCH 1/6] feat(browser): Send additional LCP timing info --- .../suites/tracing/metrics/web-vitals-lcp/test.ts | 2 ++ packages/browser-utils/src/metrics/browserMetrics.ts | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts index 50a309d22272..afaeb6ed4c88 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts @@ -29,4 +29,6 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN // 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.renderTime']).toBeGreaterThan(0); + expect(eventData.contexts?.trace?.data?.['lcp.loadTime']).toBeGreaterThan(0); }); diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index 9d7faeb4e9c4..853e0d72d713 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -635,6 +635,18 @@ function _setWebVitalAttributes(span: Span): void { span.setAttribute('lcp.url', _lcpEntry.url.trim().slice(0, 200)); } + if (_lcpEntry.renderTime) { + // renderTime is the time portion of LCP that's related to rendering the LCP element. + // 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); + } + + if (_lcpEntry.loadTime) { + // loadTime is the time portion of LCP that's related to loading the LCP element. + span.setAttribute('lcp.loadTime', _lcpEntry.loadTime); + } + span.setAttribute('lcp.size', _lcpEntry.size); } From 437d21a2b5708623487089581e9bc34a56be8b5e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 19 Nov 2024 17:01:16 -0500 Subject: [PATCH 2/6] add additional test --- .../tracing/metrics/web-vitals-lcp/test.ts | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts index afaeb6ed4c88..ef030d299ebd 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts @@ -29,6 +29,46 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN // 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.renderTime']).toBeGreaterThan(0); expect(eventData.contexts?.trace?.data?.['lcp.loadTime']).toBeGreaterThan(0); + + // renderTime is not set because we do not return the `Timing-Allow-Origin` header + // and the image is loaded from a 3rd party origin + expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBeUndefined(); }); + +sentryTest( + 'captures LCP renderTime when returning Timing-Allow-Origin header.', + async ({ browserName, getLocalTestPath, 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 getLocalTestPath({ testDir: __dirname }); + const [eventData] = await Promise.all([ + getFirstSentryEnvelopeRequest(page), + page.goto(url), + page.locator('button').click(), + ]); + + 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 not set because we do not return the `Timing-Allow-Origin` header + // and the image is loaded from a 3rd party origin + expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBeGreaterThan(0); + }, +); From 3b2491390ca8407cb7fdc5163c5753a11884a6b9 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 19 Nov 2024 21:46:02 -0500 Subject: [PATCH 3/6] fix definedness check --- .../tracing/metrics/web-vitals-lcp/test.ts | 26 ++++++++++++------- .../src/metrics/browserMetrics.ts | 14 +++++----- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts index ef030d299ebd..d22382444c95 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts @@ -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(); } @@ -25,8 +32,6 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN 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); @@ -34,11 +39,14 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN // renderTime is not set because we do not return the `Timing-Allow-Origin` header // and the image is loaded from a 3rd party origin expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBeUndefined(); + + // 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, getLocalTestPath, page }) => { + async ({ browserName, getLocalTestUrl, page }) => { if (shouldSkipTracingTest() || browserName !== 'chromium') { sentryTest.skip(); } @@ -51,7 +59,7 @@ sentryTest( }); }); - const url = await getLocalTestPath({ testDir: __dirname }); + const url = await getLocalTestUrl({ testDir: __dirname }); const [eventData] = await Promise.all([ getFirstSentryEnvelopeRequest(page), page.goto(url), @@ -61,14 +69,12 @@ sentryTest( 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 not set because we do not return the `Timing-Allow-Origin` header - // and the image is loaded from a 3rd party origin 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']); }, ); diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index 853e0d72d713..d64bba34509a 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -635,18 +635,18 @@ function _setWebVitalAttributes(span: Span): void { span.setAttribute('lcp.url', _lcpEntry.url.trim().slice(0, 200)); } - if (_lcpEntry.renderTime) { - // renderTime is the time portion of LCP that's related to rendering the LCP element. + if (_lcpEntry.loadTime != null) { + // 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); } - if (_lcpEntry.loadTime) { - // loadTime is the time portion of LCP that's related to loading the LCP element. - span.setAttribute('lcp.loadTime', _lcpEntry.loadTime); - } - span.setAttribute('lcp.size', _lcpEntry.size); } From 78f8ff687197ae5de36d97d0fa906db32892e5be Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 20 Nov 2024 09:35:47 -0500 Subject: [PATCH 4/6] fix test --- .../suites/tracing/metrics/web-vitals-lcp/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts index d22382444c95..99f5693c1524 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts @@ -36,9 +36,9 @@ sentryTest('captures LCP vitals with element details.', async ({ browserName, ge expect(eventData.contexts?.trace?.data?.['lcp.size']).toBeGreaterThan(0); expect(eventData.contexts?.trace?.data?.['lcp.loadTime']).toBeGreaterThan(0); - // renderTime is not set because we do not return the `Timing-Allow-Origin` header + // 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']).toBeUndefined(); + 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']); From 917d09e5b09416cd1472868d89844fa13ddd2d31 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 20 Nov 2024 09:59:35 -0500 Subject: [PATCH 5/6] maybe fix test flakiness? --- .../tracing/metrics/web-vitals-lcp/template.html | 1 - .../suites/tracing/metrics/web-vitals-lcp/test.ts | 12 ++---------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html index 502f4dde80c2..bf7617ea8709 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html @@ -6,6 +6,5 @@
- diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts index 99f5693c1524..84e93b54ac9b 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts @@ -23,11 +23,7 @@ sentryTest('captures LCP vitals with element details.', async ({ browserName, ge }); const url = await getLocalTestUrl({ testDir: __dirname }); - const [eventData] = await Promise.all([ - getFirstSentryEnvelopeRequest(page), - page.goto(url), - page.locator('button').click(), - ]); + const [eventData] = await Promise.all([getFirstSentryEnvelopeRequest(page), page.goto(url)]); expect(eventData.measurements).toBeDefined(); expect(eventData.measurements?.lcp?.value).toBeDefined(); @@ -60,11 +56,7 @@ sentryTest( }); const url = await getLocalTestUrl({ testDir: __dirname }); - const [eventData] = await Promise.all([ - getFirstSentryEnvelopeRequest(page), - page.goto(url), - page.locator('button').click(), - ]); + const [eventData] = await Promise.all([getFirstSentryEnvelopeRequest(page), page.goto(url)]); expect(eventData.measurements).toBeDefined(); expect(eventData.measurements?.lcp?.value).toBeDefined(); From a90c6283b20c10362078cde98b1d00613a18e637 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 20 Nov 2024 14:12:38 -0500 Subject: [PATCH 6/6] fix e2e test --- .../react-create-hash-router/tests/transactions.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts index cc5edc1fd878..7fd15646dff2 100644 --- a/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts @@ -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),