From ef26423b8b3e8bcd622d8c748c20db473c21fe82 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 16 May 2024 09:42:08 +0200 Subject: [PATCH] revert: "feat(browser): Add `interactionsSampleRate` to `browserTracingIntegration` options (#12023)" (#12048) The reason for the revert is that we're concerned about 1. the usefulness and 2. the name of this option and would like to revisit this without blocking the 8.1.0 release. This is not semver-breaking because in 8.0.0, this API did not exist. --- packages/browser-utils/src/metrics/inp.ts | 14 +++-------- .../src/tracing/browserTracingIntegration.ts | 22 +---------------- .../tracing/browserTracingIntegration.test.ts | 24 ------------------- 3 files changed, 4 insertions(+), 56 deletions(-) diff --git a/packages/browser-utils/src/metrics/inp.ts b/packages/browser-utils/src/metrics/inp.ts index 2eb20c1d4c43..eeead7b12018 100644 --- a/packages/browser-utils/src/metrics/inp.ts +++ b/packages/browser-utils/src/metrics/inp.ts @@ -17,10 +17,10 @@ import { getBrowserPerformanceAPI, msToSec } from './utils'; /** * Start tracking INP webvital events. */ -export function startTrackingINP(interactionsSampleRate: number): () => void { +export function startTrackingINP(): () => void { const performance = getBrowserPerformanceAPI(); if (performance && browserPerformanceTimeOrigin) { - const inpCallback = _trackINP(interactionsSampleRate); + const inpCallback = _trackINP(); return (): void => { inpCallback(); @@ -60,16 +60,8 @@ const INP_ENTRY_MAP: Record = { }; /** Starts tracking the Interaction to Next Paint on the current page. */ -function _trackINP(interactionsSampleRate: number): () => void { +function _trackINP(): () => void { return addInpInstrumentationHandler(({ metric }) => { - // As specified in the `interactionsSampleRate` option, the sampling decision shall be based on - // `tracesSampleRate` x `interactionsSampleRate` - // This is the same as sampling here first on `interactionsSampleRate` and later again on `tracesSampleRate` - // (which is done in `startInactiveSpan`). Doing it this way is easier and more bundle-size efficient. - if (Math.random() > interactionsSampleRate) { - return; - } - const client = getClient(); if (!client || metric.value == undefined) { return; diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 5690c7332c79..b3d530ee3653 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -103,17 +103,6 @@ export interface BrowserTracingOptions { */ enableInp: boolean; - /** - * Sample rate to determine interaction (INP) span sampling. - * - * The `interactionsSampleRate` is applied on top of the global `tracesSampleRate`. - * For example, a tracesSampleRate of 0.1 and interactionsSampleRate of 0.5 will result in a 0.05 sample rate - * for interactions. - * - * Default: 1 - */ - interactionsSampleRate: number; - /** * Flag to disable patching all together for fetch requests. * @@ -166,7 +155,6 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = { markBackgroundSpan: true, enableLongTask: true, enableInp: true, - interactionsSampleRate: 1, _experiments: {}, ...defaultRequestInstrumentationOptions, }; @@ -185,7 +173,6 @@ export const browserTracingIntegration = ((_options: Partial= 0 && interactionsSampleRate <= 1; - if (isValidInteractionsSampleRate) { - DEBUG_BUILD && - logger.warn( - `[Tracing] \`interactionsSampleRate\` must be between 0 and 1. Got: ${interactionsSampleRate}. Setting to 100%`, - ); - } - startTrackingINP(isValidInteractionsSampleRate ? interactionsSampleRate : 1); + startTrackingINP(); } if (enableLongTask) { diff --git a/packages/browser/test/unit/tracing/browserTracingIntegration.test.ts b/packages/browser/test/unit/tracing/browserTracingIntegration.test.ts index fb2f984e459e..3eb8b43139bb 100644 --- a/packages/browser/test/unit/tracing/browserTracingIntegration.test.ts +++ b/packages/browser/test/unit/tracing/browserTracingIntegration.test.ts @@ -36,8 +36,6 @@ import { } from '../../../src/tracing/browserTracingIntegration'; import { getDefaultBrowserClientOptions } from '../helper/browser-client-options'; -import * as browserUtils from '@sentry-internal/browser-utils'; - // We're setting up JSDom here because the Next.js routing instrumentations requires a few things to be present on pageload: // 1. Access to window.document API for `window.document.getElementById` // 2. Access to window.location API for `window.location.pathname` @@ -1000,28 +998,6 @@ describe('browserTracingIntegration', () => { }); }); - describe('INP - interactionsSampleRate', () => { - const startTrackingInpSpy = jest.spyOn(browserUtils, 'startTrackingINP'); - - it('sets interactionsSampleRate to 1 by default', () => { - browserTracingIntegration(); - expect(startTrackingInpSpy).toHaveBeenCalledWith(1); - }); - - it.each([0, 0.5, 1])('passes on user-defined interactionsSampleRate', interactionsSampleRate => { - browserTracingIntegration({ interactionsSampleRate }); - expect(startTrackingInpSpy).toHaveBeenCalledWith(interactionsSampleRate); - }); - - it.each([-1, 1.1, NaN, Infinity])( - 'falls back to 100% when receiving an invalid interactionsSampleRate', - interactionsSampleRate => { - browserTracingIntegration({ interactionsSampleRate }); - expect(startTrackingInpSpy).toHaveBeenCalledWith(1); - }, - ); - }); - // TODO(lforst): I cannot manage to get this test to pass. /* it('heartbeatInterval can be a custom value', () => {