fix(tracing): Ensure sent spans are limited to 1000 (#12252)

To avoid too large payloads, we should only send up to 1000 spans.

We actually had two different problems here in browser and node:

1. In browser, we _did_ limit to 1000, but we did so in an unfortunate
way, which is to reset the set after 1000 spans. So if a span had 1010
children, only the last 10 would be send.
2. In node, we just sent all spans.

I rewrote this to now consistently send the first 1000 spans of a
transaction.

I decided to keep all spans (no matter the limit) on the internal
parent-child map. Due to this this may grow a lot, but we can avoid
inconsistent state (e.g. what happens if a span has a parent in the map,
but the parent does not have the span as children, ...)
All of these should be garbage collected together ideally, so this
should be fine hopefully.

Author: mydea

dev-packages/browser-integration-tests/suites/tracing/maxSpans/init.js

@@ -0,0 +1,15 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [],
+  tracesSampleRate: 1,
+});
+
+Sentry.startSpan({ name: 'parent' }, () => {
+  for (let i = 0; i < 5000; i++) {
+    Sentry.startInactiveSpan({ name: `child ${i}` }).end();
+  }
+});

dev-packages/browser-integration-tests/suites/tracing/maxSpans/test.ts

@@ -0,0 +1,20 @@
+import { expect } from '@playwright/test';
+import { sentryTest } from '../../../utils/fixtures';
+import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequestOnUrl } from '../../../utils/helpers';
+
+sentryTest('it limits spans to 1000', async ({ getLocalTestUrl, page }) => {
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestUrl({ testDir: __dirname });
+  await page.goto(url);
+
+  const req = await waitForTransactionRequestOnUrl(page, url);
+  const transaction = envelopeRequestParser(req);
+
+  expect(transaction.spans).toHaveLength(1000);
+  expect(transaction.spans).toContainEqual(expect.objectContaining({ description: 'child 0' }));
+  expect(transaction.spans).toContainEqual(expect.objectContaining({ description: 'child 999' }));
+  expect(transaction.spans).not.toContainEqual(expect.objectContaining({ description: 'child 1000' }));
+});

dev-packages/node-integration-tests/suites/tracing/maxSpans/scenario.ts

@@ -0,0 +1,15 @@
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+});
+
+Sentry.startSpan({ name: 'parent' }, () => {
+  for (let i = 0; i < 5000; i++) {
+    Sentry.startInactiveSpan({ name: `child ${i}` }).end();
+  }
+});

dev-packages/node-integration-tests/suites/tracing/maxSpans/test.ts

@@ -0,0 +1,19 @@
+import type { SpanJSON } from '@sentry/types';
+import { createRunner } from '../../../utils/runner';
+
+test('it limits spans to 1000', done => {
+  const expectedSpans: SpanJSON[] = [];
+  for (let i = 0; i < 1000; i++) {
+    expectedSpans.push(expect.objectContaining({ description: `child ${i}` }));
+  }
+
+  createRunner(__dirname, 'scenario.ts')
+    .ignore('session', 'sessions')
+    .expect({
+      transaction: {
+        transaction: 'parent',
+        spans: expectedSpans,
+      },
+    })
+    .start(done);
+});

packages/core/src/tracing/sentrySpan.ts

@@ -41,6 +41,8 @@ import { logSpanEnd } from './logSpans';
 import { timedEventsToMeasurements } from './measurement';
 import { getCapturedScopesOnSpan } from './utils';
 
+const MAX_SPAN_COUNT = 1000;
+
 /**
  * Span contains all data about a span
  */
@@ -310,7 +312,12 @@ export class SentrySpan implements Span {
       contexts: {
         trace: spanToTransactionTraceContext(this),
       },
-      spans,
+      spans:
+        // spans.sort() mutates the array, but `spans` is already a copy so we can safely do this here
+        // we do not use spans anymore after this point
+        spans.length > MAX_SPAN_COUNT
+          ? spans.sort((a, b) => a.start_timestamp - b.start_timestamp).slice(0, MAX_SPAN_COUNT)
+          : spans,
       start_timestamp: this._startTime,
       timestamp: this._endTime,
       transaction: this._name,

packages/core/src/utils/spanUtils.ts

@@ -208,7 +208,7 @@ export function addChildSpanToSpan(span: SpanWithPotentialChildren, childSpan: S
 
   // We store a list of child spans on the parent span
   // We need this for `getSpanDescendants()` to work
-  if (span[CHILD_SPANS_FIELD] && span[CHILD_SPANS_FIELD].size < 1000) {
+  if (span[CHILD_SPANS_FIELD]) {
     span[CHILD_SPANS_FIELD].add(childSpan);
   } else {
     addNonEnumerableProperty(span, CHILD_SPANS_FIELD, new Set([childSpan]));

packages/opentelemetry/src/spanExporter.ts

@@ -31,6 +31,8 @@ import { parseSpanDescription } from './utils/parseSpanDescription';
 
 type SpanNodeCompleted = SpanNode & { span: ReadableSpan };
 
+const MAX_SPAN_COUNT = 1000;
+
 /**
  * A Sentry-specific exporter that converts OpenTelemetry Spans to Sentry Spans & Transactions.
  */
@@ -140,7 +142,12 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] {
       createAndFinishSpanForOtelSpan(child, spans, remaining);
     });
 
-    transactionEvent.spans = spans;
+    // spans.sort() mutates the array, but we do not use this anymore after this point
+    // so we can safely mutate it here
+    transactionEvent.spans =
+      spans.length > MAX_SPAN_COUNT
+        ? spans.sort((a, b) => a.start_timestamp - b.start_timestamp).slice(0, MAX_SPAN_COUNT)
+        : spans;
 
     const measurements = timedEventsToMeasurements(span.events);
     if (measurements) {