Skip to content

Commit 6a547af

Browse files
mydeamydea
committed
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`
1 parent 4b7deee commit 6a547af

File tree

12 files changed

+107
-85
lines changed

12 files changed

+107
-85
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { Route } from '@playwright/test';
22
import { expect } from '@playwright/test';
3-
import type { Event, Span, SpanContext, Transaction } from '@sentry/types';
3+
import type { Event, SpanContext, SpanJSON } from '@sentry/types';
44

55
import { sentryTest } from '../../../../utils/fixtures';
66
import {
@@ -9,8 +9,8 @@ import {
99
shouldSkipTracingTest,
1010
} from '../../../../utils/helpers';
1111

12-
type TransactionJSON = ReturnType<Transaction['toJSON']> & {
13-
spans: ReturnType<Span['toJSON']>[];
12+
type TransactionJSON = SpanJSON & {
13+
spans: SpanJSON[];
1414
contexts: SpanContext;
1515
platform: string;
1616
type: string;

‎dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,8 @@ const EXPECTED_TRANSACTION = {
4141
sum: 4,
4242
tags: {
4343
release: '1.0',
44-
transaction: 'Test Transaction',
4544
},
45+
transaction: 'Test Transaction',
4646
},
4747
],
4848
's:root-set@none': [
@@ -53,8 +53,8 @@ const EXPECTED_TRANSACTION = {
5353
sum: 2,
5454
tags: {
5555
release: '1.0',
56-
transaction: 'Test Transaction',
5756
},
57+
transaction: 'Test Transaction',
5858
},
5959
],
6060
'g:root-gauge@none': [
@@ -65,8 +65,8 @@ const EXPECTED_TRANSACTION = {
6565
sum: 62,
6666
tags: {
6767
release: '1.0',
68-
transaction: 'Test Transaction',
6968
},
69+
transaction: 'Test Transaction',
7070
},
7171
],
7272
'd:root-distribution@none': [
@@ -77,8 +77,8 @@ const EXPECTED_TRANSACTION = {
7777
sum: 62,
7878
tags: {
7979
release: '1.0',
80-
transaction: 'Test Transaction',
8180
},
81+
transaction: 'Test Transaction',
8282
},
8383
],
8484
},

‎packages/core/src/tracing/trace.ts‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,11 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span |
4242
scope,
4343
});
4444

45+
if (activeSpan) {
46+
// eslint-disable-next-line deprecation/deprecation
47+
scope.setSpan(activeSpan);
48+
}
49+
4550
return handleCallbackErrors(
4651
() => callback(activeSpan),
4752
() => {
@@ -91,6 +96,11 @@ export function startSpanManual<T>(
9196
scope,
9297
});
9398

99+
if (activeSpan) {
100+
// eslint-disable-next-line deprecation/deprecation
101+
scope.setSpan(activeSpan);
102+
}
103+
94104
function finishAndSetSpan(): void {
95105
activeSpan && activeSpan.end();
96106
}
@@ -141,16 +151,11 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
141151

142152
const scope = context.scope || getCurrentScope();
143153

144-
// Even though we don't actually want to make this span active on the current scope,
145-
// we need to make it active on a temporary scope that we use for event processing
146-
// as otherwise, it won't pick the correct span for the event when processing it
147-
const temporaryScope = scope.clone();
148-
149154
return createChildSpanOrTransaction(hub, {
150155
parentSpan,
151156
spanContext,
152157
forceTransaction: context.forceTransaction,
153-
scope: temporaryScope,
158+
scope,
154159
});
155160
}
156161

@@ -319,12 +324,6 @@ function createChildSpanOrTransaction(
319324
});
320325
}
321326

322-
// We always set this as active span on the scope
323-
// In the case of this being an inactive span, we ensure to pass a detached scope in here in the first place
324-
// But by having this here, we can ensure that the lookup through `getCapturedScopesOnSpan` results in the correct scope & span combo
325-
// eslint-disable-next-line deprecation/deprecation
326-
scope.setSpan(span);
327-
328327
setCapturedScopesOnSpan(span, scope, isolationScope);
329328

330329
return span;

‎packages/core/src/utils/applyScopeDataToEvent.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,10 @@ function applySpanToEvent(event: Event, span: Span): void {
178178
dynamicSamplingContext: getDynamicSamplingContextFromSpan(span),
179179
...event.sdkProcessingMetadata,
180180
};
181+
181182
const transactionName = spanToJSON(rootSpan).description;
182-
if (transactionName) {
183-
event.tags = { transaction: transactionName, ...event.tags };
183+
if (transactionName && !event.transaction) {
184+
event.transaction = transactionName;
184185
}
185186
}
186187
}

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

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -360,9 +360,7 @@ describe('startSpan', () => {
360360
},
361361
});
362362
expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]);
363-
expect(outerTransaction?.tags).toEqual({
364-
transaction: 'outer transaction',
365-
});
363+
expect(outerTransaction?.transaction).toEqual('outer transaction');
366364
expect(outerTransaction?.sdkProcessingMetadata).toEqual({
367365
dynamicSamplingContext: {
368366
environment: 'production',
@@ -386,9 +384,7 @@ describe('startSpan', () => {
386384
},
387385
});
388386
expect(innerTransaction?.spans).toEqual([{ name: 'inner span 2', id: expect.any(String) }]);
389-
expect(innerTransaction?.tags).toEqual({
390-
transaction: 'inner transaction',
391-
});
387+
expect(innerTransaction?.transaction).toEqual('inner transaction');
392388
expect(innerTransaction?.sdkProcessingMetadata).toEqual({
393389
dynamicSamplingContext: {
394390
environment: 'production',
@@ -643,9 +639,7 @@ describe('startSpanManual', () => {
643639
},
644640
});
645641
expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]);
646-
expect(outerTransaction?.tags).toEqual({
647-
transaction: 'outer transaction',
648-
});
642+
expect(outerTransaction?.transaction).toEqual('outer transaction');
649643
expect(outerTransaction?.sdkProcessingMetadata).toEqual({
650644
dynamicSamplingContext: {
651645
environment: 'production',
@@ -669,9 +663,7 @@ describe('startSpanManual', () => {
669663
},
670664
});
671665
expect(innerTransaction?.spans).toEqual([{ name: 'inner span 2', id: expect.any(String) }]);
672-
expect(innerTransaction?.tags).toEqual({
673-
transaction: 'inner transaction',
674-
});
666+
expect(innerTransaction?.transaction).toEqual('inner transaction');
675667
expect(innerTransaction?.sdkProcessingMetadata).toEqual({
676668
dynamicSamplingContext: {
677669
environment: 'production',
@@ -854,9 +846,7 @@ describe('startInactiveSpan', () => {
854846
},
855847
});
856848
expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]);
857-
expect(outerTransaction?.tags).toEqual({
858-
transaction: 'outer transaction',
859-
});
849+
expect(outerTransaction?.transaction).toEqual('outer transaction');
860850
expect(outerTransaction?.sdkProcessingMetadata).toEqual({
861851
dynamicSamplingContext: {
862852
environment: 'production',
@@ -880,9 +870,7 @@ describe('startInactiveSpan', () => {
880870
},
881871
});
882872
expect(innerTransaction?.spans).toEqual([]);
883-
expect(innerTransaction?.tags).toEqual({
884-
transaction: 'inner transaction',
885-
});
873+
expect(innerTransaction?.transaction).toEqual('inner transaction');
886874
expect(innerTransaction?.sdkProcessingMetadata).toEqual({
887875
dynamicSamplingContext: {
888876
environment: 'production',
@@ -948,9 +936,13 @@ describe('startInactiveSpan', () => {
948936

949937
let span: Span | undefined;
950938

939+
const scope = getCurrentScope();
940+
scope.setTag('outer', 'foo');
941+
951942
withScope(scope => {
952943
scope.setTag('scope', 1);
953944
span = startInactiveSpan({ name: 'my-span' });
945+
scope.setTag('scope_after_span', 2);
954946
});
955947

956948
withScope(scope => {
@@ -964,7 +956,9 @@ describe('startInactiveSpan', () => {
964956
expect(beforeSendTransaction).toHaveBeenCalledWith(
965957
expect.objectContaining({
966958
tags: expect.objectContaining({
959+
outer: 'foo',
967960
scope: 1,
961+
scope_after_span: 2,
968962
}),
969963
}),
970964
expect.anything(),

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ describe('Integration | Scope', () => {
8282
tag2: 'val2',
8383
tag3: 'val3',
8484
tag4: 'val4',
85-
...(enableTracing ? { transaction: 'outer' } : {}),
8685
},
86+
...(enableTracing ? { transaction: 'outer' } : {}),
8787
}),
8888
{
8989
event_id: expect.any(String),
@@ -118,7 +118,6 @@ describe('Integration | Scope', () => {
118118
tag2: 'val2',
119119
tag3: 'val3',
120120
tag4: 'val4',
121-
transaction: 'outer',
122121
},
123122
timestamp: expect.any(Number),
124123
transaction: 'outer',
@@ -203,8 +202,8 @@ describe('Integration | Scope', () => {
203202
tag2: 'val2a',
204203
tag3: 'val3a',
205204
tag4: 'val4a',
206-
...(enableTracing ? { transaction: 'outer' } : {}),
207205
},
206+
...(enableTracing ? { transaction: 'outer' } : {}),
208207
}),
209208
{
210209
event_id: expect.any(String),
@@ -229,8 +228,8 @@ describe('Integration | Scope', () => {
229228
tag2: 'val2b',
230229
tag3: 'val3b',
231230
tag4: 'val4b',
232-
...(enableTracing ? { transaction: 'outer' } : {}),
233231
},
232+
...(enableTracing ? { transaction: 'outer' } : {}),
234233
}),
235234
{
236235
event_id: expect.any(String),

‎packages/node-experimental/test/integration/transactions.test.ts‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,6 @@ describe('Integration | Transactions', () => {
124124
tags: {
125125
'outer.tag': 'test value',
126126
'test.tag': 'test value',
127-
transaction: 'test name',
128127
},
129128
timestamp: expect.any(Number),
130129
transaction: 'test name',
@@ -267,7 +266,9 @@ describe('Integration | Transactions', () => {
267266
}),
268267
spans: [expect.any(Object), expect.any(Object)],
269268
start_timestamp: expect.any(Number),
270-
tags: { 'test.tag': 'test value', transaction: 'test name' },
269+
tags: {
270+
'test.tag': 'test value',
271+
},
271272
timestamp: expect.any(Number),
272273
transaction: 'test name',
273274
transaction_info: { source: 'task' },
@@ -310,7 +311,9 @@ describe('Integration | Transactions', () => {
310311
}),
311312
spans: [expect.any(Object), expect.any(Object)],
312313
start_timestamp: expect.any(Number),
313-
tags: { 'test.tag': 'test value b', transaction: 'test name b' },
314+
tags: {
315+
'test.tag': 'test value b',
316+
},
314317
timestamp: expect.any(Number),
315318
transaction: 'test name b',
316319
transaction_info: { source: 'custom' },
@@ -417,7 +420,9 @@ describe('Integration | Transactions', () => {
417420
}),
418421
spans: [expect.any(Object), expect.any(Object)],
419422
start_timestamp: expect.any(Number),
420-
tags: { 'test.tag': 'test value', transaction: 'test name' },
423+
tags: {
424+
'test.tag': 'test value',
425+
},
421426
timestamp: expect.any(Number),
422427
transaction: 'test name',
423428
type: 'transaction',
@@ -456,7 +461,9 @@ describe('Integration | Transactions', () => {
456461
}),
457462
spans: [expect.any(Object), expect.any(Object)],
458463
start_timestamp: expect.any(Number),
459-
tags: { 'test.tag': 'test value b', transaction: 'test name b' },
464+
tags: {
465+
'test.tag': 'test value b',
466+
},
460467
timestamp: expect.any(Number),
461468
transaction: 'test name b',
462469
type: 'transaction',

‎packages/opentelemetry/src/setupEventContextTrace.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ export function setupEventContextTrace(client: Client): void {
2828

2929
const rootSpan = getRootSpan(span);
3030
const transactionName = spanHasName(rootSpan) ? rootSpan.name : undefined;
31-
if (transactionName) {
32-
event.tags = { transaction: transactionName, ...event.tags };
31+
if (transactionName && !event.transaction) {
32+
event.transaction = transactionName;
3333
}
3434

3535
return event;

‎packages/opentelemetry/src/spanExporter.ts‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -200,14 +200,7 @@ function createTransactionForOtelSpan(span: ReadableSpan): Transaction {
200200
});
201201

202202
if (capturedSpanScopes) {
203-
// Ensure the `transaction` tag is correctly set on the transaction event
204-
const scope = capturedSpanScopes.scope.clone();
205-
scope.addEventProcessor(event => {
206-
event.tags = { transaction: description, ...event.tags };
207-
return event;
208-
});
209-
210-
setCapturedScopesOnTransaction(transaction, scope, capturedSpanScopes.isolationScope);
203+
setCapturedScopesOnTransaction(transaction, capturedSpanScopes.scope, capturedSpanScopes.isolationScope);
211204
}
212205

213206
return transaction;

‎packages/opentelemetry/test/integration/scope.test.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,8 @@ describe('Integration | Scope', () => {
9090
tag2: 'val2',
9191
tag3: 'val3',
9292
tag4: 'val4',
93-
...(enableTracing ? { transaction: 'outer' } : {}),
9493
},
94+
...(enableTracing ? { transaction: 'outer' } : undefined),
9595
}),
9696
{
9797
event_id: expect.any(String),
@@ -126,7 +126,6 @@ describe('Integration | Scope', () => {
126126
tag2: 'val2',
127127
tag3: 'val3',
128128
tag4: 'val4',
129-
transaction: 'outer',
130129
},
131130
timestamp: expect.any(Number),
132131
transaction: 'outer',
@@ -220,8 +219,8 @@ describe('Integration | Scope', () => {
220219
tag2: 'val2a',
221220
tag3: 'val3a',
222221
tag4: 'val4a',
223-
...(enableTracing ? { transaction: 'outer' } : {}),
224222
},
223+
...(enableTracing ? { transaction: 'outer' } : undefined),
225224
}),
226225
{
227226
event_id: expect.any(String),
@@ -246,8 +245,8 @@ describe('Integration | Scope', () => {
246245
tag2: 'val2b',
247246
tag3: 'val3b',
248247
tag4: 'val4b',
249-
...(enableTracing ? { transaction: 'outer' } : {}),
250248
},
249+
...(enableTracing ? { transaction: 'outer' } : undefined),
251250
}),
252251
{
253252
event_id: expect.any(String),
@@ -346,8 +345,8 @@ describe('Integration | Scope', () => {
346345
tag4: 'val4a',
347346
isolationTag1: 'val1',
348347
isolationTag2: 'val2',
349-
...(enableTracing ? { transaction: 'outer' } : {}),
350348
},
349+
...(enableTracing ? { transaction: 'outer' } : undefined),
351350
}),
352351
{
353352
event_id: expect.any(String),
@@ -374,8 +373,8 @@ describe('Integration | Scope', () => {
374373
tag4: 'val4b',
375374
isolationTag1: 'val1',
376375
isolationTag2: 'val2b',
377-
...(enableTracing ? { transaction: 'outer' } : {}),
378376
},
377+
...(enableTracing ? { transaction: 'outer' } : undefined),
379378
}),
380379
{
381380
event_id: expect.any(String),

0 commit comments

Comments
 (0)