From 36ffb6d448144837a80841a6bdb59ea64e154dba Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 8 Mar 2023 18:22:02 +0100 Subject: [PATCH 1/3] fix(tracing): Record LCP on transaction finish --- .../suites/tracing/metrics/web-vitals-lcp/test.ts | 2 -- packages/tracing/src/browser/browsertracing.ts | 7 ++++++- packages/tracing/src/browser/metrics/index.ts | 10 ++++++---- packages/tracing/src/browser/web-vitals/getLCP.ts | 8 +++++++- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/packages/integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/packages/integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts index 28b85d518e80..7511abf60d09 100644 --- a/packages/integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts +++ b/packages/integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts @@ -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(page); expect(eventData.measurements).toBeDefined(); diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index edd2baf71d9f..030bd0d8ace6 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -164,6 +164,8 @@ export class BrowserTracing implements Integration { private _latestRouteName?: string; private _latestRouteSource?: TransactionSource; + private _stopListeningForLCP?: () => void; + public constructor(_options?: Partial) { this.options = { ...DEFAULT_BROWSER_TRACING_OPTIONS, @@ -185,7 +187,7 @@ export class BrowserTracing implements Integration { this.options.tracePropagationTargets = _options.tracingOrigins; } - startTrackingWebVitals(); + this._stopListeningForLCP = startTrackingWebVitals(); if (this.options.enableLongTask) { startTrackingLongTasks(); } @@ -303,6 +305,9 @@ export class BrowserTracing implements Integration { heartbeatInterval, ); idleTransaction.registerBeforeFinishCallback(transaction => { + if (this._stopListeningForLCP) { + this._stopListeningForLCP(); + } addPerformanceEntries(transaction); }); diff --git a/packages/tracing/src/browser/metrics/index.ts b/packages/tracing/src/browser/metrics/index.ts index 2d37ef8c1919..18891d53bb73 100644 --- a/packages/tracing/src/browser/metrics/index.ts +++ b/packages/tracing/src/browser/metrics/index.ts @@ -34,16 +34,18 @@ let _clsEntry: LayoutShift | undefined; /** * Start tracking web vitals */ -export function startTrackingWebVitals(): void { +export function startTrackingWebVitals(): ReturnType { const performance = getBrowserPerformanceAPI(); if (performance && browserPerformanceTimeOrigin) { if (performance.mark) { WINDOW.performance.mark('sentry-tracing-init'); } _trackCLS(); - _trackLCP(); _trackFID(); + return _trackLCP(); } + + return; } /** @@ -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 { + return onLCP(metric => { const entry = metric.entries.pop(); if (!entry) { return; diff --git a/packages/tracing/src/browser/web-vitals/getLCP.ts b/packages/tracing/src/browser/web-vitals/getLCP.ts index bf834c07ce4e..ce9694e4b615 100644 --- a/packages/tracing/src/browser/web-vitals/getLCP.ts +++ b/packages/tracing/src/browser/web-vitals/getLCP.ts @@ -24,13 +24,15 @@ import type { LCPMetric, ReportCallback } from './types'; const reportedMetricIDs: Record = {}; +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; @@ -75,5 +77,9 @@ export const onLCP = (onReport: ReportCallback): void => { }); onHidden(stopListening, true); + + return stopListening; } + + return undefined; }; From 56919f970cb96af1e163ed38fb21b5f3db1c2220 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 9 Mar 2023 10:13:50 +0100 Subject: [PATCH 2/3] feat: Also track CLS the same way --- .../tracing/src/browser/browsertracing.ts | 8 +++---- packages/tracing/src/browser/metrics/index.ts | 23 ++++++++++++++----- .../tracing/src/browser/web-vitals/getCLS.ts | 14 +++++++---- .../tracing/src/browser/web-vitals/getLCP.ts | 6 ++--- .../src/browser/web-vitals/types/base.ts | 2 ++ 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 029e68512ae5..3575c0a20e91 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -169,7 +169,7 @@ export class BrowserTracing implements Integration { private _latestRouteName?: string; private _latestRouteSource?: TransactionSource; - private _stopListeningForLCP?: () => void; + private _collectWebVitals: () => void; public constructor(_options?: Partial) { this.options = { @@ -192,7 +192,7 @@ export class BrowserTracing implements Integration { this.options.tracePropagationTargets = _options.tracingOrigins; } - this._stopListeningForLCP = startTrackingWebVitals(); + this._collectWebVitals = startTrackingWebVitals(); if (this.options.enableLongTask) { startTrackingLongTasks(); } @@ -313,9 +313,7 @@ export class BrowserTracing implements Integration { heartbeatInterval, ); idleTransaction.registerBeforeFinishCallback(transaction => { - if (this._stopListeningForLCP) { - this._stopListeningForLCP(); - } + this._collectWebVitals(); addPerformanceEntries(transaction); }); diff --git a/packages/tracing/src/browser/metrics/index.ts b/packages/tracing/src/browser/metrics/index.ts index 3807aaca1cb1..788f21aafaa4 100644 --- a/packages/tracing/src/browser/metrics/index.ts +++ b/packages/tracing/src/browser/metrics/index.ts @@ -33,19 +33,30 @@ let _clsEntry: LayoutShift | undefined; /** * Start tracking web vitals + * + * @returns A function that forces web vitals collection */ -export function startTrackingWebVitals(): ReturnType { +export function startTrackingWebVitals(): () => void { const performance = getBrowserPerformanceAPI(); if (performance && browserPerformanceTimeOrigin) { if (performance.mark) { WINDOW.performance.mark('sentry-tracing-init'); } - _trackCLS(); _trackFID(); - return _trackLCP(); + const clsCallback = _trackCLS(); + const lcpCallback = _trackLCP(); + + return (): void => { + if (clsCallback) { + clsCallback(); + } + if (lcpCallback) { + lcpCallback(); + } + }; } - return; + return () => undefined; } /** @@ -102,11 +113,11 @@ export function startTrackingInteractions(): void { } /** Starts tracking the Cumulative Layout Shift on the current page. */ -function _trackCLS(): void { +function _trackCLS(): ReturnType { // See: // https://web.dev/evolving-cls/ // https://web.dev/cls-web-tooling/ - onCLS(metric => { + return onCLS(metric => { const entry = metric.entries.pop(); if (!entry) { return; diff --git a/packages/tracing/src/browser/web-vitals/getCLS.ts b/packages/tracing/src/browser/web-vitals/getCLS.ts index 3abddfac07cb..fdd1e867adfa 100644 --- a/packages/tracing/src/browser/web-vitals/getCLS.ts +++ b/packages/tracing/src/browser/web-vitals/getCLS.ts @@ -18,7 +18,7 @@ import { bindReporter } from './lib/bindReporter'; import { initMetric } from './lib/initMetric'; import { observe } from './lib/observe'; import { onHidden } from './lib/onHidden'; -import type { CLSMetric, ReportCallback } from './types'; +import type { CLSMetric, ReportCallback, StopListening } from './types'; /** * Calculates the [CLS](https://web.dev/cls/) value for the current page and @@ -41,7 +41,7 @@ import type { CLSMetric, ReportCallback } from './types'; * hidden. As a result, the `callback` function might be called multiple times * during the same page load._ */ -export const onCLS = (onReport: ReportCallback): void => { +export const onCLS = (onReport: ReportCallback): StopListening | undefined => { const metric = initMetric('CLS', 0); let report: ReturnType; @@ -89,9 +89,15 @@ export const onCLS = (onReport: ReportCallback): void => { if (po) { report = bindReporter(onReport, metric); - onHidden(() => { + const stopListening = (): void => { handleEntries(po.takeRecords() as CLSMetric['entries']); report(true); - }); + }; + + onHidden(stopListening); + + return stopListening; } + + return; }; diff --git a/packages/tracing/src/browser/web-vitals/getLCP.ts b/packages/tracing/src/browser/web-vitals/getLCP.ts index ce9694e4b615..37e37c01eebd 100644 --- a/packages/tracing/src/browser/web-vitals/getLCP.ts +++ b/packages/tracing/src/browser/web-vitals/getLCP.ts @@ -20,12 +20,10 @@ import { getVisibilityWatcher } from './lib/getVisibilityWatcher'; import { initMetric } from './lib/initMetric'; import { observe } from './lib/observe'; import { onHidden } from './lib/onHidden'; -import type { LCPMetric, ReportCallback } from './types'; +import type { LCPMetric, ReportCallback, StopListening } from './types'; const reportedMetricIDs: Record = {}; -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 @@ -81,5 +79,5 @@ export const onLCP = (onReport: ReportCallback): StopListening | undefined => { return stopListening; } - return undefined; + return; }; diff --git a/packages/tracing/src/browser/web-vitals/types/base.ts b/packages/tracing/src/browser/web-vitals/types/base.ts index 5dc45f00558d..ea2764c8ea64 100644 --- a/packages/tracing/src/browser/web-vitals/types/base.ts +++ b/packages/tracing/src/browser/web-vitals/types/base.ts @@ -104,3 +104,5 @@ export interface ReportOpts { * loading. This is equivalent to the corresponding `readyState` value. */ export type LoadState = 'loading' | 'dom-interactive' | 'dom-content-loaded' | 'complete'; + +export type StopListening = () => void; From 8e6d884822fbfe1ea6076a25585859d8111a6f16 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 9 Mar 2023 10:33:24 +0100 Subject: [PATCH 3/3] add test --- .../test/browser/browsertracing.test.ts | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index b89a6d791267..e2bee71db0a8 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -28,7 +28,14 @@ jest.mock('@sentry/utils', () => { }; }); -jest.mock('../../src/browser/metrics'); +const mockStartTrackingWebVitals = jest.fn().mockReturnValue(() => () => {}); + +jest.mock('../../src/browser/metrics', () => ({ + addPerformanceEntries: jest.fn(), + startTrackingInteractions: jest.fn(), + startTrackingLongTasks: jest.fn(), + startTrackingWebVitals: () => mockStartTrackingWebVitals(), +})); const instrumentOutgoingRequestsMock = jest.fn(); jest.mock('./../../src/browser/request', () => { @@ -57,6 +64,8 @@ describe('BrowserTracing', () => { hub = new Hub(new BrowserClient(options)); makeMain(hub); document.head.innerHTML = ''; + + mockStartTrackingWebVitals.mockClear(); }); afterEach(() => { @@ -371,6 +380,17 @@ describe('BrowserTracing', () => { jest.advanceTimersByTime(2000); expect(mockFinish).toHaveBeenCalledTimes(1); }); + + it('calls `_collectWebVitals` if enabled', () => { + createBrowserTracing(true, { routingInstrumentation: customInstrumentRouting }); + const transaction = getActiveTransaction(hub) as IdleTransaction; + + const span = transaction.startChild(); // activities = 1 + span.finish(); // activities = 0 + + jest.advanceTimersByTime(TRACING_DEFAULTS.idleTimeout); + expect(mockStartTrackingWebVitals).toHaveBeenCalledTimes(1); + }); }); describe('heartbeatInterval', () => {