From 039aa1d45bb86a8b0a52fa30489cb4f292e59e3c Mon Sep 17 00:00:00 2001
From: edwardgou-sentry <83961295+edwardgou-sentry@users.noreply.github.com>
Date: Thu, 6 Jun 2024 10:55:06 -0400
Subject: [PATCH] =?UTF-8?q?fix(performance):=20Fix=20LCP=20not=20getting?=
 =?UTF-8?q?=20picked=20up=20on=20initial=20pageload=20transaction=C2=A0by?=
 =?UTF-8?q?=20setting=20reportAllChanges=20to=20true=20(#12360)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

There's currently an issue where initial pageloads frequently do not
capture LCP. This is because LCP is only reported when there is a user
interaction on the page, or the user triggers a pagehide. Either of
these events must happen within the initial pageload transaction
boundary for the LCP value to be capture.

Previously, in v7, we added custom code the the vendored LCP library to
force report when closing the initial pageload transaction. This custom
code was removed when we moved to v8.

In this change, we are using the `reportAllChanges` option, so that any
time a new LCP is detected, we add the LCP value to the initial pageload
transaction, as long as the transaction has not closed yet.

This solution is no worse than the previous solution in v7 because:
- The risk of reporting an early LCP is the same because in either
situation this is completely dependant on when the pageload transaction
closes, which this PR does not change.
- using `reportAllChanges`, we always go with the longest LCP recorded

- We avoid introducing custom code to the vendored LCP library, and we
use the intended `reportAllChanges`, so there's less risk of introducing
a regression the next time we update the library.

It's not a perfect solution, but I think it's reasonable to go with this
until we come up with a better one (specifically to resolve the
span/transaction boundary issue).
---
 .../tests/transactions.test.ts                  |  3 +++
 .../browser-utils/src/metrics/instrument.ts     | 17 +++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

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 e39db36ea6cb..10442a4a2bde 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
@@ -14,6 +14,9 @@ test('Captures a pageload transaction', async ({ page }) => {
       deviceMemory: expect.any(String),
       effectiveConnectionType: expect.any(String),
       hardwareConcurrency: expect.any(String),
+      'lcp.element': 'body > div#root > input#exception-button[type="button"]',
+      'lcp.id': 'exception-button',
+      'lcp.size': 1650,
       'sentry.idle_span_finish_reason': 'idleTimeout',
       'sentry.op': 'pageload',
       'sentry.origin': 'auto.pageload.react.reactrouter_v6',
diff --git a/packages/browser-utils/src/metrics/instrument.ts b/packages/browser-utils/src/metrics/instrument.ts
index acd3717e8bd4..06a27c4225ff 100644
--- a/packages/browser-utils/src/metrics/instrument.ts
+++ b/packages/browser-utils/src/metrics/instrument.ts
@@ -225,12 +225,17 @@ function instrumentFid(): void {
 }
 
 function instrumentLcp(): StopListening {
-  return onLCP(metric => {
-    triggerHandlers('lcp', {
-      metric,
-    });
-    _previousLcp = metric;
-  });
+  return onLCP(
+    metric => {
+      triggerHandlers('lcp', {
+        metric,
+      });
+      _previousLcp = metric;
+    },
+    // We want the callback to be called whenever the LCP value updates.
+    // By default, the callback is only called when the tab goes to the background.
+    { reportAllChanges: true },
+  );
 }
 
 function instrumentTtfb(): StopListening {