fix: Create spanRecorder whenever transactions are sampled (#3255)

There were cases in which the sample function would return early without
correctly setting up a spanRecorder, causing child spans to be lost.

Author: rhcarvalho

packages/tracing/src/hubextensions.ts

@@ -1,5 +1,11 @@
 import { getMainCarrier, Hub } from '@sentry/hub';
-import { CustomSamplingContext, SamplingContext, TransactionContext, TransactionSamplingMethod } from '@sentry/types';
+import {
+  CustomSamplingContext,
+  Options,
+  SamplingContext,
+  TransactionContext,
+  TransactionSamplingMethod,
+} from '@sentry/types';
 import { logger } from '@sentry/utils';
 
 import { registerErrorInstrumentation } from './errors';
@@ -33,12 +39,9 @@ function traceHeaders(this: Hub): { [key: string]: string } {
  *
  * @returns The given transaction with its `sampled` value set
  */
-function sample<T extends Transaction>(hub: Hub, transaction: T, samplingContext: SamplingContext): T {
-  const client = hub.getClient();
-  const options = (client && client.getOptions()) || {};
-
-  // nothing to do if there's no client or if tracing is disabled
-  if (!client || !hasTracingEnabled(options)) {
+function sample<T extends Transaction>(transaction: T, options: Options, samplingContext: SamplingContext): T {
+  // nothing to do if tracing is not enabled
+  if (!hasTracingEnabled(options)) {
     transaction.sampled = false;
     return transaction;
   }
@@ -114,10 +117,6 @@ function sample<T extends Transaction>(hub: Hub, transaction: T, samplingContext
     return transaction;
   }
 
-  // at this point we know we're keeping the transaction, whether because of an inherited decision or because it got
-  // lucky with the dice roll
-  transaction.initSpanRecorder(options._experiments?.maxSpans as number);
-
   logger.log(`[Tracing] starting ${transaction.op} transaction - ${transaction.name}`);
   return transaction;
 }
@@ -165,12 +164,18 @@ function _startTransaction(
   transactionContext: TransactionContext,
   customSamplingContext?: CustomSamplingContext,
 ): Transaction {
-  const transaction = new Transaction(transactionContext, this);
-  return sample(this, transaction, {
+  const options = this.getClient()?.getOptions() || {};
+
+  let transaction = new Transaction(transactionContext, this);
+  transaction = sample(transaction, options, {
     parentSampled: transactionContext.parentSampled,
     transactionContext,
     ...customSamplingContext,
   });
+  if (transaction.sampled) {
+    transaction.initSpanRecorder(options._experiments?.maxSpans as number);
+  }
+  return transaction;
 }
 
 /**
@@ -183,12 +188,18 @@ export function startIdleTransaction(
   onScope?: boolean,
   customSamplingContext?: CustomSamplingContext,
 ): IdleTransaction {
-  const transaction = new IdleTransaction(transactionContext, hub, idleTimeout, onScope);
-  return sample(hub, transaction, {
+  const options = hub.getClient()?.getOptions() || {};
+
+  let transaction = new IdleTransaction(transactionContext, hub, idleTimeout, onScope);
+  transaction = sample(transaction, options, {
     parentSampled: transactionContext.parentSampled,
     transactionContext,
     ...customSamplingContext,
   });
+  if (transaction.sampled) {
+    transaction.initSpanRecorder(options._experiments?.maxSpans as number);
+  }
+  return transaction;
 }
 
 /**

packages/tracing/test/span.test.ts

@@ -167,6 +167,18 @@ describe('Span', () => {
         expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext());
       });
 
+      // See https://github.com/getsentry/sentry-javascript/issues/3254
+      test('finish a transaction + child span + sampled:true', () => {
+        const spy = jest.spyOn(hub as any, 'captureEvent') as any;
+        const transaction = hub.startTransaction({ name: 'test', op: 'parent', sampled: true });
+        const childSpan = transaction.startChild({ op: 'child' });
+        childSpan.finish();
+        transaction.finish();
+        expect(spy).toHaveBeenCalled();
+        expect(spy.mock.calls[0][0].spans).toHaveLength(1);
+        expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext());
+      });
+
       test("finish a child span shouldn't trigger captureEvent", () => {
         const spy = jest.spyOn(hub as any, 'captureEvent') as any;
         const transaction = hub.startTransaction({ name: 'test' });