Skip to content

feat: Report LCP metric on pageload transactions #2624

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 11 commits into from
Jun 8, 2020
Merged

feat: Report LCP metric on pageload transactions #2624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dd6758a
feat: Report LCP metric on pageload transactions
rhcarvalho May 28, 2020
91948bb
ref: Move LCP tracking to static side of Tracing
rhcarvalho Jun 4, 2020
64ab997
feat: Increase default normalizeDepth
rhcarvalho Jun 4, 2020
ec371db
Revert "feat: Increase default normalizeDepth"
rhcarvalho Jun 4, 2020
19a6723
feat: Remove normalization of contexts
rhcarvalho Jun 4, 2020
eace0c2
misc: CHANGELOG
rhcarvalho Jun 4, 2020
9d114cc
Revert "feat: Remove normalization of contexts"
rhcarvalho Jun 5, 2020
66d2329
misc: Update type definition
rhcarvalho Jun 5, 2020
3f9e921
misc: Rename keys leading up to LCP metric
rhcarvalho Jun 5, 2020
0fd278d
fix: Include elementId and elementSize only when available
rhcarvalho Jun 8, 2020
c98d924
Update CHANGELOG.md
rhcarvalho Jun 8, 2020
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: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
- [react] feat: Add @sentry/react package (#2631)
- [browser] Change XHR instrumentation order to handle `onreadystatechange` breadcrumbs correctly (#2643)
- [apm] fix: Re-add TraceContext for all events (#2656)
- [apm] feat: Report LCP metric on pageload transactions (#2624)

## 5.16.1

Expand Down
84 changes: 83 additions & 1 deletion packages/apm/src/integrations/tracing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ export class Tracing implements Integration {

private static _heartbeatCounter: number = 0;

/** Holds the latest LargestContentfulPaint value (it changes during page load). */
private static _lcp?: { [key: string]: any };

/** Force any pending LargestContentfulPaint records to be dispatched. */
private static _forceLCP = () => {
/* No-op, replaced later if LCP API is available. */
};

/**
* Constructor for Tracing
*
Expand All @@ -163,6 +171,7 @@ export class Tracing implements Integration {
public constructor(_options?: Partial<TracingOptions>) {
if (global.performance) {
global.performance.mark('sentry-tracing-init');
Tracing._trackLCP();
}
const defaults = {
debug: {
Expand Down Expand Up @@ -450,7 +459,7 @@ export class Tracing implements Integration {
}

/**
* Finshes the current active transaction
* Finishes the current active transaction
*/
public static finishIdleTransaction(endTimestamp: number): void {
const active = Tracing._activeTransaction;
Expand Down Expand Up @@ -508,6 +517,16 @@ export class Tracing implements Integration {

Tracing._log('[Tracing] Adding & adjusting spans using Performance API');

// FIXME: depending on the 'op' directly is brittle.
if (transactionSpan.op === 'pageload') {
Comment on lines +520 to +521
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see an obvious quick fix for this brittleness.

The problem:

  • Any transaction could be called pageload, and that should not mean it should report the LCP;
  • The "pageload" transaction could be called something else, and we'd miss the LCP;

Copy link
Member

Choose a reason for hiding this comment

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

We could add additional things into the data property and check for that but tbh I think for now this is enough.

// Force any pending records to be dispatched.
Tracing._forceLCP();
if (Tracing._lcp) {
// Set the last observed LCP score.
transactionSpan.setData('_sentry_web_vitals', { LCP: Tracing._lcp });
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the reference implementation sends the LCP when the page visibility changes to hidden. That happens when a user switches tabs, navigates away, etc.

Instead here, as per Daniel's suggestion, we send the current known LCP at the time of sending the "idle transaction".
Local exploratory tests suggest that it is easy to miss the proper LCP entry (either get nothing or get an earlier element).

We may discuss this behavior and decide whether we're happy with it for starters.

Two alternative ways forward are delaying the submission of the pageload transaction (wait for page visibility hidden event) or sending a separate transaction for LCP (in which case we would benefit from a being able to send a trimmed down payload, e.g. without breadcrumbs and other non-essential or redundant data).

Snippet from the reference implementation

image


const timeOrigin = Tracing._msToSec(performance.timeOrigin);

// tslint:disable-next-line: completed-docs
Expand Down Expand Up @@ -632,6 +651,69 @@ export class Tracing implements Integration {
// tslint:enable: no-unsafe-any
}

/**
* Starts tracking the Largest Contentful Paint on the current page.
*/
private static _trackLCP(): void {
// Based on reference implementation from https://web.dev/lcp/#measure-lcp-in-javascript.

// Use a try/catch instead of feature detecting `largest-contentful-paint`
// support, since some browsers throw when using the new `type` option.
// https://bugs.webkit.org/show_bug.cgi?id=209216
try {
// Keep track of whether (and when) the page was first hidden, see:
// https://github.com/w3c/page-visibility/issues/29
// NOTE: ideally this check would be performed in the document <head>
// to avoid cases where the visibility state changes before this code runs.
let firstHiddenTime = document.visibilityState === 'hidden' ? 0 : Infinity;
document.addEventListener(
'visibilitychange',
event => {
firstHiddenTime = Math.min(firstHiddenTime, event.timeStamp);
},
{ once: true },
);

const updateLCP = (entry: PerformanceEntry) => {
// Only include an LCP entry if the page wasn't hidden prior to
// the entry being dispatched. This typically happens when a page is
// loaded in a background tab.
if (entry.startTime < firstHiddenTime) {
// NOTE: the `startTime` value is a getter that returns the entry's
// `renderTime` value, if available, or its `loadTime` value otherwise.
// The `renderTime` value may not be available if the element is an image
// that's loaded cross-origin without the `Timing-Allow-Origin` header.
Tracing._lcp = {
// @ts-ignore
...(entry.id && { elementId: entry.id }),
// @ts-ignore
...(entry.size && { elementSize: entry.size }),
value: entry.startTime,
};
}
};

// Create a PerformanceObserver that calls `updateLCP` for each entry.
const po = new PerformanceObserver(entryList => {
entryList.getEntries().forEach(updateLCP);
});

// Observe entries of type `largest-contentful-paint`, including buffered entries,
// i.e. entries that occurred before calling `observe()` below.
po.observe({
buffered: true,
// @ts-ignore
type: 'largest-contentful-paint',
});

Tracing._forceLCP = () => {
po.takeRecords().forEach(updateLCP);
};
} catch (e) {
// Do nothing if the browser doesn't support this API.
}
}

/**
* Sets the status of the current active transaction (if there is one)
*/
Expand Down