Skip to content

fix(opentelemetry): Do not stomp span error status #11169

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.

Sign up for GitHub

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 2 commits into from
Mar 19, 2024
Merged

fix(opentelemetry): Do not stomp span error status #11169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0be84b
fix(opentelemetry): Do not stomp span error status
lforst Mar 18, 2024
9020290
remix tests
lforst Mar 18, 2024
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
3 changes: 3 additions & 0 deletions packages/core/src/tracing/spanstatus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export const SPAN_STATUS_ERROR = 2;
* @param httpStatus The HTTP response status code.
* @returns The span status or unknown_error.
*/
// https://develop.sentry.dev/sdk/event-payloads/span/
Copy link
Member

Choose a reason for hiding this comment

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

l (feel free to ignore): Any reason for not adding this link directly to the JSDoc header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a bit cautious with links in js doc bc they might become stale. If it's just in our codebase it's not that bad but as a user it's frustrating if links are dead.

Copy link
Member

Choose a reason for hiding this comment

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

Ok fair point, in this case I think it's totally fine. In other cases I think it's still worth to take that risk but admittedly, I didn't think too much about it until now 😅

export function getSpanStatusFromHttpCode(httpStatus: number): SpanStatus {
if (httpStatus < 400 && httpStatus >= 100) {
return { code: SPAN_STATUS_OK };
Expand All @@ -29,6 +30,8 @@ export function getSpanStatusFromHttpCode(httpStatus: number): SpanStatus {
return { code: SPAN_STATUS_ERROR, message: 'failed_precondition' };
case 429:
return { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' };
case 499:
return { code: SPAN_STATUS_ERROR, message: 'cancelled' };
default:
return { code: SPAN_STATUS_ERROR, message: 'invalid_argument' };
}
Expand Down
65 changes: 31 additions & 34 deletions packages/opentelemetry/src/utils/mapStatus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,13 @@
import { SpanStatusCode } from '@opentelemetry/api';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import { SPAN_STATUS_ERROR, SPAN_STATUS_OK } from '@sentry/core';
import { SPAN_STATUS_ERROR, SPAN_STATUS_OK, getSpanStatusFromHttpCode } from '@sentry/core';
import type { SpanStatus } from '@sentry/types';

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

// canonicalCodesHTTPMap maps some HTTP codes to Sentry's span statuses. See possible mapping in https://develop.sentry.dev/sdk/event-payloads/span/
const canonicalCodesHTTPMap: Record<string, SpanStatus['message']> = {
'400': 'failed_precondition',
'401': 'unauthenticated',
'403': 'permission_denied',
'404': 'not_found',
'409': 'aborted',
'429': 'resource_exhausted',
'499': 'cancelled',
'500': 'internal_error',
'501': 'unimplemented',
'503': 'unavailable',
'504': 'deadline_exceeded',
} as const;

// canonicalCodesGrpcMap maps some GRPC codes to Sentry's span statuses. See description in grpc documentation.
const canonicalCodesGrpcMap: Record<string, SpanStatus['message']> = {
const canonicalGrpcErrorCodesMap: Record<string, SpanStatus['message']> = {
'1': 'cancelled',
'2': 'unknown_error',
'3': 'invalid_argument',
Expand All @@ -48,28 +33,40 @@ export function mapStatus(span: AbstractSpan): SpanStatus {
const attributes = spanHasAttributes(span) ? span.attributes : {};
const status = spanHasStatus(span) ? span.status : undefined;

const httpCode = attributes[SemanticAttributes.HTTP_STATUS_CODE];
const grpcCode = attributes[SemanticAttributes.RPC_GRPC_STATUS_CODE];

const code = typeof httpCode === 'string' ? httpCode : typeof httpCode === 'number' ? httpCode.toString() : undefined;
if (code) {
const sentryStatus = canonicalCodesHTTPMap[code];
if (sentryStatus) {
return { code: SPAN_STATUS_ERROR, message: sentryStatus };
if (status) {
// Since span status OK is not set by default, we give it priority: https://opentelemetry.io/docs/concepts/signals/traces/#span-status
if (status.code === SpanStatusCode.OK) {
return { code: SPAN_STATUS_OK };
// If the span is already marked as erroneous we return that exact status
} else if (status.code === SpanStatusCode.ERROR) {
return { code: SPAN_STATUS_ERROR, message: status.message };
}
}

if (typeof grpcCode === 'string') {
const sentryStatus = canonicalCodesGrpcMap[grpcCode];
if (sentryStatus) {
return { code: SPAN_STATUS_ERROR, message: sentryStatus };
}
// If the span status is UNSET, we try to infer it from HTTP or GRPC status codes.

const httpCodeAttribute = attributes[SemanticAttributes.HTTP_STATUS_CODE];
const grpcCodeAttribute = attributes[SemanticAttributes.RPC_GRPC_STATUS_CODE];

const numberHttpCode =
typeof httpCodeAttribute === 'number'
? httpCodeAttribute
: typeof httpCodeAttribute === 'string'
? parseInt(httpCodeAttribute)
: undefined;

if (numberHttpCode) {
return getSpanStatusFromHttpCode(numberHttpCode);
}

const statusCode = status && status.code;
if (statusCode === SpanStatusCode.OK || statusCode === SpanStatusCode.UNSET) {
return { code: SPAN_STATUS_OK };
if (typeof grpcCodeAttribute === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

m: can grpcCodeAttribute be of any other type than string like httpCodeAttribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened an issue to track this, as I didn't touch that particular logic: #11195

It may very well be that this is a leftover from our first initial OTEL implementation draft.

return { code: SPAN_STATUS_ERROR, message: canonicalGrpcErrorCodesMap[grpcCodeAttribute] || 'unknown_error' };
}

return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
// We default to setting the spans status to ok.
if (status && status.code === SpanStatusCode.UNSET) {
return { code: SPAN_STATUS_OK };
} else {
return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
}
}
143 changes: 79 additions & 64 deletions packages/opentelemetry/test/utils/mapStatus.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,79 +6,94 @@ import { mapStatus } from '../../src/utils/mapStatus';
import { createSpan } from '../helpers/createSpan';

describe('mapStatus', () => {
const statusTestTable: [number, undefined | number | string, undefined | string, SpanStatus][] = [
[-1, undefined, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
[3, undefined, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
[0, undefined, undefined, { code: SPAN_STATUS_OK }],
[1, undefined, undefined, { code: SPAN_STATUS_OK }],
[2, undefined, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],

const statusTestTable: [undefined | number | string, undefined | string, SpanStatus][] = [
// http codes
[2, 400, undefined, { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
[2, 401, undefined, { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
[2, 403, undefined, { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
[2, 404, undefined, { code: SPAN_STATUS_ERROR, message: 'not_found' }],
[2, 409, undefined, { code: SPAN_STATUS_ERROR, message: 'aborted' }],
[2, 429, undefined, { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
[2, 499, undefined, { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
[2, 500, undefined, { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
[2, 501, undefined, { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
[2, 503, undefined, { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
[2, 504, undefined, { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
[2, 999, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
[400, undefined, { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
[401, undefined, { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
[403, undefined, { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
[404, undefined, { code: SPAN_STATUS_ERROR, message: 'not_found' }],
[409, undefined, { code: SPAN_STATUS_ERROR, message: 'already_exists' }],
[429, undefined, { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
[499, undefined, { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
[500, undefined, { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
[501, undefined, { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
[503, undefined, { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
[504, undefined, { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
[999, undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],

[2, '400', undefined, { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
[2, '401', undefined, { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
[2, '403', undefined, { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
[2, '404', undefined, { code: SPAN_STATUS_ERROR, message: 'not_found' }],
[2, '409', undefined, { code: SPAN_STATUS_ERROR, message: 'aborted' }],
[2, '429', undefined, { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
[2, '499', undefined, { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
[2, '500', undefined, { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
[2, '501', undefined, { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
[2, '503', undefined, { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
[2, '504', undefined, { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
[2, '999', undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
['400', undefined, { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
['401', undefined, { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
['403', undefined, { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
['404', undefined, { code: SPAN_STATUS_ERROR, message: 'not_found' }],
['409', undefined, { code: SPAN_STATUS_ERROR, message: 'already_exists' }],
['429', undefined, { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
['499', undefined, { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
['500', undefined, { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
['501', undefined, { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
['503', undefined, { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
['504', undefined, { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
['999', undefined, { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],

// grpc codes
[2, undefined, '1', { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
[2, undefined, '2', { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
[2, undefined, '3', { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
[2, undefined, '4', { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
[2, undefined, '5', { code: SPAN_STATUS_ERROR, message: 'not_found' }],
[2, undefined, '6', { code: SPAN_STATUS_ERROR, message: 'already_exists' }],
[2, undefined, '7', { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
[2, undefined, '8', { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
[2, undefined, '9', { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
[2, undefined, '10', { code: SPAN_STATUS_ERROR, message: 'aborted' }],
[2, undefined, '11', { code: SPAN_STATUS_ERROR, message: 'out_of_range' }],
[2, undefined, '12', { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
[2, undefined, '13', { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
[2, undefined, '14', { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
[2, undefined, '15', { code: SPAN_STATUS_ERROR, message: 'data_loss' }],
[2, undefined, '16', { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
[2, undefined, '999', { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
[undefined, '1', { code: SPAN_STATUS_ERROR, message: 'cancelled' }],
[undefined, '2', { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],
[undefined, '3', { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
[undefined, '4', { code: SPAN_STATUS_ERROR, message: 'deadline_exceeded' }],
[undefined, '5', { code: SPAN_STATUS_ERROR, message: 'not_found' }],
[undefined, '6', { code: SPAN_STATUS_ERROR, message: 'already_exists' }],
[undefined, '7', { code: SPAN_STATUS_ERROR, message: 'permission_denied' }],
[undefined, '8', { code: SPAN_STATUS_ERROR, message: 'resource_exhausted' }],
[undefined, '9', { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
[undefined, '10', { code: SPAN_STATUS_ERROR, message: 'aborted' }],
[undefined, '11', { code: SPAN_STATUS_ERROR, message: 'out_of_range' }],
[undefined, '12', { code: SPAN_STATUS_ERROR, message: 'unimplemented' }],
[undefined, '13', { code: SPAN_STATUS_ERROR, message: 'internal_error' }],
[undefined, '14', { code: SPAN_STATUS_ERROR, message: 'unavailable' }],
[undefined, '15', { code: SPAN_STATUS_ERROR, message: 'data_loss' }],
[undefined, '16', { code: SPAN_STATUS_ERROR, message: 'unauthenticated' }],
[undefined, '999', { code: SPAN_STATUS_ERROR, message: 'unknown_error' }],

// http takes precedence over grpc
[2, '400', '2', { code: SPAN_STATUS_ERROR, message: 'failed_precondition' }],
['400', '2', { code: SPAN_STATUS_ERROR, message: 'invalid_argument' }],
];

it.each(statusTestTable)(
'works with otelStatus=%i, httpCode=%s, grpcCode=%s',
(otelStatus, httpCode, grpcCode, expected) => {
const span = createSpan();
span.setStatus({ code: otelStatus });
it.each(statusTestTable)('works with httpCode=%s, grpcCode=%s', (httpCode, grpcCode, expected) => {
const span = createSpan();
span.setStatus({ code: 0 }); // UNSET

if (httpCode) {
span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, httpCode);
}

if (grpcCode) {
span.setAttribute(SemanticAttributes.RPC_GRPC_STATUS_CODE, grpcCode);
}

const actual = mapStatus(span);
expect(actual).toEqual(expected);
});

it('returns ok span status when is UNSET present on span', () => {
const span = createSpan();
span.setStatus({ code: 0 }); // UNSET
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_OK });
});

if (httpCode) {
span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, httpCode);
}
it('returns ok span status when already present on span', () => {
const span = createSpan();
span.setStatus({ code: 1 }); // OK
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_OK });
});

if (grpcCode) {
span.setAttribute(SemanticAttributes.RPC_GRPC_STATUS_CODE, grpcCode);
}
it('returns error status when span already has error status', () => {
const span = createSpan();
span.setStatus({ code: 2, message: 'invalid_argument' }); // ERROR
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'invalid_argument' });
});

const actual = mapStatus(span);
expect(actual).toEqual(expected);
},
);
it('returns unknown error status when code is unknown', () => {
const span = createSpan();
span.setStatus({ code: -1 as 0 });
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'unknown_error' });
});
});
16 changes: 8 additions & 8 deletions packages/remix/test/integration/test/server/action.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
assertSentryTransaction(transaction[2], {
contexts: {
trace: {
status: 'unknown_error',
status: 'internal_error',
data: {
'http.response.status_code': 500,
},
Expand Down Expand Up @@ -169,7 +169,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
contexts: {
trace: {
op: 'http.server',
status: 'unknown_error',
status: 'internal_error',
data: {
method: 'GET',
'http.response.status_code': 500,
Expand Down Expand Up @@ -217,7 +217,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
contexts: {
trace: {
op: 'http.server',
status: 'unknown_error',
status: 'internal_error',
data: {
method: 'POST',
'http.response.status_code': 500,
Expand Down Expand Up @@ -265,7 +265,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
contexts: {
trace: {
op: 'http.server',
status: 'unknown_error',
status: 'internal_error',
data: {
method: 'POST',
'http.response.status_code': 500,
Expand Down Expand Up @@ -313,7 +313,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
contexts: {
trace: {
op: 'http.server',
status: 'unknown_error',
status: 'internal_error',
data: {
method: 'POST',
'http.response.status_code': 500,
Expand Down Expand Up @@ -361,7 +361,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
contexts: {
trace: {
op: 'http.server',
status: 'unknown_error',
status: 'internal_error',
data: {
method: 'POST',
'http.response.status_code': 500,
Expand Down Expand Up @@ -409,7 +409,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
contexts: {
trace: {
op: 'http.server',
status: 'unknown_error',
status: 'internal_error',
data: {
method: 'POST',
'http.response.status_code': 500,
Expand Down Expand Up @@ -457,7 +457,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
contexts: {
trace: {
op: 'http.server',
status: 'unknown_error',
status: 'internal_error',
data: {
method: 'POST',
'http.response.status_code': 500,
Expand Down
6 changes: 3 additions & 3 deletions packages/remix/test/integration/test/server/loader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
assertSentryTransaction(transaction, {
contexts: {
trace: {
status: 'unknown_error',
status: 'internal_error',
data: {
'http.response.status_code': 500,
},
Expand Down Expand Up @@ -61,7 +61,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
assertSentryTransaction(transaction, {
contexts: {
trace: {
status: 'unknown_error',
status: 'internal_error',
data: {
'http.response.status_code': 500,
},
Expand Down Expand Up @@ -148,7 +148,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
contexts: {
trace: {
op: 'http.server',
status: 'unknown_error',
status: 'internal_error',
data: {
method: 'GET',
'http.response.status_code': 500,
Expand Down
2 changes: 1 addition & 1 deletion packages/remix/test/integration/test/server/ssr.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('Server Side Rendering', () => {
assertSentryTransaction(transaction[2], {
contexts: {
trace: {
status: 'unknown_error',
status: 'internal_error',
data: {
'http.response.status_code': 500,
},
Expand Down