diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 328cc1e9f1..95be6b4e3d 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -12,6 +12,8 @@ All notable changes to experimental packages in this project will be documented ### :bug: (Bug Fix) +* fix(instrumentation-http): add `http.host` attribute before sending the request #3054 @cuichenli + ### :books: (Refine Doc) ### :house: (Internal) diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts index c0c01b762c..5459e1d26f 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts @@ -292,7 +292,6 @@ export class HttpInstrumentation extends InstrumentationBase { (response: http.IncomingMessage & { aborted?: boolean }) => { const responseAttributes = utils.getOutgoingRequestAttributesOnResponse( response, - { hostname } ); span.setAttributes(responseAttributes); if (this._getConfig().responseHook) { @@ -561,13 +560,11 @@ export class HttpInstrumentation extends InstrumentationBase { } const operationName = `${component.toUpperCase()} ${method}`; + const { hostname, port } = utils.extractHostnameAndPort(optionsParsed); - const hostname = - optionsParsed.hostname || - optionsParsed.host?.replace(/^(.*)(:[0-9]{1,5})/, '$1') || - 'localhost'; const attributes = utils.getOutgoingRequestAttributes(optionsParsed, { component, + port, hostname, hookAttributes: instrumentation._callStartSpanHook( optionsParsed, diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts b/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts index d21dd105a9..f42f9955b3 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts @@ -295,6 +295,26 @@ export const isValidOptionsType = (options: unknown): boolean => { return type === 'string' || (type === 'object' && !Array.isArray(options)); }; +export const extractHostnameAndPort = ( + requestOptions: Pick +): { hostname: string, port: number | string } => { + if (requestOptions.hostname && requestOptions.port) { + return {hostname: requestOptions.hostname, port: requestOptions.port}; + } + const matches = requestOptions.host?.match(/^([^:/ ]+)(:\d{1,5})?/) || null; + const hostname = requestOptions.hostname || (matches === null ? 'localhost' : matches[1]); + let port = requestOptions.port; + if (!port) { + if (matches && matches[2]) { + // remove the leading ":". The extracted port would be something like ":8080" + port = matches[2].substring(1); + } else { + port = requestOptions.protocol === 'https:' ? '443' : '80'; + } + } + return {hostname, port}; +}; + /** * Returns outgoing request attributes scoped to the options passed to the request * @param {ParsedRequestOptions} requestOptions the same options used to make the request @@ -302,13 +322,10 @@ export const isValidOptionsType = (options: unknown): boolean => { */ export const getOutgoingRequestAttributes = ( requestOptions: ParsedRequestOptions, - options: { component: string; hostname: string; hookAttributes?: SpanAttributes } + options: { component: string; hostname: string; port: string | number, hookAttributes?: SpanAttributes } ): SpanAttributes => { - const host = requestOptions.host; - const hostname = - requestOptions.hostname || - host?.replace(/^(.*)(:[0-9]{1,5})/, '$1') || - 'localhost'; + const hostname = options.hostname; + const port = options.port; const requestMethod = requestOptions.method; const method = requestMethod ? requestMethod.toUpperCase() : 'GET'; const headers = requestOptions.headers || {}; @@ -322,6 +339,7 @@ export const getOutgoingRequestAttributes = ( [SemanticAttributes.HTTP_METHOD]: method, [SemanticAttributes.HTTP_TARGET]: requestOptions.path || '/', [SemanticAttributes.NET_PEER_NAME]: hostname, + [SemanticAttributes.HTTP_HOST]: requestOptions.headers?.host ?? `${hostname}:${port}`, }; if (userAgent !== undefined) { @@ -354,14 +372,12 @@ export const getAttributesFromHttpKind = (kind?: string): SpanAttributes => { */ export const getOutgoingRequestAttributesOnResponse = ( response: IncomingMessage, - options: { hostname: string } ): SpanAttributes => { const { statusCode, statusMessage, httpVersion, socket } = response; const { remoteAddress, remotePort } = socket; const attributes: SpanAttributes = { [SemanticAttributes.NET_PEER_IP]: remoteAddress, [SemanticAttributes.NET_PEER_PORT]: remotePort, - [SemanticAttributes.HTTP_HOST]: `${options.hostname}:${remotePort}`, }; setResponseContentLengthAttribute(response, attributes); 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 545ad56742..689d97f70e 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts @@ -28,11 +28,12 @@ import { IncomingMessage, ServerResponse } from 'http'; import { Socket } from 'net'; import * as sinon from 'sinon'; import * as url from 'url'; -import { IgnoreMatcher } from '../../src/types'; +import {IgnoreMatcher, ParsedRequestOptions} from '../../src/types'; import * as utils from '../../src/utils'; import { AttributeNames } from '../../src/enums/AttributeNames'; import { RPCType, setRPCMetadata } from '@opentelemetry/core'; import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; +import {extractHostnameAndPort} from '../../src/utils'; describe('Utility', () => { describe('parseResponseStatus()', () => { @@ -536,4 +537,58 @@ describe('Utility', () => { assert.deepStrictEqual(span.attributes['http.request.header.accept'], undefined); }); }); + + describe('extractHostnameAndPort', () => { + it('should return the hostname and port defined in the parsedOptions', () => { + type tmpParsedOption = Pick; + const parsedOption: tmpParsedOption = { + hostname: 'www.google.com', + port: '80', + host: 'www.google.com', + protocol: 'http:' + }; + const {hostname, port} = extractHostnameAndPort(parsedOption); + assert.strictEqual(hostname, parsedOption.hostname); + assert.strictEqual(port, parsedOption.port); + }); + + it('should return the hostname and port based on host field defined in the parsedOptions when hostname and port are missing', () => { + type tmpParsedOption = Pick; + const parsedOption: tmpParsedOption = { + hostname: null, + port: null, + host: 'www.google.com:8181', + protocol: 'http:' + }; + const {hostname, port} = extractHostnameAndPort(parsedOption); + assert.strictEqual(hostname, 'www.google.com'); + assert.strictEqual(port, '8181'); + }); + + it('should infer the port number based on protocol https when can not extract it from host field', () => { + type tmpParsedOption = Pick; + const parsedOption: tmpParsedOption = { + hostname: null, + port: null, + host: 'www.google.com', + protocol: 'https:' + }; + const {hostname, port} = extractHostnameAndPort(parsedOption); + assert.strictEqual(hostname, 'www.google.com'); + assert.strictEqual(port, '443'); + }); + + it('should infer the port number based on protocol http when can not extract it from host field', () => { + type tmpParsedOption = Pick; + const parsedOption: tmpParsedOption = { + hostname: null, + port: null, + host: 'www.google.com', + protocol: 'http:' + }; + const {hostname, port} = extractHostnameAndPort(parsedOption); + assert.strictEqual(hostname, 'www.google.com'); + assert.strictEqual(port, '80'); + }); + }); }); diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/http-enable.test.ts b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/http-enable.test.ts index c9a170861b..307dfcfea8 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/http-enable.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/http-enable.test.ts @@ -57,6 +57,11 @@ describe('HttpInstrumentation Integration tests', () => { const sockets: Array = []; before(done => { mockServer = http.createServer((req, res) => { + if (req.url === '/timeout') { + setTimeout(() => { + res.end(); + }, 1000); + } res.statusCode = 200; res.setHeader('content-type', 'application/json'); res.write( @@ -359,5 +364,24 @@ describe('HttpInstrumentation Integration tests', () => { assert.strictEqual(spans.length, 2); assert.strictEqual(span.name, 'HTTP GET'); }); + + it('should have correct spans even when request timeout', async () => { + let spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 0); + + try { + await httpRequest.get( + `${protocol}://localhost:${mockServerPort}/timeout`, {timeout: 1} + ); + } catch (err) { + assert.ok(err.message.startsWith('timeout')); + } + + spans = memoryExporter.getFinishedSpans(); + const span = spans.find(s => s.kind === SpanKind.CLIENT); + assert.ok(span); + assert.strictEqual(span.name, 'HTTP GET'); + assert.strictEqual(span.attributes[SemanticAttributes.HTTP_HOST], `localhost:${mockServerPort}`); + }); }); }); diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/utils/httpRequest.ts b/experimental/packages/opentelemetry-instrumentation-http/test/utils/httpRequest.ts index f507b7f1f1..b2c2d828cb 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/utils/httpRequest.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/utils/httpRequest.ts @@ -59,6 +59,11 @@ function get(input: any, options?: any): GetResult { req.on('error', err => { reject(err); }); + req.on('timeout', () => { + const err = new Error('timeout'); + req.emit('error', err); + reject(err); + }); return req; }); }