Skip to content

Commit 110f6c4

Browse files
authored
ref(core): Ensure error span status is always valid (#22522)
We have some code in otel span serialization that ensures that span statuses are correct. if we remove this layer, however we can "invalid" span statuses (e.g. random error messages etc). This change ensures we always have valid span statuses everywhere, in the legacy transaction mode.
1 parent fe90ccd commit 110f6c4

11 files changed

Lines changed: 69 additions & 50 deletions

File tree

packages/core/src/tracing/index.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,14 @@ export { startIdleSpan, TRACING_DEFAULTS } from './idleSpan';
1313
export { SentrySpan } from './sentrySpan';
1414
export { _INTERNAL_setDeferSegmentSpanCapture } from './deferSegmentSpanCapture';
1515
export { SentryNonRecordingSpan } from './sentryNonRecordingSpan';
16-
export { setHttpStatus, getSpanStatusFromHttpCode } from './spanstatus';
17-
export { SPAN_STATUS_ERROR, SPAN_STATUS_OK, SPAN_STATUS_UNSET } from './spanstatus';
16+
export {
17+
setHttpStatus,
18+
getSpanStatusFromHttpCode,
19+
isStatusErrorMessageValid,
20+
SPAN_STATUS_ERROR,
21+
SPAN_STATUS_OK,
22+
SPAN_STATUS_UNSET,
23+
} from './spanstatus';
1824
export {
1925
startSpan,
2026
startInactiveSpan,

packages/core/src/tracing/spanstatus.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,15 @@
11
import type { Span } from '../types/span';
2-
import type { SpanStatus } from '../types/spanStatus';
2+
import type { SpanStatusType } from '../types/spanStatus';
3+
import { SPAN_STATUS_TYPES, type SpanStatus } from '../types/spanStatus';
34

45
export const SPAN_STATUS_UNSET = 0;
56
export const SPAN_STATUS_OK = 1;
67
export const SPAN_STATUS_ERROR = 2;
78

9+
export function isStatusErrorMessageValid(message: string): boolean {
10+
return message !== 'ok' && SPAN_STATUS_TYPES.includes(message as SpanStatusType);
11+
}
12+
813
/**
914
* Converts a HTTP status code into a sentry status with a message.
1015
*

packages/core/src/types/spanStatus.ts

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,41 @@
1-
export type SpanStatusType =
1+
export const SPAN_STATUS_TYPES = [
22
/** The operation completed successfully. */
3-
| 'ok'
3+
'ok',
44
/** Deadline expired before operation could complete. */
5-
| 'deadline_exceeded'
5+
'deadline_exceeded',
66
/** 401 Unauthorized (actually does mean unauthenticated according to RFC 7235) */
7-
| 'unauthenticated'
7+
'unauthenticated',
88
/** 403 Forbidden */
9-
| 'permission_denied'
9+
'permission_denied',
1010
/** 404 Not Found. Some requested entity (file or directory) was not found. */
11-
| 'not_found'
11+
'not_found',
1212
/** 429 Too Many Requests */
13-
| 'resource_exhausted'
13+
'resource_exhausted',
1414
/** Client specified an invalid argument. 4xx. */
15-
| 'invalid_argument'
15+
'invalid_argument',
1616
/** 501 Not Implemented */
17-
| 'unimplemented'
17+
'unimplemented',
1818
/** 503 Service Unavailable */
19-
| 'unavailable'
19+
'unavailable',
2020
/** Other/generic 5xx. */
21-
| 'internal_error'
21+
'internal_error',
2222
/** Unknown. Any non-standard HTTP status code. */
23-
| 'unknown_error'
23+
'unknown_error',
2424
/** The operation was cancelled (typically by the user). */
25-
| 'cancelled'
25+
'cancelled',
2626
/** Already exists (409) */
27-
| 'already_exists'
27+
'already_exists',
2828
/** Operation was rejected because the system is not in a state required for the operation's */
29-
| 'failed_precondition'
29+
'failed_precondition',
3030
/** The operation was aborted, typically due to a concurrency issue. */
31-
| 'aborted'
31+
'aborted',
3232
/** Operation was attempted past the valid range. */
33-
| 'out_of_range'
33+
'out_of_range',
3434
/** Unrecoverable data loss or corruption */
35-
| 'data_loss';
35+
'data_loss',
36+
] as const;
37+
38+
export type SpanStatusType = (typeof SPAN_STATUS_TYPES)[number];
3639

3740
// These are aligned with OpenTelemetry span status codes
3841
const SPAN_STATUS_UNSET = 0;

packages/core/src/utils/spanUtils.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {
1212
SEMANTIC_ATTRIBUTE_SENTRY_STATUS_MESSAGE,
1313
} from '../semanticAttributes';
1414
import type { SentrySpan } from '../tracing/sentrySpan';
15-
import { SPAN_STATUS_OK, SPAN_STATUS_UNSET } from '../tracing/spanstatus';
15+
import { isStatusErrorMessageValid, SPAN_STATUS_OK, SPAN_STATUS_UNSET } from '../tracing/spanstatus';
1616
import { getCapturedScopesOnSpan } from '../tracing/utils';
1717
import type { TraceContext } from '../types/context';
1818
import type { SpanLink, SpanLinkJSON } from '../types/link';
@@ -327,7 +327,7 @@ export function getStatusMessage(status: SpanStatus | undefined): string {
327327
return 'ok';
328328
}
329329

330-
return status.message || 'internal_error';
330+
return status.message && isStatusErrorMessageValid(status.message) ? status.message : 'internal_error';
331331
}
332332

333333
/**

packages/core/test/lib/tracing/sentrySpan.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ describe('SentrySpan', () => {
143143
describe('tracer-provider span sealing', () => {
144144
it('seals a tracer-provider span against all mutation after it ends', () => {
145145
const span = new SentrySpan({ name: 'original', startTimestamp: 1, attributes: { key: 'before' } });
146-
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'before' });
146+
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'permission_denied' });
147147
span.addEvent('measurement', {
148148
[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE]: 1,
149149
[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT]: 'millisecond',
@@ -156,7 +156,7 @@ describe('SentrySpan', () => {
156156
// Every mutator must no-op on a tracer-provider span once it has ended, mirroring OTel SDK spans.
157157
span.setAttribute('key', 'after');
158158
span.setAttributes({ key2: 'after' });
159-
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'after' });
159+
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'already_exists' });
160160
span.updateName('after');
161161
span.updateStartTime(999);
162162
span.addLink({ context: linked.spanContext() });
@@ -169,7 +169,7 @@ describe('SentrySpan', () => {
169169
const json = spanToJSON(span);
170170
expect(json.data?.['key']).toBe('before');
171171
expect(json.data?.['key2']).toBeUndefined();
172-
expect(json.status).toBe('before');
172+
expect(json.status).toBe('permission_denied');
173173
expect(json.description).toBe('original');
174174
expect(json.start_timestamp).toBe(1);
175175
expect(json.links).toBeUndefined();

packages/deno/test/deno-redis.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ Deno.test('denoRedisIntegration: errors on the command channel set span status',
132132
// as `status: 'X'` (the message takes the slot). Both "not ok" and the
133133
// forwarded message confirm the error path fired.
134134
assert(redisSpan!.status && redisSpan!.status !== 'ok', `expected error-shaped status, got ${redisSpan!.status}`);
135-
assertEquals(redisSpan!.status, 'ECONNREFUSED');
135+
assertEquals(redisSpan!.status, 'internal_error');
136136
});
137137

138138
Deno.test('denoRedisIntegration: ioredis:command channel produces a db.redis child span', async () => {

packages/opentelemetry/src/applyOtelSpanData.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,10 @@ import {
1010
spanToJSON,
1111
SPAN_STATUS_ERROR,
1212
SPAN_STATUS_OK,
13+
isStatusErrorMessageValid,
1314
} from '@sentry/core';
1415
import type { Span, SpanAttributes } from '@sentry/core';
15-
import { inferStatusFromAttributes, isStatusErrorMessageValid } from './utils/mapStatus';
16+
import { inferStatusFromAttributes } from './utils/mapStatus';
1617
import { inferSpanData } from './utils/parseSpanDescription';
1718

1819
/**

packages/opentelemetry/src/utils/mapStatus.ts

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { SpanStatusCode } from '@opentelemetry/api';
22
import { HTTP_RESPONSE_STATUS_CODE, HTTP_STATUS_CODE, RPC_GRPC_STATUS_CODE } from '@sentry/conventions/attributes';
33
import type { SpanAttributes, SpanStatus } from '@sentry/core';
4-
import { getSpanStatusFromHttpCode, SPAN_STATUS_ERROR, SPAN_STATUS_OK } from '@sentry/core';
4+
import { getSpanStatusFromHttpCode, isStatusErrorMessageValid, SPAN_STATUS_ERROR, SPAN_STATUS_OK } from '@sentry/core';
55
import type { AbstractSpan } from '../types';
66
import { spanHasAttributes, spanHasStatus } from './spanTypes';
77

@@ -25,10 +25,6 @@ const canonicalGrpcErrorCodesMap: Record<string, SpanStatus['message']> = {
2525
'16': 'unauthenticated',
2626
} as const;
2727

28-
export const isStatusErrorMessageValid = (message: string): boolean => {
29-
return Object.values(canonicalGrpcErrorCodesMap).includes(message as SpanStatus['message']);
30-
};
31-
3228
/**
3329
* Get a Sentry span status from an otel span.
3430
*/

packages/opentelemetry/test/tracerProvider.test.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
getCapturedScopesOnSpan,
66
getRootSpan,
77
spanToJSON,
8+
spanToStreamedSpanJSON,
89
SPAN_STATUS_ERROR,
910
SPAN_STATUS_OK,
1011
startSpanManual,
@@ -194,15 +195,17 @@ describe('SentryTracerProvider', () => {
194195

195196
it('preserves a non-canonical error status message under span streaming', () => {
196197
// Under streaming the streamed serializer surfaces the raw message as `sentry.status.message`, so
197-
// finalizing must not normalize it to `internal_error` the way it does for the non-streamed
198-
// transaction status field. Without streaming, `finalizes span statuses` covers the `internal_error` case.
198+
// finalizing must not overwrite the live span status. The transaction `status` field is always
199+
// normalized to a valid value (`internal_error`), but the raw message survives on the streamed span.
199200
initTestClient({ tracesSampleRate: 1, traceLifecycle: 'stream' });
200201
const span = trace.getTracer('test').startSpan('db-error');
201202
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'Cannot enqueue Query after fatal error.' });
202203

203204
applyOtelSpanData(span as Span, { finalizeStatus: true });
204205

205-
expect(spanToJSON(span as Span).status).toBe('Cannot enqueue Query after fatal error.');
206+
const streamed = spanToStreamedSpanJSON(span as Span);
207+
expect(streamed.status).toBe('error');
208+
expect(streamed.attributes?.['sentry.status.message']).toBe('Cannot enqueue Query after fatal error.');
206209
});
207210

208211
it('infers route source, op, and name for HTTP server spans', () => {

packages/server-utils/test/mysql2/mysql2-dc-subscriber.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ describe('subscribeMysql2DiagnosticChannels', () => {
236236
{ error: new Error('table missing') },
237237
);
238238

239-
expect(spanToJSON(span!).status).toBe('table missing');
239+
expect(spanToJSON(span!).status).toBe('internal_error');
240240
expect(spanToJSON(span!).timestamp).toBeDefined();
241241
expect(captureExceptionSpy).not.toHaveBeenCalled();
242242
});

0 commit comments

Comments
 (0)