Skip to content

Commit

Permalink
fix(performance): Fix LCP not getting picked up on initial pageload t…
Browse files Browse the repository at this point in the history
…ransaction by setting reportAllChanges to true (#12360)

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).
  • Loading branch information
edwardgou-sentry authored Jun 6, 2024
1 parent e84230e commit 039aa1d
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
17 changes: 11 additions & 6 deletions packages/browser-utils/src/metrics/instrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 039aa1d

Please sign in to comment.