Skip to content

fix(tracing): Record LCP and CLS on transaction finish #7386

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 4 commits into from
Mar 9, 2023
Merged

fix(tracing): Record LCP and CLS on transaction finish #7386

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36ffb6d
fix(tracing): Record LCP on transaction finish
AbhiPrasad Mar 8, 2023
b652c2a
Merge branch 'develop' into abhi-force-finish-lcp
AbhiPrasad Mar 9, 2023
56919f9
feat: Also track CLS the same way
AbhiPrasad Mar 9, 2023
8e6d884
add test
AbhiPrasad Mar 9, 2023
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
2 changes: 0 additions & 2 deletions packages/integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN
const url = await getLocalTestPath({ testDir: __dirname });
await page.goto(url);

// Force closure of LCP listener.
await page.click('body');
const eventData = await getFirstSentryEnvelopeRequest<Event>(page);

expect(eventData.measurements).toBeDefined();
Expand Down
7 changes: 6 additions & 1 deletion packages/tracing/src/browser/browsertracing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ export class BrowserTracing implements Integration {
private _latestRouteName?: string;
private _latestRouteSource?: TransactionSource;

private _stopListeningForLCP?: () => void;

public constructor(_options?: Partial<BrowserTracingOptions>) {
this.options = {
...DEFAULT_BROWSER_TRACING_OPTIONS,
Expand All @@ -185,7 +187,7 @@ export class BrowserTracing implements Integration {
this.options.tracePropagationTargets = _options.tracingOrigins;
}

startTrackingWebVitals();
this._stopListeningForLCP = startTrackingWebVitals();
if (this.options.enableLongTask) {
startTrackingLongTasks();
}
Expand Down Expand Up @@ -303,6 +305,9 @@ export class BrowserTracing implements Integration {
heartbeatInterval,
);
idleTransaction.registerBeforeFinishCallback(transaction => {
if (this._stopListeningForLCP) {
this._stopListeningForLCP();
}
addPerformanceEntries(transaction);
});

Expand Down
10 changes: 6 additions & 4 deletions packages/tracing/src/browser/metrics/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,18 @@ let _clsEntry: LayoutShift | undefined;
/**
* Start tracking web vitals
*/
export function startTrackingWebVitals(): void {
export function startTrackingWebVitals(): ReturnType<typeof _trackLCP> {
const performance = getBrowserPerformanceAPI();
if (performance && browserPerformanceTimeOrigin) {
if (performance.mark) {
WINDOW.performance.mark('sentry-tracing-init');
}
_trackCLS();
_trackLCP();
_trackFID();
return _trackLCP();
Copy link
Member

Choose a reason for hiding this comment

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

Does CLS also have a stop listen? we should be returning all of them in a wrapper function and just settling them whenever we finish the transaction

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah that makes sense to me, will make the change.

}

return;
}

/**
Expand Down Expand Up @@ -89,8 +91,8 @@ function _trackCLS(): void {
}

/** Starts tracking the Largest Contentful Paint on the current page. */
function _trackLCP(): void {
onLCP(metric => {
function _trackLCP(): ReturnType<typeof onLCP> {
return onLCP(metric => {
const entry = metric.entries.pop();
if (!entry) {
return;
Expand Down
8 changes: 7 additions & 1 deletion packages/tracing/src/browser/web-vitals/getLCP.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@ import type { LCPMetric, ReportCallback } from './types';

const reportedMetricIDs: Record<string, boolean> = {};

type StopListening = () => void;

/**
* Calculates the [LCP](https://web.dev/lcp/) value for the current page and
* calls the `callback` function once the value is ready (along with the
* relevant `largest-contentful-paint` performance entry used to determine the
* value). The reported value is a `DOMHighResTimeStamp`.
*/
export const onLCP = (onReport: ReportCallback): void => {
export const onLCP = (onReport: ReportCallback): StopListening | undefined => {
const visibilityWatcher = getVisibilityWatcher();
const metric = initMetric('LCP');
let report: ReturnType<typeof bindReporter>;
Expand Down Expand Up @@ -75,5 +77,9 @@ export const onLCP = (onReport: ReportCallback): void => {
});

onHidden(stopListening, true);

return stopListening;
}

return undefined;
};