From 90968bdb176aab40f4cca84a175bebfd9c355b79 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 1 Mar 2024 11:58:14 +0000 Subject: [PATCH 1/2] feat(core): Fix span scope handling & transaction setting 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` --- .../interactions/test.ts | 6 +- .../angular-17/tests/errors.test.ts | 2 +- .../tests/route-handlers.test.ts | 2 +- .../sveltekit-2/test/errors.server.test.ts | 5 +- .../sveltekit/test/errors.server.test.ts | 5 +- .../vue-3/tests/errors.test.ts | 2 +- .../suites/tracing/metric-summaries/test.ts | 8 +-- packages/core/src/tracing/trace.ts | 23 ++++--- .../core/src/utils/applyScopeDataToEvent.ts | 5 +- packages/core/test/lib/tracing/trace.test.ts | 30 ++++----- .../test/integration/scope.test.ts | 7 +-- .../test/integration/transactions.test.ts | 17 +++-- .../src/setupEventContextTrace.ts | 4 +- packages/opentelemetry/src/spanExporter.ts | 9 +-- .../test/integration/scope.test.ts | 11 ++-- .../test/integration/transactions.test.ts | 9 ++- packages/opentelemetry/test/trace.test.ts | 63 +++++++++++++------ .../test/client/capture-exception.test.ts | 4 +- .../test/client/capture-message.test.ts | 4 +- 19 files changed, 114 insertions(+), 102 deletions(-) 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 index 50c095dbcc57..a66d447d92ec 100644 --- 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 & { - spans: ReturnType[]; +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 index d34f6a83eb29..e4d687ca3199 100644 --- 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 index 560a5911522f..421914877ce2 100644 --- 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 index b7966325560a..972ee59955dd 100644 --- 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 index 5493659b19db..f2b7d2d21531 100644 --- 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 index 508fe738bbc5..b9933299c8c0 100644 --- 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 index 94f5fdc30c70..9496e510f680 100644 --- 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 index 561f61ce9265..684633991fa6 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -42,6 +42,11 @@ export function startSpan(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( 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 index 92a5e6747cd8..2d0d20661fe2 100644 --- 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 index bd298fd4d5ff..7c4c92ccc8c8 100644 --- 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 index ed5815e2df40..19848c4c78fc 100644 --- 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 index 05a7f1840ec1..73d536c06cc1 100644 --- 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 index 8d45fb787326..93e9033f4969 100644 --- 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 index b673930be9a4..348304c82a92 100644 --- 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 index 7a32acbe2364..36c794b1dbb0 100644 --- a/packages/opentelemetry/test/integration/scope.test.ts +++ b/packages/opentelemetry/test/integration/scope.test.ts @@ -90,8 +90,8 @@ describe('Integration | Scope', () => { tag2: 'val2', tag3: 'val3', tag4: 'val4', - ...(enableTracing ? { transaction: 'outer' } : {}), }, + ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), @@ -126,7 +126,6 @@ describe('Integration | Scope', () => { tag2: 'val2', tag3: 'val3', tag4: 'val4', - transaction: 'outer', }, timestamp: expect.any(Number), transaction: 'outer', @@ -220,8 +219,8 @@ describe('Integration | Scope', () => { tag2: 'val2a', tag3: 'val3a', tag4: 'val4a', - ...(enableTracing ? { transaction: 'outer' } : {}), }, + ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), @@ -246,8 +245,8 @@ describe('Integration | Scope', () => { tag2: 'val2b', tag3: 'val3b', tag4: 'val4b', - ...(enableTracing ? { transaction: 'outer' } : {}), }, + ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), @@ -346,8 +345,8 @@ describe('Integration | Scope', () => { tag4: 'val4a', isolationTag1: 'val1', isolationTag2: 'val2', - ...(enableTracing ? { transaction: 'outer' } : {}), }, + ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), @@ -374,8 +373,8 @@ describe('Integration | Scope', () => { tag4: 'val4b', isolationTag1: 'val1', isolationTag2: 'val2b', - ...(enableTracing ? { transaction: 'outer' } : {}), }, + ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index 4e915101d77e..86f16f994217 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -128,7 +128,6 @@ describe('Integration | Transactions', () => { tags: { 'outer.tag': 'test value', 'test.tag': 'test value', - transaction: 'test name', }, timestamp: expect.any(Number), transaction: 'test name', @@ -271,7 +270,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' }, @@ -314,7 +315,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' }, diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 126586d29538..b6967d654127 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -11,6 +11,7 @@ import { getClient, getCurrentScope, spanToJSON, + withScope, } from '@sentry/core'; import type { Event, PropagationContext, Scope } from '@sentry/types'; @@ -369,9 +370,7 @@ describe('trace', () => { status: 'ok', }); 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', @@ -397,9 +396,7 @@ describe('trace', () => { status: 'ok', }); 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', @@ -615,9 +612,7 @@ describe('trace', () => { status: 'ok', }); 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', @@ -643,9 +638,7 @@ describe('trace', () => { status: 'ok', }); expect(innerTransaction?.spans).toEqual([]); - expect(innerTransaction?.tags).toEqual({ - transaction: 'inner transaction', - }); + expect(innerTransaction?.transaction).toEqual('inner transaction'); expect(innerTransaction?.sdkProcessingMetadata).toEqual({ dynamicSamplingContext: { environment: 'production', @@ -675,6 +668,44 @@ describe('trace', () => { expect(span).toBeInstanceOf(SpanClass); }); }); + + it('includes the scope at the time the span was started when finished', async () => { + const beforeSendTransaction = jest.fn(event => event); + + const client = getClient()!; + + client.getOptions().beforeSendTransaction = beforeSendTransaction; + + 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 => { + scope.setTag('scope', 2); + span?.end(); + }); + + await client.flush(); + + expect(beforeSendTransaction).toHaveBeenCalledTimes(1); + expect(beforeSendTransaction).toHaveBeenCalledWith( + expect.objectContaining({ + tags: expect.objectContaining({ + outer: 'foo', + scope: 1, + scope_after_span: 2, + }), + }), + expect.anything(), + ); + }); }); describe('startSpanManual', () => { @@ -852,9 +883,7 @@ describe('trace', () => { status: 'ok', }); 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 +909,7 @@ describe('trace', () => { status: 'ok', }); 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', diff --git a/packages/remix/test/integration/test/client/capture-exception.test.ts b/packages/remix/test/integration/test/client/capture-exception.test.ts index e13136f59565..b7e38abf2f4c 100644 --- a/packages/remix/test/integration/test/client/capture-exception.test.ts +++ b/packages/remix/test/integration/test/client/capture-exception.test.ts @@ -8,7 +8,7 @@ test('should report a manually captured error.', async ({ page }) => { const [errorEnvelope, pageloadEnvelope] = envelopes; expect(errorEnvelope.level).toBe('error'); - expect(errorEnvelope.tags?.transaction).toBe('/capture-exception'); + expect(errorEnvelope.transaction).toBe('/capture-exception'); expect(errorEnvelope.exception?.values).toMatchObject([ { type: 'Error', @@ -18,7 +18,7 @@ test('should report a manually captured error.', async ({ page }) => { }, ]); - expect(pageloadEnvelope.contexts?.trace.op).toBe('pageload'); + expect(pageloadEnvelope.contexts?.trace?.op).toBe('pageload'); expect(pageloadEnvelope.type).toBe('transaction'); expect(pageloadEnvelope.transaction).toBe('routes/capture-exception'); }); diff --git a/packages/remix/test/integration/test/client/capture-message.test.ts b/packages/remix/test/integration/test/client/capture-message.test.ts index 2259c271565d..234b6ee3d961 100644 --- a/packages/remix/test/integration/test/client/capture-message.test.ts +++ b/packages/remix/test/integration/test/client/capture-message.test.ts @@ -8,10 +8,10 @@ test('should report a manually captured message.', async ({ page }) => { const [messageEnvelope, pageloadEnvelope] = envelopes; expect(messageEnvelope.level).toBe('info'); - expect(messageEnvelope.tags?.transaction).toBe('/capture-message'); + expect(messageEnvelope.transaction).toBe('/capture-message'); expect(messageEnvelope.message).toBe('Sentry Manually Captured Message'); - expect(pageloadEnvelope.contexts?.trace.op).toBe('pageload'); + expect(pageloadEnvelope.contexts?.trace?.op).toBe('pageload'); expect(pageloadEnvelope.type).toBe('transaction'); expect(pageloadEnvelope.transaction).toBe('routes/capture-message'); }); From 3a0bdef2491c563d1e2a6e36df4deb988104dd59 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 4 Mar 2024 08:30:18 +0000 Subject: [PATCH 2/2] fix tests --- .../suites/tracing/metric-summaries/test.ts | 8 +++-- .../integration/test/server/action.test.ts | 32 +++++-------------- .../integration/test/server/loader.test.ts | 8 ++--- 3 files changed, 16 insertions(+), 32 deletions(-) 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 index 9496e510f680..94f5fdc30c70 100644 --- 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,6 +41,7 @@ const EXPECTED_TRANSACTION = { sum: 4, tags: { release: '1.0', + transaction: 'Test Transaction', }, }, ], @@ -52,6 +53,7 @@ const EXPECTED_TRANSACTION = { sum: 2, tags: { release: '1.0', + transaction: 'Test Transaction', }, }, ], @@ -63,6 +65,7 @@ const EXPECTED_TRANSACTION = { sum: 62, tags: { release: '1.0', + transaction: 'Test Transaction', }, }, ], @@ -74,6 +77,7 @@ const EXPECTED_TRANSACTION = { sum: 62, tags: { release: '1.0', + transaction: 'Test Transaction', }, }, ], diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index 9c4c6effa828..fe0b4a749c43 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -162,9 +162,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }); assertSentryTransaction(transaction_2[2], { @@ -178,9 +176,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }); assertSentryEvent(event[2], { @@ -228,9 +224,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }); assertSentryEvent(event[2], { @@ -278,9 +272,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }); assertSentryEvent(event[2], { @@ -328,9 +320,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }); assertSentryEvent(event[2], { @@ -378,9 +368,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }); assertSentryEvent(event[2], { @@ -428,9 +416,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/server-side-unexpected-errors${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/server-side-unexpected-errors${useV2 ? '.' : '/'}$id`, }); assertSentryEvent(event[2], { @@ -478,9 +464,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/server-side-unexpected-errors${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/server-side-unexpected-errors${useV2 ? '.' : '/'}$id`, }); assertSentryEvent(event[2], { diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index aa25ca13cbb2..c40282096e18 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -126,9 +126,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, }); assertSentryTransaction(transaction_2[2], { @@ -142,9 +140,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }, }, }, - tags: { - transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, - }, + transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, }); assertSentryEvent(event[2], {