From c1041b624c5787ceae651b6668e791bbe0159ca8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 27 Mar 2024 12:07:23 +0100 Subject: [PATCH] feat(core): Handle string sample rates If users pass e.g. `sampleRate: process.env.SAMPLE_RATE`, this will be somewhat silently ignored because we ignore non-number sample rates. This is a bit of a footgun. So this PR changes this so that we actually accept such sample rates. In the process, I also removed the `isNaN` polyfill as this is not really needed anymore. --- .../public-api/init/stringSampleRate/init.js | 14 +++ .../init/stringSampleRate/subject.js | 3 + .../public-api/init/stringSampleRate/test.ts | 16 +++ .../suites/tracing/stringSampleRate/init.js | 8 ++ .../tracing/stringSampleRate/subject.js | 3 + .../suites/tracing/stringSampleRate/test.ts | 17 +++ packages/core/src/baseclient.ts | 4 +- packages/core/src/index.ts | 1 + packages/core/src/tracing/index.ts | 1 + packages/core/src/tracing/sampling.ts | 64 +++-------- packages/core/src/tracing/trace.ts | 4 +- packages/core/src/utils/parseSampleRate.ts | 34 ++++++ .../test/lib/utils/parseSampleRate.test.ts | 23 ++++ packages/opentelemetry/src/sampler.ts | 102 ++---------------- packages/replay-internal/src/integration.ts | 15 +-- .../integration/integrationSettings.test.ts | 26 +++++ packages/utils/src/is.ts | 11 -- packages/utils/src/normalize.ts | 4 +- packages/utils/test/is.test.ts | 16 --- 19 files changed, 184 insertions(+), 182 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/init.js create mode 100644 dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/test.ts create mode 100644 dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/init.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts create mode 100644 packages/core/src/utils/parseSampleRate.ts create mode 100644 packages/core/test/lib/utils/parseSampleRate.test.ts diff --git a/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/init.js b/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/init.js new file mode 100644 index 000000000000..bebdec192df0 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/init.js @@ -0,0 +1,14 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +window._errorCount = 0; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + sampleRate: '0', + beforeSend() { + window._errorCount++; + return null; + }, +}); diff --git a/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/subject.js b/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/subject.js new file mode 100644 index 000000000000..12de659fad4e --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/subject.js @@ -0,0 +1,3 @@ +Sentry.captureException(new Error('test error')); + +window._testDone = true; diff --git a/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/test.ts b/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/test.ts new file mode 100644 index 000000000000..b5aeb4a3182f --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/init/stringSampleRate/test.ts @@ -0,0 +1,16 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; + +sentryTest('parses a string sample rate', async ({ getLocalTestUrl, page }) => { + const url = await getLocalTestUrl({ testDir: __dirname }); + + await page.goto(url); + + await page.waitForFunction('window._testDone'); + await page.evaluate('window.Sentry.getClient().flush()'); + + const count = await page.evaluate('window._errorCount'); + + expect(count).toStrictEqual(0); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/init.js b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/init.js new file mode 100644 index 000000000000..dbefdbdfcda5 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/init.js @@ -0,0 +1,8 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: '1', +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/subject.js b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/subject.js new file mode 100644 index 000000000000..a14b9cd597ac --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/subject.js @@ -0,0 +1,3 @@ +Sentry.startSpan({ name: 'test span' }, () => { + // noop +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts new file mode 100644 index 000000000000..1b411d16e85b --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/stringSampleRate/test.ts @@ -0,0 +1,17 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../utils/fixtures'; +import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequestOnUrl } from '../../../utils/helpers'; + +sentryTest('parses a string sample rate', async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ testDir: __dirname }); + + const req = await waitForTransactionRequestOnUrl(page, url); + const eventData = envelopeRequestParser(req); + + expect(eventData.contexts?.trace?.data?.['sentry.sample_rate']).toStrictEqual(1); +}); diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index aa292e44cf33..454a37071205 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -53,6 +53,7 @@ import { setupIntegration, setupIntegrations } from './integration'; import type { Scope } from './scope'; import { updateSession } from './session'; import { getDynamicSamplingContextFromClient } from './tracing/dynamicSamplingContext'; +import { parseSampleRate } from './utils/parseSampleRate'; import { prepareEvent } from './utils/prepareEvent'; const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured."; @@ -702,7 +703,8 @@ export abstract class BaseClient implements Client { // 1.0 === 100% events are sent // 0.0 === 0% events are sent // Sampling for transaction happens somewhere else - if (isError && typeof sampleRate === 'number' && Math.random() > sampleRate) { + const parsedSampleRate = typeof sampleRate === 'undefined' ? undefined : parseSampleRate(sampleRate); + if (isError && typeof parsedSampleRate === 'number' && Math.random() > parsedSampleRate) { this.recordDroppedEvent('sample_rate', 'error', event); return rejectedSyncPromise( new SentryError( diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 3a5cab6986b3..75efa8b163ff 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -86,6 +86,7 @@ export { getActiveSpan, addChildSpanToSpan, } from './utils/spanUtils'; +export { parseSampleRate } from './utils/parseSampleRate'; export { applySdkMetadata } from './utils/sdkMetadata'; export { DEFAULT_ENVIRONMENT } from './constants'; export { addBreadcrumb } from './breadcrumbs'; diff --git a/packages/core/src/tracing/index.ts b/packages/core/src/tracing/index.ts index c8e38bd9095a..49052bda6371 100644 --- a/packages/core/src/tracing/index.ts +++ b/packages/core/src/tracing/index.ts @@ -17,3 +17,4 @@ export { } from './trace'; export { getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; export { setMeasurement } from './measurement'; +export { sampleSpan } from './sampling'; diff --git a/packages/core/src/tracing/sampling.ts b/packages/core/src/tracing/sampling.ts index 9fa6b29aa902..1f8e0d0eda83 100644 --- a/packages/core/src/tracing/sampling.ts +++ b/packages/core/src/tracing/sampling.ts @@ -1,20 +1,17 @@ -import type { Options, SamplingContext, TransactionArguments } from '@sentry/types'; -import { isNaN, logger } from '@sentry/utils'; +import type { Options, SamplingContext } from '@sentry/types'; +import { logger } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; +import { parseSampleRate } from '../utils/parseSampleRate'; /** - * Makes a sampling decision for the given transaction and stores it on the transaction. + * Makes a sampling decision for the given options. * - * Called every time a transaction is created. Only transactions which emerge with a `sampled` value of `true` will be + * Called every time a root span is created. Only root spans which emerge with a `sampled` value of `true` will be * sent to Sentry. - * - * This method muttes the given `transaction` and will set the `sampled` value on it. - * It returns the same transaction, for convenience. */ -export function sampleTransaction( - transactionContext: TransactionArguments, +export function sampleSpan( options: Pick, samplingContext: SamplingContext, ): [sampled: boolean, sampleRate?: number] { @@ -23,13 +20,6 @@ export function sampleTransaction( return [false]; } - const transactionContextSampled = transactionContext.sampled; - // if the user has forced a sampling decision by passing a `sampled` value in - // their transaction context, go with that. - if (transactionContextSampled !== undefined) { - return [transactionContextSampled, Number(transactionContextSampled)]; - } - // we would have bailed already if neither `tracesSampler` nor `tracesSampleRate` nor `enableTracing` were defined, so one of these should // work; prefer the hook if so let sampleRate; @@ -44,15 +34,17 @@ export function sampleTransaction( sampleRate = 1; } - // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The - // only valid values are booleans or numbers between 0 and 1.) - if (!isValidSampleRate(sampleRate)) { + // Since this is coming from the user (or from a function provided by the user), who knows what we might get. + // (The only valid values are booleans or numbers between 0 and 1.) + const parsedSampleRate = parseSampleRate(sampleRate); + + if (parsedSampleRate === undefined) { DEBUG_BUILD && logger.warn('[Tracing] Discarding transaction because of invalid sample rate.'); return [false]; } // if the function returned 0 (or false), or if `tracesSampleRate` is 0, it's a sign the transaction should be dropped - if (!sampleRate) { + if (!parsedSampleRate) { DEBUG_BUILD && logger.log( `[Tracing] Discarding transaction because ${ @@ -61,12 +53,12 @@ export function sampleTransaction( : 'a negative sampling decision was inherited or tracesSampleRate is set to 0' }`, ); - return [false, Number(sampleRate)]; + return [false, parsedSampleRate]; } // Now we roll the dice. Math.random is inclusive of 0, but not of 1, so strict < is safe here. In case sampleRate is // a boolean, the < comparison will cause it to be automatically cast to 1 if it's true and 0 if it's false. - const shouldSample = Math.random() < sampleRate; + const shouldSample = Math.random() < parsedSampleRate; // if we're not going to keep it, we're done if (!shouldSample) { @@ -76,32 +68,8 @@ export function sampleTransaction( sampleRate, )})`, ); - return [false, Number(sampleRate)]; - } - - return [true, Number(sampleRate)]; -} - -/** - * Checks the given sample rate to make sure it is valid type and value (a boolean, or a number between 0 and 1). - */ -function isValidSampleRate(rate: unknown): rate is number | boolean { - // we need to check NaN explicitly because it's of type 'number' and therefore wouldn't get caught by this typecheck - if (isNaN(rate) || !(typeof rate === 'number' || typeof rate === 'boolean')) { - DEBUG_BUILD && - logger.warn( - `[Tracing] Given sample rate is invalid. Sample rate must be a boolean or a number between 0 and 1. Got ${JSON.stringify( - rate, - )} of type ${JSON.stringify(typeof rate)}.`, - ); - return false; + return [false, parsedSampleRate]; } - // in case sampleRate is a boolean, it will get automatically cast to 1 if it's true and 0 if it's false - if (rate < 0 || rate > 1) { - DEBUG_BUILD && - logger.warn(`[Tracing] Given sample rate is invalid. Sample rate must be between 0 and 1. Got ${rate}.`); - return false; - } - return true; + return [true, parsedSampleRate]; } diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index c4eb70be53b8..a0cda8d88af0 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -18,7 +18,7 @@ import { spanToJSON, } from '../utils/spanUtils'; import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; -import { sampleTransaction } from './sampling'; +import { sampleSpan } from './sampling'; import { SentryNonRecordingSpan } from './sentryNonRecordingSpan'; import type { SentrySpan } from './sentrySpan'; import { SPAN_STATUS_ERROR } from './spanstatus'; @@ -301,7 +301,7 @@ function _startTransaction(transactionContext: TransactionArguments): Transactio const client = getClient(); const options: Partial = (client && client.getOptions()) || {}; - const [sampled, sampleRate] = sampleTransaction(transactionContext, options, { + const [sampled, sampleRate] = sampleSpan(options, { name: transactionContext.name, parentSampled: transactionContext.parentSampled, transactionContext, diff --git a/packages/core/src/utils/parseSampleRate.ts b/packages/core/src/utils/parseSampleRate.ts new file mode 100644 index 000000000000..96bb8c98dec2 --- /dev/null +++ b/packages/core/src/utils/parseSampleRate.ts @@ -0,0 +1,34 @@ +import { logger } from '@sentry/utils'; +import { DEBUG_BUILD } from '../debug-build'; + +/** + * Parse a sample rate from a given value. + * This will either return a boolean or number sample rate, if the sample rate is valid (between 0 and 1). + * If a string is passed, we try to convert it to a number. + * + * Any invalid sample rate will return `undefined`. + */ +export function parseSampleRate(sampleRate: unknown): number | undefined { + if (typeof sampleRate === 'boolean') { + return Number(sampleRate); + } + + const rate = typeof sampleRate === 'string' ? parseFloat(sampleRate) : sampleRate; + if (typeof rate !== 'number' || isNaN(rate)) { + DEBUG_BUILD && + logger.warn( + `[Tracing] Given sample rate is invalid. Sample rate must be a boolean or a number between 0 and 1. Got ${JSON.stringify( + sampleRate, + )} of type ${JSON.stringify(typeof sampleRate)}.`, + ); + return undefined; + } + + if (rate < 0 || rate > 1) { + DEBUG_BUILD && + logger.warn(`[Tracing] Given sample rate is invalid. Sample rate must be between 0 and 1. Got ${rate}.`); + return undefined; + } + + return rate; +} diff --git a/packages/core/test/lib/utils/parseSampleRate.test.ts b/packages/core/test/lib/utils/parseSampleRate.test.ts new file mode 100644 index 000000000000..9c0d91b1810e --- /dev/null +++ b/packages/core/test/lib/utils/parseSampleRate.test.ts @@ -0,0 +1,23 @@ +import { parseSampleRate } from '../../../src/utils/parseSampleRate'; + +describe('parseSampleRate', () => { + it.each([ + [undefined, undefined], + [null, undefined], + [0, 0], + [1, 1], + [0.555, 0.555], + [2, undefined], + [false, 0], + [true, 1], + ['', undefined], + ['aha', undefined], + ['1', 1], + ['1.5', undefined], + ['0.555', 0.555], + ['0', 0], + ])('works with %p', (input, sampleRate) => { + const actual = parseSampleRate(input); + expect(actual).toBe(sampleRate); + }); +}); diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 9ae0b60699ca..3d70a7c35621 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -3,9 +3,9 @@ import { isSpanContextValid, trace } from '@opentelemetry/api'; import { TraceState } from '@opentelemetry/core'; import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base'; import { SamplingDecision } from '@opentelemetry/sdk-trace-base'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, hasTracingEnabled } from '@sentry/core'; -import type { Client, ClientOptions, SamplingContext } from '@sentry/types'; -import { isNaN, logger } from '@sentry/utils'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, hasTracingEnabled, sampleSpan } from '@sentry/core'; +import type { Client } from '@sentry/types'; +import { logger } from '@sentry/utils'; import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from './constants'; import { DEBUG_BUILD } from './debug-build'; @@ -56,7 +56,7 @@ export class SentrySampler implements Sampler { } } - const sampleRate = getSampleRate(options, { + const [sampled, sampleRate] = sampleSpan(options, { name: spanName, attributes: spanAttributes, transactionContext: { @@ -67,52 +67,10 @@ export class SentrySampler implements Sampler { }); const attributes: Attributes = { - [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: Number(sampleRate), + [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: sampleRate, }; - // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The - // only valid values are booleans or numbers between 0 and 1.) - if (!isValidSampleRate(sampleRate)) { - DEBUG_BUILD && logger.warn('[Tracing] Discarding span because of invalid sample rate.'); - - return { - decision: SamplingDecision.NOT_RECORD, - attributes, - traceState: traceState.set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'), - }; - } - - // if the function returned 0 (or false), or if `tracesSampleRate` is 0, it's a sign the transaction should be dropped - if (!sampleRate) { - DEBUG_BUILD && - logger.log( - `[Tracing] Discarding span because ${ - typeof options.tracesSampler === 'function' - ? 'tracesSampler returned 0 or false' - : 'a negative sampling decision was inherited or tracesSampleRate is set to 0' - }`, - ); - - return { - decision: SamplingDecision.NOT_RECORD, - attributes, - traceState: traceState.set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'), - }; - } - - // Now we roll the dice. Math.random is inclusive of 0, but not of 1, so strict < is safe here. In case sampleRate is - // a boolean, the < comparison will cause it to be automatically cast to 1 if it's true and 0 if it's false. - const isSampled = Math.random() < (sampleRate as number | boolean); - - // if we're not going to keep it, we're done - if (!isSampled) { - DEBUG_BUILD && - logger.log( - `[Tracing] Discarding span because it's not included in the random sample (sampling rate = ${Number( - sampleRate, - )})`, - ); - + if (!sampled) { return { decision: SamplingDecision.NOT_RECORD, attributes, @@ -132,54 +90,6 @@ export class SentrySampler implements Sampler { } } -function getSampleRate( - options: Pick, - samplingContext: SamplingContext, -): number | boolean { - if (typeof options.tracesSampler === 'function') { - return options.tracesSampler(samplingContext); - } - - if (samplingContext.parentSampled !== undefined) { - return samplingContext.parentSampled; - } - - if (typeof options.tracesSampleRate !== 'undefined') { - return options.tracesSampleRate; - } - - // When `enableTracing === true`, we use a sample rate of 100% - if (options.enableTracing) { - return 1; - } - - return 0; -} - -/** - * Checks the given sample rate to make sure it is valid type and value (a boolean, or a number between 0 and 1). - */ -function isValidSampleRate(rate: unknown): boolean { - // we need to check NaN explicitly because it's of type 'number' and therefore wouldn't get caught by this typecheck - if (isNaN(rate) || !(typeof rate === 'number' || typeof rate === 'boolean')) { - DEBUG_BUILD && - logger.warn( - `[Tracing] Given sample rate is invalid. Sample rate must be a boolean or a number between 0 and 1. Got ${JSON.stringify( - rate, - )} of type ${JSON.stringify(typeof rate)}.`, - ); - return false; - } - - // in case sampleRate is a boolean, it will get automatically cast to 1 if it's true and 0 if it's false - if (rate < 0 || rate > 1) { - DEBUG_BUILD && - logger.warn(`[Tracing] Given sample rate is invalid. Sample rate must be between 0 and 1. Got ${rate}.`); - return false; - } - return true; -} - function getParentRemoteSampled(spanContext: SpanContext): boolean | undefined { const traceId = spanContext.traceId; const traceparentData = getPropagationContextFromSpanContext(spanContext); diff --git a/packages/replay-internal/src/integration.ts b/packages/replay-internal/src/integration.ts index 89b1cd8709ad..f4759ebff26c 100644 --- a/packages/replay-internal/src/integration.ts +++ b/packages/replay-internal/src/integration.ts @@ -1,4 +1,4 @@ -import { getClient } from '@sentry/core'; +import { getClient, parseSampleRate } from '@sentry/core'; import type { BrowserClientReplayOptions, Integration, IntegrationFn } from '@sentry/types'; import { consoleSandbox, dropUndefinedKeys, isBrowser } from '@sentry/utils'; @@ -367,7 +367,10 @@ function loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions) return finalOptions; } - if (opt.replaysSessionSampleRate == null && opt.replaysOnErrorSampleRate == null) { + const replaysSessionSampleRate = parseSampleRate(opt.replaysSessionSampleRate); + const replaysOnErrorSampleRate = parseSampleRate(opt.replaysOnErrorSampleRate); + + if (replaysSessionSampleRate == null && replaysOnErrorSampleRate == null) { consoleSandbox(() => { // eslint-disable-next-line no-console console.warn( @@ -376,12 +379,12 @@ function loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions) }); } - if (typeof opt.replaysSessionSampleRate === 'number') { - finalOptions.sessionSampleRate = opt.replaysSessionSampleRate; + if (replaysSessionSampleRate != null) { + finalOptions.sessionSampleRate = replaysSessionSampleRate; } - if (typeof opt.replaysOnErrorSampleRate === 'number') { - finalOptions.errorSampleRate = opt.replaysOnErrorSampleRate; + if (replaysOnErrorSampleRate != null) { + finalOptions.errorSampleRate = replaysOnErrorSampleRate; } return finalOptions; diff --git a/packages/replay-internal/test/integration/integrationSettings.test.ts b/packages/replay-internal/test/integration/integrationSettings.test.ts index 118b0511ffce..3d7c180faf2f 100644 --- a/packages/replay-internal/test/integration/integrationSettings.test.ts +++ b/packages/replay-internal/test/integration/integrationSettings.test.ts @@ -37,6 +37,19 @@ describe('Integration | integrationSettings', () => { expect(replay.getOptions().sessionSampleRate).toBe(0); expect(mockConsole).toBeCalledTimes(0); }); + + it('works with defining a string rate in SDK', async () => { + const { replay } = await mockSdk({ + sentryOptions: { + // @ts-expect-error We want to test setting a string here + replaysSessionSampleRate: '0.5', + }, + replayOptions: {}, + }); + + expect(replay.getOptions().sessionSampleRate).toStrictEqual(0.5); + expect(mockConsole).toBeCalledTimes(0); + }); }); describe('replaysOnErrorSampleRate', () => { @@ -63,6 +76,19 @@ describe('Integration | integrationSettings', () => { expect(replay.getOptions().errorSampleRate).toBe(0); expect(mockConsole).toBeCalledTimes(0); }); + + it('works with defining a string rate in SDK', async () => { + const { replay } = await mockSdk({ + sentryOptions: { + // @ts-expect-error We want to test setting a string here + replaysOnErrorSampleRate: '0.5', + }, + replayOptions: {}, + }); + + expect(replay.getOptions().errorSampleRate).toStrictEqual(0.5); + expect(mockConsole).toBeCalledTimes(0); + }); }); describe('all sample rates', () => { diff --git a/packages/utils/src/is.ts b/packages/utils/src/is.ts index fea0402053cc..13ae0edce489 100644 --- a/packages/utils/src/is.ts +++ b/packages/utils/src/is.ts @@ -168,17 +168,6 @@ export function isSyntheticEvent(wat: unknown): boolean { return isPlainObject(wat) && 'nativeEvent' in wat && 'preventDefault' in wat && 'stopPropagation' in wat; } -/** - * Checks whether given value is NaN - * {@link isNaN}. - * - * @param wat A value to be checked. - * @returns A boolean representing the result. - */ -export function isNaN(wat: unknown): boolean { - return typeof wat === 'number' && wat !== wat; -} - /** * Checks whether given value's type is an instance of provided constructor. * {@link isInstanceOf}. diff --git a/packages/utils/src/normalize.ts b/packages/utils/src/normalize.ts index 064ea0ee934e..d86af9561c89 100644 --- a/packages/utils/src/normalize.ts +++ b/packages/utils/src/normalize.ts @@ -1,6 +1,6 @@ import type { Primitive } from '@sentry/types'; -import { isNaN, isSyntheticEvent, isVueViewModel } from './is'; +import { isSyntheticEvent, isVueViewModel } from './is'; import type { MemoFunc } from './memo'; import { memoBuilder } from './memo'; import { convertToPlainObject } from './object'; @@ -81,7 +81,7 @@ function visit( // Get the simple cases out of the way first if ( value == null || // this matches null and undefined -> eqeq not eqeqeq - (['number', 'boolean', 'string'].includes(typeof value) && !isNaN(value)) + (['number', 'boolean', 'string'].includes(typeof value) && !Number.isNaN(value)) ) { return value as Primitive; } diff --git a/packages/utils/test/is.test.ts b/packages/utils/test/is.test.ts index c97c751cb6d3..1ccfc2cd1754 100644 --- a/packages/utils/test/is.test.ts +++ b/packages/utils/test/is.test.ts @@ -4,7 +4,6 @@ import { isError, isErrorEvent, isInstanceOf, - isNaN, isPlainObject, isPrimitive, isThenable, @@ -122,21 +121,6 @@ describe('isInstanceOf()', () => { }); }); -describe('isNaN()', () => { - test('should work as advertised', () => { - expect(isNaN(NaN)).toEqual(true); - - expect(isNaN(null)).toEqual(false); - expect(isNaN(true)).toEqual(false); - expect(isNaN('foo')).toEqual(false); - expect(isNaN(42)).toEqual(false); - expect(isNaN({})).toEqual(false); - expect(isNaN([])).toEqual(false); - expect(isNaN(new Error('foo'))).toEqual(false); - expect(isNaN(new Date())).toEqual(false); - }); -}); - describe('isVueViewModel()', () => { test('should work as advertised', () => { expect(isVueViewModel({ _isVue: true })).toEqual(true);