Skip to content

feat(core): Deprecate Span.origin in favor of sentry.origin attribute #10260

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 6 commits into from
Jan 19, 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 @@ -24,11 +24,11 @@ sentryTest('should report finished spans as children of the root transaction', a
const url = await getLocalTestPath({ testDir: __dirname });
const transaction = await getFirstSentryEnvelopeRequest<SerializedEvent>(page, url);

const rootSpanId = transaction?.contexts?.trace?.span_id;

expect(transaction.spans).toHaveLength(1);

const span_1 = transaction.spans?.[0];
expect(span_1?.description).toBe('child_span');
expect(span_1?.parent_span_id).toEqual(rootSpanId);
expect(span_1?.parent_span_id).toEqual(transaction?.contexts?.trace?.span_id);
expect(span_1?.origin).toEqual('manual');
expect(span_1?.data?.['sentry.origin']).toEqual('manual');
});
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ test('Should trace outgoing fetch requests inside middleware and create breadcru
type: 'fetch',
url: 'http://localhost:3030/',
'sentry.op': 'http.client',
'sentry.origin': 'auto.http.wintercg_fetch',
},
description: 'GET http://localhost:3030/',
op: 'http.client',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ test('Propagates trace for outgoing http requests', async ({ baseURL }) => {
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
},
op: 'http.server',
span_id: expect.any(String),
Expand All @@ -87,6 +88,7 @@ test('Propagates trace for outgoing http requests', async ({ baseURL }) => {
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
},
op: 'http.server',
parent_span_id: outgoingHttpSpanId,
Expand Down Expand Up @@ -159,6 +161,7 @@ test('Propagates trace for outgoing fetch requests', async ({ baseURL }) => {
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
},
op: 'http.server',
span_id: expect.any(String),
Expand All @@ -182,6 +185,7 @@ test('Propagates trace for outgoing fetch requests', async ({ baseURL }) => {
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
},
op: 'http.server',
parent_span_id: outgoingHttpSpanId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
},
op: 'http.server',
span_id: expect.any(String),
Expand All @@ -48,6 +49,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
'fastify.type': 'request_handler',
'http.route': '/test-transaction',
'otel.kind': 'INTERNAL',
'sentry.origin': 'auto.http.otel.fastify',
},
description: 'request handler - fastify -> app-auto-0',
parent_span_id: expect.any(String),
Expand All @@ -61,6 +63,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
{
data: {
'otel.kind': 'INTERNAL',
'sentry.origin': 'manual',
},
description: 'test-span',
parent_span_id: expect.any(String),
Expand All @@ -74,6 +77,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
{
data: {
'otel.kind': 'INTERNAL',
'sentry.origin': 'manual',
},
description: 'child-span',
parent_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 @@ -14,3 +14,8 @@ 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';

/**
* Use this attribute to represent the origin of a span.
*/
export const SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN = 'sentry.origin';
31 changes: 23 additions & 8 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +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 { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../semanticAttributes';
import { getRootSpan } from '../utils/getRootSpan';
import {
TRACE_FLAG_NONE,
Expand Down Expand Up @@ -100,11 +100,6 @@ export class Span implements SpanInterface {
*/
public instrumenter: Instrumenter;

/**
* The origin of the span, giving context about what created the span.
*/
public origin?: SpanOrigin;

protected _traceId: string;
protected _spanId: string;
protected _parentSpanId?: string;
Expand Down Expand Up @@ -138,7 +133,9 @@ export class Span implements SpanInterface {
this._attributes = spanContext.attributes ? { ...spanContext.attributes } : {};
// eslint-disable-next-line deprecation/deprecation
this.instrumenter = spanContext.instrumenter || 'sentry';
this.origin = spanContext.origin || 'manual';

this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, spanContext.origin || 'manual');

// eslint-disable-next-line deprecation/deprecation
this._name = spanContext.name || spanContext.description;

Expand Down Expand Up @@ -346,6 +343,24 @@ export class Span implements SpanInterface {
this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, op);
}

/**
* The origin of the span, giving context about what created the span.
*
* @deprecated Use `spanToJSON().origin` to read the origin instead.
*/
public get origin(): SpanOrigin | undefined {
return this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] as SpanOrigin | undefined;
}

/**
* The origin of the span, giving context about what created the span.
*
* @deprecated Use `startSpan()` functions to set the origin instead.
*/
public set origin(origin: SpanOrigin | undefined) {
this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, origin);
}

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

/** @inheritdoc */
Expand Down Expand Up @@ -611,7 +626,7 @@ export class Span implements SpanInterface {
tags: Object.keys(this.tags).length > 0 ? this.tags : undefined,
timestamp: this._endTime,
trace_id: this._traceId,
origin: this.origin,
origin: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] as SpanOrigin | undefined,
});
}

Expand Down
51 changes: 40 additions & 11 deletions packages/core/test/lib/tracing/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ describe('span', () => {
strArray: ['aa', 'bb'],
boolArray: [true, false],
arrayWithUndefined: [1, undefined, 2],
// origin is set by default to 'manual' in the Span constructor
'sentry.origin': 'manual',
});
});

Expand All @@ -92,11 +94,12 @@ describe('span', () => {

span.setAttribute('str', 'bar');

expect(Object.keys(span['_attributes']).length).toEqual(1);
// length 2 because `sentry.origin` is always set by default
expect(Object.keys(span['_attributes']).length).toEqual(2);

span.setAttribute('str', undefined);

expect(Object.keys(span['_attributes']).length).toEqual(0);
expect(Object.keys(span['_attributes']).length).toEqual(1);
});

it('disallows invalid attribute types', () => {
Expand All @@ -119,7 +122,10 @@ describe('span', () => {

const initialAttributes = span['_attributes'];

expect(initialAttributes).toEqual({});
expect(initialAttributes).toEqual({
// origin is set by default to 'manual' in the Span constructor
'sentry.origin': 'manual',
});

const newAttributes = {
str: 'bar',
Expand All @@ -145,6 +151,7 @@ describe('span', () => {
strArray: ['aa', 'bb'],
boolArray: [true, false],
arrayWithUndefined: [1, undefined, 2],
'sentry.origin': 'manual',
});

expect(span['_attributes']).not.toBe(newAttributes);
Expand All @@ -164,6 +171,7 @@ describe('span', () => {
strArray: ['aa', 'bb'],
boolArray: [true, false],
arrayWithUndefined: [1, undefined, 2],
'sentry.origin': 'manual',
});
});

Expand All @@ -172,11 +180,12 @@ describe('span', () => {

span.setAttribute('str', 'bar');

expect(Object.keys(span['_attributes']).length).toEqual(1);
// length 2 because `sentry.origin` is always set by default
expect(Object.keys(span['_attributes']).length).toEqual(2);

span.setAttributes({ str: undefined });

expect(Object.keys(span['_attributes']).length).toEqual(0);
expect(Object.keys(span['_attributes']).length).toEqual(1);
});
});

Expand Down Expand Up @@ -275,24 +284,38 @@ describe('span', () => {
it('works without data & attributes', () => {
const span = new Span();

expect(span['_getData']()).toEqual(undefined);
expect(span['_getData']()).toEqual({
// origin is set by default to 'manual' in the Span constructor
'sentry.origin': 'manual',
});
});

it('works with data only', () => {
const span = new Span();
// eslint-disable-next-line deprecation/deprecation
span.setData('foo', 'bar');

expect(span['_getData']()).toEqual({ foo: 'bar' });
// eslint-disable-next-line deprecation/deprecation
expect(span['_getData']()).toBe(span.data);
expect(span['_getData']()).toEqual({
foo: 'bar',
// origin is set by default to 'manual' in the Span constructor
'sentry.origin': 'manual',
});
expect(span['_getData']()).toStrictEqual({
// eslint-disable-next-line deprecation/deprecation
...span.data,
'sentry.origin': 'manual',
});
});

it('works with attributes only', () => {
const span = new Span();
span.setAttribute('foo', 'bar');

expect(span['_getData']()).toEqual({ foo: 'bar' });
expect(span['_getData']()).toEqual({
foo: 'bar',
// origin is set by default to 'manual' in the Span constructor
'sentry.origin': 'manual',
});
// eslint-disable-next-line deprecation/deprecation
expect(span['_getData']()).toBe(span.attributes);
});
Expand All @@ -306,7 +329,13 @@ describe('span', () => {
// eslint-disable-next-line deprecation/deprecation
span.setData('baz', 'baz');

expect(span['_getData']()).toEqual({ foo: 'foo', bar: 'bar', baz: 'baz' });
expect(span['_getData']()).toEqual({
foo: 'foo',
bar: 'bar',
baz: 'baz',
// origin is set by default to 'manual' in the Span constructor
'sentry.origin': 'manual',
});
// eslint-disable-next-line deprecation/deprecation
expect(span['_getData']()).not.toBe(span.attributes);
// eslint-disable-next-line deprecation/deprecation
Expand Down
4 changes: 4 additions & 0 deletions packages/core/test/lib/utils/spanUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ describe('spanToJSON', () => {
trace_id: span.spanContext().traceId,
origin: 'manual',
start_timestamp: span['_startTime'],
data: {
'sentry.origin': 'manual',
},
});
});

Expand Down Expand Up @@ -89,6 +92,7 @@ describe('spanToJSON', () => {
timestamp: 456,
data: {
'sentry.op': 'test op',
'sentry.origin': 'auto',
},
});
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, test } from '@playwright/test';
import { Transaction } from '@sentry/types';
import { SerializedEvent } from '@sentry/types';
import { countEnvelopes, getMultipleSentryEnvelopeRequests } from './utils/helpers';

test('should correctly instrument `fetch` for performance tracing', async ({ page }) => {
Expand All @@ -12,7 +12,7 @@ test('should correctly instrument `fetch` for performance tracing', async ({ pag
});
});

const transaction = await getMultipleSentryEnvelopeRequests<Transaction>(page, 1, {
const transaction = await getMultipleSentryEnvelopeRequests<SerializedEvent>(page, 1, {
url: '/fetch',
envelopeType: 'transaction',
});
Expand All @@ -27,7 +27,6 @@ test('should correctly instrument `fetch` for performance tracing', async ({ pag
},
});

// @ts-expect-error - We know that `spans` is inside Transaction envelopes
expect(transaction[0].spans).toEqual(
expect.arrayContaining([
expect.objectContaining({
Expand All @@ -38,6 +37,7 @@ test('should correctly instrument `fetch` for performance tracing', async ({ pag
'http.response_content_length': expect.any(Number),
'http.response.status_code': 200,
'sentry.op': 'http.client',
'sentry.origin': 'auto.http.browser',
},
description: 'GET http://example.com',
op: 'http.client',
Expand All @@ -47,6 +47,7 @@ test('should correctly instrument `fetch` for performance tracing', async ({ pag
timestamp: expect.any(Number),
trace_id: expect.any(String),
status: expect.any(String),
origin: 'auto.http.browser',
}),
]),
);
Expand Down
2 changes: 1 addition & 1 deletion packages/node-experimental/test/integration/scope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ describe('Integration | Scope', () => {
expect.objectContaining({
contexts: expect.objectContaining({
trace: {
data: { 'otel.kind': 'INTERNAL' },
data: { 'otel.kind': 'INTERNAL', 'sentry.origin': 'manual' },
span_id: spanId,
status: 'ok',
trace_id: traceId,
Expand Down
Loading