Skip to content
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.

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

feat(perf): Add experimental option to improve LCP collection #3879

Merged
merged 6 commits into from Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions packages/tracing/src/browser/browsertracing.ts
Expand Up @@ -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 { DEFAULT_METRICS_INSTR_OPTIONS, MetricsInstrumentation, MetricsInstrumentationOptions } from './metrics';
import {
defaultRequestInstrumentationOptions,
instrumentOutgoingRequests,
Expand Down Expand Up @@ -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?: Partial<MetricsInstrumentationOptions>;

/**
* 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).
Expand Down Expand Up @@ -116,7 +125,7 @@ export class BrowserTracing implements Integration {

private _getCurrentHub?: () => Hub;

private readonly _metrics: MetricsInstrumentation = new MetricsInstrumentation();
private readonly _metrics: MetricsInstrumentation;

private readonly _emitOptionsWarning: boolean = false;

Expand All @@ -139,6 +148,8 @@ export class BrowserTracing implements Integration {
..._options,
tracingOrigins,
};

this._metrics = new MetricsInstrumentation({ ...DEFAULT_METRICS_INSTR_OPTIONS, ...this.options._metricOptions });
}

/**
Expand Down
19 changes: 15 additions & 4 deletions packages/tracing/src/browser/metrics.ts
Expand Up @@ -14,6 +14,17 @@ import { NavigatorDeviceMemory, NavigatorNetworkInformation } from './web-vitals

const global = getGlobalObject<Window>();

/**
* Exports a way to add options to our metric collection. Currently experimental.
*/
export interface MetricsInstrumentationOptions {
_reportAllChanges: boolean;
}

export const DEFAULT_METRICS_INSTR_OPTIONS: MetricsInstrumentationOptions = {
_reportAllChanges: false,
};

/** Class tracking metrics */
export class MetricsInstrumentation {
private _measurements: Measurements = {};
Expand All @@ -22,14 +33,14 @@ export class MetricsInstrumentation {
private _lcpEntry: LargestContentfulPaint | undefined;
private _clsEntry: LayoutShift | undefined;

public constructor() {
public constructor(_options: MetricsInstrumentationOptions) {
if (!isNodeEnv() && global?.performance && global?.document) {
if (global.performance.mark) {
global.performance.mark('sentry-tracing-init');
}

this._trackCLS();
this._trackLCP();
this._trackLCP(_options._reportAllChanges);
this._trackFID();
}
}
Expand Down Expand Up @@ -285,7 +296,7 @@ export class MetricsInstrumentation {
}

/** Starts tracking the Largest Contentful Paint on the current page. */
private _trackLCP(): void {
private _trackLCP(reportAllChanges: boolean): void {
getLCP(metric => {
const entry = metric.entries.pop();

Expand All @@ -299,7 +310,7 @@ export class MetricsInstrumentation {
this._measurements['lcp'] = { value: metric.value };
this._measurements['mark.lcp'] = { value: timeOrigin + startTime };
this._lcpEntry = entry as LargestContentfulPaint;
});
}, reportAllChanges);
}

/** Starts tracking the First Input Delay on the current page. */
Expand Down
30 changes: 30 additions & 0 deletions packages/tracing/test/browser/browsertracing.test.ts
Expand Up @@ -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';
Expand All @@ -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');

Expand Down Expand Up @@ -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,
});
});
});
});
13 changes: 9 additions & 4 deletions 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
Expand Down Expand Up @@ -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());
});
Expand All @@ -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());
Expand Down