fix definedness check

getsentry · Nov 20, 2024 · d0e0bac · d0e0bac
1 parent 4452e24
commit d0e0bac
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 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
@@ -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,20 +32,21 @@ 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);
 
   // 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<Event>(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
@@ -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);
   }