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

fix(browser): Remove browserPerformanceTimeOrigin side-effects #14025

Merged
merged 24 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
5012125
fix(browser): Remove `browserPerformanceTimeOrigin` side effects
timfish Oct 18, 2024
1b694e0
oops
timfish Oct 18, 2024
1f5a9ed
See if this is better
timfish Oct 18, 2024
1280d80
Fix tests
timfish Oct 18, 2024
818a500
another fix
timfish Oct 18, 2024
f1e186c
simplify more
timfish Oct 19, 2024
1f73e1d
Merge branch 'develop' into timfish/time-side-effects
timfish Oct 23, 2024
35f5d9b
Merge branch 'develop' into timfish/time-side-effects
timfish Oct 31, 2024
4a3d4d6
Merge remote-tracking branch 'upstream/develop' into timfish/time-sid…
timfish Nov 26, 2024
49c2bf7
Merge branch 'timfish/time-side-effects' of github.com:getsentry/sent…
timfish Nov 26, 2024
2ec6738
Fix linting
timfish Nov 26, 2024
253b7f4
Merge branch 'develop' into timfish/time-side-effects
timfish Dec 4, 2024
c7226d9
Merge branch 'develop' into timfish/time-side-effects
timfish Dec 13, 2024
e2332c7
fix dodgy merge
timfish Dec 13, 2024
36dfb93
Merge branch 'develop' into timfish/time-side-effects
timfish Dec 13, 2024
ad2ab7f
Merge branch 'develop' into timfish/time-side-effects
timfish Dec 22, 2024
aa7fcbc
Merge remote-tracking branch 'upstream/develop' into timfish/time-sid…
timfish Jan 2, 2025
93227cb
Merge branch 'develop' into timfish/time-side-effects
timfish Jan 6, 2025
d549967
Merge remote-tracking branch 'upstream/develop' into timfish/time-sid…
timfish Jan 10, 2025
3f51a8b
oh linting
timfish Jan 10, 2025
b790c35
More linting!
timfish Jan 10, 2025
e11964f
Merge remote-tracking branch 'upstream/develop' into timfish/time-sid…
timfish Jan 13, 2025
11d2986
Fix test mock
timfish Jan 13, 2025
78c6433
Merge branch 'develop' into timfish/time-side-effects
timfish Jan 13, 2025
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
16 changes: 8 additions & 8 deletions packages/browser-utils/src/metrics/browserMetrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ interface StartTrackingWebVitalsOptions {
*/
export function startTrackingWebVitals({ recordClsStandaloneSpans }: StartTrackingWebVitalsOptions): () => void {
const performance = getBrowserPerformanceAPI();
if (performance && browserPerformanceTimeOrigin) {
if (performance && browserPerformanceTimeOrigin()) {
// @ts-expect-error we want to make sure all of these are available, even if TS is sure they are
if (performance.mark) {
WINDOW.performance.mark('sentry-tracing-init');
Expand Down Expand Up @@ -117,7 +117,7 @@ export function startTrackingLongTasks(): void {
const { op: parentOp, start_timestamp: parentStartTimestamp } = spanToJSON(parent);

for (const entry of entries) {
const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime);
const startTime = msToSec((browserPerformanceTimeOrigin() as number) + entry.startTime);
const duration = msToSec(entry.duration);

if (parentOp === 'navigation' && parentStartTimestamp && startTime < parentStartTimestamp) {
Expand Down Expand Up @@ -156,7 +156,7 @@ export function startTrackingLongAnimationFrames(): void {
continue;
}

const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime);
const startTime = msToSec((browserPerformanceTimeOrigin() as number) + entry.startTime);

const { start_timestamp: parentStartTimestamp, op: parentOp } = spanToJSON(parent);

Expand All @@ -167,7 +167,6 @@ export function startTrackingLongAnimationFrames(): void {
// routing instrumentations
continue;
}

const duration = msToSec(entry.duration);

const attributes: SpanAttributes = {
Expand Down Expand Up @@ -210,7 +209,7 @@ export function startTrackingInteractions(): void {
}
for (const entry of entries) {
if (entry.name === 'click') {
const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime);
const startTime = msToSec((browserPerformanceTimeOrigin() as number) + entry.startTime);
const duration = msToSec(entry.duration);

const spanOptions: StartSpanOptions & Required<Pick<StartSpanOptions, 'attributes'>> = {
Expand Down Expand Up @@ -271,7 +270,7 @@ function _trackFID(): () => void {
return;
}

const timeOrigin = msToSec(browserPerformanceTimeOrigin as number);
const timeOrigin = msToSec(browserPerformanceTimeOrigin() as number);
const startTime = msToSec(entry.startTime);
_measurements['fid'] = { value: metric.value, unit: 'millisecond' };
_measurements['mark.fid'] = { value: timeOrigin + startTime, unit: 'second' };
Expand Down Expand Up @@ -300,12 +299,13 @@ interface AddPerformanceEntriesOptions {
/** Add performance related spans to a transaction */
export function addPerformanceEntries(span: Span, options: AddPerformanceEntriesOptions): void {
const performance = getBrowserPerformanceAPI();
if (!performance?.getEntries || !browserPerformanceTimeOrigin) {
const origin = browserPerformanceTimeOrigin();
if (!performance?.getEntries || !origin) {
// Gatekeeper if performance API not available
return;
}

const timeOrigin = msToSec(browserPerformanceTimeOrigin);
const timeOrigin = msToSec(origin);

const performanceEntries = performance.getEntries();

Expand Down
2 changes: 1 addition & 1 deletion packages/browser-utils/src/metrics/cls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export function trackClsAsStandaloneSpan(): void {
function sendStandaloneClsSpan(clsValue: number, entry: LayoutShift | undefined, pageloadSpanId: string) {
DEBUG_BUILD && logger.log(`Sending CLS span (${clsValue})`);

const startTime = msToSec((browserPerformanceTimeOrigin || 0) + (entry?.startTime || 0));
const startTime = msToSec((browserPerformanceTimeOrigin() || 0) + (entry?.startTime || 0));
const routeName = getCurrentScope().getScopeData().transactionName;

const name = entry ? htmlTreeAsString(entry.sources[0]?.node) : 'Layout shift';
Expand Down
4 changes: 2 additions & 2 deletions packages/browser-utils/src/metrics/inp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const INTERACTIONS_SPAN_MAP = new Map<number, Span>();
*/
export function startTrackingINP(): () => void {
const performance = getBrowserPerformanceAPI();
if (performance && browserPerformanceTimeOrigin) {
if (performance && browserPerformanceTimeOrigin()) {
const inpCallback = _trackINP();

return (): void => {
Expand Down Expand Up @@ -85,7 +85,7 @@ function _trackINP(): () => void {
const interactionType = INP_ENTRY_MAP[entry.name];

/** Build the INP span, create an envelope from the span, and then send the envelope */
const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime);
const startTime = msToSec((browserPerformanceTimeOrigin() as number) + entry.startTime);
const duration = msToSec(metric.value);
const activeSpan = getActiveSpan();
const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined;
Expand Down
6 changes: 3 additions & 3 deletions packages/browser/src/profiling/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,9 @@ export function convertJSSelfProfileToSampledFormat(input: JSSelfProfile): Profi
// when that happens, we need to ensure we are correcting the profile timings so the two timelines stay in sync.
// Since JS self profiling time origin is always initialized to performance.timeOrigin, we need to adjust for
// the drift between the SDK selected value and our profile time origin.
const origin =
typeof performance.timeOrigin === 'number' ? performance.timeOrigin : browserPerformanceTimeOrigin || 0;
const adjustForOriginChange = origin - (browserPerformanceTimeOrigin || origin);
const perfOrigin = browserPerformanceTimeOrigin();
const origin = typeof performance.timeOrigin === 'number' ? performance.timeOrigin : perfOrigin || 0;
const adjustForOriginChange = origin - (perfOrigin || origin);

input.samples.forEach((jsSample, i) => {
// If sample has no stack, add an empty sample
Expand Down
3 changes: 2 additions & 1 deletion packages/browser/src/tracing/browserTracingIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,10 +364,11 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio

if (WINDOW.location) {
if (instrumentPageLoad) {
const origin = browserPerformanceTimeOrigin();
startBrowserTracingPageLoadSpan(client, {
name: WINDOW.location.pathname,
// pageload should always start at timeOrigin (and needs to be in s, not ms)
startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined,
startTime: origin ? origin / 1000 : undefined,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
Expand Down
4 changes: 2 additions & 2 deletions packages/browser/src/tracing/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export function extractNetworkProtocol(nextHopProtocol: string): { name: string;
}

function getAbsoluteTime(time: number = 0): number {
return ((browserPerformanceTimeOrigin || performance.timeOrigin) + time) / 1000;
return ((browserPerformanceTimeOrigin() || performance.timeOrigin) + time) / 1000;
}

function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming): [string, string | number][] {
Expand All @@ -270,7 +270,7 @@ function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming

timingSpanData.push(['network.protocol.version', version], ['network.protocol.name', name]);

if (!browserPerformanceTimeOrigin) {
if (!browserPerformanceTimeOrigin()) {
return timingSpanData;
}
return [
Expand Down
2 changes: 0 additions & 2 deletions packages/core/src/utils-hoist/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,6 @@ export {
} from './supports';
export { SyncPromise, rejectedSyncPromise, resolvedSyncPromise } from './syncpromise';
export {
// eslint-disable-next-line deprecation/deprecation
_browserPerformanceTimeOriginMode,
browserPerformanceTimeOrigin,
dateTimestampInSeconds,
timestampInSeconds,
Expand Down
41 changes: 21 additions & 20 deletions packages/core/src/utils-hoist/time.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,26 +67,21 @@ function createUnixTimestampInSecondsFunc(): () => number {
export const timestampInSeconds = createUnixTimestampInSecondsFunc();

/**
* Internal helper to store what is the source of browserPerformanceTimeOrigin below. For debugging only.
*
* @deprecated This variable will be removed in the next major version.
* Cached result of getBrowserTimeOrigin.
*/
export let _browserPerformanceTimeOriginMode: string;
let cachedTimeOrigin: [number | undefined, string] | undefined;

/**
* The number of milliseconds since the UNIX epoch. This value is only usable in a browser, and only when the
* performance API is available.
* Gets the time origin and the mode used to determine it.
*/
export const browserPerformanceTimeOrigin = ((): number | undefined => {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how much of this logic we can simplify 🤔

Probably not worth the effort.

function getBrowserTimeOrigin(): [number | undefined, string] {
// Unfortunately browsers may report an inaccurate time origin data, through either performance.timeOrigin or
// performance.timing.navigationStart, which results in poor results in performance data. We only treat time origin
// data as reliable if they are within a reasonable threshold of the current time.

const { performance } = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window;
if (!performance?.now) {
// eslint-disable-next-line deprecation/deprecation
_browserPerformanceTimeOriginMode = 'none';
return undefined;
return [undefined, 'none'];
}

const threshold = 3600 * 1000;
Expand Down Expand Up @@ -114,18 +109,24 @@ export const browserPerformanceTimeOrigin = ((): number | undefined => {
if (timeOriginIsReliable || navigationStartIsReliable) {
// Use the more reliable time origin
if (timeOriginDelta <= navigationStartDelta) {
// eslint-disable-next-line deprecation/deprecation
_browserPerformanceTimeOriginMode = 'timeOrigin';
return performance.timeOrigin;
return [performance.timeOrigin, 'timeOrigin'];
} else {
// eslint-disable-next-line deprecation/deprecation
_browserPerformanceTimeOriginMode = 'navigationStart';
return navigationStart;
return [navigationStart, 'navigationStart'];
}
}

// Either both timeOrigin and navigationStart are skewed or neither is available, fallback to Date.
// eslint-disable-next-line deprecation/deprecation
_browserPerformanceTimeOriginMode = 'dateNow';
return dateNow;
})();
return [dateNow, 'dateNow'];
}

/**
* The number of milliseconds since the UNIX epoch. This value is only usable in a browser, and only when the
* performance API is available.
*/
export function browserPerformanceTimeOrigin(): number | undefined {
timfish marked this conversation as resolved.
Show resolved Hide resolved
if (!cachedTimeOrigin) {
cachedTimeOrigin = getBrowserTimeOrigin();
}

return cachedTimeOrigin[0];
}
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,9 @@ function _instrumentInitialLoad(config: EmberSentryConfig): void {
return;
}

const origin = browserPerformanceTimeOrigin();
// Split performance check in two so clearMarks still happens even if timeOrigin isn't available.
if (!HAS_PERFORMANCE_TIMING || browserPerformanceTimeOrigin === undefined) {
if (!HAS_PERFORMANCE_TIMING || origin === undefined) {
return;
}
const measureName = '@sentry/ember:initial-load';
Expand All @@ -383,7 +384,7 @@ function _instrumentInitialLoad(config: EmberSentryConfig): void {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const measure = measures[0]!;

const startTime = (measure.startTime + browserPerformanceTimeOrigin) / 1000;
const startTime = (measure.startTime + origin) / 1000;
const endTime = startTime + measure.duration / 1000;

startInactiveSpan({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@ export const INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME = 'incomplet

/** Instruments the Next.js app router for pageloads. */
export function appRouterInstrumentPageLoad(client: Client): void {
const origin = browserPerformanceTimeOrigin();
startBrowserTracingPageLoadSpan(client, {
name: WINDOW.location.pathname,
// pageload should always start at timeOrigin (and needs to be in s, not ms)
startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined,
startTime: origin ? origin / 1000 : undefined,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.nextjs.app_router_instrumentation',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,13 @@ export function pagesRouterInstrumentPageLoad(client: Client): void {
name = name.replace(/^(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS|TRACE|CONNECT)\s+/i, '');
}

const origin = browserPerformanceTimeOrigin();
startBrowserTracingPageLoadSpan(
client,
{
name,
// pageload should always start at timeOrigin (and needs to be in s, not ms)
startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined,
startTime: origin ? origin / 1000 : undefined,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.nextjs.pages_router_instrumentation',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ function createPerformanceEntry(entry: AllPerformanceEntry): ReplayPerformanceEn
function getAbsoluteTime(time: number): number {
// browserPerformanceTimeOrigin can be undefined if `performance` or
// `performance.now` doesn't exist, but this is already checked by this integration
return ((browserPerformanceTimeOrigin || WINDOW.performance.timeOrigin) + time) / 1000;
return ((browserPerformanceTimeOrigin() || WINDOW.performance.timeOrigin) + time) / 1000;
}

function createPaintEntry(entry: PerformancePaintTiming): ReplayPerformanceEntry<PaintData> {
Expand Down
4 changes: 2 additions & 2 deletions packages/replay-internal/test/integration/flush.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ describe('Integration | flush', () => {
mockEventBufferFinish.mockClear();

Object.defineProperty(SentryUtils, 'browserPerformanceTimeOrigin', {
value: BASE_TIMESTAMP,
value: () => BASE_TIMESTAMP,
writable: true,
});
});
Expand All @@ -107,7 +107,7 @@ describe('Integration | flush', () => {
writable: true,
});
Object.defineProperty(SentryUtils, 'browserPerformanceTimeOrigin', {
value: prevBrowserPerformanceTimeOrigin,
value: () => prevBrowserPerformanceTimeOrigin,
writable: true,
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ vi.setSystemTime(new Date('2023-01-01'));

vi.mock('@sentry/core', async () => ({
...(await vi.importActual('@sentry/core')),
browserPerformanceTimeOrigin: new Date('2023-01-01').getTime(),
browserPerformanceTimeOrigin: () => new Date('2023-01-01').getTime(),
}));

import { WINDOW } from '../../../src/constants';
Expand Down
Loading