Skip to content

feat(core): Deprecate span tags, data, context & setters #10053

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

Merged
merged 1 commit into from
Jan 10, 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
5 changes: 5 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
* `span.description`: Use `spanToJSON(span).description` instead.
* `transaction.setMetadata()`: Use attributes instead, or set data on the scope.
* `transaction.metadata`: Use attributes instead, or set data on the scope.
* `span.tags`: Set tags on the surrounding scope instead, or use attributes.
* `span.data`: Use `spanToJSON(span).data` instead.
* `span.setTag()`: Use `span.setAttribute()` instead or set tags on the surrounding scope.
* `span.setData()`: Use `span.setAttribute()` instead.
* `transaction.setContext()`: Set context on the surrounding scope instead.

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ sentryTest('should report finished spans as children of the root transaction', a
const span_1 = transaction.spans?.[0];
expect(span_1?.op).toBe('span_1');
expect(span_1?.parentSpanId).toEqual(rootSpanId);
// eslint-disable-next-line deprecation/deprecation
expect(span_1?.data).toMatchObject({ foo: 'bar', baz: [1, 2, 3] });

const span_3 = transaction.spans?.[1];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ test('Should send a transaction for instrumented server actions', async ({ page
await page.getByText('Run Action').click();

expect(await serverComponentTransactionPromise).toBeDefined();
expect((await serverComponentTransactionPromise).contexts?.trace?.data?.['server_action_form_data']).toEqual(
expect.objectContaining({ 'some-text-value': 'some-default-value' }),
);
expect(
(await serverComponentTransactionPromise).contexts?.trace?.data?.['server_action_form_data.some-text-value'],
).toEqual('some-default-value');
expect((await serverComponentTransactionPromise).contexts?.trace?.data?.['server_action_result']).toEqual({
city: 'Vienna',
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ const query = connection.query('SELECT 1 + 1 AS solution');
const query2 = connection.query('SELECT NOW()', ['1', '2']);

query.on('end', () => {
transaction.setTag('result_done', 'yes');
transaction.setAttribute('result_done', 'yes');

query2.on('end', () => {
transaction.setTag('result_done2', 'yes');
transaction.setAttribute('result_done2', 'yes');

// Wait a bit to ensure the queries completed
setTimeout(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@ test('should auto-instrument `mysql` package when using query without callback',
expect(envelope).toHaveLength(3);

assertSentryTransaction(envelope[2], {
transaction: 'Test Transaction',
tags: {
result_done: 'yes',
result_done2: 'yes',
contexts: {
trace: {
data: {
result_done: 'yes',
result_done2: 'yes',
},
},
},
transaction: 'Test Transaction',
spans: [
{
description: 'SELECT 1 + 1 AS solution',
Expand Down
10 changes: 5 additions & 5 deletions docs/v8-new-performance-apis.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ below to see which things used to exist, and how they can/should be mapped going
| `status` | use utility method TODO |
| `sampled` | `spanIsSampled(span)` |
| `startTimestamp` | `startTime` - note that this has a different format! |
| `tags` | `spanGetAttributes(span)`, or set tags on the scope |
| `data` | `spanGetAttributes(span)` |
| `tags` | use attributes, or set tags on the scope |
| `data` | `spanToJSON(span).data` |
| `transaction` | ??? Removed |
| `instrumenter` | Removed |
| `finish()` | `end()` |
Expand All @@ -72,13 +72,13 @@ In addition, a transaction has this API:

| Old name | Replace with |
| --------------------------- | ------------------------------------------------ |
| `name` | `spanGetName(span)` (TODO) |
| `name` | `spanToJSON(span).description` |
| `trimEnd` | Removed |
| `parentSampled` | `spanIsSampled(span)` & `spanContext().isRemote` |
| `metadata` | `spanGetMetadata(span)` |
| `metadata` | Use attributes instead or set on scope |
| `setContext()` | Set context on scope instead |
| `setMeasurement()` | ??? TODO |
| `setMetadata()` | `spanSetMetadata(span, metadata)` |
| `setMetadata()` | Use attributes instead or set on scope |
| `getDynamicSamplingContext` | ??? TODO |

### Attributes vs. Data vs. Tags vs. Context
Expand Down
2 changes: 2 additions & 0 deletions packages/browser/src/profiling/hubextensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ export function startProfileForTransaction(transaction: Transaction): Transactio
// Always call onProfileHandler to ensure stopProfiling is called and the timeout is cleared.
void onProfileHandler().then(
() => {
// TODO: Can we rewrite this to use attributes?
// eslint-disable-next-line deprecation/deprecation
transaction.setContext('profile', { profile_id: profileId, start_timestamp: startTimestamp });
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @JonasBa we either need to move this to be an attribute (~~data) on the transaction, or find a way to set these on the surrounding scope of the transaction instead 🤔 nothing to do in this PR but when we actually remove this in v8 we need to handle this somehow.

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds good. Thanks for the ping!

originalEnd();
},
Expand Down
6 changes: 4 additions & 2 deletions packages/bun/src/integrations/bunserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
captureException,
continueTrace,
convertIntegrationFnToClass,
getCurrentScope,
runWithAsyncContext,
startSpan,
} from '@sentry/core';
Expand Down Expand Up @@ -90,9 +91,10 @@ function instrumentBunServeOptions(serveOptions: Parameters<typeof Bun.serve>[0]
>);
if (response && response.status) {
span?.setHttpStatus(response.status);
span?.setData('http.response.status_code', response.status);
span?.setAttribute('http.response.status_code', response.status);
if (span instanceof Transaction) {
span.setContext('response', {
const scope = getCurrentScope();
scope.setContext('response', {
headers: response.headers.toJSON(),
status_code: response.status,
});
Expand Down
2 changes: 2 additions & 0 deletions packages/bun/test/integrations/bunserver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ describe('Bun Serve Integration', () => {
test('generates a transaction around a request', async () => {
client.on('finishTransaction', transaction => {
expect(transaction.status).toBe('ok');
// eslint-disable-next-line deprecation/deprecation
expect(transaction.tags).toEqual({
'http.status_code': '200',
});
Expand All @@ -48,6 +49,7 @@ describe('Bun Serve Integration', () => {
test('generates a post transaction', async () => {
client.on('finishTransaction', transaction => {
expect(transaction.status).toBe('ok');
// eslint-disable-next-line deprecation/deprecation
expect(transaction.tags).toEqual({
'http.status_code': '200',
});
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/tracing/idletransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export class IdleTransaction extends Transaction {
this.activities = {};

if (this.op === 'ui.action.click') {
this.setTag(FINISH_REASON_TAG, this._finishReason);
this.setAttribute(FINISH_REASON_TAG, this._finishReason);
}

if (this.spanRecorder) {
Expand Down
63 changes: 49 additions & 14 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,21 +86,18 @@ export class Span implements SpanInterface {
public op?: string;

/**
* @inheritDoc
* Tags for the span.
* @deprecated Use `getSpanAttributes(span)` instead.
*/
public tags: { [key: string]: Primitive };

/**
* @inheritDoc
* Data for the span.
* @deprecated Use `getSpanAttributes(span)` instead.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public data: { [key: string]: any };

/**
* @inheritDoc
*/
public attributes: SpanAttributes;

/**
* List of spans that were finalized
*/
Expand All @@ -125,6 +122,7 @@ export class Span implements SpanInterface {
protected _spanId: string;
protected _sampled: boolean | undefined;
protected _name?: string;
protected _attributes: SpanAttributes;

private _logMessage?: string;

Expand All @@ -139,9 +137,11 @@ export class Span implements SpanInterface {
this._traceId = spanContext.traceId || uuid4();
this._spanId = spanContext.spanId || uuid4().substring(16);
this.startTimestamp = spanContext.startTimestamp || timestampInSeconds();
// eslint-disable-next-line deprecation/deprecation
this.tags = spanContext.tags ? { ...spanContext.tags } : {};
// eslint-disable-next-line deprecation/deprecation
this.data = spanContext.data ? { ...spanContext.data } : {};
this.attributes = spanContext.attributes ? { ...spanContext.attributes } : {};
this._attributes = spanContext.attributes ? { ...spanContext.attributes } : {};
this.instrumenter = spanContext.instrumenter || 'sentry';
this.origin = spanContext.origin || 'manual';
// eslint-disable-next-line deprecation/deprecation
Expand All @@ -165,7 +165,7 @@ export class Span implements SpanInterface {
}
}

// This rule conflicts with another rule :(
// This rule conflicts with another eslint rule :(
/* eslint-disable @typescript-eslint/member-ordering */

/**
Expand All @@ -175,6 +175,7 @@ export class Span implements SpanInterface {
public get name(): string {
return this._name || '';
}

/**
* Update the name of the span.
* @deprecated Use `spanToJSON(span).description` instead.
Expand Down Expand Up @@ -247,6 +248,22 @@ export class Span implements SpanInterface {
this._sampled = sampled;
}

/**
* Attributes for the span.
* @deprecated Use `getSpanAttributes(span)` instead.
*/
public get attributes(): SpanAttributes {
return this._attributes;
}

/**
* Attributes for the span.
* @deprecated Use `setAttributes()` instead.
*/
public set attributes(attributes: SpanAttributes) {
this._attributes = attributes;
}

/* eslint-enable @typescript-eslint/member-ordering */

/** @inheritdoc */
Expand Down Expand Up @@ -296,18 +313,29 @@ export class Span implements SpanInterface {
}

/**
* @inheritDoc
* Sets the tag attribute on the current span.
*
* Can also be used to unset a tag, by passing `undefined`.
*
* @param key Tag key
* @param value Tag value
* @deprecated Use `setAttribute()` instead.
*/
public setTag(key: string, value: Primitive): this {
// eslint-disable-next-line deprecation/deprecation
this.tags = { ...this.tags, [key]: value };
return this;
}

/**
* @inheritDoc
* Sets the data attribute on the current span
* @param key Data key
* @param value Data value
* @deprecated Use `setAttribute()` instead.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public setData(key: string, value: any): this {
// eslint-disable-next-line deprecation/deprecation
this.data = { ...this.data, [key]: value };
return this;
}
Expand All @@ -316,9 +344,9 @@ export class Span implements SpanInterface {
public setAttribute(key: string, value: SpanAttributeValue | undefined): void {
if (value === undefined) {
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete
delete this.attributes[key];
delete this._attributes[key];
} else {
this.attributes[key] = value;
this._attributes[key] = value;
}
}

Expand All @@ -339,7 +367,9 @@ export class Span implements SpanInterface {
* @inheritDoc
*/
public setHttpStatus(httpStatus: number): this {
// eslint-disable-next-line deprecation/deprecation
this.setTag('http.status_code', String(httpStatus));
// eslint-disable-next-line deprecation/deprecation
this.setData('http.response.status_code', httpStatus);
const spanStatus = spanStatusfromHttpCode(httpStatus);
if (spanStatus !== 'unknown_error') {
Expand Down Expand Up @@ -415,6 +445,7 @@ export class Span implements SpanInterface {
spanId: this._spanId,
startTimestamp: this.startTimestamp,
status: this.status,
// eslint-disable-next-line deprecation/deprecation
tags: this.tags,
traceId: this._traceId,
});
Expand All @@ -424,6 +455,7 @@ export class Span implements SpanInterface {
* @inheritDoc
*/
public updateWithContext(spanContext: SpanContext): this {
// eslint-disable-next-line deprecation/deprecation
this.data = spanContext.data || {};
// eslint-disable-next-line deprecation/deprecation
this._name = spanContext.name || spanContext.description;
Expand All @@ -434,6 +466,7 @@ export class Span implements SpanInterface {
this._spanId = spanContext.spanId || this._spanId;
this.startTimestamp = spanContext.startTimestamp || this.startTimestamp;
this.status = spanContext.status;
// eslint-disable-next-line deprecation/deprecation
this.tags = spanContext.tags || {};
this._traceId = spanContext.traceId || this._traceId;

Expand All @@ -459,6 +492,7 @@ export class Span implements SpanInterface {
span_id: this._spanId,
start_timestamp: this.startTimestamp,
status: this.status,
// eslint-disable-next-line deprecation/deprecation
tags: Object.keys(this.tags).length > 0 ? this.tags : undefined,
timestamp: this.endTimestamp,
trace_id: this._traceId,
Expand Down Expand Up @@ -490,7 +524,8 @@ export class Span implements SpanInterface {
[key: string]: any;
}
| undefined {
const { data, attributes } = this;
// eslint-disable-next-line deprecation/deprecation
const { data, _attributes: attributes } = this;

const hasData = Object.keys(data).length > 0;
const hasAttributes = Object.keys(attributes).length > 0;
Expand Down
12 changes: 7 additions & 5 deletions packages/core/src/tracing/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,11 @@ export class Transaction extends SpanClass implements TransactionInterface {
...this._metadata,

// From attributes
...(this.attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] && {
source: this.attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionMetadata['source'],
...(this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] && {
source: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionMetadata['source'],
}),
...(this.attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] && {
sampleRate: this.attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] as TransactionMetadata['sampleRate'],
...(this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] && {
sampleRate: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] as TransactionMetadata['sampleRate'],
}),
};
}
Expand Down Expand Up @@ -157,7 +157,8 @@ export class Transaction extends SpanClass implements TransactionInterface {
}

/**
* @inheritDoc
* Set the context of a transaction event.
* @deprecated Use either `.setAttribute()`, or set the context on the scope before creating the transaction.
*/
public setContext(key: string, context: Context | null): void {
if (context === null) {
Expand Down Expand Up @@ -334,6 +335,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
// TODO: Pass spans serialized via `spanToJSON()` here instead in v8.
spans: finishedSpans,
start_timestamp: this.startTimestamp,
// eslint-disable-next-line deprecation/deprecation
tags: this.tags,
timestamp: this.endTimestamp,
transaction: this._name,
Expand Down
11 changes: 8 additions & 3 deletions packages/core/src/utils/prepareEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { DEFAULT_ENVIRONMENT } from '../constants';
import { getGlobalEventProcessors, notifyEventProcessors } from '../eventProcessors';
import { Scope, getGlobalScope } from '../scope';
import { applyScopeDataToEvent, mergeScopeData } from './applyScopeDataToEvent';
import { spanToJSON } from './spanUtils';

/**
* This type makes sure that we get either a CaptureContext, OR an EventHint.
Expand Down Expand Up @@ -326,10 +327,14 @@ function normalizeEvent(event: Event | null, depth: number, maxBreadth: number):
// event.spans[].data may contain circular/dangerous data so we need to normalize it
if (event.spans) {
normalized.spans = event.spans.map(span => {
// We cannot use the spread operator here because `toJSON` on `span` is non-enumerable
if (span.data) {
span.data = normalize(span.data, depth, maxBreadth);
const data = spanToJSON(span).data;

if (data) {
// This is a bit weird, as we generally have `Span` instances here, but to be safe we do not assume so
// eslint-disable-next-line deprecation/deprecation
span.data = normalize(data, depth, maxBreadth);
}

return span;
});
}
Expand Down
Loading