diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 4a38faa6de2..16163032003 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -12,6 +12,8 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2 ### :bug: Bug Fixes +* fix(instrumentation-http): set `error.type` on spans whose status code makes them an error [#7061](https://github.com/open-telemetry/opentelemetry-js/pull/7061) @mwear + ### :books: Documentation ### :house: Internal diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts index 976b6607ee0..3592822308a 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts @@ -66,6 +66,7 @@ import { isURLLike, headerCapture, isValidOptionsType, + parseErrorType, parseResponseStatus, setSpanWithError, } from './utils'; @@ -432,6 +433,13 @@ export class HttpInstrumentation extends InstrumentationBase { + const lowerBound = kind === SpanKind.CLIENT ? 400 : 500; + if ( + typeof statusCode === 'number' && + statusCode >= lowerBound && + statusCode < 600 + ) { + return String(statusCode); + } + + return undefined; +}; + /** * Check whether the given obj match pattern * @param constant e.g URL of request @@ -529,12 +550,9 @@ export const getOutgoingStableRequestMetricAttributesOnResponse = ( const statusCode = spanAttributes[ATTR_HTTP_RESPONSE_STATUS_CODE]; if (statusCode) { metricAttributes[ATTR_HTTP_RESPONSE_STATUS_CODE] = statusCode; - if ( - typeof statusCode === 'number' && - statusCode >= 400 && - statusCode < 600 - ) { - metricAttributes[ATTR_ERROR_TYPE] ??= String(statusCode); + const errorType = parseErrorType(SpanKind.CLIENT, statusCode); + if (errorType !== undefined) { + metricAttributes[ATTR_ERROR_TYPE] ??= errorType; } } return metricAttributes; @@ -860,12 +878,9 @@ export const getIncomingStableRequestMetricAttributesOnResponse = ( const statusCode = spanAttributes[ATTR_HTTP_RESPONSE_STATUS_CODE]; if (statusCode) { metricAttributes[ATTR_HTTP_RESPONSE_STATUS_CODE] = statusCode; - if ( - typeof statusCode === 'number' && - statusCode >= 500 && - statusCode < 600 - ) { - metricAttributes[ATTR_ERROR_TYPE] ??= String(statusCode); + const errorType = parseErrorType(SpanKind.SERVER, statusCode); + if (errorType !== undefined) { + metricAttributes[ATTR_ERROR_TYPE] ??= errorType; } } diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts index aefb0b04214..ddf0b558ff9 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts @@ -20,6 +20,7 @@ import { } from '@opentelemetry/sdk-trace'; import { ATTR_CLIENT_ADDRESS, + ATTR_ERROR_TYPE, ATTR_HTTP_REQUEST_METHOD, ATTR_HTTP_RESPONSE_STATUS_CODE, ATTR_HTTP_ROUTE, @@ -344,6 +345,10 @@ describe('HttpInstrumentation', () => { if (request.url?.includes('/withQuery')) { assert.match(request.url, /withQuery\?foo=bar$/); } + const status = request.url?.match(/\/status\/(\d+)/); + if (status) { + response.statusCode = Number(status[1]); + } response.end('Test Server Response'); }); @@ -418,6 +423,43 @@ describe('HttpInstrumentation', () => { assert.strictEqual(span.name, 'GET TheRoute'); }); + it('should set error.type to the status code on a failing span', async () => { + await httpRequest.get( + `${protocol}://${hostname}:${serverPort}/status/500` + ); + const spans = memoryExporter.getFinishedSpans(); + const incomingSpan = spans.find(s => s.kind === SpanKind.SERVER); + const outgoingSpan = spans.find(s => s.kind === SpanKind.CLIENT); + assert.ok(incomingSpan); + assert.ok(outgoingSpan); + + for (const span of [incomingSpan, outgoingSpan]) { + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.strictEqual(span.attributes[ATTR_ERROR_TYPE], '500'); + } + }); + + it('should treat 4xx as an error on the client span only', async () => { + await httpRequest.get( + `${protocol}://${hostname}:${serverPort}/status/404` + ); + const spans = memoryExporter.getFinishedSpans(); + const incomingSpan = spans.find(s => s.kind === SpanKind.SERVER); + const outgoingSpan = spans.find(s => s.kind === SpanKind.CLIENT); + assert.ok(incomingSpan); + assert.ok(outgoingSpan); + + assert.strictEqual(incomingSpan.status.code, SpanStatusCode.UNSET); + assert.strictEqual( + incomingSpan.attributes[ATTR_ERROR_TYPE], + undefined, + "a 4xx is the caller's error, not the server's" + ); + + assert.strictEqual(outgoingSpan.status.code, SpanStatusCode.ERROR); + assert.strictEqual(outgoingSpan.attributes[ATTR_ERROR_TYPE], '404'); + }); + const httpErrorCodes = [ 400, 401, 403, 404, 429, 501, 503, 504, 500, 505, 597, ]; diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts index ea0080b6a00..e5b2a509c71 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts @@ -60,6 +60,52 @@ describe('Utility', () => { }); }); + describe('parseErrorType()', () => { + it('should return the status code as a string for an error', () => { + assert.strictEqual(utils.parseErrorType(SpanKind.CLIENT, 404), '404'); + assert.strictEqual(utils.parseErrorType(SpanKind.CLIENT, 500), '500'); + assert.strictEqual(utils.parseErrorType(SpanKind.SERVER, 500), '500'); + }); + + it('should return undefined for a successful status code', () => { + for (let index = 100; index < 400; index++) { + assert.strictEqual( + utils.parseErrorType(SpanKind.CLIENT, index), + undefined + ); + assert.strictEqual( + utils.parseErrorType(SpanKind.SERVER, index), + undefined + ); + } + }); + + it('should treat 4xx as an error on a client span only', () => { + for (let index = 400; index < 500; index++) { + assert.strictEqual( + utils.parseErrorType(SpanKind.CLIENT, index), + String(index) + ); + assert.strictEqual( + utils.parseErrorType(SpanKind.SERVER, index), + undefined + ); + } + }); + + it('should return undefined when no status code was received', () => { + assert.strictEqual( + utils.parseErrorType(SpanKind.CLIENT, undefined), + undefined + ); + assert.strictEqual( + utils.parseErrorType(SpanKind.CLIENT, '500'), + undefined + ); + assert.strictEqual(utils.parseErrorType(SpanKind.CLIENT, 600), undefined); + }); + }); + describe('getRequestInfo()', () => { it('should get options object', () => { const webUrl = 'http://u:p@google.fr/aPath?qu=ry'; diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts b/experimental/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts index 1bc2144b00d..568fe69dc8f 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts @@ -7,6 +7,7 @@ import { isValidSpanId, SpanKind } from '@opentelemetry/api'; import { hrTimeToNanoseconds } from '@opentelemetry/core'; import type { ReadableSpan } from '@opentelemetry/sdk-trace'; import { + ATTR_ERROR_TYPE, ATTR_HTTP_REQUEST_METHOD, ATTR_HTTP_RESPONSE_STATUS_CODE, ATTR_NETWORK_PEER_ADDRESS, @@ -79,6 +80,14 @@ export const assertSpan = ( } ); + // A forced status comes from an exception, which carries its own error.type. + if (!validations.forceStatus) { + assert.strictEqual( + span.attributes[ATTR_ERROR_TYPE], + utils.parseErrorType(span.kind, validations.httpStatusCode) + ); + } + assert.ok(span.endTime, 'must be finished'); assert.ok(hrTimeToNanoseconds(span.duration), 'must have positive duration');