Skip to content

Commit 655e8a7

Browse files
mydeamydea
authored
feat(core): Streamline SpanJSON type (#14693)
Previously `spanToJSON` would return `Partial<SpanJSON>`. This meant that you always had to guard all the stuff you picked from it - even though in reality, we know that certain stuff will always be there. To alleviate this, this PR changes this so that `spanToJSON` actually returns `SpanJSON`. This means that in the fallback case, we return `data: {}`, as well as a random (current) timestamp. Since we know that in reality we will only have the two scenarios that we properly handle, this is fine IMHO and makes usage of this everywhere else a little bit less painful. In a follow up, we can get rid of a bunch of `const data = spanToJSON(span).data || {}` type code. While at it, I also updated the type of `data` to `SpanAttributes`, which is correct (it was `Record<string, any>` before). Since we only allow span attributes to be set on this anyhow now, we can type this better. This also uncovered a few places with slightly "incorrect" type checks, I updated those too. This change is on the v9 branch - I think it should not really be breaking to a user in any way, as we simply return more data from `spanToJSON`, and type what `data` is on there more tightly, so no existing code relying on this should break. But to be safe, I figured we may as well do that on v9 only.
1 parent 8fa14aa commit 655e8a7

File tree

11 files changed

+81
-49
lines changed

11 files changed

+81
-49
lines changed

‎dev-packages/node-integration-tests/suites/public-api/startSpan/with-nested-spans/test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,12 @@ test('should report finished spans as children of the root transaction.', done =
3030
{
3131
description: 'span_3',
3232
parent_span_id: rootSpanId,
33+
data: {},
3334
},
3435
{
3536
description: 'span_5',
3637
parent_span_id: span3Id,
38+
data: {},
3739
},
3840
] as SpanJSON[],
3941
});

‎packages/core/src/types-hoist/span.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ export type SpanTimeInput = HrTime | number | Date;
4444

4545
/** A JSON representation of a span. */
4646
export interface SpanJSON {
47-
data?: { [key: string]: any };
47+
data: SpanAttributes;
4848
description?: string;
4949
op?: string;
5050
parent_span_id?: string;

‎packages/core/src/utils/spanUtils.ts‎

Lines changed: 27 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -112,42 +112,41 @@ function ensureTimestampInSeconds(timestamp: number): number {
112112
// Note: Because of this, we currently have a circular type dependency (which we opted out of in package.json).
113113
// This is not avoidable as we need `spanToJSON` in `spanUtils.ts`, which in turn is needed by `span.ts` for backwards compatibility.
114114
// And `spanToJSON` needs the Span class from `span.ts` to check here.
115-
export function spanToJSON(span: Span): Partial<SpanJSON> {
115+
export function spanToJSON(span: Span): SpanJSON {
116116
if (spanIsSentrySpan(span)) {
117117
return span.getSpanJSON();
118118
}
119119

120-
try {
121-
const { spanId: span_id, traceId: trace_id } = span.spanContext();
122-
123-
// Handle a span from @opentelemetry/sdk-base-trace's `Span` class
124-
if (spanIsOpenTelemetrySdkTraceBaseSpan(span)) {
125-
const { attributes, startTime, name, endTime, parentSpanId, status } = span;
126-
127-
return dropUndefinedKeys({
128-
span_id,
129-
trace_id,
130-
data: attributes,
131-
description: name,
132-
parent_span_id: parentSpanId,
133-
start_timestamp: spanTimeInputToSeconds(startTime),
134-
// This is [0,0] by default in OTEL, in which case we want to interpret this as no end time
135-
timestamp: spanTimeInputToSeconds(endTime) || undefined,
136-
status: getStatusMessage(status),
137-
op: attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP],
138-
origin: attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] as SpanOrigin | undefined,
139-
_metrics_summary: getMetricSummaryJsonForSpan(span),
140-
});
141-
}
120+
const { spanId: span_id, traceId: trace_id } = span.spanContext();
142121

143-
// Finally, at least we have `spanContext()`....
144-
return {
122+
// Handle a span from @opentelemetry/sdk-base-trace's `Span` class
123+
if (spanIsOpenTelemetrySdkTraceBaseSpan(span)) {
124+
const { attributes, startTime, name, endTime, parentSpanId, status } = span;
125+
126+
return dropUndefinedKeys({
145127
span_id,
146128
trace_id,
147-
};
148-
} catch {
149-
return {};
129+
data: attributes,
130+
description: name,
131+
parent_span_id: parentSpanId,
132+
start_timestamp: spanTimeInputToSeconds(startTime),
133+
// This is [0,0] by default in OTEL, in which case we want to interpret this as no end time
134+
timestamp: spanTimeInputToSeconds(endTime) || undefined,
135+
status: getStatusMessage(status),
136+
op: attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP],
137+
origin: attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] as SpanOrigin | undefined,
138+
_metrics_summary: getMetricSummaryJsonForSpan(span),
139+
});
150140
}
141+
142+
// Finally, at least we have `spanContext()`....
143+
// This should not actually happen in reality, but we need to handle it for type safety.
144+
return {
145+
span_id,
146+
trace_id,
147+
start_timestamp: 0,
148+
data: {},
149+
};
151150
}
152151

153152
function spanIsOpenTelemetrySdkTraceBaseSpan(span: Span): span is OpenTelemetrySdkTraceBaseSpan {

‎packages/core/test/lib/baseclient.test.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -938,14 +938,14 @@ describe('BaseClient', () => {
938938
event_id: '972f45b826a248bba98e990878a177e1',
939939
spans: [
940940
{
941-
data: { _sentry_extra_metrics: { M1: { value: 1 }, M2: { value: 2 } } },
942941
description: 'first-paint',
943942
timestamp: 1591603196.637835,
944943
op: 'paint',
945944
parent_span_id: 'a3df84a60c2e4e76',
946945
span_id: '9e15bf99fbe4bc80',
947946
start_timestamp: 1591603196.637835,
948947
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
948+
data: {},
949949
},
950950
{
951951
description: 'first-contentful-paint',
@@ -955,6 +955,7 @@ describe('BaseClient', () => {
955955
span_id: 'aa554c1f506b0783',
956956
start_timestamp: 1591603196.637835,
957957
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
958+
data: {},
958959
},
959960
],
960961
start_timestamp: 1591603196.614865,
@@ -1016,12 +1017,14 @@ describe('BaseClient', () => {
10161017
span_id: '9e15bf99fbe4bc80',
10171018
start_timestamp: 1591603196.637835,
10181019
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
1020+
data: {},
10191021
},
10201022
{
10211023
description: 'second span',
10221024
span_id: 'aa554c1f506b0783',
10231025
start_timestamp: 1591603196.637835,
10241026
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
1027+
data: {},
10251028
},
10261029
],
10271030
};
@@ -1076,9 +1079,9 @@ describe('BaseClient', () => {
10761079
transaction: '/dogs/are/great',
10771080
type: 'transaction',
10781081
spans: [
1079-
{ span_id: 'span1', trace_id: 'trace1', start_timestamp: 1234 },
1080-
{ span_id: 'span2', trace_id: 'trace1', start_timestamp: 1234 },
1081-
{ span_id: 'span3', trace_id: 'trace1', start_timestamp: 1234 },
1082+
{ span_id: 'span1', trace_id: 'trace1', start_timestamp: 1234, data: {} },
1083+
{ span_id: 'span2', trace_id: 'trace1', start_timestamp: 1234, data: {} },
1084+
{ span_id: 'span3', trace_id: 'trace1', start_timestamp: 1234, data: {} },
10821085
],
10831086
});
10841087

@@ -1107,12 +1110,14 @@ describe('BaseClient', () => {
11071110
span_id: '9e15bf99fbe4bc80',
11081111
start_timestamp: 1591603196.637835,
11091112
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
1113+
data: {},
11101114
},
11111115
{
11121116
description: 'second span',
11131117
span_id: 'aa554c1f506b0783',
11141118
start_timestamp: 1591603196.637835,
11151119
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
1120+
data: {},
11161121
},
11171122
],
11181123
};
@@ -1181,12 +1186,14 @@ describe('BaseClient', () => {
11811186
span_id: '9e15bf99fbe4bc80',
11821187
start_timestamp: 1591603196.637835,
11831188
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
1189+
data: {},
11841190
},
11851191
{
11861192
description: 'second span',
11871193
span_id: 'aa554c1f506b0783',
11881194
start_timestamp: 1591603196.637835,
11891195
trace_id: '86f39e84263a4de99c326acab3bfe3bd',
1196+
data: {},
11901197
},
11911198
],
11921199
};

‎packages/core/test/lib/tracing/sentryNonRecordingSpan.test.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ describe('SentryNonRecordingSpan', () => {
1818
expect(spanToJSON(span)).toEqual({
1919
span_id: expect.stringMatching(/[a-f0-9]{16}/),
2020
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
21+
data: {},
22+
start_timestamp: 0,
2123
});
2224

2325
// Ensure all methods work
@@ -32,6 +34,8 @@ describe('SentryNonRecordingSpan', () => {
3234
expect(spanToJSON(span)).toEqual({
3335
span_id: expect.stringMatching(/[a-f0-9]{16}/),
3436
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
37+
data: {},
38+
start_timestamp: 0,
3539
});
3640
});
3741
});

‎packages/core/test/lib/utils/spanUtils.test.ts‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,10 +287,21 @@ describe('spanToJSON', () => {
287287
});
288288
});
289289

290-
it('returns empty object for unknown span implementation', () => {
291-
const span = { other: 'other' };
292-
293-
expect(spanToJSON(span as unknown as Span)).toEqual({});
290+
it('returns minimal object for unknown span implementation', () => {
291+
const span = {
292+
// This is the minimal interface we require from a span
293+
spanContext: () => ({
294+
spanId: 'SPAN-1',
295+
traceId: 'TRACE-1',
296+
}),
297+
};
298+
299+
expect(spanToJSON(span as unknown as Span)).toEqual({
300+
span_id: 'SPAN-1',
301+
trace_id: 'TRACE-1',
302+
start_timestamp: 0,
303+
data: {},
304+
});
294305
});
295306
});
296307

‎packages/node/src/integrations/tracing/graphql.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,9 @@ export const instrumentGraphql = generateInstrumentOnce<GraphqlOptions>(
6767
// We keep track of each operation on the root span
6868
// This can either be a string, or an array of strings (if there are multiple operations)
6969
if (Array.isArray(existingOperations)) {
70-
existingOperations.push(newOperation);
70+
(existingOperations as string[]).push(newOperation);
7171
rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION, existingOperations);
72-
} else if (existingOperations) {
72+
} else if (typeof existingOperations === 'string') {
7373
rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION, [existingOperations, newOperation]);
7474
} else {
7575
rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION, newOperation);

‎packages/node/src/integrations/tracing/vercelai/index.ts‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ const _vercelAIIntegration = (() => {
3232
span.data['ai.prompt_tokens.used'] = attributes['ai.usage.promptTokens'];
3333
}
3434
if (
35-
attributes['ai.usage.completionTokens'] != undefined &&
36-
attributes['ai.usage.promptTokens'] != undefined
35+
typeof attributes['ai.usage.completionTokens'] == 'number' &&
36+
typeof attributes['ai.usage.promptTokens'] == 'number'
3737
) {
3838
span.data['ai.total_tokens.used'] =
3939
attributes['ai.usage.completionTokens'] + attributes['ai.usage.promptTokens'];
@@ -56,13 +56,13 @@ const _vercelAIIntegration = (() => {
5656
}
5757

5858
// The id of the model
59-
const aiModelId: string | undefined = attributes['ai.model.id'];
59+
const aiModelId = attributes['ai.model.id'];
6060

6161
// the provider of the model
62-
const aiModelProvider: string | undefined = attributes['ai.model.provider'];
62+
const aiModelProvider = attributes['ai.model.provider'];
6363

6464
// both of these must be defined for the integration to work
65-
if (!aiModelId || !aiModelProvider) {
65+
if (typeof aiModelId !== 'string' || typeof aiModelProvider !== 'string' || !aiModelId || !aiModelProvider) {
6666
return;
6767
}
6868

@@ -137,9 +137,10 @@ const _vercelAIIntegration = (() => {
137137
span.updateName(nameWthoutAi);
138138

139139
// If a Telemetry name is set and it is a pipeline span, use that as the operation name
140-
if (attributes['ai.telemetry.functionId'] && isPipelineSpan) {
141-
span.updateName(attributes['ai.telemetry.functionId']);
142-
span.setAttribute('ai.pipeline.name', attributes['ai.telemetry.functionId']);
140+
const functionId = attributes['ai.telemetry.functionId'];
141+
if (functionId && typeof functionId === 'string' && isPipelineSpan) {
142+
span.updateName(functionId);
143+
span.setAttribute('ai.pipeline.name', functionId);
143144
}
144145

145146
if (attributes['ai.prompt']) {

‎packages/opentelemetry/src/propagator.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ function getCurrentURL(span: Span): string | undefined {
316316
// `ATTR_URL_FULL` is the new attribute, but we still support the old one, `SEMATTRS_HTTP_URL`, for now.
317317
// eslint-disable-next-line deprecation/deprecation
318318
const urlAttribute = spanData?.[SEMATTRS_HTTP_URL] || spanData?.[ATTR_URL_FULL];
319-
if (urlAttribute) {
319+
if (typeof urlAttribute === 'string') {
320320
return urlAttribute;
321321
}
322322

‎packages/opentelemetry/test/trace.test.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1565,6 +1565,8 @@ describe('continueTrace', () => {
15651565
expect(spanToJSON(span)).toEqual({
15661566
span_id: '1121201211212012',
15671567
trace_id: '12312012123120121231201212312012',
1568+
data: {},
1569+
start_timestamp: 0,
15681570
});
15691571
expect(getSamplingDecision(span.spanContext())).toBe(false);
15701572
expect(spanIsSampled(span)).toBe(false);
@@ -1596,6 +1598,8 @@ describe('continueTrace', () => {
15961598
expect(spanToJSON(span)).toEqual({
15971599
span_id: '1121201211212012',
15981600
trace_id: '12312012123120121231201212312012',
1601+
data: {},
1602+
start_timestamp: 0,
15991603
});
16001604
expect(getSamplingDecision(span.spanContext())).toBe(true);
16011605
expect(spanIsSampled(span)).toBe(true);
@@ -1630,6 +1634,8 @@ describe('continueTrace', () => {
16301634
expect(spanToJSON(span)).toEqual({
16311635
span_id: '1121201211212012',
16321636
trace_id: '12312012123120121231201212312012',
1637+
data: {},
1638+
start_timestamp: 0,
16331639
});
16341640
expect(getSamplingDecision(span.spanContext())).toBe(true);
16351641
expect(spanIsSampled(span)).toBe(true);

0 commit comments

Comments
 (0)