fix(core): Restore previously active span when passing a custom scope to startSpanManual

Author: Lms24

packages/core/src/tracing/trace.ts

@@ -96,7 +96,9 @@ export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) =
 
 /**
  * Similar to `Sentry.startSpan`. Wraps a function with a transaction/span, but does not finish the span
- * after the function is done automatically. You'll have to call `span.end()` manually.
+ * after the function is done automatically. Use the `finish` function passed to the callback,
+ * to finish the span manually and sett he previous span as the active span again. If you don't use `finish`
+ * but `span.end()` instead, the span will be ended but remain as the active span.
  *
  * The created span is the active span and will be used as parent by other spans created inside the function
  * and can be accessed via `Sentry.getActiveSpan()`, as long as the function is executed while the scope is active.
@@ -111,7 +113,7 @@ export function startSpanManual<T>(options: StartSpanOptions, callback: (span: S
   }
 
   const spanArguments = parseSentrySpanArguments(options);
-  const { forceTransaction, parentSpan: customParentSpan } = options;
+  const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options;
 
   return withScope(options.scope, () => {
     // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
@@ -131,10 +133,16 @@ export function startSpanManual<T>(options: StartSpanOptions, callback: (span: S
             scope,
           });
 
+      const previousActiveSpanOnCustomScope = customScope && _getSpanForScope(customScope);
       _setSpanForScope(scope, activeSpan);
 
       function finishAndSetSpan(): void {
         activeSpan.end();
+        if (customScope) {
+          // If a custom scope is passed, we don't fork the scope. Therefore, we need to reset the
+          // active span on the scope to the previous active span (or to undefined if there was none)
+          _setSpanForScope(customScope, previousActiveSpanOnCustomScope);
+        }
       }
 
       return handleCallbackErrors(

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

@@ -779,27 +779,76 @@ describe('startSpanManual', () => {
     expect(getActiveSpan()).toBe(undefined);
   });
 
-  it('allows to pass a scope', () => {
-    const initialScope = getCurrentScope();
+  describe('allows to pass a scope', () => {
+    it('with parent span', () => {
+      const initialScope = getCurrentScope();
 
-    const manualScope = initialScope.clone();
-    const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true });
-    _setSpanForScope(manualScope, parentSpan);
+      const manualScope = initialScope.clone();
+      const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true });
+      _setSpanForScope(manualScope, parentSpan);
 
-    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');
+      let span1: Span | undefined;
 
-      span.end();
+      startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => {
+        span1 = span;
+        expect(getCurrentScope()).not.toBe(initialScope);
+        expect(getCurrentScope()).toBe(manualScope);
+        expect(getActiveSpan()).toBe(span);
+        expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
 
-      // Is still the active span
-      expect(getActiveSpan()).toBe(span);
+        span.end();
+
+        // Is still the active span
+        expect(getActiveSpan()).toBe(span);
+      });
+
+      startSpanManual({ name: 'POST users/[id]', scope: manualScope }, (span, finish) => {
+        expect(getCurrentScope()).not.toBe(initialScope);
+        expect(getCurrentScope()).toBe(manualScope);
+        expect(getActiveSpan()).toBe(span);
+        expect(spanToJSON(span).parent_span_id).toBe(span1?.spanContext().spanId);
+
+        finish();
+
+        // using finish() resets the scope correctly
+        expect(getActiveSpan()).toBe(span1);
+      });
+
+      expect(getCurrentScope()).toBe(initialScope);
+      expect(getActiveSpan()).toBe(undefined);
     });
 
-    expect(getCurrentScope()).toBe(initialScope);
-    expect(getActiveSpan()).toBe(undefined);
+    it('without parent span', () => {
+      const initialScope = getCurrentScope();
+      const manualScope = initialScope.clone();
+
+      startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => {
+        expect(getCurrentScope()).not.toBe(initialScope);
+        expect(getCurrentScope()).toBe(manualScope);
+        expect(getActiveSpan()).toBe(span);
+        expect(spanToJSON(span).parent_span_id).toBe(undefined);
+
+        finish();
+
+        // Is still the active span
+        expect(getActiveSpan()).toBe(undefined);
+      });
+
+      startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => {
+        expect(getCurrentScope()).not.toBe(initialScope);
+        expect(getCurrentScope()).toBe(manualScope);
+        expect(getActiveSpan()).toBe(span);
+        expect(spanToJSON(span).parent_span_id).toBe(undefined);
+
+        finish();
+
+        // using finish() resets the scope correctly
+        expect(getActiveSpan()).toBe(undefined);
+      });
+
+      expect(getCurrentScope()).toBe(initialScope);
+      expect(getActiveSpan()).toBe(undefined);
+    });
   });
 
   it('allows to pass a parentSpan', () => {