ref: Collect child spans references via non-enumerable on Span object (#10715)

Author: Luca Forstner

packages/core/src/tracing/hubextensions.ts

@@ -75,7 +75,7 @@ The transaction will not be sampled. Please use the ${configInstrumenter} instru
     ...customSamplingContext,
   });
   if (transaction.isRecording()) {
-    transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
+    transaction.initSpanRecorder();
   }
   if (client) {
     client.emit('startTransaction', transaction);
@@ -122,7 +122,7 @@ export function startIdleTransaction(
     ...customSamplingContext,
   });
   if (transaction.isRecording()) {
-    transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
+    transaction.initSpanRecorder();
   }
   if (client) {
     client.emit('startTransaction', transaction);

packages/core/src/tracing/sentrySpan.ts

@@ -29,6 +29,7 @@ import {
 } from '../utils/spanUtils';
 import type { SpanStatusType } from './spanstatus';
 import { setHttpStatus } from './spanstatus';
+import { addChildSpanToSpan } from './trace';
 
 /**
  * Keeps track of finished spans for a given transaction
@@ -401,6 +402,11 @@ export class SentrySpan implements SpanInterface {
       childSpan.spanRecorder.add(childSpan);
     }
 
+    // To allow for interoperability we track the children of a span twice: Once with the span recorder (old) once with
+    // the `addChildSpanToSpan`. Eventually we will only use `addChildSpanToSpan` and drop the span recorder.
+    // To ensure interoperability with the `startSpan` API, `addChildSpanToSpan` is also called here.
+    addChildSpanToSpan(this, childSpan);
+
     const rootSpan = getRootSpan(this);
     // TODO: still set span.transaction here until we have a more permanent solution
     // Probably similarly to the weakmap we hold in node-experimental

packages/core/src/tracing/trace.ts

@@ -156,6 +156,10 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
     });
   }
 
+  if (parentSpan) {
+    addChildSpanToSpan(parentSpan, span);
+  }
+
   setCapturedScopesOnSpan(span, scope, isolationScope);
 
   return span;
@@ -307,6 +311,10 @@ function createChildSpanOrTransaction(
     });
   }
 
+  if (parentSpan) {
+    addChildSpanToSpan(parentSpan, span);
+  }
+
   setCapturedScopesOnSpan(span, scope, isolationScope);
 
   return span;
@@ -330,6 +338,47 @@ function normalizeContext(context: StartSpanOptions): TransactionContext {
   return context;
 }
 
+const CHILD_SPANS_FIELD = '_sentryChildSpans';
+
+type SpanWithPotentialChildren = Span & {
+  [CHILD_SPANS_FIELD]?: Set<Span>;
+};
+
+/**
+ * Adds an opaque child span reference to a span.
+ */
+export function addChildSpanToSpan(span: SpanWithPotentialChildren, childSpan: Span): void {
+  if (span[CHILD_SPANS_FIELD] && span[CHILD_SPANS_FIELD].size < 1000) {
+    span[CHILD_SPANS_FIELD].add(childSpan);
+  } else {
+    span[CHILD_SPANS_FIELD] = new Set([childSpan]);
+  }
+}
+
+/**
+ * Obtains the entire span tree, meaning a span + all of its descendants for a particular span.
+ */
+export function getSpanTree(span: SpanWithPotentialChildren): Span[] {
+  const resultSet = new Set<Span>();
+
+  function addSpanChildren(span: SpanWithPotentialChildren): void {
+    // This exit condition is required to not infinitely loop in case of a circular dependency.
+    if (resultSet.has(span)) {
+      return;
+    } else {
+      resultSet.add(span);
+      const childSpans = span[CHILD_SPANS_FIELD] ? Array.from(span[CHILD_SPANS_FIELD]) : [];
+      for (const childSpan of childSpans) {
+        addSpanChildren(childSpan);
+      }
+    }
+  }
+
+  addSpanChildren(span);
+
+  return Array.from(resultSet);
+}
+
 const SCOPE_ON_START_SPAN_FIELD = '_sentryScope';
 const ISOLATION_SCOPE_ON_START_SPAN_FIELD = '_sentryIsolationScope';
 

packages/core/src/tracing/transaction.ts

@@ -21,7 +21,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE
 import { spanTimeInputToSeconds, spanToJSON, spanToTraceContext } from '../utils/spanUtils';
 import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
 import { SentrySpan, SpanRecorder } from './sentrySpan';
-import { getCapturedScopesOnSpan } from './trace';
+import { getCapturedScopesOnSpan, getSpanTree } from './trace';
 
 /** JSDoc */
 export class Transaction extends SentrySpan implements TransactionInterface {
@@ -293,11 +293,8 @@ export class Transaction extends SentrySpan implements TransactionInterface {
       return undefined;
     }
 
-    // eslint-disable-next-line deprecation/deprecation
-    const finishedSpans = this.spanRecorder
-      ? // eslint-disable-next-line deprecation/deprecation
-        this.spanRecorder.spans.filter(span => span !== this && spanToJSON(span).timestamp)
-      : [];
+    // We only want to include finished spans in the event
+    const finishedSpans = getSpanTree(this).filter(span => span !== this && spanToJSON(span).timestamp);
 
     if (this._trimEnd && finishedSpans.length > 0) {
       const endTimes = finishedSpans.map(span => spanToJSON(span).timestamp).filter(Boolean) as number[];

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

@@ -436,6 +436,16 @@ describe('startSpan', () => {
       expect.anything(),
     );
   });
+
+  it('sets a child span reference on the parent span', () => {
+    expect.assertions(1);
+    startSpan({ name: 'outer' }, (outerSpan: any) => {
+      startSpan({ name: 'inner' }, innerSpan => {
+        const childSpans = Array.from(outerSpan._sentryChildSpans);
+        expect(childSpans).toContain(innerSpan);
+      });
+    });
+  });
 });
 
 describe('startSpanManual', () => {
@@ -545,6 +555,16 @@ describe('startSpanManual', () => {
       expect(span).toBeDefined();
     });
   });
+
+  it('sets a child span reference on the parent span', () => {
+    expect.assertions(1);
+    startSpan({ name: 'outer' }, (outerSpan: any) => {
+      startSpanManual({ name: 'inner' }, innerSpan => {
+        const childSpans = Array.from(outerSpan._sentryChildSpans);
+        expect(childSpans).toContain(innerSpan);
+      });
+    });
+  });
 });
 
 describe('startInactiveSpan', () => {
@@ -674,6 +694,15 @@ describe('startInactiveSpan', () => {
       expect.anything(),
     );
   });
+
+  it('sets a child span reference on the parent span', () => {
+    expect.assertions(1);
+    startSpan({ name: 'outer' }, (outerSpan: any) => {
+      const innerSpan = startInactiveSpan({ name: 'inner' });
+      const childSpans = Array.from(outerSpan._sentryChildSpans);
+      expect(childSpans).toContain(innerSpan);
+    });
+  });
 });
 
 describe('continueTrace', () => {

packages/opentelemetry/src/custom/transaction.ts

@@ -1,6 +1,6 @@
 import type { Hub } from '@sentry/core';
 import { Transaction } from '@sentry/core';
-import type { ClientOptions, Hub as HubInterface, TransactionContext } from '@sentry/types';
+import type { Hub as HubInterface, TransactionContext } from '@sentry/types';
 
 /**
  * This is a fork of core's tracing/hubextensions.ts _startTransaction,
@@ -9,13 +9,12 @@ import type { ClientOptions, Hub as HubInterface, TransactionContext } from '@se
 export function startTransaction(hub: HubInterface, transactionContext: TransactionContext): Transaction {
   // eslint-disable-next-line deprecation/deprecation
   const client = hub.getClient();
-  const options: Partial<ClientOptions> = (client && client.getOptions()) || {};
 
   // eslint-disable-next-line deprecation/deprecation
   const transaction = new Transaction(transactionContext, hub as Hub);
   // Since we do not do sampling here, we assume that this is _always_ sampled
   // Any sampling decision happens in OpenTelemetry's sampler
-  transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
+  transaction.initSpanRecorder();
 
   if (client) {
     client.emit('startTransaction', transaction);

packages/tracing/test/span.test.ts

@@ -272,22 +272,6 @@ describe('SentrySpan', () => {
         expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext());
       });
 
-      test('maxSpans correctly limits number of spans', () => {
-        const options = getDefaultBrowserClientOptions({
-          _experiments: { maxSpans: 3 },
-          tracesSampleRate: 1,
-        });
-        const _hub = new Hub(new BrowserClient(options));
-        const spy = jest.spyOn(_hub as any, 'captureEvent') as any;
-        const transaction = _hub.startTransaction({ name: 'test' });
-        for (let i = 0; i < 10; i++) {
-          const child = transaction.startChild();
-          child.end();
-        }
-        transaction.end();
-        expect(spy.mock.calls[0][0].spans).toHaveLength(3);
-      });
-
       test('no span recorder created if transaction.sampled is false', () => {
         const options = getDefaultBrowserClientOptions({
           tracesSampleRate: 1,
@@ -421,22 +405,6 @@ describe('SentrySpan', () => {
         expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext());
       });
 
-      test('maxSpans correctly limits number of spans', () => {
-        const options = getDefaultBrowserClientOptions({
-          _experiments: { maxSpans: 3 },
-          tracesSampleRate: 1,
-        });
-        const _hub = new Hub(new BrowserClient(options));
-        const spy = jest.spyOn(_hub as any, 'captureEvent') as any;
-        const transaction = _hub.startTransaction({ name: 'test' });
-        for (let i = 0; i < 10; i++) {
-          const child = transaction.startChild();
-          child.end();
-        }
-        transaction.end();
-        expect(spy.mock.calls[0][0].spans).toHaveLength(3);
-      });
-
       test('no span recorder created if transaction.sampled is false', () => {
         const options = getDefaultBrowserClientOptions({
           tracesSampleRate: 1,