Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Decouple scope.transactionName from root spans #10991

Merged
merged 4 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ 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.transaction).toBe('PUT /route-handlers/[param]/error');
// TODO: Uncomment once we update the scope transaction name on the server side
// expect(routehandlerError.transaction).toBe('PUT /route-handlers/[param]/error');
});

test.describe('Edge runtime', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ test.describe('server-side errors', () => {
}),
);

expect(errorEvent.transaction).toEqual('GET /server-route-error');
// TODO: Uncomment once we update the scope transaction name on the server side
// expect(errorEvent.transaction).toEqual('GET /server-route-error');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ test.describe('server-side errors', () => {
}),
);

expect(errorEvent.transaction).toEqual('GET /server-route-error');
// TODO: Uncomment once we update the scope transaction name on the server side
// expect(errorEvent.transaction).toEqual('GET /server-route-error');
});
});
5 changes: 4 additions & 1 deletion packages/core/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ export class Scope implements ScopeInterface {

/**
* Transaction Name
*
* IMPORTANT: The transaction name on the scope has nothing to do with root spans/transaction objects.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good comment!

* It's purpose is to assign a transaction to the scope that's added to non-transaction events.
*/
protected _transactionName?: string;

Expand Down Expand Up @@ -278,7 +281,7 @@ export class Scope implements ScopeInterface {
}

/**
* Sets the transaction name on the scope for future events.
* @inheritDoc
*/
public setTransactionName(name?: string): this {
this._transactionName = name;
Expand Down
17 changes: 4 additions & 13 deletions packages/core/src/utils/applyScopeDataToEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ export function mergeScopeData(data: ScopeData, mergeData: ScopeData): void {
eventProcessors,
attachments,
propagationContext,
// eslint-disable-next-line deprecation/deprecation
transactionName,
span,
} = mergeData;
Expand All @@ -54,7 +53,6 @@ export function mergeScopeData(data: ScopeData, mergeData: ScopeData): void {
}

if (transactionName) {
// eslint-disable-next-line deprecation/deprecation
data.transactionName = transactionName;
}

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

function applyDataToEvent(event: Event, data: ScopeData): void {
const {
extra,
tags,
user,
contexts,
level,
// eslint-disable-next-line deprecation/deprecation
transactionName,
} = data;
const { extra, tags, user, contexts, level, transactionName } = data;

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

if (transactionName) {
// transaction events get their `transaction` from the root span name
if (transactionName && event.type !== 'transaction') {
Lms24 marked this conversation as resolved.
Show resolved Hide resolved
event.transaction = transactionName;
}
}
Expand All @@ -179,7 +170,7 @@ function applySpanToEvent(event: Event, span: Span): void {

const rootSpan = getRootSpan(span);
const transactionName = spanToJSON(rootSpan).description;
if (transactionName && !event.transaction) {
if (transactionName && !event.transaction && event.type === 'transaction') {
event.transaction = transactionName;
}
}
Expand Down
110 changes: 108 additions & 2 deletions packages/core/test/lib/utils/applyScopeDataToEvent.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import type { Attachment, Breadcrumb, EventProcessor, ScopeData } from '@sentry/types';
import { mergeAndOverwriteScopeData, mergeArray, mergeScopeData } from '../../../src/utils/applyScopeDataToEvent';
import type { Attachment, Breadcrumb, Event, EventProcessor, EventType, ScopeData } from '@sentry/types';
import { startInactiveSpan } from '../../../src';
import {
applyScopeDataToEvent,
mergeAndOverwriteScopeData,
mergeArray,
mergeScopeData,
} from '../../../src/utils/applyScopeDataToEvent';

describe('mergeArray', () => {
it.each([
Expand Down Expand Up @@ -158,3 +164,103 @@ describe('mergeScopeData', () => {
});
});
});

describe('applyScopeDataToEvent', () => {
it("doesn't apply scope.transactionName to transaction events", () => {
const data: ScopeData = {
eventProcessors: [],
breadcrumbs: [],
user: {},
tags: {},
extra: {},
contexts: {},
attachments: [],
propagationContext: { spanId: '1', traceId: '1' },
sdkProcessingMetadata: {},
fingerprint: [],
transactionName: 'foo',
};
Lms24 marked this conversation as resolved.
Show resolved Hide resolved
const event: Event = { type: 'transaction', transaction: '/users/:id' };

applyScopeDataToEvent(event, data);

expect(event.transaction).toBe('/users/:id');
});

it('applies the root span name to transaction events', () => {
const data: ScopeData = {
eventProcessors: [],
breadcrumbs: [],
user: {},
tags: {},
extra: {},
contexts: {},
attachments: [],
propagationContext: { spanId: '1', traceId: '1' },
sdkProcessingMetadata: {},
fingerprint: [],
transactionName: 'foo',
span: {
attributes: {},
startTime: 1,
endTime: 2,
status: 'ok',
name: 'bar',
// @ts-expect-error - we don't need to provide all span context fields
spanContext: () => ({}),
},
};

const event: Event = { type: 'transaction' };

applyScopeDataToEvent(event, data);

expect(event.transaction).toBe('bar');
});

it("doesn't apply the root span name to non-transaction events", () => {
const data: ScopeData = {
eventProcessors: [],
breadcrumbs: [],
user: {},
tags: {},
extra: {},
contexts: {},
attachments: [],
propagationContext: { spanId: '1', traceId: '1' },
sdkProcessingMetadata: {},
fingerprint: [],
transactionName: '/users/:id',
span: startInactiveSpan({ name: 'foo' }),
};
const event: Event = { type: undefined };

applyScopeDataToEvent(event, data);

expect(event.transaction).toBe('/users/:id');
});

it.each([undefined, 'profile', 'replay_event', 'feedback'])(
'applies scope.transactionName to event with type %s',
type => {
const data: ScopeData = {
eventProcessors: [],
breadcrumbs: [],
user: {},
tags: {},
extra: {},
contexts: {},
attachments: [],
propagationContext: { spanId: '1', traceId: '1' },
sdkProcessingMetadata: {},
fingerprint: [],
transactionName: 'foo',
};
const event: Event = { type: type as EventType, transaction: '/users/:id' };

applyScopeDataToEvent(event, data);

expect(event.transaction).toBe('foo');
},
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ test('should report a manually captured error.', async ({ page }) => {
const [errorEnvelope, pageloadEnvelope] = envelopes;

expect(errorEnvelope.level).toBe('error');
expect(errorEnvelope.transaction).toBe('/capture-exception');
// TODO: Comment back in once we update the scope transaction name on the client side
// expect(errorEnvelope.transaction).toBe('/capture-exception');
expect(errorEnvelope.exception?.values).toMatchObject([
{
type: 'Error',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ test('should report a manually captured message.', async ({ page }) => {
const [messageEnvelope, pageloadEnvelope] = envelopes;

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

expect(pageloadEnvelope.contexts?.trace?.op).toBe('pageload');
Expand Down
11 changes: 9 additions & 2 deletions packages/types/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export interface ScopeData {
sdkProcessingMetadata: { [key: string]: unknown };
fingerprint: string[];
level?: SeverityLevel;
/** @deprecated This will be removed in v8. */
transactionName?: string;
span?: Span;
}
Expand Down Expand Up @@ -127,7 +126,15 @@ export interface Scope {
setLevel(level: SeverityLevel): this;

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

Expand Down
Loading