Skip to content

Commit 40c847c

Browse files
Lms24mydea
andauthored
feat(core): Decouple scope.transactionName from root spans (#10991)
To decouple the scope's `transaction` field from a currently active transaction/root span, we now change event.tranasction assignment behaviour so that: * only non-transaction events are assigned `event.transaction` from `scopeData.transactionName` (e.g. errors, profiles, replay) * only transaction events are assigned `event.transaction` from the root span name Furthermore, this patch also undeprecates `ScopeData.transactionName` as we'll still need it. --------- Co-authored-by: Francesco Novy <[email protected]>
1 parent 2274b27 commit 40c847c

File tree

9 files changed

+135
-23
lines changed

9 files changed

+135
-23
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ test('Should record exceptions and transactions for faulty route handlers', asyn
5252
expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server');
5353

5454
expect(routehandlerError.exception?.values?.[0].value).toBe('route-handler-error');
55-
expect(routehandlerError.transaction).toBe('PUT /route-handlers/[param]/error');
55+
// TODO: Uncomment once we update the scope transaction name on the server side
56+
// expect(routehandlerError.transaction).toBe('PUT /route-handlers/[param]/error');
5657
});
5758

5859
test.describe('Edge runtime', () => {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ test.describe('server-side errors', () => {
6060
}),
6161
);
6262

63-
expect(errorEvent.transaction).toEqual('GET /server-route-error');
63+
// TODO: Uncomment once we update the scope transaction name on the server side
64+
// expect(errorEvent.transaction).toEqual('GET /server-route-error');
6465
});
6566
});

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ test.describe('server-side errors', () => {
6363
}),
6464
);
6565

66-
expect(errorEvent.transaction).toEqual('GET /server-route-error');
66+
// TODO: Uncomment once we update the scope transaction name on the server side
67+
// expect(errorEvent.transaction).toEqual('GET /server-route-error');
6768
});
6869
});

packages/core/src/scope.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,9 @@ export class Scope implements ScopeInterface {
8181

8282
/**
8383
* Transaction Name
84+
*
85+
* IMPORTANT: The transaction name on the scope has nothing to do with root spans/transaction objects.
86+
* It's purpose is to assign a transaction to the scope that's added to non-transaction events.
8487
*/
8588
protected _transactionName?: string;
8689

@@ -278,7 +281,7 @@ export class Scope implements ScopeInterface {
278281
}
279282

280283
/**
281-
* Sets the transaction name on the scope for future events.
284+
* @inheritDoc
282285
*/
283286
public setTransactionName(name?: string): this {
284287
this._transactionName = name;

packages/core/src/utils/applyScopeDataToEvent.ts

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ export function mergeScopeData(data: ScopeData, mergeData: ScopeData): void {
3838
eventProcessors,
3939
attachments,
4040
propagationContext,
41-
// eslint-disable-next-line deprecation/deprecation
4241
transactionName,
4342
span,
4443
} = mergeData;
@@ -54,7 +53,6 @@ export function mergeScopeData(data: ScopeData, mergeData: ScopeData): void {
5453
}
5554

5655
if (transactionName) {
57-
// eslint-disable-next-line deprecation/deprecation
5856
data.transactionName = transactionName;
5957
}
6058

@@ -118,15 +116,7 @@ export function mergeArray<Prop extends 'breadcrumbs' | 'fingerprint'>(
118116
}
119117

120118
function applyDataToEvent(event: Event, data: ScopeData): void {
121-
const {
122-
extra,
123-
tags,
124-
user,
125-
contexts,
126-
level,
127-
// eslint-disable-next-line deprecation/deprecation
128-
transactionName,
129-
} = data;
119+
const { extra, tags, user, contexts, level, transactionName } = data;
130120

131121
const cleanedExtra = dropUndefinedKeys(extra);
132122
if (cleanedExtra && Object.keys(cleanedExtra).length) {
@@ -152,7 +142,8 @@ function applyDataToEvent(event: Event, data: ScopeData): void {
152142
event.level = level;
153143
}
154144

155-
if (transactionName) {
145+
// transaction events get their `transaction` from the root span name
146+
if (transactionName && event.type !== 'transaction') {
156147
event.transaction = transactionName;
157148
}
158149
}
@@ -179,7 +170,7 @@ function applySpanToEvent(event: Event, span: Span): void {
179170

180171
const rootSpan = getRootSpan(span);
181172
const transactionName = spanToJSON(rootSpan).description;
182-
if (transactionName && !event.transaction) {
173+
if (transactionName && !event.transaction && event.type === 'transaction') {
183174
event.transaction = transactionName;
184175
}
185176
}

packages/core/test/lib/utils/applyScopeDataToEvent.test.ts

Lines changed: 108 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
1-
import type { Attachment, Breadcrumb, EventProcessor, ScopeData } from '@sentry/types';
2-
import { mergeAndOverwriteScopeData, mergeArray, mergeScopeData } from '../../../src/utils/applyScopeDataToEvent';
1+
import type { Attachment, Breadcrumb, Event, EventProcessor, EventType, ScopeData } from '@sentry/types';
2+
import { startInactiveSpan } from '../../../src';
3+
import {
4+
applyScopeDataToEvent,
5+
mergeAndOverwriteScopeData,
6+
mergeArray,
7+
mergeScopeData,
8+
} from '../../../src/utils/applyScopeDataToEvent';
39

410
describe('mergeArray', () => {
511
it.each([
@@ -158,3 +164,103 @@ describe('mergeScopeData', () => {
158164
});
159165
});
160166
});
167+
168+
describe('applyScopeDataToEvent', () => {
169+
it("doesn't apply scope.transactionName to transaction events", () => {
170+
const data: ScopeData = {
171+
eventProcessors: [],
172+
breadcrumbs: [],
173+
user: {},
174+
tags: {},
175+
extra: {},
176+
contexts: {},
177+
attachments: [],
178+
propagationContext: { spanId: '1', traceId: '1' },
179+
sdkProcessingMetadata: {},
180+
fingerprint: [],
181+
transactionName: 'foo',
182+
};
183+
const event: Event = { type: 'transaction', transaction: '/users/:id' };
184+
185+
applyScopeDataToEvent(event, data);
186+
187+
expect(event.transaction).toBe('/users/:id');
188+
});
189+
190+
it('applies the root span name to transaction events', () => {
191+
const data: ScopeData = {
192+
eventProcessors: [],
193+
breadcrumbs: [],
194+
user: {},
195+
tags: {},
196+
extra: {},
197+
contexts: {},
198+
attachments: [],
199+
propagationContext: { spanId: '1', traceId: '1' },
200+
sdkProcessingMetadata: {},
201+
fingerprint: [],
202+
transactionName: 'foo',
203+
span: {
204+
attributes: {},
205+
startTime: 1,
206+
endTime: 2,
207+
status: 'ok',
208+
name: 'bar',
209+
// @ts-expect-error - we don't need to provide all span context fields
210+
spanContext: () => ({}),
211+
},
212+
};
213+
214+
const event: Event = { type: 'transaction' };
215+
216+
applyScopeDataToEvent(event, data);
217+
218+
expect(event.transaction).toBe('bar');
219+
});
220+
221+
it("doesn't apply the root span name to non-transaction events", () => {
222+
const data: ScopeData = {
223+
eventProcessors: [],
224+
breadcrumbs: [],
225+
user: {},
226+
tags: {},
227+
extra: {},
228+
contexts: {},
229+
attachments: [],
230+
propagationContext: { spanId: '1', traceId: '1' },
231+
sdkProcessingMetadata: {},
232+
fingerprint: [],
233+
transactionName: '/users/:id',
234+
span: startInactiveSpan({ name: 'foo' }),
235+
};
236+
const event: Event = { type: undefined };
237+
238+
applyScopeDataToEvent(event, data);
239+
240+
expect(event.transaction).toBe('/users/:id');
241+
});
242+
243+
it.each([undefined, 'profile', 'replay_event', 'feedback'])(
244+
'applies scope.transactionName to event with type %s',
245+
type => {
246+
const data: ScopeData = {
247+
eventProcessors: [],
248+
breadcrumbs: [],
249+
user: {},
250+
tags: {},
251+
extra: {},
252+
contexts: {},
253+
attachments: [],
254+
propagationContext: { spanId: '1', traceId: '1' },
255+
sdkProcessingMetadata: {},
256+
fingerprint: [],
257+
transactionName: 'foo',
258+
};
259+
const event: Event = { type: type as EventType, transaction: '/users/:id' };
260+
261+
applyScopeDataToEvent(event, data);
262+
263+
expect(event.transaction).toBe('foo');
264+
},
265+
);
266+
});

packages/remix/test/integration/test/client/capture-exception.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ test('should report a manually captured error.', async ({ page }) => {
88
const [errorEnvelope, pageloadEnvelope] = envelopes;
99

1010
expect(errorEnvelope.level).toBe('error');
11-
expect(errorEnvelope.transaction).toBe('/capture-exception');
11+
// TODO: Comment back in once we update the scope transaction name on the client side
12+
// expect(errorEnvelope.transaction).toBe('/capture-exception');
1213
expect(errorEnvelope.exception?.values).toMatchObject([
1314
{
1415
type: 'Error',

packages/remix/test/integration/test/client/capture-message.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ test('should report a manually captured message.', async ({ page }) => {
88
const [messageEnvelope, pageloadEnvelope] = envelopes;
99

1010
expect(messageEnvelope.level).toBe('info');
11-
expect(messageEnvelope.transaction).toBe('/capture-message');
11+
// TODO: Comment back in once we update the scope transaction name on the client side
12+
// expect(messageEnvelope.transaction).toBe('/capture-message');
1213
expect(messageEnvelope.message).toBe('Sentry Manually Captured Message');
1314

1415
expect(pageloadEnvelope.contexts?.trace?.op).toBe('pageload');

packages/types/src/scope.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ export interface ScopeData {
4040
sdkProcessingMetadata: { [key: string]: unknown };
4141
fingerprint: string[];
4242
level?: SeverityLevel;
43-
/** @deprecated This will be removed in v8. */
4443
transactionName?: string;
4544
span?: Span;
4645
}
@@ -127,7 +126,15 @@ export interface Scope {
127126
setLevel(level: SeverityLevel): this;
128127

129128
/**
130-
* Sets the transaction name on the scope for future events.
129+
* Sets the transaction name on the scope so that the name of the transaction
130+
* (e.g. taken server route or page location) is attached to future events.
131+
*
132+
* IMPORTANT: Calling this function does NOT change the name of the currently active
133+
* span. If you want to change the name of the active span, use `span.updateName()`
134+
* instead.
135+
*
136+
* By default, the SDK updates the scope's transaction name automatically on sensible
137+
* occasions, such as a page navigation or when handling a new request on the server.
131138
*/
132139
setTransactionName(name?: string): this;
133140

0 commit comments

Comments
 (0)