From 137bf9209605dc3909a97cc1bfd89a3170301eab Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Wed, 14 Jul 2021 17:33:49 +0300 Subject: [PATCH 1/5] fix(instrumentation-http): set outgoing request attributes on start span --- .../src/http.ts | 47 +++++++++++-------- .../test/functionals/http-enable.test.ts | 21 ++++++++- .../test/utils/assertSpan.ts | 21 +++++---- 3 files changed, 59 insertions(+), 30 deletions(-) diff --git a/packages/opentelemetry-instrumentation-http/src/http.ts b/packages/opentelemetry-instrumentation-http/src/http.ts index 4b52635eef..5b29b5c05b 100644 --- a/packages/opentelemetry-instrumentation-http/src/http.ts +++ b/packages/opentelemetry-instrumentation-http/src/http.ts @@ -38,7 +38,6 @@ import { HttpInstrumentationConfig, HttpRequestArgs, Https, - ParsedRequestOptions, ResponseEndArgs, } from './types'; import * as utils from './utils'; @@ -72,7 +71,9 @@ export class HttpInstrumentation extends InstrumentationBase { return this._config; } - override setConfig(config: HttpInstrumentationConfig & InstrumentationConfig = {}) { + override setConfig( + config: HttpInstrumentationConfig & InstrumentationConfig = {} + ) { this._config = Object.assign({}, config); } @@ -272,20 +273,10 @@ export class HttpInstrumentation extends InstrumentationBase { * @param span representing the current operation */ private _traceClientRequest( - component: 'http' | 'https', request: http.ClientRequest, - options: ParsedRequestOptions, + hostname: string, span: Span ): http.ClientRequest { - const hostname = - options.hostname || - options.host?.replace(/^(.*)(:[0-9]{1,5})/, '$1') || - 'localhost'; - const attributes = utils.getOutgoingRequestAttributes(options, { - component, - hostname, - }); - span.setAttributes(attributes); if (this._getConfig().requestHook) { this._callRequestHook(span, request); } @@ -378,13 +369,17 @@ export class HttpInstrumentation extends InstrumentationBase { : '/'; const method = request.method || 'GET'; - instrumentation._diag.debug('%s instrumentation incomingRequest', component); + instrumentation._diag.debug( + '%s instrumentation incomingRequest', + component + ); if ( utils.isIgnored( pathname, instrumentation._getConfig().ignoreIncomingPaths, - (e: Error) => instrumentation._diag.error('caught ignoreIncomingPaths error: ', e) + (e: Error) => + instrumentation._diag.error('caught ignoreIncomingPaths error: ', e) ) ) { return context.with(suppressTracing(context.active()), () => { @@ -528,15 +523,27 @@ export class HttpInstrumentation extends InstrumentationBase { utils.isIgnored( origin + pathname, instrumentation._getConfig().ignoreOutgoingUrls, - (e: Error) => instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e) + (e: Error) => + instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e) ) ) { return original.apply(this, [optionsParsed, ...args]); } const operationName = `${component.toUpperCase()} ${method}`; + + const hostname = + optionsParsed.hostname || + optionsParsed.host?.replace(/^(.*)(:[0-9]{1,5})/, '$1') || + 'localhost'; + const attributes = utils.getOutgoingRequestAttributes(optionsParsed, { + component, + hostname, + }); + const spanOptions: SpanOptions = { kind: SpanKind.CLIENT, + attributes, }; const span = instrumentation._startHttpSpan(operationName, spanOptions); @@ -569,12 +576,14 @@ export class HttpInstrumentation extends InstrumentationBase { } ); - instrumentation._diag.debug('%s instrumentation outgoingRequest', component); + instrumentation._diag.debug( + '%s instrumentation outgoingRequest', + component + ); context.bind(parentContext, request); return instrumentation._traceClientRequest( - component, request, - optionsParsed, + hostname, span ); }); diff --git a/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts b/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts index 218dbf2f4d..460cc4f776 100644 --- a/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts +++ b/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts @@ -479,12 +479,29 @@ describe('HttpInstrumentation', () => { }); } - it('should have 1 ended span when request throw on bad "options" object', () => { + it('should have 1 ended span when request throw on bad "options" object', done => { try { - http.request({ protocol: 'telnet' }); + http.request({ headers: { cookie: undefined} }); // <===== this makes http.request throw + done('exception should have being thrown but did not'); } catch (error) { const spans = memoryExporter.getFinishedSpans(); assert.strictEqual(spans.length, 1); + + const validations = { + httpStatusCode: undefined, + httpMethod: 'GET', + resHeaders: {}, + hostname: 'localhost', + pathname: '/', + forceStatus: { + code: SpanStatusCode.ERROR, + message: 'Invalid value "undefined" for header "cookie"', + }, + component: 'http', + noNetPeer: true, + } + assertSpan(spans[0], SpanKind.CLIENT, validations); + done(); } }); diff --git a/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts b/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts index 91263eb945..b28c7dfcaf 100644 --- a/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts +++ b/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts @@ -27,7 +27,7 @@ export const assertSpan = ( span: ReadableSpan, kind: SpanKind, validations: { - httpStatusCode: number; + httpStatusCode?: number; httpMethod: string; resHeaders: http.IncomingHttpHeaders; hostname: string; @@ -37,6 +37,7 @@ export const assertSpan = ( forceStatus?: SpanStatus; serverName?: string; component: string; + noNetPeer?: boolean; // we don't expect net peer info when request throw before being sent } ) => { assert.strictEqual(span.spanContext().traceId.length, 32); @@ -110,14 +111,16 @@ export const assertSpan = ( validations.hostname, 'must be consistent (PEER_NAME and hostname)' ); - assert.ok( - span.attributes[SemanticAttributes.NET_PEER_IP], - 'must have PEER_IP' - ); - assert.ok( - span.attributes[SemanticAttributes.NET_PEER_PORT], - 'must have PEER_PORT' - ); + if(!validations.noNetPeer) { + assert.ok( + span.attributes[SemanticAttributes.NET_PEER_IP], + 'must have PEER_IP' + ); + assert.ok( + span.attributes[SemanticAttributes.NET_PEER_PORT], + 'must have PEER_PORT' + ); + } assert.ok( (span.attributes[SemanticAttributes.HTTP_URL] as string).indexOf( span.attributes[SemanticAttributes.NET_PEER_NAME] as string From ce3f90916bad4bdaa8b97cf15d902e390206d1a3 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Wed, 14 Jul 2021 18:03:15 +0300 Subject: [PATCH 2/5] fix(instrumentation-http): build error --- packages/opentelemetry-instrumentation-http/src/utils.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-instrumentation-http/src/utils.ts b/packages/opentelemetry-instrumentation-http/src/utils.ts index fa1c55362a..194195142f 100644 --- a/packages/opentelemetry-instrumentation-http/src/utils.ts +++ b/packages/opentelemetry-instrumentation-http/src/utils.ts @@ -69,8 +69,13 @@ export const getAbsoluteUrl = ( * Parse status code from HTTP response. [More details](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#status) */ export const parseResponseStatus = ( - statusCode: number + statusCode: number | undefined, ): Omit => { + + if(statusCode === undefined) { + return { code: SpanStatusCode.ERROR }; + } + // 1xx, 2xx, 3xx are OK if (statusCode >= 100 && statusCode < 400) { return { code: SpanStatusCode.OK }; From 2ab846827fb15406e9d654c01dda4bee6b74a469 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Wed, 14 Jul 2021 18:25:50 +0300 Subject: [PATCH 3/5] fix(instrumentation-http): error message in node 8 is different --- .../test/functionals/http-enable.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts b/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts index 460cc4f776..86bd1a7081 100644 --- a/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts +++ b/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts @@ -482,7 +482,7 @@ describe('HttpInstrumentation', () => { it('should have 1 ended span when request throw on bad "options" object', done => { try { http.request({ headers: { cookie: undefined} }); // <===== this makes http.request throw - done('exception should have being thrown but did not'); + done('exception should have been thrown but did not'); } catch (error) { const spans = memoryExporter.getFinishedSpans(); assert.strictEqual(spans.length, 1); @@ -495,7 +495,7 @@ describe('HttpInstrumentation', () => { pathname: '/', forceStatus: { code: SpanStatusCode.ERROR, - message: 'Invalid value "undefined" for header "cookie"', + message: error.message, }, component: 'http', noNetPeer: true, From 6cd74b8cee4cdf0c1c499150dd6bfa589aafe908 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Wed, 14 Jul 2021 18:35:35 +0300 Subject: [PATCH 4/5] revert(instrumentation-http): line width changes --- .../src/http.ts | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/packages/opentelemetry-instrumentation-http/src/http.ts b/packages/opentelemetry-instrumentation-http/src/http.ts index 5b29b5c05b..579d257003 100644 --- a/packages/opentelemetry-instrumentation-http/src/http.ts +++ b/packages/opentelemetry-instrumentation-http/src/http.ts @@ -71,9 +71,7 @@ export class HttpInstrumentation extends InstrumentationBase { return this._config; } - override setConfig( - config: HttpInstrumentationConfig & InstrumentationConfig = {} - ) { + override setConfig(config: HttpInstrumentationConfig & InstrumentationConfig = {}) { this._config = Object.assign({}, config); } @@ -369,17 +367,13 @@ export class HttpInstrumentation extends InstrumentationBase { : '/'; const method = request.method || 'GET'; - instrumentation._diag.debug( - '%s instrumentation incomingRequest', - component - ); + instrumentation._diag.debug('%s instrumentation incomingRequest', component); if ( utils.isIgnored( pathname, instrumentation._getConfig().ignoreIncomingPaths, - (e: Error) => - instrumentation._diag.error('caught ignoreIncomingPaths error: ', e) + (e: Error) => instrumentation._diag.error('caught ignoreIncomingPaths error: ', e) ) ) { return context.with(suppressTracing(context.active()), () => { @@ -523,8 +517,7 @@ export class HttpInstrumentation extends InstrumentationBase { utils.isIgnored( origin + pathname, instrumentation._getConfig().ignoreOutgoingUrls, - (e: Error) => - instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e) + (e: Error) => instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e) ) ) { return original.apply(this, [optionsParsed, ...args]); @@ -576,10 +569,7 @@ export class HttpInstrumentation extends InstrumentationBase { } ); - instrumentation._diag.debug( - '%s instrumentation outgoingRequest', - component - ); + instrumentation._diag.debug('%s instrumentation outgoingRequest', component); context.bind(parentContext, request); return instrumentation._traceClientRequest( request, From 22b61663132a79924e1e3a17d736ee1d8c20fc3f Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Thu, 15 Jul 2021 10:39:04 +0300 Subject: [PATCH 5/5] refactor(instrumentation-http): use assert.throws in test --- .../test/functionals/http-enable.test.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts b/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts index 86bd1a7081..0debb99b71 100644 --- a/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts +++ b/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts @@ -479,11 +479,8 @@ describe('HttpInstrumentation', () => { }); } - it('should have 1 ended span when request throw on bad "options" object', done => { - try { - http.request({ headers: { cookie: undefined} }); // <===== this makes http.request throw - done('exception should have been thrown but did not'); - } catch (error) { + it('should have 1 ended span when request throw on bad "options" object', () => { + assert.throws(() => http.request({ headers: { cookie: undefined} }), err => { const spans = memoryExporter.getFinishedSpans(); assert.strictEqual(spans.length, 1); @@ -495,14 +492,14 @@ describe('HttpInstrumentation', () => { pathname: '/', forceStatus: { code: SpanStatusCode.ERROR, - message: error.message, + message: err.message, }, component: 'http', noNetPeer: true, } assertSpan(spans[0], SpanKind.CLIENT, validations); - done(); - } + return true; + }); }); it('should have 1 ended span when response.end throw an exception', async () => {