Skip to content

Commit f032f3f

Browse files
mydeamydea
authored
fix(node): Ensure status is correct for http server span errors (#12477)
I noticed that we were not correctly setting a status message based on http status for errored http.server span. The problem was in our logic, where we already had the status set to `{ code: 2, message: undefined }`, and whenever the code was not 0 (unset), we'd never look at the attributes to infer the proper status message. Now, if we have code: 2 but not message, we try to infer the message from the attributes, if possible.
1 parent b2bdf1a commit f032f3f

3 files changed

Lines changed: 121 additions & 10 deletions

File tree

‎dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts‎

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ test('Sends an API route transaction', async ({ baseURL }) => {
1212
await fetch(`${baseURL}/test-transaction`);
1313

1414
const transactionEvent = await pageloadTransactionEventPromise;
15-
const transactionEventId = transactionEvent.event_id;
1615

1716
expect(transactionEvent.contexts?.trace).toEqual({
1817
data: {
@@ -119,3 +118,84 @@ test('Sends an API route transaction', async ({ baseURL }) => {
119118
trace_id: expect.any(String),
120119
});
121120
});
121+
122+
test('Sends an API route transaction for an errored route', async ({ baseURL }) => {
123+
const transactionEventPromise = waitForTransaction('node-express', transactionEvent => {
124+
return (
125+
transactionEvent.contexts?.trace?.op === 'http.server' &&
126+
transactionEvent.transaction === 'GET /test-exception/:id' &&
127+
transactionEvent.request?.url === 'http://localhost:3030/test-exception/777'
128+
);
129+
});
130+
131+
await fetch(`${baseURL}/test-exception/777`);
132+
133+
const transactionEvent = await transactionEventPromise;
134+
135+
expect(transactionEvent.contexts?.trace?.op).toEqual('http.server');
136+
expect(transactionEvent.transaction).toEqual('GET /test-exception/:id');
137+
expect(transactionEvent.contexts?.trace?.status).toEqual('internal_error');
138+
expect(transactionEvent.contexts?.trace?.data?.['http.status_code']).toEqual(500);
139+
140+
const spans = transactionEvent.spans || [];
141+
142+
expect(spans).toContainEqual({
143+
data: {
144+
'sentry.origin': 'auto.http.otel.express',
145+
'sentry.op': 'middleware.express',
146+
'http.route': '/',
147+
'express.name': 'query',
148+
'express.type': 'middleware',
149+
'otel.kind': 'INTERNAL',
150+
},
151+
description: 'query',
152+
op: 'middleware.express',
153+
origin: 'auto.http.otel.express',
154+
parent_span_id: expect.any(String),
155+
span_id: expect.any(String),
156+
start_timestamp: expect.any(Number),
157+
status: 'ok',
158+
timestamp: expect.any(Number),
159+
trace_id: expect.any(String),
160+
});
161+
162+
expect(spans).toContainEqual({
163+
data: {
164+
'sentry.origin': 'auto.http.otel.express',
165+
'sentry.op': 'middleware.express',
166+
'http.route': '/',
167+
'express.name': 'expressInit',
168+
'express.type': 'middleware',
169+
'otel.kind': 'INTERNAL',
170+
},
171+
description: 'expressInit',
172+
op: 'middleware.express',
173+
origin: 'auto.http.otel.express',
174+
parent_span_id: expect.any(String),
175+
span_id: expect.any(String),
176+
start_timestamp: expect.any(Number),
177+
status: 'ok',
178+
timestamp: expect.any(Number),
179+
trace_id: expect.any(String),
180+
});
181+
182+
expect(spans).toContainEqual({
183+
data: {
184+
'sentry.origin': 'auto.http.otel.express',
185+
'sentry.op': 'request_handler.express',
186+
'http.route': '/test-exception/:id',
187+
'express.name': '/test-exception/:id',
188+
'express.type': 'request_handler',
189+
'otel.kind': 'INTERNAL',
190+
},
191+
description: '/test-exception/:id',
192+
op: 'request_handler.express',
193+
origin: 'auto.http.otel.express',
194+
parent_span_id: expect.any(String),
195+
span_id: expect.any(String),
196+
start_timestamp: expect.any(Number),
197+
status: 'ok',
198+
timestamp: expect.any(Number),
199+
trace_id: expect.any(String),
200+
});
201+
});

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

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { SpanStatusCode } from '@opentelemetry/api';
22
import { SEMATTRS_HTTP_STATUS_CODE, SEMATTRS_RPC_GRPC_STATUS_CODE } from '@opentelemetry/semantic-conventions';
33
import { SPAN_STATUS_ERROR, SPAN_STATUS_OK, getSpanStatusFromHttpCode } from '@sentry/core';
4-
import type { SpanStatus } from '@sentry/types';
4+
import type { SpanAttributes, SpanStatus } from '@sentry/types';
55

66
import type { AbstractSpan } from '../types';
77
import { spanHasAttributes, spanHasStatus } from './spanTypes';
@@ -43,14 +43,37 @@ export function mapStatus(span: AbstractSpan): SpanStatus {
4343
return { code: SPAN_STATUS_OK };
4444
// If the span is already marked as erroneous we return that exact status
4545
} else if (status.code === SpanStatusCode.ERROR) {
46-
if (typeof status.message === 'undefined' || isStatusErrorMessageValid(status.message)) {
46+
if (typeof status.message === 'undefined') {
47+
const inferredStatus = inferStatusFromAttributes(attributes);
48+
if (inferredStatus) {
49+
return inferredStatus;
50+
}
51+
}
52+
53+
if (status.message && isStatusErrorMessageValid(status.message)) {
4754
return { code: SPAN_STATUS_ERROR, message: status.message };
4855
} else {
4956
return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
5057
}
5158
}
5259
}
5360

61+
// If the span status is UNSET, we try to infer it from HTTP or GRPC status codes.
62+
const inferredStatus = inferStatusFromAttributes(attributes);
63+
64+
if (inferredStatus) {
65+
return inferredStatus;
66+
}
67+
68+
// We default to setting the spans status to ok.
69+
if (status && status.code === SpanStatusCode.UNSET) {
70+
return { code: SPAN_STATUS_OK };
71+
} else {
72+
return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
73+
}
74+
}
75+
76+
function inferStatusFromAttributes(attributes: SpanAttributes): SpanStatus | undefined {
5477
// If the span status is UNSET, we try to infer it from HTTP or GRPC status codes.
5578

5679
const httpCodeAttribute = attributes[SEMATTRS_HTTP_STATUS_CODE];
@@ -63,18 +86,13 @@ export function mapStatus(span: AbstractSpan): SpanStatus {
6386
? parseInt(httpCodeAttribute)
6487
: undefined;
6588

66-
if (numberHttpCode) {
89+
if (typeof numberHttpCode === 'number') {
6790
return getSpanStatusFromHttpCode(numberHttpCode);
6891
}
6992

7093
if (typeof grpcCodeAttribute === 'string') {
7194
return { code: SPAN_STATUS_ERROR, message: canonicalGrpcErrorCodesMap[grpcCodeAttribute] || 'unknown_error' };
7295
}
7396

74-
// We default to setting the spans status to ok.
75-
if (status && status.code === SpanStatusCode.UNSET) {
76-
return { code: SPAN_STATUS_OK };
77-
} else {
78-
return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
79-
}
97+
return undefined;
8098
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,19 @@ describe('mapStatus', () => {
9191
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'invalid_argument' });
9292
});
9393

94+
it('returns error status when span already has error status without message', () => {
95+
const span = createSpan();
96+
span.setStatus({ code: 2 }); // ERROR
97+
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'unknown_error' });
98+
});
99+
100+
it('infers error status form attributes when span already has error status without message', () => {
101+
const span = createSpan();
102+
span.setAttribute(SEMATTRS_HTTP_STATUS_CODE, 500);
103+
span.setStatus({ code: 2 }); // ERROR
104+
expect(mapStatus(span)).toEqual({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
105+
});
106+
94107
it('returns unknown error status when code is unknown', () => {
95108
const span = createSpan();
96109
span.setStatus({ code: -1 as 0 });

0 commit comments

Comments
 (0)