feat(core): Ensure trace APIs always return a span

When tracing is disabled or otherwise skipped, they return a non recording span instead of undefined.

Author: mydea

packages/bun/src/integrations/bunserver.ts

@@ -96,9 +96,7 @@ function instrumentBunServeOptions(serveOptions: Parameters<typeof Bun.serve>[0]
                     typeof serveOptions.fetch
                   >);
                   if (response && response.status) {
-                    if (span) {
-                      setHttpStatus(span, response.status);
-                    }
+                    setHttpStatus(span, response.status);
                     if (span instanceof Transaction) {
                       const scope = getCurrentScope();
                       scope.setContext('response', {

packages/core/src/fetch.ts

@@ -14,6 +14,7 @@ import {
   setHttpStatus,
   startInactiveSpan,
 } from './tracing';
+import { SentryNonRecordingSpan } from './tracing/sentryNonRecordingSpan';
 import { hasTracingEnabled } from './utils/hasTracingEnabled';
 import { spanToTraceHeader } from './utils/spanUtils';
 
@@ -92,12 +93,10 @@ export function instrumentFetchRequest(
         },
         op: 'http.client',
       })
-    : undefined;
+    : new SentryNonRecordingSpan();
 
-  if (span) {
-    handlerData.fetchData.__span = span.spanContext().spanId;
-    spans[span.spanContext().spanId] = span;
-  }
+  handlerData.fetchData.__span = span.spanContext().spanId;
+  spans[span.spanContext().spanId] = span;
 
   if (shouldAttachHeaders(handlerData.fetchData.url) && client) {
     const request: string | Request = handlerData.args[0];

packages/core/src/tracing/index.ts

@@ -2,6 +2,7 @@ export { startIdleTransaction, addTracingExtensions } from './hubextensions';
 export { IdleTransaction, TRACING_DEFAULTS } from './idletransaction';
 export type { BeforeFinishCallback } from './idletransaction';
 export { SentrySpan } from './sentrySpan';
+export { SentryNonRecordingSpan } from './sentryNonRecordingSpan';
 export { Transaction } from './transaction';
 // eslint-disable-next-line deprecation/deprecation
 export { getActiveTransaction, getActiveSpan, getSpanDescendants } from './utils';

packages/core/src/tracing/sentryNonRecordingSpan.ts

@@ -0,0 +1,62 @@
+import type {
+  Span,
+  SpanAttributeValue,
+  SpanAttributes,
+  SpanContext,
+  SpanContextData,
+  SpanStatus,
+  SpanTimeInput,
+} from '@sentry/types';
+import { uuid4 } from '@sentry/utils';
+import { TRACE_FLAG_NONE } from '../utils/spanUtils';
+
+/**
+ * A Sentry Span that is non-recording, meaning it will not be sent to Sentry.
+ */
+export class SentryNonRecordingSpan implements Span {
+  private _traceId: string;
+  private _spanId: string;
+
+  public constructor(spanContext: SpanContext = {}) {
+    this._traceId = spanContext.traceId || uuid4();
+    this._spanId = spanContext.spanId || uuid4().substring(16);
+  }
+
+  /** @inheritdoc */
+  public spanContext(): SpanContextData {
+    return {
+      spanId: this._spanId,
+      traceId: this._traceId,
+      traceFlags: TRACE_FLAG_NONE,
+    };
+  }
+
+  /** @inheritdoc */
+  // eslint-disable-next-line @typescript-eslint/no-empty-function
+  public end(_timestamp?: SpanTimeInput): void {}
+
+  /** @inheritdoc */
+  public setAttribute(_key: string, _value: SpanAttributeValue | undefined): this {
+    return this;
+  }
+
+  /** @inheritdoc */
+  public setAttributes(_values: SpanAttributes): this {
+    return this;
+  }
+
+  /** @inheritdoc */
+  public setStatus(_status: SpanStatus): this {
+    return this;
+  }
+
+  /** @inheritdoc */
+  public updateName(_name: string): this {
+    return this;
+  }
+
+  /** @inheritdoc */
+  public isRecording(): boolean {
+    return false;
+  }
+}

packages/core/src/tracing/trace.ts

@@ -9,6 +9,7 @@ import { handleCallbackErrors } from '../utils/handleCallbackErrors';
 import { hasTracingEnabled } from '../utils/hasTracingEnabled';
 import { spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils';
 import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
+import { SentryNonRecordingSpan } from './sentryNonRecordingSpan';
 import type { SentrySpan } from './sentrySpan';
 import { SPAN_STATUS_ERROR } from './spanstatus';
 import { addChildSpanToSpan, getActiveSpan, setCapturedScopesOnSpan } from './utils';
@@ -24,7 +25,7 @@ import { addChildSpanToSpan, getActiveSpan, setCapturedScopesOnSpan } from './ut
  * or you didn't set `tracesSampleRate`, this function will not generate spans
  * and the `span` returned from the callback will be undefined.
  */
-export function startSpan<T>(context: StartSpanOptions, callback: (span: Span | undefined) => T): T {
+export function startSpan<T>(context: StartSpanOptions, callback: (span: Span) => T): T {
   const spanContext = normalizeContext(context);
 
   return withScope(context.scope, scope => {
@@ -35,18 +36,16 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span |
 
     const shouldSkipSpan = context.onlyIfParent && !parentSpan;
     const activeSpan = shouldSkipSpan
-      ? undefined
+      ? new SentryNonRecordingSpan()
       : createChildSpanOrTransaction(hub, {
           parentSpan,
           spanContext,
           forceTransaction: context.forceTransaction,
           scope,
         });
 
-    if (activeSpan) {
-      // eslint-disable-next-line deprecation/deprecation
-      scope.setSpan(activeSpan);
-    }
+    // eslint-disable-next-line deprecation/deprecation
+    scope.setSpan(activeSpan);
 
     return handleCallbackErrors(
       () => callback(activeSpan),
@@ -75,10 +74,7 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span |
  * or you didn't set `tracesSampleRate`, this function will not generate spans
  * and the `span` returned from the callback will be undefined.
  */
-export function startSpanManual<T>(
-  context: StartSpanOptions,
-  callback: (span: Span | undefined, finish: () => void) => T,
-): T {
+export function startSpanManual<T>(context: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T {
   const spanContext = normalizeContext(context);
 
   return withScope(context.scope, scope => {
@@ -89,18 +85,16 @@ export function startSpanManual<T>(
 
     const shouldSkipSpan = context.onlyIfParent && !parentSpan;
     const activeSpan = shouldSkipSpan
-      ? undefined
+      ? new SentryNonRecordingSpan()
       : createChildSpanOrTransaction(hub, {
           parentSpan,
           spanContext,
           forceTransaction: context.forceTransaction,
           scope,
         });
 
-    if (activeSpan) {
-      // eslint-disable-next-line deprecation/deprecation
-      scope.setSpan(activeSpan);
-    }
+    // eslint-disable-next-line deprecation/deprecation
+    scope.setSpan(activeSpan);
 
     function finishAndSetSpan(): void {
       activeSpan && activeSpan.end();
@@ -131,11 +125,7 @@ export function startSpanManual<T>(
  * or you didn't set `tracesSampleRate` or `tracesSampler`, this function will not generate spans
  * and the `span` returned from the callback will be undefined.
  */
-export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
-  if (!hasTracingEnabled()) {
-    return undefined;
-  }
-
+export function startInactiveSpan(context: StartSpanOptions): Span {
   const spanContext = normalizeContext(context);
   // eslint-disable-next-line deprecation/deprecation
   const hub = getCurrentHub();
@@ -147,7 +137,7 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
   const shouldSkipSpan = context.onlyIfParent && !parentSpan;
 
   if (shouldSkipSpan) {
-    return undefined;
+    return new SentryNonRecordingSpan();
   }
 
   const scope = context.scope || getCurrentScope();
@@ -275,14 +265,14 @@ function createChildSpanOrTransaction(
     forceTransaction?: boolean;
     scope: Scope;
   },
-): Span | undefined {
+): Span {
   if (!hasTracingEnabled()) {
-    return undefined;
+    return new SentryNonRecordingSpan();
   }
 
   const isolationScope = getIsolationScope();
 
-  let span: Span | undefined;
+  let span: Span;
   if (parentSpan && !forceTransaction) {
     // eslint-disable-next-line deprecation/deprecation
     span = parentSpan.startChild(spanContext);

packages/core/src/tracing/utils.ts

@@ -5,6 +5,7 @@ import { getCurrentScope } from '../currentScopes';
 
 import type { Hub } from '../hub';
 import { getCurrentHub } from '../hub';
+import { spanIsSampled } from '../utils/spanUtils';
 
 /**
  * Grabs active transaction off scope.
@@ -58,7 +59,8 @@ export function getSpanDescendants(span: SpanWithPotentialChildren): Span[] {
     // This exit condition is required to not infinitely loop in case of a circular dependency.
     if (resultSet.has(span)) {
       return;
-    } else {
+      // We want to ignore unsampled spans (e.g. non recording spans)
+    } else if (spanIsSampled(span)) {
       resultSet.add(span);
       const childSpans = span[CHILD_SPANS_FIELD] ? Array.from(span[CHILD_SPANS_FIELD]) : [];
       for (const childSpan of childSpans) {

packages/core/src/utils/spanUtils.ts

@@ -10,8 +10,8 @@ import type {
 import { dropUndefinedKeys, generateSentryTraceHeader, timestampInSeconds } from '@sentry/utils';
 import { getMetricSummaryJsonForSpan } from '../metrics/metric-summary';
 import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../semanticAttributes';
-import { SPAN_STATUS_OK, SPAN_STATUS_UNSET } from '../tracing';
 import type { SentrySpan } from '../tracing/sentrySpan';
+import { SPAN_STATUS_OK, SPAN_STATUS_UNSET } from '../tracing/spanstatus';
 
 // These are aligned with OpenTelemetry trace flags
 export const TRACE_FLAG_NONE = 0x0;

packages/core/test/lib/scope.test.ts

@@ -956,21 +956,21 @@ describe('withActiveSpan()', () => {
 
     expect(getActiveSpan()).not.toBe(inactiveSpan);
 
-    withActiveSpan(inactiveSpan!, () => {
+    withActiveSpan(inactiveSpan, () => {
       expect(getActiveSpan()).toBe(inactiveSpan);
     });
   });
 
   it('should create child spans when calling startSpan within the callback', () => {
     const inactiveSpan = startInactiveSpan({ name: 'inactive-span' });
 
-    const parentSpanId = withActiveSpan(inactiveSpan!, () => {
+    const parentSpanId = withActiveSpan(inactiveSpan, () => {
       return startSpan({ name: 'child-span' }, childSpan => {
-        return spanToJSON(childSpan!).parent_span_id;
+        return spanToJSON(childSpan).parent_span_id;
       });
     });
 
-    expect(parentSpanId).toBe(inactiveSpan?.spanContext().spanId);
+    expect(parentSpanId).toBe(inactiveSpan.spanContext().spanId);
   });
 
   it('when `null` is passed, no span should be active within the callback', () => {

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

@@ -68,7 +68,7 @@ describe('registerErrorHandlers()', () => {
 
     startSpan({ name: 'test' }, span => {
       mockErrorCallback({} as HandlerDataError);
-      expect(spanToJSON(span!).status).toBe('internal_error');
+      expect(spanToJSON(span).status).toBe('internal_error');
     });
   });
 
@@ -77,7 +77,7 @@ describe('registerErrorHandlers()', () => {
 
     startSpan({ name: 'test' }, span => {
       mockUnhandledRejectionCallback({});
-      expect(spanToJSON(span!).status).toBe('internal_error');
+      expect(spanToJSON(span).status).toBe('internal_error');
     });
   });
 });

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

@@ -170,10 +170,10 @@ describe('IdleTransaction', () => {
     startSpanManual({ name: 'inner1' }, span => {
       const childSpan = startInactiveSpan({ name: 'inner2' })!;
       expect(transaction.activities).toMatchObject({
-        [span!.spanContext().spanId]: true,
+        [span.spanContext().spanId]: true,
         [childSpan.spanContext().spanId]: true,
       });
-      span?.end();
+      span.end();
       jest.advanceTimersByTime(TRACING_DEFAULTS.idleTimeout + 1);
 
       expect(mockFinish).toHaveBeenCalledTimes(0);

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

@@ -0,0 +1,37 @@
+import type { Span } from '@sentry/types';
+import { SPAN_STATUS_ERROR } from '../../../src/tracing';
+import { SentryNonRecordingSpan } from '../../../src/tracing/sentryNonRecordingSpan';
+import { TRACE_FLAG_NONE, spanIsSampled, spanToJSON } from '../../../src/utils/spanUtils';
+
+describe('SentryNonRecordingSpan', () => {
+  it('satisfies the Span interface', () => {
+    const span: Span = new SentryNonRecordingSpan();
+
+    expect(span.spanContext()).toEqual({
+      spanId: expect.any(String),
+      traceId: expect.any(String),
+      traceFlags: TRACE_FLAG_NONE,
+    });
+
+    expect(spanIsSampled(span)).toBe(false);
+    expect(span.isRecording()).toBe(false);
+    expect(spanToJSON(span)).toEqual({
+      span_id: expect.any(String),
+      trace_id: expect.any(String),
+    });
+
+    // Ensure all methods work
+    span.end();
+    span.end(123);
+    span.updateName('name');
+    span.setAttribute('key', 'value');
+    span.setAttributes({ key: 'value' });
+    span.setStatus({ code: SPAN_STATUS_ERROR });
+
+    // but nothing is actually set/readable
+    expect(spanToJSON(span)).toEqual({
+      span_id: expect.any(String),
+      trace_id: expect.any(String),
+    });
+  });
+});

packages/core/test/lib/tracing/span.test.ts renamed to packages/core/test/lib/tracing/sentrySpan.test.ts

@@ -1,9 +1,9 @@
 import { timestampInSeconds } from '@sentry/utils';
-import { SentrySpan } from '../../../src';
+import { SentrySpan } from '../../../src/tracing/sentrySpan';
 import { SPAN_STATUS_ERROR } from '../../../src/tracing/spanstatus';
 import { TRACE_FLAG_NONE, TRACE_FLAG_SAMPLED, spanToJSON, spanToTraceContext } from '../../../src/utils/spanUtils';
 
-describe('span', () => {
+describe('SentrySpan', () => {
   describe('name', () => {
     it('works with name', () => {
       const span = new SentrySpan({ name: 'span name' });

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

@@ -18,9 +18,9 @@ describe('setHttpStatus', () => {
   ])('applies the correct span status and http status code to the span (%s - $%s)', (code, status) => {
     const span = new SentrySpan({ name: 'test' });
 
-    setHttpStatus(span!, code);
+    setHttpStatus(span, code);
 
-    const { status: spanStatus, data } = spanToJSON(span!);
+    const { status: spanStatus, data } = spanToJSON(span);
 
     expect(spanStatus).toBe(status);
     expect(data).toMatchObject({ 'http.response.status_code': code });
@@ -29,9 +29,9 @@ describe('setHttpStatus', () => {
   it("doesn't set the status for an unknown http status code", () => {
     const span = new SentrySpan({ name: 'test' });
 
-    setHttpStatus(span!, 600);
+    setHttpStatus(span, 600);
 
-    const { status: spanStatus, data } = spanToJSON(span!);
+    const { status: spanStatus, data } = spanToJSON(span);
 
     expect(spanStatus).toBeUndefined();
     expect(data).toMatchObject({ 'http.response.status_code': 600 });