feat(core): Handle string sample rates (#11305)

mydea · web-flow · commit ad02a7942df9 · 2024-03-27T14:54:45.000+01:00
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. I also updated OTEL to use the same sampling function as core (to avoid this drifting apart), and am using this for `sampleRate`, `tracesSampleRate` and the replay sample rates now. See e.g. #11262
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<O extends ClientOptions> implements Client<O> {
     // 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
@@ -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
@@ -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
@@ -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<Options, 'tracesSampleRate' | 'tracesSampler' | 'enableTracing'>,
   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
@@ -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<ClientOptions> = (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
@@ -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
@@ -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
diff --git a/packages/replay-internal/src/integration.ts b/packages/replay-internal/src/integration.ts
diff --git a/packages/replay-internal/test/integration/integrationSettings.test.ts b/packages/replay-internal/test/integration/integrationSettings.test.ts
diff --git a/packages/utils/src/is.ts b/packages/utils/src/is.ts
diff --git a/packages/utils/src/normalize.ts b/packages/utils/src/normalize.ts
diff --git a/packages/utils/test/is.test.ts b/packages/utils/test/is.test.ts

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+Sentry.captureException(new Error('test error'));`
	`2`	`+`
	`3`	`+window._testDone = true;`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+Sentry.startSpan({ name: 'test span' }, () => {`
	`2`	`+ // noop`
	`3`	`+});`