Skip to content

Commit

Permalink
feat(core): Add span.updateName() and deprecate span.setName() (#…
Browse files Browse the repository at this point in the history
…10018)

You can call `span.setMetadata()` if you need to update the source.

This aligns with the OTEL API for spans.
  • Loading branch information
mydea authored and anonrig committed Jan 3, 2024
1 parent 6f3f053 commit d026929
Show file tree
Hide file tree
Showing 25 changed files with 194 additions and 67 deletions.
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

0 comments on commit d026929

Please sign in to comment.