feat(core): Add parentSpan option to startSpan* APIs (#12567)

With this PR, you can now pass a `parentSpan` optionally to `startSpan*`
APIs to create a span as a child of a specific span:


```js
const span = Sentry.startInactiveSpan({ name: 'xxx', parentSpan: parent });

Sentry.startSpan({ name: 'xxx', parentSpan: parent }, () => {});

Sentry.startSpanManual({ name: 'xxx', parentSpan: parent }, () => {});
```

With this, it should be easier to understand how you can manually work
with spans in v8.

Closes #12539

ref #12566, which I
noticed while working on this. For now I just cast the span which should
be fine, but is not ideal.

Author: mydea

packages/core/src/tracing/trace.ts

@@ -1,3 +1,5 @@
+/* eslint-disable max-lines */
+
 import type { ClientOptions, Scope, SentrySpanArguments, Span, SpanTimeInput, StartSpanOptions } from '@sentry/types';
 import { generatePropagationContext, logger, propagationContextFromHeaders } from '@sentry/utils';
 import type { AsyncContextStrategy } from '../asyncContext/types';
@@ -32,40 +34,47 @@ const SUPPRESS_TRACING_KEY = '__SENTRY_SUPPRESS_TRACING__';
  * You'll always get a span passed to the callback,
  * it may just be a non-recording span if the span is not sampled or if tracing is disabled.
  */
-export function startSpan<T>(context: StartSpanOptions, callback: (span: Span) => T): T {
+export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) => T): T {
   const acs = getAcs();
   if (acs.startSpan) {
-    return acs.startSpan(context, callback);
+    return acs.startSpan(options, callback);
   }
 
-  const spanContext = normalizeContext(context);
-
-  return withScope(context.scope, scope => {
-    const parentSpan = getParentSpan(scope);
-
-    const shouldSkipSpan = context.onlyIfParent && !parentSpan;
-    const activeSpan = shouldSkipSpan
-      ? new SentryNonRecordingSpan()
-      : createChildOrRootSpan({
-          parentSpan,
-          spanContext,
-          forceTransaction: context.forceTransaction,
-          scope,
-        });
-
-    _setSpanForScope(scope, activeSpan);
-
-    return handleCallbackErrors(
-      () => callback(activeSpan),
-      () => {
-        // Only update the span status if it hasn't been changed yet, and the span is not yet finished
-        const { status } = spanToJSON(activeSpan);
-        if (activeSpan.isRecording() && (!status || status === 'ok')) {
-          activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
-        }
-      },
-      () => activeSpan.end(),
-    );
+  const spanArguments = parseSentrySpanArguments(options);
+  const { forceTransaction, parentSpan: customParentSpan } = options;
+
+  return withScope(options.scope, () => {
+    // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
+    const wrapper = getActiveSpanWrapper<T>(customParentSpan);
+
+    return wrapper(() => {
+      const scope = getCurrentScope();
+      const parentSpan = getParentSpan(scope);
+
+      const shouldSkipSpan = options.onlyIfParent && !parentSpan;
+      const activeSpan = shouldSkipSpan
+        ? new SentryNonRecordingSpan()
+        : createChildOrRootSpan({
+            parentSpan,
+            spanArguments,
+            forceTransaction,
+            scope,
+          });
+
+      _setSpanForScope(scope, activeSpan);
+
+      return handleCallbackErrors(
+        () => callback(activeSpan),
+        () => {
+          // Only update the span status if it hasn't been changed yet, and the span is not yet finished
+          const { status } = spanToJSON(activeSpan);
+          if (activeSpan.isRecording() && (!status || status === 'ok')) {
+            activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
+          }
+        },
+        () => activeSpan.end(),
+      );
+    });
   });
 }
 
@@ -79,43 +88,50 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span) =
  * You'll always get a span passed to the callback,
  * it may just be a non-recording span if the span is not sampled or if tracing is disabled.
  */
-export function startSpanManual<T>(context: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T {
+export function startSpanManual<T>(options: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T {
   const acs = getAcs();
   if (acs.startSpanManual) {
-    return acs.startSpanManual(context, callback);
+    return acs.startSpanManual(options, callback);
   }
 
-  const spanContext = normalizeContext(context);
-
-  return withScope(context.scope, scope => {
-    const parentSpan = getParentSpan(scope);
-
-    const shouldSkipSpan = context.onlyIfParent && !parentSpan;
-    const activeSpan = shouldSkipSpan
-      ? new SentryNonRecordingSpan()
-      : createChildOrRootSpan({
-          parentSpan,
-          spanContext,
-          forceTransaction: context.forceTransaction,
-          scope,
-        });
-
-    _setSpanForScope(scope, activeSpan);
-
-    function finishAndSetSpan(): void {
-      activeSpan.end();
-    }
-
-    return handleCallbackErrors(
-      () => callback(activeSpan, finishAndSetSpan),
-      () => {
-        // Only update the span status if it hasn't been changed yet, and the span is not yet finished
-        const { status } = spanToJSON(activeSpan);
-        if (activeSpan.isRecording() && (!status || status === 'ok')) {
-          activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
-        }
-      },
-    );
+  const spanArguments = parseSentrySpanArguments(options);
+  const { forceTransaction, parentSpan: customParentSpan } = options;
+
+  return withScope(options.scope, () => {
+    // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
+    const wrapper = getActiveSpanWrapper<T>(customParentSpan);
+
+    return wrapper(() => {
+      const scope = getCurrentScope();
+      const parentSpan = getParentSpan(scope);
+
+      const shouldSkipSpan = options.onlyIfParent && !parentSpan;
+      const activeSpan = shouldSkipSpan
+        ? new SentryNonRecordingSpan()
+        : createChildOrRootSpan({
+            parentSpan,
+            spanArguments,
+            forceTransaction,
+            scope,
+          });
+
+      _setSpanForScope(scope, activeSpan);
+
+      function finishAndSetSpan(): void {
+        activeSpan.end();
+      }
+
+      return handleCallbackErrors(
+        () => callback(activeSpan, finishAndSetSpan),
+        () => {
+          // Only update the span status if it hasn't been changed yet, and the span is not yet finished
+          const { status } = spanToJSON(activeSpan);
+          if (activeSpan.isRecording() && (!status || status === 'ok')) {
+            activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
+          }
+        },
+      );
+    });
   });
 }
 
@@ -128,28 +144,39 @@ export function startSpanManual<T>(context: StartSpanOptions, callback: (span: S
  * This function will always return a span,
  * it may just be a non-recording span if the span is not sampled or if tracing is disabled.
  */
-export function startInactiveSpan(context: StartSpanOptions): Span {
+export function startInactiveSpan(options: StartSpanOptions): Span {
   const acs = getAcs();
   if (acs.startInactiveSpan) {
-    return acs.startInactiveSpan(context);
+    return acs.startInactiveSpan(options);
   }
 
-  const spanContext = normalizeContext(context);
+  const spanArguments = parseSentrySpanArguments(options);
+  const { forceTransaction, parentSpan: customParentSpan } = options;
 
-  const scope = context.scope || getCurrentScope();
-  const parentSpan = getParentSpan(scope);
+  // If `options.scope` is defined, we use this as as a wrapper,
+  // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
+  const wrapper = options.scope
+    ? (callback: () => Span) => withScope(options.scope, callback)
+    : customParentSpan
+      ? (callback: () => Span) => withActiveSpan(customParentSpan, callback)
+      : (callback: () => Span) => callback();
 
-  const shouldSkipSpan = context.onlyIfParent && !parentSpan;
+  return wrapper(() => {
+    const scope = getCurrentScope();
+    const parentSpan = getParentSpan(scope);
 
-  if (shouldSkipSpan) {
-    return new SentryNonRecordingSpan();
-  }
+    const shouldSkipSpan = options.onlyIfParent && !parentSpan;
+
+    if (shouldSkipSpan) {
+      return new SentryNonRecordingSpan();
+    }
 
-  return createChildOrRootSpan({
-    parentSpan,
-    spanContext,
-    forceTransaction: context.forceTransaction,
-    scope,
+    return createChildOrRootSpan({
+      parentSpan,
+      spanArguments,
+      forceTransaction,
+      scope,
+    });
   });
 }
 
@@ -239,12 +266,12 @@ export function startNewTrace<T>(callback: () => T): T {
 
 function createChildOrRootSpan({
   parentSpan,
-  spanContext,
+  spanArguments,
   forceTransaction,
   scope,
 }: {
   parentSpan: SentrySpan | undefined;
-  spanContext: SentrySpanArguments;
+  spanArguments: SentrySpanArguments;
   forceTransaction?: boolean;
   scope: Scope;
 }): Span {
@@ -256,7 +283,7 @@ function createChildOrRootSpan({
 
   let span: Span;
   if (parentSpan && !forceTransaction) {
-    span = _startChildSpan(parentSpan, scope, spanContext);
+    span = _startChildSpan(parentSpan, scope, spanArguments);
     addChildSpanToSpan(parentSpan, span);
   } else if (parentSpan) {
     // If we forced a transaction but have a parent span, make sure to continue from the parent span, not the scope
@@ -268,7 +295,7 @@ function createChildOrRootSpan({
       {
         traceId,
         parentSpanId,
-        ...spanContext,
+        ...spanArguments,
       },
       scope,
       parentSampled,
@@ -290,7 +317,7 @@ function createChildOrRootSpan({
       {
         traceId,
         parentSpanId,
-        ...spanContext,
+        ...spanArguments,
       },
       scope,
       parentSampled,
@@ -312,19 +339,17 @@ function createChildOrRootSpan({
  * This converts StartSpanOptions to SentrySpanArguments.
  * For the most part (for now) we accept the same options,
  * but some of them need to be transformed.
- *
- * Eventually the StartSpanOptions will be more aligned with OpenTelemetry.
  */
-function normalizeContext(context: StartSpanOptions): SentrySpanArguments {
-  const exp = context.experimental || {};
+function parseSentrySpanArguments(options: StartSpanOptions): SentrySpanArguments {
+  const exp = options.experimental || {};
   const initialCtx: SentrySpanArguments = {
     isStandalone: exp.standalone,
-    ...context,
+    ...options,
   };
 
-  if (context.startTime) {
+  if (options.startTime) {
     const ctx: SentrySpanArguments & { startTime?: SpanTimeInput } = { ...initialCtx };
-    ctx.startTimestamp = spanTimeInputToSeconds(context.startTime);
+    ctx.startTimestamp = spanTimeInputToSeconds(options.startTime);
     delete ctx.startTime;
     return ctx;
   }
@@ -419,3 +444,11 @@ function getParentSpan(scope: Scope): SentrySpan | undefined {
 
   return span;
 }
+
+function getActiveSpanWrapper<T>(parentSpan?: Span): (callback: () => T) => T {
+  return parentSpan
+    ? (callback: () => T) => {
+        return withActiveSpan(parentSpan, callback);
+      }
+    : (callback: () => T) => callback();
+}

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

@@ -271,6 +271,17 @@ describe('startSpan', () => {
     expect(getActiveSpan()).toBe(undefined);
   });
 
+  it('allows to pass a parentSpan', () => {
+    const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' });
+
+    startSpan({ name: 'GET users/[id]', parentSpan }, span => {
+      expect(getActiveSpan()).toBe(span);
+      expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
+    });
+
+    expect(getActiveSpan()).toBe(undefined);
+  });
+
   it('allows to force a transaction with forceTransaction=true', async () => {
     const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
     client = new TestClient(options);
@@ -653,13 +664,13 @@ describe('startSpanManual', () => {
     const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true });
     _setSpanForScope(manualScope, parentSpan);
 
-    startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => {
+    startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => {
       expect(getCurrentScope()).not.toBe(initialScope);
       expect(getCurrentScope()).toBe(manualScope);
       expect(getActiveSpan()).toBe(span);
       expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
 
-      finish();
+      span.end();
 
       // Is still the active span
       expect(getActiveSpan()).toBe(span);
@@ -669,6 +680,19 @@ describe('startSpanManual', () => {
     expect(getActiveSpan()).toBe(undefined);
   });
 
+  it('allows to pass a parentSpan', () => {
+    const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' });
+
+    startSpanManual({ name: 'GET users/[id]', parentSpan }, span => {
+      expect(getActiveSpan()).toBe(span);
+      expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
+
+      span.end();
+    });
+
+    expect(getActiveSpan()).toBe(undefined);
+  });
+
   it('allows to force a transaction with forceTransaction=true', async () => {
     const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
     client = new TestClient(options);
@@ -977,6 +1001,19 @@ describe('startInactiveSpan', () => {
     expect(getActiveSpan()).toBeUndefined();
   });
 
+  it('allows to pass a parentSpan', () => {
+    const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' });
+
+    const span = startInactiveSpan({ name: 'GET users/[id]', parentSpan });
+
+    expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
+    expect(getActiveSpan()).toBe(undefined);
+
+    span.end();
+
+    expect(getActiveSpan()).toBeUndefined();
+  });
+
   it('allows to force a transaction with forceTransaction=true', async () => {
     const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
     client = new TestClient(options);