From f6a81b1bd9bdcd8ba367a848d60ceb42c1b32f40 Mon Sep 17 00:00:00 2001 From: k-fish Date: Fri, 6 Aug 2021 14:31:26 -0400 Subject: [PATCH 1/5] feat(perf): Add experimental option to improve LCP collection This will add an experimental option (options._metricOptions._reportAllChanges) that will set 'getLCP' to always report. Now that we are recording LCP element, it should be sufficient to collection the additional information and let the user pick the appropriate LCP element in the ui to improve accuracy dynamically instead. We'd like to test this out on Sentry first. --- .../tracing/src/browser/browsertracing.ts | 20 ++++++++++++++++++- packages/tracing/src/browser/metrics.ts | 17 ++++++++-------- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index c0db1e0c0e7b..bbee3aae8f6b 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -60,6 +60,15 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { */ markBackgroundTransactions: boolean; + /** + * _metricOptions allows the user to send options to change how metrics are collected. + * + * _metricOptions is currently experimental. + * + * Default: undefined + */ + _metricOptions?: BrowserMetricOptions; + /** * beforeNavigate is called before a pageload/navigation transaction is created and allows users to modify transaction * context data, or drop the transaction entirely (by setting `sampled = false` in the context). @@ -83,6 +92,13 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { ): void; } +/** + * Exports a way to add options to our metric collection. Currently experimental. + */ +export interface BrowserMetricOptions { + _reportAllChanges?: boolean; +} + const DEFAULT_BROWSER_TRACING_OPTIONS = { idleTimeout: DEFAULT_IDLE_TIMEOUT, markBackgroundTransactions: true, @@ -116,7 +132,7 @@ export class BrowserTracing implements Integration { private _getCurrentHub?: () => Hub; - private readonly _metrics: MetricsInstrumentation = new MetricsInstrumentation(); + private readonly _metrics: MetricsInstrumentation; private readonly _emitOptionsWarning: boolean = false; @@ -139,6 +155,8 @@ export class BrowserTracing implements Integration { ..._options, tracingOrigins, }; + + this._metrics = new MetricsInstrumentation(this.options._metricOptions); } /** diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index b8b3e51f3467..5b23c3676a36 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -6,6 +6,7 @@ import { browserPerformanceTimeOrigin, getGlobalObject, htmlTreeAsString, isNode import { Span } from '../span'; import { Transaction } from '../transaction'; import { msToSec } from '../utils'; +import { BrowserMetricOptions } from './browsertracing'; import { getCLS, LayoutShift } from './web-vitals/getCLS'; import { getFID } from './web-vitals/getFID'; import { getLCP, LargestContentfulPaint } from './web-vitals/getLCP'; @@ -22,15 +23,15 @@ export class MetricsInstrumentation { private _lcpEntry: LargestContentfulPaint | undefined; private _clsEntry: LayoutShift | undefined; - public constructor() { + public constructor(_options?: BrowserMetricOptions) { if (!isNodeEnv() && global?.performance && global?.document) { if (global.performance.mark) { global.performance.mark('sentry-tracing-init'); } - this._trackCLS(); - this._trackLCP(); - this._trackFID(); + this._trackCLS(_options); + this._trackLCP(_options); + this._trackFID(_options); } } @@ -230,7 +231,7 @@ export class MetricsInstrumentation { } /** Starts tracking the Cumulative Layout Shift on the current page. */ - private _trackCLS(): void { + private _trackCLS(_options?: BrowserMetricOptions): void { // See: // https://web.dev/evolving-cls/ // https://web.dev/cls-web-tooling/ @@ -285,7 +286,7 @@ export class MetricsInstrumentation { } /** Starts tracking the Largest Contentful Paint on the current page. */ - private _trackLCP(): void { + private _trackLCP(_options?: BrowserMetricOptions): void { getLCP(metric => { const entry = metric.entries.pop(); @@ -299,11 +300,11 @@ export class MetricsInstrumentation { this._measurements['lcp'] = { value: metric.value }; this._measurements['mark.lcp'] = { value: timeOrigin + startTime }; this._lcpEntry = entry as LargestContentfulPaint; - }); + }, _options?._reportAllChanges); } /** Starts tracking the First Input Delay on the current page. */ - private _trackFID(): void { + private _trackFID(_options?: BrowserMetricOptions): void { getFID(metric => { const entry = metric.entries.pop(); From 8dda9d43a5d58badb1c2c9776c3e5f080c9b03be Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 17 Aug 2021 18:11:52 +0200 Subject: [PATCH 2/5] ref: Use explicit defaults --- .../tracing/src/browser/browsertracing.ts | 11 ++------ packages/tracing/src/browser/metrics.ts | 28 +++++++++++++------ 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index bbee3aae8f6b..a2161fc590ed 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -7,7 +7,7 @@ import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../idletransaction'; import { SpanStatus } from '../spanstatus'; import { extractTraceparentData, secToMs } from '../utils'; import { registerBackgroundTabDetection } from './backgroundtab'; -import { MetricsInstrumentation } from './metrics'; +import { MetricsInstrumentation, MetricsInstrumentationOptions } from './metrics'; import { defaultRequestInstrumentationOptions, instrumentOutgoingRequests, @@ -67,7 +67,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { * * Default: undefined */ - _metricOptions?: BrowserMetricOptions; + _metricOptions?: Partial; /** * beforeNavigate is called before a pageload/navigation transaction is created and allows users to modify transaction @@ -92,13 +92,6 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { ): void; } -/** - * Exports a way to add options to our metric collection. Currently experimental. - */ -export interface BrowserMetricOptions { - _reportAllChanges?: boolean; -} - const DEFAULT_BROWSER_TRACING_OPTIONS = { idleTimeout: DEFAULT_IDLE_TIMEOUT, markBackgroundTransactions: true, diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 5b23c3676a36..c29f03110d66 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -6,7 +6,6 @@ import { browserPerformanceTimeOrigin, getGlobalObject, htmlTreeAsString, isNode import { Span } from '../span'; import { Transaction } from '../transaction'; import { msToSec } from '../utils'; -import { BrowserMetricOptions } from './browsertracing'; import { getCLS, LayoutShift } from './web-vitals/getCLS'; import { getFID } from './web-vitals/getFID'; import { getLCP, LargestContentfulPaint } from './web-vitals/getLCP'; @@ -15,6 +14,17 @@ import { NavigatorDeviceMemory, NavigatorNetworkInformation } from './web-vitals const global = getGlobalObject(); +/** + * Exports a way to add options to our metric collection. Currently experimental. + */ +export interface MetricsInstrumentationOptions { + _reportAllChanges: boolean; +} + +const DEFAULT_METRICS_INSTR_OPTIONS: MetricsInstrumentationOptions = { + _reportAllChanges: false, +}; + /** Class tracking metrics */ export class MetricsInstrumentation { private _measurements: Measurements = {}; @@ -23,15 +33,15 @@ export class MetricsInstrumentation { private _lcpEntry: LargestContentfulPaint | undefined; private _clsEntry: LayoutShift | undefined; - public constructor(_options?: BrowserMetricOptions) { + public constructor(_options: MetricsInstrumentationOptions = DEFAULT_METRICS_INSTR_OPTIONS) { if (!isNodeEnv() && global?.performance && global?.document) { if (global.performance.mark) { global.performance.mark('sentry-tracing-init'); } - this._trackCLS(_options); - this._trackLCP(_options); - this._trackFID(_options); + this._trackCLS(); + this._trackLCP(_options._reportAllChanges); + this._trackFID(); } } @@ -231,7 +241,7 @@ export class MetricsInstrumentation { } /** Starts tracking the Cumulative Layout Shift on the current page. */ - private _trackCLS(_options?: BrowserMetricOptions): void { + private _trackCLS(): void { // See: // https://web.dev/evolving-cls/ // https://web.dev/cls-web-tooling/ @@ -286,7 +296,7 @@ export class MetricsInstrumentation { } /** Starts tracking the Largest Contentful Paint on the current page. */ - private _trackLCP(_options?: BrowserMetricOptions): void { + private _trackLCP(reportAllChanges: boolean): void { getLCP(metric => { const entry = metric.entries.pop(); @@ -300,11 +310,11 @@ export class MetricsInstrumentation { this._measurements['lcp'] = { value: metric.value }; this._measurements['mark.lcp'] = { value: timeOrigin + startTime }; this._lcpEntry = entry as LargestContentfulPaint; - }, _options?._reportAllChanges); + }, reportAllChanges); } /** Starts tracking the First Input Delay on the current page. */ - private _trackFID(_options?: BrowserMetricOptions): void { + private _trackFID(): void { getFID(metric => { const entry = metric.entries.pop(); From 9ffbac9b7e58e09c1863fd52de10a0caeeef6601 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 17 Aug 2021 18:22:44 +0200 Subject: [PATCH 3/5] fix type --- packages/tracing/src/browser/browsertracing.ts | 4 ++-- packages/tracing/src/browser/metrics.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index a2161fc590ed..751e342d85a6 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -7,7 +7,7 @@ import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../idletransaction'; import { SpanStatus } from '../spanstatus'; import { extractTraceparentData, secToMs } from '../utils'; import { registerBackgroundTabDetection } from './backgroundtab'; -import { MetricsInstrumentation, MetricsInstrumentationOptions } from './metrics'; +import { DEFAULT_METRICS_INSTR_OPTIONS, MetricsInstrumentation, MetricsInstrumentationOptions } from './metrics'; import { defaultRequestInstrumentationOptions, instrumentOutgoingRequests, @@ -149,7 +149,7 @@ export class BrowserTracing implements Integration { tracingOrigins, }; - this._metrics = new MetricsInstrumentation(this.options._metricOptions); + this._metrics = new MetricsInstrumentation({ ...this.options._metricOptions, ...DEFAULT_METRICS_INSTR_OPTIONS }); } /** diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index c29f03110d66..702dd34077a5 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -21,7 +21,7 @@ export interface MetricsInstrumentationOptions { _reportAllChanges: boolean; } -const DEFAULT_METRICS_INSTR_OPTIONS: MetricsInstrumentationOptions = { +export const DEFAULT_METRICS_INSTR_OPTIONS: MetricsInstrumentationOptions = { _reportAllChanges: false, }; @@ -33,7 +33,7 @@ export class MetricsInstrumentation { private _lcpEntry: LargestContentfulPaint | undefined; private _clsEntry: LayoutShift | undefined; - public constructor(_options: MetricsInstrumentationOptions = DEFAULT_METRICS_INSTR_OPTIONS) { + public constructor(_options: MetricsInstrumentationOptions) { if (!isNodeEnv() && global?.performance && global?.document) { if (global.performance.mark) { global.performance.mark('sentry-tracing-init'); From b6f8ecede83591a6cffc5369876acdae05138791 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 19 Aug 2021 12:19:08 +0200 Subject: [PATCH 4/5] fix tests --- packages/tracing/test/browser/metrics.test.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/tracing/test/browser/metrics.test.ts b/packages/tracing/test/browser/metrics.test.ts index 5dc7ae50ff3e..0f230a9680aa 100644 --- a/packages/tracing/test/browser/metrics.test.ts +++ b/packages/tracing/test/browser/metrics.test.ts @@ -1,5 +1,11 @@ import { Span, Transaction } from '../../src'; -import { _startChild, addResourceSpans, MetricsInstrumentation, ResourceEntry } from '../../src/browser/metrics'; +import { + _startChild, + addResourceSpans, + DEFAULT_METRICS_INSTR_OPTIONS, + MetricsInstrumentation, + ResourceEntry, +} from '../../src/browser/metrics'; import { addDOMPropertiesToGlobal } from '../testutils'; // eslint-disable-next-line @typescript-eslint/no-explicit-any, no-var @@ -182,7 +188,7 @@ describe('MetricsInstrumentation', () => { jest.spyOn(MetricsInstrumentation.prototype as any, tracker), ); - new MetricsInstrumentation(); + new MetricsInstrumentation(DEFAULT_METRICS_INSTR_OPTIONS); trackers.forEach(tracker => expect(tracker).not.toBeCalled()); }); @@ -196,8 +202,7 @@ describe('MetricsInstrumentation', () => { const trackers = ['_trackCLS', '_trackLCP', '_trackFID'].map(tracker => jest.spyOn(MetricsInstrumentation.prototype as any, tracker), ); - - new MetricsInstrumentation(); + new MetricsInstrumentation(DEFAULT_METRICS_INSTR_OPTIONS); global.process = backup; trackers.forEach(tracker => expect(tracker).toBeCalled()); From 99bd218ce4735ad17493d009e559269a7a11a194 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 26 Aug 2021 11:28:32 -0400 Subject: [PATCH 5/5] Make sure option is set correctly --- .../tracing/src/browser/browsertracing.ts | 2 +- .../test/browser/browsertracing.test.ts | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 751e342d85a6..803fa8934457 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -149,7 +149,7 @@ export class BrowserTracing implements Integration { tracingOrigins, }; - this._metrics = new MetricsInstrumentation({ ...this.options._metricOptions, ...DEFAULT_METRICS_INSTR_OPTIONS }); + this._metrics = new MetricsInstrumentation({ ...DEFAULT_METRICS_INSTR_OPTIONS, ...this.options._metricOptions }); } /** diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 7c7d1d6306cc..bc94f7ed73bb 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -11,6 +11,7 @@ import { getHeaderContext, getMetaContent, } from '../../src/browser/browsertracing'; +import { DEFAULT_METRICS_INSTR_OPTIONS, MetricsInstrumentation } from '../../src/browser/metrics'; import { defaultRequestInstrumentationOptions } from '../../src/browser/request'; import { instrumentRoutingWithDefaults } from '../../src/browser/router'; import * as hubExtensions from '../../src/hubextensions'; @@ -32,6 +33,8 @@ jest.mock('@sentry/utils', () => { }; }); +jest.mock('../../src/browser/metrics'); + const { logger } = jest.requireActual('@sentry/utils'); const warnSpy = jest.spyOn(logger, 'warn'); @@ -493,4 +496,31 @@ describe('BrowserTracing', () => { ); }); }); + + describe('metrics', () => { + beforeEach(() => { + // @ts-ignore mock clear + MetricsInstrumentation.mockClear(); + }); + + it('creates metrics instrumentation', () => { + createBrowserTracing(true, {}); + + expect(MetricsInstrumentation).toHaveBeenCalledTimes(1); + expect(MetricsInstrumentation).toHaveBeenLastCalledWith(DEFAULT_METRICS_INSTR_OPTIONS); + }); + + it('creates metrics instrumentation with custom options', () => { + createBrowserTracing(true, { + _metricOptions: { + _reportAllChanges: true, + }, + }); + + expect(MetricsInstrumentation).toHaveBeenCalledTimes(1); + expect(MetricsInstrumentation).toHaveBeenLastCalledWith({ + _reportAllChanges: true, + }); + }); + }); });