feat(core): Fix span scope handling & transaction setting (#10886)

This does the following things:
1. Instead of setting `transaction` tag, we ensure to set the
`transaction` on the event itself.
2. This also allows us to remove code that lead to other bugs, which is
that we needed to fork the scope for `startInactiveSpan` just to get the
correct transaction tag.
3. Add tests to ensure that the scope is correctly applied when using
`startInactiveSpan`

## Background

I initially implemented `startInactiveSpan` to fork the current scope &
set the span on that to satisfy two things:
1. We do not want to set this span as active on the actual current scope
2. We needed a way to set the transaction tag on the span (if it was a
transaction) in an event processor

For 2., in order for `getActiveSpan()` to work in there I had to make
the span active in the forked scope.

This kind of worked, BUT I now noticed that this has the massive
problem:

```js
const scope = getCurrentScope();
const span = startInactiveSpan(...);
scope.setTag(...); // <-- this will not be applied to the span!
```

So I went in and fixed this - and since we talked about not needing to
set the `transaction` tag anyhow (instead, just set the `transaction`
itself!), I figured I can fix these in one go.

Author: mydea

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

@@ -1,6 +1,6 @@
 import type { Route } from '@playwright/test';
 import { expect } from '@playwright/test';
-import type { Event, Span, SpanContext, Transaction } from '@sentry/types';
+import type { Event, SpanContext, SpanJSON } from '@sentry/types';
 
 import { sentryTest } from '../../../../utils/fixtures';
 import {
@@ -9,8 +9,8 @@ import {
   shouldSkipTracingTest,
 } from '../../../../utils/helpers';
 
-type TransactionJSON = ReturnType<Transaction['toJSON']> & {
-  spans: ReturnType<Span['toJSON']>[];
+type TransactionJSON = SpanJSON & {
+  spans: SpanJSON[];
   contexts: SpanContext;
   platform: string;
   type: string;

dev-packages/e2e-tests/test-applications/angular-17/tests/errors.test.ts

@@ -3,7 +3,7 @@ import { waitForError } from '../event-proxy-server';
 
 test('sends an error', async ({ page }) => {
   const errorPromise = waitForError('angular-17', async errorEvent => {
-    return !errorEvent?.transaction;
+    return !errorEvent.type;
   });
 
   await page.goto(`/`);

dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts

@@ -52,7 +52,7 @@ test('Should record exceptions and transactions for faulty route handlers', asyn
   expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server');
 
   expect(routehandlerError.exception?.values?.[0].value).toBe('route-handler-error');
-  expect(routehandlerError.tags?.transaction).toBe('PUT /route-handlers/[param]/error');
+  expect(routehandlerError.transaction).toBe('PUT /route-handlers/[param]/error');
 });
 
 test.describe('Edge runtime', () => {

dev-packages/e2e-tests/test-applications/sveltekit-2/test/errors.server.test.ts

@@ -60,9 +60,6 @@ test.describe('server-side errors', () => {
       }),
     );
 
-    expect(errorEvent.tags).toMatchObject({
-      runtime: 'node',
-      transaction: 'GET /server-route-error',
-    });
+    expect(errorEvent.transaction).toEqual('GET /server-route-error');
   });
 });

dev-packages/e2e-tests/test-applications/sveltekit/test/errors.server.test.ts

@@ -63,9 +63,6 @@ test.describe('server-side errors', () => {
       }),
     );
 
-    expect(errorEvent.tags).toMatchObject({
-      runtime: 'node',
-      transaction: 'GET /server-route-error',
-    });
+    expect(errorEvent.transaction).toEqual('GET /server-route-error');
   });
 });

dev-packages/e2e-tests/test-applications/vue-3/tests/errors.test.ts

@@ -3,7 +3,7 @@ import { waitForError } from '../event-proxy-server';
 
 test('sends an error', async ({ page }) => {
   const errorPromise = waitForError('vue-3', async errorEvent => {
-    return !errorEvent?.transaction;
+    return !errorEvent.type;
   });
 
   await page.goto(`/`);

packages/core/src/tracing/trace.ts

@@ -42,6 +42,11 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span |
           scope,
         });
 
+    if (activeSpan) {
+      // eslint-disable-next-line deprecation/deprecation
+      scope.setSpan(activeSpan);
+    }
+
     return handleCallbackErrors(
       () => callback(activeSpan),
       () => {
@@ -91,6 +96,11 @@ export function startSpanManual<T>(
           scope,
         });
 
+    if (activeSpan) {
+      // eslint-disable-next-line deprecation/deprecation
+      scope.setSpan(activeSpan);
+    }
+
     function finishAndSetSpan(): void {
       activeSpan && activeSpan.end();
     }
@@ -141,16 +151,11 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
 
   const scope = context.scope || getCurrentScope();
 
-  // Even though we don't actually want to make this span active on the current scope,
-  // we need to make it active on a temporary scope that we use for event processing
-  // as otherwise, it won't pick the correct span for the event when processing it
-  const temporaryScope = scope.clone();
-
   return createChildSpanOrTransaction(hub, {
     parentSpan,
     spanContext,
     forceTransaction: context.forceTransaction,
-    scope: temporaryScope,
+    scope,
   });
 }
 
@@ -319,12 +324,6 @@ function createChildSpanOrTransaction(
     });
   }
 
-  // We always set this as active span on the scope
-  // In the case of this being an inactive span, we ensure to pass a detached scope in here in the first place
-  // But by having this here, we can ensure that the lookup through `getCapturedScopesOnSpan` results in the correct scope & span combo
-  // eslint-disable-next-line deprecation/deprecation
-  scope.setSpan(span);
-
   setCapturedScopesOnSpan(span, scope, isolationScope);
 
   return span;

packages/core/src/utils/applyScopeDataToEvent.ts

@@ -178,9 +178,10 @@ function applySpanToEvent(event: Event, span: Span): void {
       dynamicSamplingContext: getDynamicSamplingContextFromSpan(span),
       ...event.sdkProcessingMetadata,
     };
+
     const transactionName = spanToJSON(rootSpan).description;
-    if (transactionName) {
-      event.tags = { transaction: transactionName, ...event.tags };
+    if (transactionName && !event.transaction) {
+      event.transaction = transactionName;
     }
   }
 }

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

@@ -360,9 +360,7 @@ describe('startSpan', () => {
       },
     });
     expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]);
-    expect(outerTransaction?.tags).toEqual({
-      transaction: 'outer transaction',
-    });
+    expect(outerTransaction?.transaction).toEqual('outer transaction');
     expect(outerTransaction?.sdkProcessingMetadata).toEqual({
       dynamicSamplingContext: {
         environment: 'production',
@@ -386,9 +384,7 @@ describe('startSpan', () => {
       },
     });
     expect(innerTransaction?.spans).toEqual([{ name: 'inner span 2', id: expect.any(String) }]);
-    expect(innerTransaction?.tags).toEqual({
-      transaction: 'inner transaction',
-    });
+    expect(innerTransaction?.transaction).toEqual('inner transaction');
     expect(innerTransaction?.sdkProcessingMetadata).toEqual({
       dynamicSamplingContext: {
         environment: 'production',
@@ -643,9 +639,7 @@ describe('startSpanManual', () => {
       },
     });
     expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]);
-    expect(outerTransaction?.tags).toEqual({
-      transaction: 'outer transaction',
-    });
+    expect(outerTransaction?.transaction).toEqual('outer transaction');
     expect(outerTransaction?.sdkProcessingMetadata).toEqual({
       dynamicSamplingContext: {
         environment: 'production',
@@ -669,9 +663,7 @@ describe('startSpanManual', () => {
       },
     });
     expect(innerTransaction?.spans).toEqual([{ name: 'inner span 2', id: expect.any(String) }]);
-    expect(innerTransaction?.tags).toEqual({
-      transaction: 'inner transaction',
-    });
+    expect(innerTransaction?.transaction).toEqual('inner transaction');
     expect(innerTransaction?.sdkProcessingMetadata).toEqual({
       dynamicSamplingContext: {
         environment: 'production',
@@ -854,9 +846,7 @@ describe('startInactiveSpan', () => {
       },
     });
     expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]);
-    expect(outerTransaction?.tags).toEqual({
-      transaction: 'outer transaction',
-    });
+    expect(outerTransaction?.transaction).toEqual('outer transaction');
     expect(outerTransaction?.sdkProcessingMetadata).toEqual({
       dynamicSamplingContext: {
         environment: 'production',
@@ -880,9 +870,7 @@ describe('startInactiveSpan', () => {
       },
     });
     expect(innerTransaction?.spans).toEqual([]);
-    expect(innerTransaction?.tags).toEqual({
-      transaction: 'inner transaction',
-    });
+    expect(innerTransaction?.transaction).toEqual('inner transaction');
     expect(innerTransaction?.sdkProcessingMetadata).toEqual({
       dynamicSamplingContext: {
         environment: 'production',
@@ -948,9 +936,13 @@ describe('startInactiveSpan', () => {
 
     let span: Span | undefined;
 
+    const scope = getCurrentScope();
+    scope.setTag('outer', 'foo');
+
     withScope(scope => {
       scope.setTag('scope', 1);
       span = startInactiveSpan({ name: 'my-span' });
+      scope.setTag('scope_after_span', 2);
     });
 
     withScope(scope => {
@@ -964,7 +956,9 @@ describe('startInactiveSpan', () => {
     expect(beforeSendTransaction).toHaveBeenCalledWith(
       expect.objectContaining({
         tags: expect.objectContaining({
+          outer: 'foo',
           scope: 1,
+          scope_after_span: 2,
         }),
       }),
       expect.anything(),

packages/node-experimental/test/integration/scope.test.ts

@@ -82,8 +82,8 @@ describe('Integration | Scope', () => {
             tag2: 'val2',
             tag3: 'val3',
             tag4: 'val4',
-            ...(enableTracing ? { transaction: 'outer' } : {}),
           },
+          ...(enableTracing ? { transaction: 'outer' } : {}),
         }),
         {
           event_id: expect.any(String),
@@ -118,7 +118,6 @@ describe('Integration | Scope', () => {
               tag2: 'val2',
               tag3: 'val3',
               tag4: 'val4',
-              transaction: 'outer',
             },
             timestamp: expect.any(Number),
             transaction: 'outer',
@@ -203,8 +202,8 @@ describe('Integration | Scope', () => {
             tag2: 'val2a',
             tag3: 'val3a',
             tag4: 'val4a',
-            ...(enableTracing ? { transaction: 'outer' } : {}),
           },
+          ...(enableTracing ? { transaction: 'outer' } : {}),
         }),
         {
           event_id: expect.any(String),
@@ -229,8 +228,8 @@ describe('Integration | Scope', () => {
             tag2: 'val2b',
             tag3: 'val3b',
             tag4: 'val4b',
-            ...(enableTracing ? { transaction: 'outer' } : {}),
           },
+          ...(enableTracing ? { transaction: 'outer' } : {}),
         }),
         {
           event_id: expect.any(String),