8000 fix(opentelemetry): Do not stomp span error status (#11169) · basarat/sentry-javascript@125b368 · GitHub
[go: up one dir, main page]
Skip to content

Commit 125b368

Browse files
lforstlforst
authored
fix(opentelemetry): Do not stomp span error status (getsentry#11169)
1 parent 82abb99 commit 125b368

File tree

6 files changed

+125
-110
lines changed

6 files changed

+125
-110
lines changed

‎packages/core/src/tracing/spanstatus.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ export const SPAN_STATUS_ERROR = 2;
1010
* @param httpStatus The HTTP response status code.
1111
* @returns The span status or unknown_error.
1212
*/
13+
// https://develop.sentry.dev/sdk/event-payloads/span/
1314
export function getSpanStatusFromHttpCode(httpStatus: number): SpanStatus {
1415
if (httpStatus < 400 && httpStatus >= 100) {
1516
return { code: SPAN_STATUS_OK };
@@ -29,6 +30,8 @@ export function getSpanStatusFromHttpCode(httpStatus: number): SpanStatus {
2930
return { code: SPAN_STATUS_ERROR, message: 'failed_precondition' };
3031
case 429:
3132
return { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' };
33+
case 499:
34+
return { code: SPAN_STATUS_ERROR, message: 'cancelled' };
3235
default:
3336
return { code: SPAN_STATUS_ERROR, message: 'invalid_argument' };
3437
}

‎packages/opentelemetry/src/utils/mapStatus.ts

Lines changed: 31 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,13 @@
11
import { SpanStatusCode } from '@opentelemetry/api';
22
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
3-
import { SPAN_STATUS_ERROR, SPAN_STATUS_OK } from '@sentry/core';
3+
import { SPAN_STATUS_ERROR, SPAN_STATUS_OK, getSpanStatusFromHttpCode } from '@sentry/core';
44
import type { SpanStatus } from '@sentry/types';
55

66
import type { AbstractSpan } from '../types';
77
import { spanHasAttributes, spanHasStatus } from './spanTypes';
88

9-
// canonicalCodesHTTPMap maps some HTTP codes to Sentry's span statuses. See possible mapping in https://develop.sentry.dev/sdk/event-payloads/span/
10-
const canonicalCodesHTTPMap: Record<string, SpanStatus['message']> = {
11-
'400': 'failed_precondition',
12-
'401': 'unauthenticated',
13-
'403': 'permission_denied',
14-
'404': 'not_found',
15-
'409': 'aborted',
16-
'429': 'resource_exhausted',
17-
'499': 'cancelled',
18-
'500': 'internal_error',
19-
'501': 'unimplemented',
20-
'503': 'unavailable',
21-
'504': 'deadline_exceeded',
22-
} as const;
23-
249
// canonicalCodesGrpcMap maps some GRPC codes to Sentry's span statuses. See description in grpc documentation.
25-
const canonicalCodesGrpcMap: Record<string, SpanStatus['message']> = {
10+
const canonicalGrpcErrorCodesMap: Record<string, SpanStatus['message']> = {
2611
'1': 'cancelled',
2712
'2': 'unknown_error',
2813
'3': 'invalid_argument',
@@ -48,28 +33,40 @@ export function mapStatus(span: AbstractSpan): SpanStatus {
4833
const attributes = spanHasAttributes(span) ? span.attributes : {};
4934
const status = spanHasStatus(span) ? span.status : undefined;
5035

51-
const httpCode = attributes[SemanticAttributes.HTTP_STATUS_CODE];
52-
const grpcCode = attributes[SemanticAttributes.RPC_GRPC_STATUS_CODE];
53-
54-
const code = typeof httpCode === 'string' ? httpCode : typeof httpCode === 'number' ? httpCode.toString() : undefined;
55-
if (code) {
56-
const sentryStatus = canonicalCodesHTTPMap[code];
57-
if (sentryStatus) {
58-
return { code: SPAN_STATUS_ERROR, message: sentryStatus };
36+
if (status) {
37+
// Since span status OK is not set by default, we give it priority: https://opentelemetry.io/docs/concepts/signals/traces/#span-status
38+
if (status.code === SpanStatusCode.OK) {
39+
return { code: SPAN_STATUS_OK };
40+
// If the span is already marked as erroneous we return that exact status
41+
} else if (status.code === SpanStatusCode.ERROR) {
42+
return { code: SPAN_STATUS_ERROR, message: status.message };
5943
}
6044
}
6145

62-
if (typeof grpcCode === 'string') {
63-
const sentryStatus = canonicalCodesGrpcMap[grpcCode];
64-
if (sentryStatus) {
65-
return { code: SPAN_STATUS_ERROR, message: sentryStatus };
66-
}
46+
// If the span status is UNSET, we try to infer it from HTTP or GRPC status codes.
47+
48+
const httpCodeAttribute = attributes[SemanticAttributes.HTTP_STATUS_CODE];
49+
const grpcCodeAttribute = attributes[SemanticAttributes.RPC_GRPC_STATUS_CODE];
50+
51+
const numberHttpCode =
52+
typeof httpCodeAttribute === 'number'
53+
? httpCodeAttribute
54+
: typeof httpCodeAttribute === 'string'
55+
? parseInt(httpCodeAttribute)
56+
: undefined;
57+
58+
if (numberHttpCode) {
59+
return getSpanStatusFromHttpCode(numberHttpCode);
6760
}
6861

69-
const statusCode = status && status.code;
70-
if (statusCode === SpanStatusCode.OK || statusCode === SpanStatusCode.UNSET) {
71-
return { code: SPAN_STATUS_OK };
62+
if (typeof grpcCodeAttribute === 'string') {
63+
return { code: SPAN_STATUS_ERROR, message: canonicalGrpcErrorCodesMap[grpcCodeAttribute] || 'unknown_error' };
7264
}
7365

74-
return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
66+
// We default to setting the spans status to ok.
67+
if (status && status.code === SpanStatusCode.UNSET) {
68+
return { code: SPAN_STATUS_OK };
69+
} else {
70+
return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
71+
}
7572
}

‎packages/opentelemetry/test/utils/mapStatus.test.ts

Lines changed: 79 additions & 64 deletions
< F438 tr class="diff-line-row">
Original file line numberDiff line numberDiff line change
@@ -6,79 +6,94 @@ import { mapStatus } from '../../src/utils/mapStatus';
66
import { createSpan } from '../helpers/createSpan';
77

88
describe('mapStatus', () => {
9-
const statusTestTable: [number, undefined | number | string, undefined | string, SpanStatus][] = [
10-
[-1, undefined, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
11-
[3, undefined, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
12-
[0, undefined, undefined, { code: SPAN_STATUS_OK }],
13-
[1, undefined, undefined, { code: SPAN_STATUS_OK }],
14-
[2, undefined, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
15-
9+
const statusTestTable: [undefined | number | string, undefined | string, SpanStatus][] = [
1610
// http codes
17-
[2, 400, undefined, { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
18-
[2, 401, undefined, { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
19-
[2, 403, undefined, { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
20-
[2, 404, undefined, { code: SPAN_STATUS_ERROR, message: 'not_found' }],
21-
[2, 409, undefined, { code: SPAN_STATUS_ERROR, message: 'aborted' }],
22-
[2, 429, undefined, { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
23-
[2, 499, undefined, { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
24-
[2, 500, undefined, { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
25-
[2, 501, undefined, { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
26-
[2, 503, undefined, { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
27-
[2, 504, undefined, { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
28-
[2, 999, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
11+
[400, undefined, { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
12+
[401, undefined, { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
13+
[403, undefined, { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
14+
[404, undefined, { code: SPAN_STATUS_ERROR, message: 'not_found' }],
15+
[409, undefined, { code: SPAN_STATUS_ERROR, message: 'already_exists' }],
16+
[429, undefined, { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
17+
[499, undefined, { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
18+
[500, undefined, { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
19+
[501, undefined, { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
20+
[503, undefined, { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
21+
[504, undefined, { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
22+
[999, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
2923

30-
[2, '400', undefined, { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
31-
[2, '401', undefined, { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
32-
[2, '403', undefined, { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
33-
[2, '404', undefined, { code: SPAN_STATUS_ERROR, message: 'not_found' }],
34-
[2, '409', undefined, { code: SPAN_STATUS_ERROR, message: 'aborted' }],
35-
[2, '429', undefined, { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
36-
[2, '499', undefined, { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
37-
[2, '500', undefined, { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
38-
[2, '501', undefined, { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
39-
[2, '503', undefined, { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
40-
[2, '504', undefined, { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
41-
[2, '999', undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
24+
['400', undefined, { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
25+
['401', undefined, { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
26+
['403', undefined, { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
27+
['404', undefined, { code: SPAN_STATUS_ERROR, message: 'not_found' }],
28+
['409', undefined, { code: SPAN_STATUS_ERROR, message: 'already_exists' }],
29+
['429', undefined, { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
30+
['499', undefined, { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
31+
['500', undefined, { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
32+
['501', undefined, { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
33+
['503', undefined, { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
34+
['504', undefined, { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
35+
['999', undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
4236

4337
// grpc codes
44-
[2, undefined, '1', { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
45-
[2, undefined, '2', { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
46-
[2, undefined, '3', { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
47-
[2, undefined, '4', { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
48-
[2, undefined, '5', { code: SPAN_STATUS_ERROR, message: 'not_found' }],
49-
[2, undefined, '6', { code: SPAN_STATUS_ERROR, message: 'already_exists' }],
50-
[2, undefined, '7', { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
51-
[2, undefined, '8', { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
52-
[2, undefined, '9', { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
53-
[2, undefined, '10', { code: SPAN_STATUS_ERROR, message: 'aborted' }],
54-
[2, undefined, '11', { code: SPAN_STATUS_ERROR, message: 'out_of_range' }],
55-
[2, undefined, '12', { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
56-
[2, undefined, '13', { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
57-
[2, undefined, '14', { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
58-
[2, undefined, '15', { code: SPAN_STATUS_ERROR, message: 'data_loss' }],
59-
[2, undefined, '16', { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
60-
[2, undefined, '999', { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
38+
[undefined, '1', { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
39+
[undefined, '2', { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
40+
[undefined, '3', { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
41+
[undefined, '4', { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
42+
[undefined, '5', { code: SPAN_STATUS_ERROR, message: 'not_found' }],
43+
[undefined, '6', { code: SPAN_STATUS_ERROR, message: 'already_exists' }],
44+
[undefined, '7', { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
45+
[undefined, '8', { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
46+
[undefined, '9', { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
47+
[undefined, '10', { code: SPAN_STATUS_ERROR, message: 'aborted' }],
48+
[undefined, '11', { code: SPAN_STATUS_ERROR, message: 'out_of_range' }],
49+
[undefined, '12', { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
50+
[undefined, '13', { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
51+
[undefined, '14', { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
52+
[undefined, '15', { code: SPAN_STATUS_ERROR, message: 'data_loss' }],
53+
[undefined, '16', { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
54+
[undefined, '999', { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
6155

6256
// http takes precedence over grpc
63-
[2, '400', '2', { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
57+
['400', '2', { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
6458
];
6559

66-
it.each(statusTestTable)(
67-
'works with otelStatus=%i, httpCode=%s, grpcCode=%s',
68-
(otelStatus, httpCode, grpcCode, expected) => {
69-
const span = createSpan();
70-
span.setStatus({ code: otelStatus });
60+
it.each(statusTestTable)('works with httpCode=%s, grpcCode=%s', (httpCode, grpcCode, expected) => {
61+
const span = createSpan();
62+
span.setStatus({ code: 0 }); // UNSET
63+
64+
if (httpCode) {
65+
span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, httpCode);
66+
}
67+
68+
if (grpcCode) {
69+
span.setAttribute(SemanticAttributes.RPC_GRPC_STATUS_CODE, grpcCode);
70+
}
71+
72+
const actual = mapStatus(span);
73+
expect(actual).toEqual(expected);
74+
});
75+
76+
it('returns ok span status when is UNSET present on span', () => {
77+
const span = createSpan();
78+
span.setStatus({ code: 0 }); // UNSET
79+
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_OK });
80+
});
7181

72-
if (httpCode) {
73-
span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, httpCode);
74-
}
82+
it('returns ok span status when already present on span', () => {
83+
const span = createSpan();
84+
span.setStatus({ code: 1 }); // OK
85+
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_OK });
86+
});
7587

76-
if (grpcCode) {
77-
span.setA 10000 ttribute(SemanticAttributes.RPC_GRPC_STATUS_CODE, grpcCode);
78-
}
88+
it('returns error status when span already has error status', () => {
89+
const span = createSpan();
90+
span.setStatus({ code: 2, message: 'invalid_argument' }); // ERROR
91+
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'invalid_argument' });
92+
});
7993

80-
const actual = mapStatus(span);
81-
expect(actual).toEqual(expected);
82-
},
83-
);
94+
it('returns unknown error status when code is unknown', () => {
95+
const span = createSpan();
96+
span.setStatus({ code: -1 as 0 });
97+
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'unknown_error' });
98+
});
8499
});

‎packages/remix/test/integration/test/server/action.test.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
6161
assertSentryTransaction(transaction[2], {
6262
contexts: {
6363
trace: {
64-
status: 'unknown_error',
64+
status: 'internal_error',
6565
data: {
6666
'http.response.status_code': 500,
6767
},
@@ -169,7 +169,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
169169
contexts: {
170170
trace: {
171171
op: 'http.server',
172-
status: 'unknown_error',
172+
status: 'internal_error',
173173
data: {
174174
method: 'GET',
175175
'http.response.status_code': 500,
@@ -217,7 +217,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
217217
contexts: {
218218
trace: {
219219
op: 'http.server',
220-
status: 'unknown_error',
220+
status: 'internal_error',
221221
data: {
222222
method: 'POST',
223223
'http.response.status_code': 500,
@@ -265,7 +265,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
265265
contexts: {
266266
trace: {
267267
op: 'http.server',
268-
status: 'unknown_error',
268+
status: 'internal_error',
269269
data: {
270270
method: 'POST',
271271
'http.response.status_code': 500,
@@ -313,7 +313,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
313313
contexts: {
314314
trace: {
315315
op: 'http.server',
316-
status: 'unknown_error',
316+
status: 'internal_error',
317317
data: {
318318
method: 'POST',
319319
'http.response.status_code': 500,
@@ -361,7 +361,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
361361
contexts: {
362362
trace: {
363363
op: 'http.server',
364-
status: 'unknown_error',
364+
status: 'internal_error',
365365
data: {
366366
method: 'POST',
367367
'http.response.status_code': 500,
@@ -409,7 +409,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
409409
contexts: {
410410
trace: {
411411
op: 'http.server',
412-
status: 'unknown_error',
412+
status: 'internal_error',
413413
data: {
414414
method: 'POST',
415415
'http.response.status_code': 500,
@@ -457,7 +457,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
457457
contexts: {
458458
trace: {
459459
op: 'http.server',
460-
status: 'unknown_error',
460+
status: 'internal_error',
461461
data: {
462462
method: 'POST',
463463
'http.response.status_code': 500,

‎packages/remix/test/integration/test/server/loader.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
1919
assertSentryTransaction(transaction, {
2020
contexts: {
2121
trace: {
22-
status: 'unknown_error',
22+
status: 'internal_error',
2323
data: {
2424
'http.response.status_code': 500,
2525
},
@@ -61,7 +61,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
6161
assertSentryTransaction(transaction, {
6262
contexts: {
6363
trace: {
64-
status: 'unknown_error',
64+
status: 'internal_error',
6565
data: {
6666
'http.response.status_code': 500,
6767
},
@@ -148,7 +148,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
148148
contexts: {
149149
trace: {
150150
op: 'http.server',
151-
status: 'unknown_error',
151+
status: 'internal_error',
152152
data: {
153153
method: 'GET',
154154
'http.response.status_code': 500,

‎packages/remix/test/integration/test/server/ssr.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ describe('Server Side Rendering', () => {
1313
assertSentryTransaction(transaction[2], {
1414
contexts: {
1515
trace: {
16-
status: 'unknown_error',
16+
status: 'internal_error',
1717
data: {
1818
'http.response.status_code': 500,
1919
},

0 commit comments

Comments
 (0)
0