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): Deprecate Span.op in favor of op attribute #10189

Merged
merged 6 commits into from
Jan 17, 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 @@ -205,6 +205,7 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
- `span.transaction`: Use `getRootSpan` utility function instead.
- `span.spanRecorder`: Span recording will be handled internally by the SDK.
- `span.status`: Use `.setStatus` to set or update and `spanToJSON()` to read the span status.
- `span.op`: Use `startSpan` functions to set, `setAttribute()` to update and `spanToJSON` to read the span operation.
- `transaction.setMetadata()`: Use attributes instead, or set data on the scope.
- `transaction.metadata`: Use attributes instead, or set data on the scope.
- `transaction.setContext()`: Set context on the surrounding scope instead.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,13 @@ test('Should trace outgoing fetch requests inside middleware and create breadcru
expect(middlewareTransaction.spans).toEqual(
expect.arrayContaining([
{
data: { 'http.method': 'GET', 'http.response.status_code': 200, type: 'fetch', url: 'http://localhost:3030/' },
data: {
'http.method': 'GET',
'http.response.status_code': 200,
type: 'fetch',
url: 'http://localhost:3030/',
'sentry.op': 'http.client',
},
description: 'GET http://localhost:3030/',
op: 'http.client',
origin: 'auto.http.wintercg_fetch',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ test('Propagates trace for outgoing http requests', async ({ baseURL }) => {
url: 'http://localhost:3030/test-outgoing-http',
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
},
op: 'http.server',
span_id: expect.any(String),
Expand All @@ -85,6 +86,7 @@ test('Propagates trace for outgoing http requests', async ({ baseURL }) => {
url: 'http://localhost:3030/test-inbound-headers',
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
},
op: 'http.server',
parent_span_id: outgoingHttpSpanId,
Expand Down Expand Up @@ -156,6 +158,7 @@ test('Propagates trace for outgoing fetch requests', async ({ baseURL }) => {
url: 'http://localhost:3030/test-outgoing-fetch',
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
},
op: 'http.server',
span_id: expect.any(String),
Expand All @@ -178,6 +181,7 @@ test('Propagates trace for outgoing fetch requests', async ({ baseURL }) => {
url: 'http://localhost:3030/test-inbound-headers',
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
},
op: 'http.server',
parent_span_id: outgoingHttpSpanId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
url: 'http://localhost:3030/test-transaction',
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
},
op: 'http.server',
span_id: expect.any(String),
Expand Down
5 changes: 5 additions & 0 deletions packages/core/src/semanticAttributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,8 @@ export const SEMANTIC_ATTRIBUTE_SENTRY_SOURCE = 'sentry.source';
* Use this attribute to represent the sample rate used for a span.
*/
export const SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE = 'sentry.sample_rate';

/**
* Use this attribute to represent the operation of a span.
*/
export const SEMANTIC_ATTRIBUTE_SENTRY_OP = 'sentry.op';
2 changes: 2 additions & 0 deletions packages/core/src/tracing/idletransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,15 @@ export class IdleTransaction extends Transaction {
this._finished = true;
this.activities = {};

// eslint-disable-next-line deprecation/deprecation
if (this.op === 'ui.action.click') {
this.setAttribute(FINISH_REASON_TAG, this._finishReason);
}

// eslint-disable-next-line deprecation/deprecation
if (this.spanRecorder) {
DEBUG_BUILD &&
// eslint-disable-next-line deprecation/deprecation
logger.log('[Tracing] finishing IdleTransaction', new Date(endTimestampInS * 1000).toISOString(), this.op);

for (const callback of this._beforeFinishCallbacks) {
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tracing/sampling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export function sampleTransaction<T extends Transaction>(
}

DEBUG_BUILD &&
// eslint-disable-next-line deprecation/deprecation
logger.log(`[Tracing] starting ${transaction.op} transaction - ${spanToJSON(transaction).description}`);
return transaction;
}
Expand Down
31 changes: 24 additions & 7 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {
import { dropUndefinedKeys, logger, timestampInSeconds, uuid4 } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
import { SEMANTIC_ATTRIBUTE_SENTRY_OP } from '../semanticAttributes';
import { getRootSpan } from '../utils/getRootSpan';
import {
TRACE_FLAG_NONE,
Expand Down Expand Up @@ -67,11 +68,6 @@ export class Span implements SpanInterface {
*/
public parentSpanId?: string;

/**
* @inheritDoc
*/
public op?: string;

/**
* Tags for the span.
* @deprecated Use `getSpanAttributes(span)` instead.
Expand Down Expand Up @@ -158,7 +154,7 @@ export class Span implements SpanInterface {
this._sampled = spanContext.sampled;
}
if (spanContext.op) {
this.op = spanContext.op;
this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, spanContext.op);
}
if (spanContext.status) {
this._status = spanContext.status;
Expand Down Expand Up @@ -317,6 +313,25 @@ export class Span implements SpanInterface {
this._status = status;
}

/**
* Operation of the span
*
* @deprecated Use `spanToJSON().op` to read the op instead.
*/
public get op(): string | undefined {
return this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] as string | undefined;
}

/**
* Operation of the span
*
* @deprecated Use `startSpan()` functions to set or `span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'op')
* to update the span instead.
*/
public set op(op: string | undefined) {
this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, op);
}

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

/** @inheritdoc */
Expand Down Expand Up @@ -512,6 +527,7 @@ export class Span implements SpanInterface {
data: this._getData(),
description: this._name,
endTimestamp: this._endTime,
// eslint-disable-next-line deprecation/deprecation
op: this.op,
parentSpanId: this.parentSpanId,
sampled: this._sampled,
Expand All @@ -535,6 +551,7 @@ export class Span implements SpanInterface {
// eslint-disable-next-line deprecation/deprecation
this._name = spanContext.name || spanContext.description;
this._endTime = spanContext.endTimestamp;
// eslint-disable-next-line deprecation/deprecation
this.op = spanContext.op;
this.parentSpanId = spanContext.parentSpanId;
this._sampled = spanContext.sampled;
Expand Down Expand Up @@ -569,7 +586,7 @@ export class Span implements SpanInterface {
return dropUndefinedKeys({
data: this._getData(),
description: this._name,
op: this.op,
op: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] as string | undefined,
parent_span_id: this.parentSpanId,
span_id: this._spanId,
start_timestamp: this._startTime,
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tracing/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
transaction.measurements = this._measurements;
}

// eslint-disable-next-line deprecation/deprecation
DEBUG_BUILD && logger.log(`[Tracing] Finishing ${this.op} transaction: ${this._name}.`);

return transaction;
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/lib/utils/spanUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ describe('spanToJSON', () => {
origin: 'auto',
start_timestamp: 123,
timestamp: 456,
data: {
'sentry.op': 'test op',
},
Comment on lines +90 to +92
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that spanToJson sets the op also in data because it's saved as an attribute. Should we change this in the spanToJson implementation? I'd imagine, this would just be duplicated data in the product as we still need the top level event.op field anyway.
thoughts @mydea @AbhiPrasad?

Copy link
Member

Choose a reason for hiding this comment

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

I would leave it in, I think - rather if we don't want to send this, we can/should remove it wherever we generate the event 🤔 eventually (after v8? let's see..) I think any special handling of op & origin should go away and they should only be kept as attributes everywhere. But for now we can just keep both I believe...?

Copy link
Member Author

@Lms24 Lms24 Jan 16, 2024

Choose a reason for hiding this comment

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

eventually (after v8? let's see..) I think any special handling of op & origin should go away and they should only be kept as attributes everywhere

Agreed, that makes sense to me, thanks! Ok, I'll leave it in then and adjust the tests to accomodate the new behaviour. Worth pointing out that this isn't breaking anything, it's just adding a new data item anyhow.

});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ test('should correctly instrument `fetch` for performance tracing', async ({ pag
type: 'fetch',
'http.response_content_length': expect.any(Number),
'http.response.status_code': 200,
'sentry.op': 'http.client',
},
description: 'GET http://example.com',
op: 'http.client',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ describe('Integration | Transactions', () => {
},
runtime: { name: 'node', version: expect.any(String) },
trace: {
data: { 'otel.kind': 'INTERNAL' },
data: { 'otel.kind': 'INTERNAL', 'sentry.op': 'test op' },
op: 'test op',
span_id: expect.any(String),
status: 'ok',
Expand Down Expand Up @@ -244,7 +244,7 @@ describe('Integration | Transactions', () => {
},
}),
trace: {
data: { 'otel.kind': 'INTERNAL' },
data: { 'otel.kind': 'INTERNAL', 'sentry.op': 'test op' },
op: 'test op',
span_id: expect.any(String),
status: 'ok',
Expand Down Expand Up @@ -286,7 +286,7 @@ describe('Integration | Transactions', () => {
},
}),
trace: {
data: { 'otel.kind': 'INTERNAL' },
data: { 'otel.kind': 'INTERNAL', 'sentry.op': 'test op b' },
op: 'test op b',
span_id: expect.any(String),
status: 'ok',
Expand Down Expand Up @@ -363,7 +363,7 @@ describe('Integration | Transactions', () => {
attributes: {},
}),
trace: {
data: { 'otel.kind': 'INTERNAL' },
data: { 'otel.kind': 'INTERNAL', 'sentry.op': 'test op' },
op: 'test op',
span_id: expect.any(String),
parent_span_id: parentSpanId,
Expand Down
8 changes: 8 additions & 0 deletions packages/node/test/integrations/http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ describe('tracing', () => {

// our span is at index 1 because the transaction itself is at index 0
expect(spanToJSON(spans[1]).description).toEqual('GET http://dogs.are.great/');
// eslint-disable-next-line deprecation/deprecation
expect(spans[1].op).toEqual('http.client');
expect(spanToJSON(spans[1]).op).toEqual('http.client');
});

it("doesn't create a span for outgoing sentry requests", () => {
Expand Down Expand Up @@ -283,7 +285,9 @@ describe('tracing', () => {

// our span is at index 1 because the transaction itself is at index 0
expect(spanToJSON(spans[1]).description).toEqual('GET http://dogs.are.great/spaniel');
// eslint-disable-next-line deprecation/deprecation
expect(spans[1].op).toEqual('http.client');
expect(spanToJSON(spans[1]).op).toEqual('http.client');

const spanAttributes = spanToJSON(spans[1]).data || {};

Expand All @@ -308,7 +312,9 @@ describe('tracing', () => {

// our span is at index 1 because the transaction itself is at index 0
expect(spanToJSON(spans[1]).description).toEqual('GET http://dogs.are.great/spaniel');
// eslint-disable-next-line deprecation/deprecation
expect(spans[1].op).toEqual('http.client');
expect(spanToJSON(spans[1]).op).toEqual('http.client');
expect(spanAttributes['http.method']).toEqual('GET');
expect(spanAttributes.url).toEqual('http://dogs.are.great/spaniel');
expect(spanAttributes['http.query']).toEqual('tail=wag&cute=true');
Expand Down Expand Up @@ -391,6 +397,7 @@ describe('tracing', () => {
const request = http.get(url);

// There should be no http spans
// eslint-disable-next-line deprecation/deprecation
const httpSpans = spans.filter(span => span.op?.startsWith('http'));
expect(httpSpans.length).toBe(0);

Expand Down Expand Up @@ -497,6 +504,7 @@ describe('tracing', () => {
const request = http.get(url);

// There should be no http spans
// eslint-disable-next-line deprecation/deprecation
const httpSpans = spans.filter(span => span.op?.startsWith('http'));
expect(httpSpans.length).toBe(0);

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 @@ -3,6 +3,7 @@ import { SpanKind, context, trace } from '@opentelemetry/api';
import { suppressTracing } from '@opentelemetry/core';
import type { Span as OtelSpan, SpanProcessor as OtelSpanProcessor } from '@opentelemetry/sdk-trace-base';
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
Transaction,
addEventProcessor,
Expand Down Expand Up @@ -221,7 +222,7 @@ function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelS

transaction.setStatus(mapOtelStatus(otelSpan));

transaction.op = op;
transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, op);
transaction.updateName(description);
transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source);
}
Expand Down
12 changes: 10 additions & 2 deletions packages/opentelemetry-node/test/spanprocessor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,7 @@ describe('SentrySpanProcessor', () => {
'http.url': 'http://example.com/my/route/123',
'otel.kind': 'INTERNAL',
url: 'http://example.com/my/route/123',
'sentry.op': 'http',
});

parentOtelSpan.end();
Expand All @@ -580,7 +581,7 @@ describe('SentrySpanProcessor', () => {

child.end();

const { description, data } = spanToJSON(sentrySpan!);
const { description, data, op } = spanToJSON(sentrySpan!);

expect(description).toBe('GET http://example.com/my/route/123');
expect(data).toEqual({
Expand All @@ -589,7 +590,9 @@ describe('SentrySpanProcessor', () => {
'http.url': 'http://example.com/my/route/123',
'otel.kind': 'INTERNAL',
url: 'http://example.com/my/route/123',
'sentry.op': 'http',
});
expect(op).toBe('http');

parentOtelSpan.end();
});
Expand All @@ -609,7 +612,7 @@ describe('SentrySpanProcessor', () => {

child.end();

const { description, data } = spanToJSON(sentrySpan!);
const { description, data, op } = spanToJSON(sentrySpan!);

expect(description).toBe('GET http://example.com/my/route/123');
expect(data).toEqual({
Expand All @@ -620,7 +623,9 @@ describe('SentrySpanProcessor', () => {
url: 'http://example.com/my/route/123',
'http.query': '?what=123',
'http.fragment': '#myHash',
'sentry.op': 'http',
});
expect(op).toBe('http');

parentOtelSpan.end();
});
Expand Down Expand Up @@ -780,7 +785,10 @@ describe('SentrySpanProcessor', () => {
parentOtelSpan.setAttribute(SemanticAttributes.FAAS_TRIGGER, 'test faas trigger');
parentOtelSpan.end();

// eslint-disable-next-line deprecation/deprecation
expect(transaction.op).toBe('test faas trigger');
expect(spanToJSON(transaction).op).toBe('test faas trigger');

expect(spanToJSON(transaction).description).toBe('test operation');
});
});
Expand Down
Loading