feat(v8/core): Remove deprecated span.sampled (#11274)

ref #10100

Removes `span.sampled`, and rewrites transaction sampling to avoid
mutating transaction.

Author: AbhiPrasad

packages/core/src/tracing/sampling.ts

@@ -1,11 +1,8 @@
-import type { Options, SamplingContext } from '@sentry/types';
+import type { Options, SamplingContext, TransactionContext } from '@sentry/types';
 import { isNaN, logger } from '@sentry/utils';
 
 import { DEBUG_BUILD } from '../debug-build';
-import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '../semanticAttributes';
 import { hasTracingEnabled } from '../utils/hasTracingEnabled';
-import { spanToJSON } from '../utils/spanUtils';
-import type { Transaction } from './transaction';
 
 /**
  * Makes a sampling decision for the given transaction and stores it on the transaction.
@@ -16,50 +13,42 @@ import type { Transaction } from './transaction';
  * This method muttes the given `transaction` and will set the `sampled` value on it.
  * It returns the same transaction, for convenience.
  */
-export function sampleTransaction<T extends Transaction>(
-  transaction: T,
+export function sampleTransaction(
+  transactionContext: TransactionContext,
   options: Pick<Options, 'tracesSampleRate' | 'tracesSampler' | 'enableTracing'>,
   samplingContext: SamplingContext,
-): T {
+): [sampled: boolean, sampleRate?: number] {
   // nothing to do if tracing is not enabled
   if (!hasTracingEnabled(options)) {
-    // eslint-disable-next-line deprecation/deprecation
-    transaction.sampled = false;
-    return transaction;
+    return [false];
   }
 
-  // if the user has forced a sampling decision by passing a `sampled` value in their transaction context, go with that
-  // eslint-disable-next-line deprecation/deprecation
-  if (transaction.sampled !== undefined) {
-    // eslint-disable-next-line deprecation/deprecation
-    transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, Number(transaction.sampled));
-    return transaction;
+  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;
   if (typeof options.tracesSampler === 'function') {
     sampleRate = options.tracesSampler(samplingContext);
-    transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, Number(sampleRate));
   } else if (samplingContext.parentSampled !== undefined) {
     sampleRate = samplingContext.parentSampled;
   } else if (typeof options.tracesSampleRate !== 'undefined') {
     sampleRate = options.tracesSampleRate;
-    transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, Number(sampleRate));
   } else {
     // When `enableTracing === true`, we use a sample rate of 100%
     sampleRate = 1;
-    transaction.setAttribute(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 transaction because of invalid sample rate.');
-    // eslint-disable-next-line deprecation/deprecation
-    transaction.sampled = false;
-    return transaction;
+    return [false];
   }
 
   // if the function returned 0 (or false), or if `tracesSampleRate` is 0, it's a sign the transaction should be dropped
@@ -72,40 +61,31 @@ export function sampleTransaction<T extends Transaction>(
             : 'a negative sampling decision was inherited or tracesSampleRate is set to 0'
         }`,
       );
-    // eslint-disable-next-line deprecation/deprecation
-    transaction.sampled = false;
-    return transaction;
+    return [false, Number(sampleRate)];
   }
 
   // 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.
-  // eslint-disable-next-line deprecation/deprecation
-  transaction.sampled = Math.random() < (sampleRate as number | boolean);
+  const shouldSample = Math.random() < sampleRate;
 
   // if we're not going to keep it, we're done
-  // eslint-disable-next-line deprecation/deprecation
-  if (!transaction.sampled) {
+  if (!shouldSample) {
     DEBUG_BUILD &&
       logger.log(
         `[Tracing] Discarding transaction because it's not included in the random sample (sampling rate = ${Number(
           sampleRate,
         )})`,
       );
-    return transaction;
+    return [false, Number(sampleRate)];
   }
 
-  if (DEBUG_BUILD) {
-    const { op, description } = spanToJSON(transaction);
-    logger.log(`[Tracing] starting ${op} transaction - ${description}`);
-  }
-
-  return transaction;
+  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): boolean {
+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 &&

packages/core/src/tracing/sentrySpan.ts

@@ -97,22 +97,6 @@ export class SentrySpan implements Span {
   // This rule conflicts with another eslint rule :(
   /* eslint-disable @typescript-eslint/member-ordering */
 
-  /**
-   * Was this span chosen to be sent as part of the sample?
-   * @deprecated Use `isRecording()` instead.
-   */
-  public get sampled(): boolean | undefined {
-    return this._sampled;
-  }
-
-  /**
-   * Was this span chosen to be sent as part of the sample?
-   * @deprecated You cannot update the sampling decision of a span after span creation.
-   */
-  public set sampled(sampled: boolean | undefined) {
-    this._sampled = sampled;
-  }
-
   /**
    * Attributes for the span.
    * @deprecated Use `spanToJSON(span).atttributes` instead.

packages/core/src/tracing/trace.ts

@@ -14,6 +14,7 @@ import { getMainCarrier } from '../asyncContext';
 import { getClient, getCurrentScope, getIsolationScope, withScope } from '../currentScopes';
 
 import { getAsyncContextStrategy, getCurrentHub } from '../hub';
+import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '../semanticAttributes';
 import { handleCallbackErrors } from '../utils/handleCallbackErrors';
 import { hasTracingEnabled } from '../utils/hasTracingEnabled';
 import {
@@ -325,9 +326,7 @@ function _startTransaction(transactionContext: TransactionContext): Transaction
   const client = getClient();
   const options: Partial<ClientOptions> = (client && client.getOptions()) || {};
 
-  // eslint-disable-next-line deprecation/deprecation
-  let transaction = new Transaction(transactionContext, getCurrentHub());
-  transaction = sampleTransaction(transaction, options, {
+  const [sampled, sampleRate] = sampleTransaction(transactionContext, options, {
     name: transactionContext.name,
     parentSampled: transactionContext.parentSampled,
     transactionContext,
@@ -337,8 +336,16 @@ function _startTransaction(transactionContext: TransactionContext): Transaction
       ...transactionContext.attributes,
     },
   });
+
+  // eslint-disable-next-line deprecation/deprecation
+  const transaction = new Transaction({ ...transactionContext, sampled }, getCurrentHub());
+  if (sampleRate !== undefined) {
+    transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, sampleRate);
+  }
+
   if (client) {
     client.emit('spanStart', transaction);
   }
+
   return transaction;
 }

packages/core/test/lib/tracing/trace.test.ts

@@ -354,6 +354,7 @@ describe('startSpan', () => {
       trace: {
         data: {
           'sentry.source': 'custom',
+          'sentry.sample_rate': 1,
           'sentry.origin': 'manual',
         },
         parent_span_id: innerParentSpanId,
@@ -679,6 +680,7 @@ describe('startSpanManual', () => {
       trace: {
         data: {
           'sentry.source': 'custom',
+          'sentry.sample_rate': 1,
           'sentry.origin': 'manual',
         },
         parent_span_id: innerParentSpanId,
@@ -932,6 +934,7 @@ describe('startInactiveSpan', () => {
       trace: {
         data: {
           'sentry.source': 'custom',
+          'sentry.sample_rate': 1,
           'sentry.origin': 'manual',
         },
         parent_span_id: innerParentSpanId,

packages/types/src/transaction.ts

@@ -58,12 +58,6 @@ export interface TraceparentData {
  * Transaction "Class", inherits Span only has `setName`
  */
 export interface Transaction extends Omit<TransactionContext, 'name' | 'op' | 'spanId' | 'traceId'>, Span {
-  /**
-   * Was this transaction chosen to be sent as part of the sample?
-   * @deprecated Use `spanIsSampled(transaction)` instead.
-   */
-  sampled?: boolean | undefined;
-
   /**
    * @inheritDoc
    */