rework implementation to fork scope instead of restoring previously active span

Author: Lms24

packages/core/src/tracing/trace.ts

@@ -96,9 +96,7 @@ 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. 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.
+ * after the function is done automatically. Use `span.end()` to end the 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.
@@ -115,7 +113,9 @@ export function startSpanManual<T>(options: StartSpanOptions, callback: (span: S
   const spanArguments = parseSentrySpanArguments(options);
   const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options;
 
-  return withScope(options.scope, () => {
+  const customForkedScope = customScope?.clone();
+
+  return withScope(customForkedScope, () => {
     // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
     const wrapper = getActiveSpanWrapper<T>(customParentSpan);
 
@@ -133,20 +133,14 @@ 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(
-        () => callback(activeSpan, finishAndSetSpan),
+        // We pass the `finish` function to the callback, so the user can finish the span manually
+        // this is mainly here for historic purposes because previously, we instructed users to call
+        // `finish` instead of `span.end()` to also clean up the scope. Nowadays, calling `span.end()`
+        // or `finish` has the same effect and we simply leave it here to avoid breaking user code.
+        () => callback(activeSpan, () => activeSpan.end()),
         () => {
           // Only update the span status if it hasn't been changed yet, and the span is not yet finished
           const { status } = spanToJSON(activeSpan);

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

@@ -779,39 +779,57 @@ describe('startSpanManual', () => {
     expect(getActiveSpan()).toBe(undefined);
   });
 
-  describe('allows to pass a scope', () => {
+  describe('starts a span on the fork of a custom scope if passed', () => {
     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 customScope = initialScope.clone();
+      customScope.setTag('dogs', 'great');
 
-      let span1: Span | undefined;
+      const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true });
+      _setSpanForScope(customScope, parentSpan);
 
-      startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => {
-        span1 = span;
+      startSpanManual({ name: 'GET users/[id]', scope: customScope }, span => {
+        // current scope is forked from the customScope
         expect(getCurrentScope()).not.toBe(initialScope);
-        expect(getCurrentScope()).toBe(manualScope);
-        expect(getActiveSpan()).toBe(span);
+        expect(getCurrentScope()).not.toBe(customScope);
         expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
 
+        // span is active span
+        expect(getActiveSpan()).toBe(span);
+
         span.end();
 
-        // Is still the active span
+        // span is still the active span (weird but it is what it is)
         expect(getActiveSpan()).toBe(span);
+
+        getCurrentScope().setTag('cats', 'great');
+        customScope.setTag('bears', 'great');
+
+        expect(getCurrentScope().getScopeData().tags).toEqual({ dogs: 'great', cats: 'great' });
+        expect(customScope.getScopeData().tags).toEqual({ dogs: 'great', bears: 'great' });
       });
 
-      startSpanManual({ name: 'POST users/[id]', scope: manualScope }, (span, finish) => {
+      expect(getCurrentScope()).toBe(initialScope);
+      expect(getActiveSpan()).toBe(undefined);
+
+      startSpanManual({ name: 'POST users/[id]', scope: customScope }, (span, finish) => {
+        // current scope is forked from the customScope
         expect(getCurrentScope()).not.toBe(initialScope);
-        expect(getCurrentScope()).toBe(manualScope);
+        expect(getCurrentScope()).not.toBe(customScope);
+        expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
+
+        // scope data modification from customScope in previous callback is persisted
+        expect(getCurrentScope().getScopeData().tags).toEqual({ dogs: 'great', bears: 'great' });
+
+        // span is active span
         expect(getActiveSpan()).toBe(span);
-        expect(spanToJSON(span).parent_span_id).toBe(span1?.spanContext().spanId);
 
+        // calling finish() or span.end() has the same effect
         finish();
 
         // using finish() resets the scope correctly
-        expect(getActiveSpan()).toBe(span1);
+        expect(getActiveSpan()).toBe(span);
       });
 
       expect(getCurrentScope()).toBe(initialScope);
@@ -823,27 +841,33 @@ describe('startSpanManual', () => {
       const manualScope = initialScope.clone();
 
       startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => {
+        // current scope is forked from the customScope
         expect(getCurrentScope()).not.toBe(initialScope);
-        expect(getCurrentScope()).toBe(manualScope);
+        expect(getCurrentScope()).not.toBe(manualScope);
+        expect(getCurrentScope()).toEqual(manualScope);
+
+        // span is active span and a root span
         expect(getActiveSpan()).toBe(span);
-        expect(spanToJSON(span).parent_span_id).toBe(undefined);
+        expect(getRootSpan(span)).toBe(span);
 
-        finish();
+        span.end();
 
-        // Is still the active span
-        expect(getActiveSpan()).toBe(undefined);
+        expect(getActiveSpan()).toBe(span);
       });
 
-      startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => {
+      startSpanManual({ name: 'POST users/[id]', scope: manualScope }, (span, finish) => {
         expect(getCurrentScope()).not.toBe(initialScope);
-        expect(getCurrentScope()).toBe(manualScope);
+        expect(getCurrentScope()).not.toBe(manualScope);
+        expect(getCurrentScope()).toEqual(manualScope);
+
+        // second span is active span and its own root span
         expect(getActiveSpan()).toBe(span);
-        expect(spanToJSON(span).parent_span_id).toBe(undefined);
+        expect(getRootSpan(span)).toBe(span);
 
         finish();
 
-        // using finish() resets the scope correctly
-        expect(getActiveSpan()).toBe(undefined);
+        // calling finish() or span.end() has the same effect
+        expect(getActiveSpan()).toBe(span);
       });
 
       expect(getCurrentScope()).toBe(initialScope);