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): Add span.updateName() and deprecate span.setName() #10018

Merged
merged 4 commits into from
Jan 3, 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
1 change: 1 addition & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ In v8, the Span class is heavily reworked. The following properties & methods ar

* `span.toContext()`: Access the fields directly instead.
* `span.updateWithContext(newSpanContext)`: Update the fields directly instead.
* `span.setName(newName)`: Use `span.updateName(newName)` instead.

## Deprecate `pushScope` & `popScope` in favor of `withScope`

Expand Down
3 changes: 2 additions & 1 deletion packages/angular/src/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ export class TraceService implements OnDestroy {
const transaction = getActiveTransaction();
// TODO (v8 / #5416): revisit the source condition. Do we want to make the parameterized route the default?
if (transaction && transaction.metadata.source === 'url') {
transaction.setName(route, 'route');
transaction.updateName(route);
transaction.setMetadata({ source: 'route' });
}
}),
);
Expand Down
10 changes: 6 additions & 4 deletions packages/angular/test/tracing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ let transaction: any;
const defaultStartTransaction = (ctx: any) => {
transaction = {
...ctx,
setName: jest.fn(name => (transaction.name = name)),
updateName: jest.fn(name => (transaction.name = name)),
setMetadata: jest.fn(),
};

return transaction;
Expand Down Expand Up @@ -110,7 +111,7 @@ describe('Angular Tracing', () => {
...ctx.metadata,
source: 'custom',
},
setName: jest.fn(name => (transaction.name = name)),
updateName: jest.fn(name => (transaction.name = name)),
};

return transaction;
Expand All @@ -137,7 +138,7 @@ describe('Angular Tracing', () => {
metadata: { source: 'url' },
});

expect(transaction.setName).toHaveBeenCalledTimes(0);
expect(transaction.updateName).toHaveBeenCalledTimes(0);
expect(transaction.name).toEqual(url);
expect(transaction.metadata.source).toBe('custom');

Expand Down Expand Up @@ -327,7 +328,8 @@ describe('Angular Tracing', () => {
origin: 'auto.navigation.angular',
metadata: { source: 'url' },
});
expect(transaction.setName).toHaveBeenCalledWith(result, 'route');
expect(transaction.updateName).toHaveBeenCalledWith(result);
expect(transaction.setMetadata).toHaveBeenCalledWith({ source: 'route' });

env.destroy();
});
Expand Down
14 changes: 11 additions & 3 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,11 @@ export class Span implements SpanInterface {
public get name(): string {
return this.description || '';
}
/** Update the name of the span. */
/**
* Update the name of the span.
*/
public set name(name: string) {
this.setName(name);
this.updateName(name);
}

/**
Expand Down Expand Up @@ -267,11 +269,17 @@ export class Span implements SpanInterface {
return this;
}

/** @inheritdoc */
public setName(name: string): void {
this.updateName(name);
}

/**
* @inheritDoc
*/
public setName(name: string): void {
public updateName(name: string): this {
this.description = name;
return this;
}

/**
Expand Down
15 changes: 13 additions & 2 deletions packages/core/src/tracing/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,19 +82,30 @@ export class Transaction extends SpanClass implements TransactionInterface {
return this._name;
}

/** Setter for `name` property, which also sets `source` as custom */
/**
* Setter for `name` property, which also sets `source` as custom.
*/
public set name(newName: string) {
// eslint-disable-next-line deprecation/deprecation
this.setName(newName);
}

/**
* JSDoc
* Setter for `name` property, which also sets `source` on the metadata.
*
* @deprecated Use `updateName()` and `setMetadata()` instead.
*/
public setName(name: string, source: TransactionMetadata['source'] = 'custom'): void {
this._name = name;
this.metadata.source = source;
}

/** @inheritdoc */
public updateName(name: string): this {
this._name = name;
return this;
}

/**
* Attaches SpanRecorder to the span itself
* @param maxlen maximum number of spans that can be recorded
Expand Down
25 changes: 24 additions & 1 deletion packages/core/test/lib/tracing/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('span', () => {
expect(span.description).toEqual(undefined);
});

it('allows to update the name', () => {
it('allows to update the name via setter', () => {
const span = new Span({ name: 'span name' });
expect(span.name).toEqual('span name');
expect(span.description).toEqual('span name');
Expand All @@ -30,6 +30,29 @@ describe('span', () => {
expect(span.description).toEqual('new name');
});

it('allows to update the name via setName', () => {
const span = new Span({ name: 'span name' });
expect(span.name).toEqual('span name');
expect(span.description).toEqual('span name');

// eslint-disable-next-line deprecation/deprecation
span.setName('new name');

expect(span.name).toEqual('new name');
expect(span.description).toEqual('new name');
});

it('allows to update the name via updateName', () => {
const span = new Span({ name: 'span name' });
expect(span.name).toEqual('span name');
expect(span.description).toEqual('span name');

span.updateName('new name');

expect(span.name).toEqual('new name');
expect(span.description).toEqual('new name');
});

describe('setAttribute', () => {
it('allows to set attributes', () => {
const span = new Span();
Expand Down
29 changes: 28 additions & 1 deletion packages/core/test/lib/tracing/transaction.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,39 @@ describe('transaction', () => {
expect(transaction.name).toEqual('span name');
});

it('allows to update the name', () => {
it('allows to update the name via setter', () => {
const transaction = new Transaction({ name: 'span name' });
transaction.setMetadata({ source: 'route' });
expect(transaction.name).toEqual('span name');

transaction.name = 'new name';

expect(transaction.name).toEqual('new name');
expect(transaction.metadata.source).toEqual('custom');
});

it('allows to update the name via setName', () => {
const transaction = new Transaction({ name: 'span name' });
transaction.setMetadata({ source: 'route' });
expect(transaction.name).toEqual('span name');

transaction.setMetadata({ source: 'route' });

// eslint-disable-next-line deprecation/deprecation
transaction.setName('new name');

expect(transaction.name).toEqual('new name');
expect(transaction.metadata.source).toEqual('custom');
});

it('allows to update the name via updateName', () => {
const transaction = new Transaction({ name: 'span name' });
transaction.setMetadata({ source: 'route' });
expect(transaction.name).toEqual('span name');

transaction.updateName('new name');

expect(transaction.name).toEqual('new name');
expect(transaction.metadata.source).toEqual('route');
});
});
3 changes: 2 additions & 1 deletion packages/node/src/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,8 @@ export function trpcMiddleware(options: SentryTrpcMiddlewareOptions = {}) {
const sentryTransaction = getCurrentScope().getTransaction();

if (sentryTransaction) {
sentryTransaction.setName(`trpc/${path}`, 'route');
sentryTransaction.updateName(`trpc/${path}`);
sentryTransaction.setMetadata({ source: 'route' });
sentryTransaction.op = 'rpc.server';

const trpcContext: Record<string, unknown> = {
Expand Down
3 changes: 2 additions & 1 deletion packages/opentelemetry-node/src/spanprocessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@ function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelS
transaction.setStatus(mapOtelStatus(otelSpan));

transaction.op = op;
transaction.setName(description, source);
transaction.updateName(description);
transaction.setMetadata({ source });
}

function convertOtelTimeToSeconds([seconds, nano]: [number, number]): number {
Expand Down
3 changes: 2 additions & 1 deletion packages/react/src/reactrouter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ export function withSentryRouting<P extends Record<string, any>, R extends React

const WrappedRoute: React.FC<P> = (props: P) => {
if (activeTransaction && props && props.computedMatch && props.computedMatch.isExact) {
activeTransaction.setName(props.computedMatch.path, 'route');
activeTransaction.updateName(props.computedMatch.path);
activeTransaction.setMetadata({ source: 'route' });
}

// @ts-expect-error Setting more specific React Component typing for `R` generic above
Expand Down
4 changes: 3 additions & 1 deletion packages/react/src/reactrouterv6.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ function updatePageloadTransaction(
: (_matchRoutes(routes, location, basename) as unknown as RouteMatch[]);

if (activeTransaction && branches) {
activeTransaction.setName(...getNormalizedName(routes, location, branches, basename));
const [name, source] = getNormalizedName(routes, location, branches, basename);
activeTransaction.updateName(name);
activeTransaction.setMetadata({ source });
}
}

Expand Down
30 changes: 18 additions & 12 deletions packages/react/test/reactrouterv4.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe('React Router v4', () => {
startTransactionOnPageLoad?: boolean;
startTransactionOnLocationChange?: boolean;
routes?: RouteConfig[];
}): [jest.Mock, any, { mockSetName: jest.Mock; mockFinish: jest.Mock }] {
}): [jest.Mock, any, { mockUpdateName: jest.Mock; mockFinish: jest.Mock; mockSetMetadata: jest.Mock }] {
const options = {
matchPath: _opts && _opts.routes !== undefined ? matchPath : undefined,
routes: undefined,
Expand All @@ -22,14 +22,17 @@ describe('React Router v4', () => {
};
const history = createMemoryHistory();
const mockFinish = jest.fn();
const mockSetName = jest.fn();
const mockStartTransaction = jest.fn().mockReturnValue({ setName: mockSetName, end: mockFinish });
const mockUpdateName = jest.fn();
const mockSetMetadata = jest.fn();
const mockStartTransaction = jest
.fn()
.mockReturnValue({ updateName: mockUpdateName, end: mockFinish, setMetadata: mockSetMetadata });
reactRouterV4Instrumentation(history, options.routes, options.matchPath)(
mockStartTransaction,
options.startTransactionOnPageLoad,
options.startTransactionOnLocationChange,
);
return [mockStartTransaction, history, { mockSetName, mockFinish }];
return [mockStartTransaction, history, { mockUpdateName, mockFinish, mockSetMetadata }];
}

it('starts a pageload transaction when instrumentation is started', () => {
Expand Down Expand Up @@ -166,7 +169,7 @@ describe('React Router v4', () => {
});

it('normalizes transaction name with custom Route', () => {
const [mockStartTransaction, history, { mockSetName }] = createInstrumentation();
const [mockStartTransaction, history, { mockUpdateName, mockSetMetadata }] = createInstrumentation();
const SentryRoute = withSentryRouting(Route);
const { getByText } = render(
<Router history={history}>
Expand All @@ -191,12 +194,13 @@ describe('React Router v4', () => {
tags: { 'routing.instrumentation': 'react-router-v4' },
metadata: { source: 'url' },
});
expect(mockSetName).toHaveBeenCalledTimes(2);
expect(mockSetName).toHaveBeenLastCalledWith('/users/:userid', 'route');
expect(mockUpdateName).toHaveBeenCalledTimes(2);
expect(mockUpdateName).toHaveBeenLastCalledWith('/users/:userid');
expect(mockSetMetadata).toHaveBeenCalledWith({ source: 'route' });
});

it('normalizes nested transaction names with custom Route', () => {
const [mockStartTransaction, history, { mockSetName }] = createInstrumentation();
const [mockStartTransaction, history, { mockUpdateName, mockSetMetadata }] = createInstrumentation();
const SentryRoute = withSentryRouting(Route);
const { getByText } = render(
<Router history={history}>
Expand All @@ -221,8 +225,9 @@ describe('React Router v4', () => {
tags: { 'routing.instrumentation': 'react-router-v4' },
metadata: { source: 'url' },
});
expect(mockSetName).toHaveBeenCalledTimes(2);
expect(mockSetName).toHaveBeenLastCalledWith('/organizations/:orgid/v1/:teamid', 'route');
expect(mockUpdateName).toHaveBeenCalledTimes(2);
expect(mockUpdateName).toHaveBeenLastCalledWith('/organizations/:orgid/v1/:teamid');
expect(mockSetMetadata).toHaveBeenLastCalledWith({ source: 'route' });

act(() => {
history.push('/organizations/543');
Expand All @@ -237,8 +242,9 @@ describe('React Router v4', () => {
tags: { 'routing.instrumentation': 'react-router-v4' },
metadata: { source: 'url' },
});
expect(mockSetName).toHaveBeenCalledTimes(3);
expect(mockSetName).toHaveBeenLastCalledWith('/organizations/:orgid', 'route');
expect(mockUpdateName).toHaveBeenCalledTimes(3);
expect(mockUpdateName).toHaveBeenLastCalledWith('/organizations/:orgid');
expect(mockSetMetadata).toHaveBeenLastCalledWith({ source: 'route' });
});

it('matches with route object', () => {
Expand Down
Loading