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

mydea · mydea · commit 90968bdb176a · 2024-03-01T12:46:21.000Z
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`
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/interactions/test.ts b/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;
diff --git a/dev-packages/e2e-tests/test-applications/angular-17/tests/errors.test.ts b/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(`/`);
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts b/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', () => {
diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/test/errors.server.test.ts b/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');
   });
 });
diff --git a/dev-packages/e2e-tests/test-applications/sveltekit/test/errors.server.test.ts b/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');
   });
 });
diff --git a/dev-packages/e2e-tests/test-applications/vue-3/tests/errors.test.ts b/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(`/`);
diff --git a/dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts b/dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts
@@ -11,9 +11,9 @@ const EXPECTED_TRANSACTION = {
         sum: 1,
         tags: {
           release: '1.0',
-          transaction: 'Test Transaction',
           email: 'jon.doe@example.com',
         },
+        transaction: 'Test Transaction',
       },
       {
         min: 1,
@@ -22,9 +22,9 @@ const EXPECTED_TRANSACTION = {
         sum: 1,
         tags: {
           release: '1.0',
-          transaction: 'Test Transaction',
           email: 'jane.doe@example.com',
         },
+        transaction: 'Test Transaction',
       },
     ],
   },
@@ -41,7 +41,6 @@ const EXPECTED_TRANSACTION = {
             sum: 4,
             tags: {
               release: '1.0',
-              transaction: 'Test Transaction',
             },
           },
         ],
@@ -53,7 +52,6 @@ const EXPECTED_TRANSACTION = {
             sum: 2,
             tags: {
               release: '1.0',
-              transaction: 'Test Transaction',
             },
           },
         ],
@@ -65,7 +63,6 @@ const EXPECTED_TRANSACTION = {
             sum: 62,
             tags: {
               release: '1.0',
-              transaction: 'Test Transaction',
             },
           },
         ],
@@ -77,7 +74,6 @@ const EXPECTED_TRANSACTION = {
             sum: 62,
             tags: {
               release: '1.0',
-              transaction: 'Test Transaction',
             },
           },
         ],
diff --git a/packages/core/src/tracing/trace.ts b/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;
diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/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;
     }
   }
 }
diff --git a/packages/core/test/lib/tracing/trace.test.ts b/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(),
diff --git a/packages/node-experimental/test/integration/scope.test.ts b/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),
diff --git a/packages/node-experimental/test/integration/transactions.test.ts b/packages/node-experimental/test/integration/transactions.test.ts
@@ -124,7 +124,6 @@ describe('Integration | Transactions', () => {
         tags: {
           'outer.tag': 'test value',
           'test.tag': 'test value',
-          transaction: 'test name',
         },
         timestamp: expect.any(Number),
         transaction: 'test name',
@@ -267,7 +266,9 @@ describe('Integration | Transactions', () => {
         }),
         spans: [expect.any(Object), expect.any(Object)],
         start_timestamp: expect.any(Number),
-        tags: { 'test.tag': 'test value', transaction: 'test name' },
+        tags: {
+          'test.tag': 'test value',
+        },
         timestamp: expect.any(Number),
         transaction: 'test name',
         transaction_info: { source: 'task' },
@@ -310,7 +311,9 @@ describe('Integration | Transactions', () => {
         }),
         spans: [expect.any(Object), expect.any(Object)],
         start_timestamp: expect.any(Number),
-        tags: { 'test.tag': 'test value b', transaction: 'test name b' },
+        tags: {
+          'test.tag': 'test value b',
+        },
         timestamp: expect.any(Number),
         transaction: 'test name b',
         transaction_info: { source: 'custom' },
@@ -417,7 +420,9 @@ describe('Integration | Transactions', () => {
         }),
         spans: [expect.any(Object), expect.any(Object)],
         start_timestamp: expect.any(Number),
-        tags: { 'test.tag': 'test value', transaction: 'test name' },
+        tags: {
+          'test.tag': 'test value',
+        },
         timestamp: expect.any(Number),
         transaction: 'test name',
         type: 'transaction',
@@ -456,7 +461,9 @@ describe('Integration | Transactions', () => {
         }),
         spans: [expect.any(Object), expect.any(Object)],
         start_timestamp: expect.any(Number),
-        tags: { 'test.tag': 'test value b', transaction: 'test name b' },
+        tags: {
+          'test.tag': 'test value b',
+        },
         timestamp: expect.any(Number),
         transaction: 'test name b',
         type: 'transaction',
diff --git a/packages/opentelemetry/src/setupEventContextTrace.ts b/packages/opentelemetry/src/setupEventContextTrace.ts
@@ -28,8 +28,8 @@ export function setupEventContextTrace(client: Client): void {
 
     const rootSpan = getRootSpan(span);
     const transactionName = spanHasName(rootSpan) ? rootSpan.name : undefined;
-    if (transactionName) {
-      event.tags = { transaction: transactionName, ...event.tags };
+    if (transactionName && !event.transaction) {
+      event.transaction = transactionName;
     }
 
     return event;
diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts
@@ -200,14 +200,7 @@ function createTransactionForOtelSpan(span: ReadableSpan): Transaction {
   });
 
   if (capturedSpanScopes) {
-    // Ensure the `transaction` tag is correctly set on the transaction event
-    const scope = capturedSpanScopes.scope.clone();
-    scope.addEventProcessor(event => {
-      event.tags = { transaction: description, ...event.tags };
-      return event;
-    });
-
-    setCapturedScopesOnTransaction(transaction, scope, capturedSpanScopes.isolationScope);
+    setCapturedScopesOnTransaction(transaction, capturedSpanScopes.scope, capturedSpanScopes.isolationScope);
   }
 
   return transaction;
diff --git a/packages/opentelemetry/test/integration/scope.test.ts b/packages/opentelemetry/test/integration/scope.test.ts
diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts
diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts
diff --git a/packages/remix/test/integration/test/client/capture-exception.test.ts b/packages/remix/test/integration/test/client/capture-exception.test.ts
diff --git a/packages/remix/test/integration/test/client/capture-message.test.ts b/packages/remix/test/integration/test/client/capture-message.test.ts

Original file line number	Diff line number	Diff line change
`@@ -178,9 +178,10 @@ function applySpanToEvent(event: Event, span: Span): void {`
`178`	`178`	`dynamicSamplingContext: getDynamicSamplingContextFromSpan(span),`
`179`	`179`	`...event.sdkProcessingMetadata,`
`180`	`180`	`};`
	`181`	`+`
`181`	`182`	`const transactionName = spanToJSON(rootSpan).description;`
`182`		`- if (transactionName) {`
`183`		`- event.tags = { transaction: transactionName, ...event.tags };`
	`183`	`+ if (transactionName && !event.transaction) {`
	`184`	`+ event.transaction = transactionName;`
`184`	`185`	`}`
`185`	`186`	`}`
`186`	`187`	`}`