diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts index 6460daf5eda..d3603ca9ab7 100644 --- a/packages/opentelemetry-plugin-http/src/http.ts +++ b/packages/opentelemetry-plugin-http/src/http.ts @@ -276,7 +276,9 @@ export class HttpPlugin extends BasePlugin { // Cannot pass args of type ResponseEndArgs, // tslint complains "Expected 1-2 arguments, but got 1 or more.", it does not make sense to me // tslint:disable-next-line:no-any - const returned = response.end.apply(this, arguments as any); + const returned = plugin._safeExecute(span, () => + response.end.apply(this, arguments as any) + ); const requestUrl = request.url ? url.parse(request.url) : null; const hostname = headers.host ? headers.host.replace(/^(.*)(\:[0-9]{1,5})/, '$1') @@ -314,7 +316,10 @@ export class HttpPlugin extends BasePlugin { span.end(); return returned; }; - return original.apply(this, [event, ...args]); + + return plugin._safeExecute(span, () => + original.apply(this, [event, ...args]) + ); }); }; } @@ -328,7 +333,7 @@ export class HttpPlugin extends BasePlugin { options: RequestOptions | string, ...args: unknown[] ): ClientRequest { - if (!options) { + if (!Utils.isValidOptionsType(options)) { return original.apply(this, [options, ...args]); } @@ -358,7 +363,9 @@ export class HttpPlugin extends BasePlugin { .getHttpTextFormat() .inject(span.context(), Format.HTTP, options.headers); - const request: ClientRequest = original.apply(this, [options, ...args]); + const request: ClientRequest = plugin._safeExecute(span, () => + original.apply(this, [options, ...args]) + ); plugin._logger.debug('%s plugin outgoingRequest', plugin.moduleName); plugin._tracer.bind(request); @@ -385,6 +392,18 @@ export class HttpPlugin extends BasePlugin { .startSpan(name, options) .setAttribute(AttributeNames.COMPONENT, HttpPlugin.component); } + + private _safeExecute ReturnType>( + span: Span, + execute: T + ): ReturnType { + try { + return execute(); + } catch (error) { + Utils.setSpanWithError(span, error); + throw error; + } + } } export const plugin = new HttpPlugin( diff --git a/packages/opentelemetry-plugin-http/src/types.ts b/packages/opentelemetry-plugin-http/src/types.ts index bde2c4a6243..831d4ba7fdb 100644 --- a/packages/opentelemetry-plugin-http/src/types.ts +++ b/packages/opentelemetry-plugin-http/src/types.ts @@ -62,3 +62,11 @@ export interface HttpPluginConfig { ignoreOutgoingUrls?: IgnoreMatcher[]; applyCustomAttributesOnSpan?: HttpCustomAttributeFunction; } + +export interface Err extends Error { + errno?: number; + code?: string; + path?: string; + syscall?: string; + stack?: string; +} diff --git a/packages/opentelemetry-plugin-http/src/utils.ts b/packages/opentelemetry-plugin-http/src/utils.ts index 73907049005..6d37539976a 100644 --- a/packages/opentelemetry-plugin-http/src/utils.ts +++ b/packages/opentelemetry-plugin-http/src/utils.ts @@ -21,7 +21,7 @@ import { ClientRequest, IncomingHttpHeaders, } from 'http'; -import { IgnoreMatcher } from './types'; +import { IgnoreMatcher, Err } from './types'; import { AttributeNames } from './enums/AttributeNames'; import * as url from 'url'; @@ -138,26 +138,40 @@ export class Utils { * @param obj to subscribe on error */ static setSpanOnError(span: Span, obj: IncomingMessage | ClientRequest) { - obj.on('error', error => { - span.setAttributes({ - [AttributeNames.HTTP_ERROR_NAME]: error.name, - [AttributeNames.HTTP_ERROR_MESSAGE]: error.message, - }); - - let status: Status; - if ((obj as IncomingMessage).statusCode) { - status = Utils.parseResponseStatus( - (obj as IncomingMessage).statusCode! - ); - } else { - status = { code: CanonicalCode.UNKNOWN }; - } + obj.on('error', (error: Err) => { + Utils.setSpanWithError(span, error, obj); + }); + } - status.message = error.message; + /** + * Sets the span with the error passed in params + * @param {Span} span the span that need to be set + * @param {Error} error error that will be set to span + * @param {(IncomingMessage | ClientRequest)} [obj] used for enriching the status by checking the statusCode. + */ + static setSpanWithError( + span: Span, + error: Err, + obj?: IncomingMessage | ClientRequest + ) { + const message = error.message; - span.setStatus(status); - span.end(); + span.setAttributes({ + [AttributeNames.HTTP_ERROR_NAME]: error.name, + [AttributeNames.HTTP_ERROR_MESSAGE]: message, }); + + let status: Status; + if (obj && (obj as IncomingMessage).statusCode) { + status = Utils.parseResponseStatus((obj as IncomingMessage).statusCode!); + } else { + status = { code: CanonicalCode.UNKNOWN }; + } + + status.message = message; + + span.setStatus(status); + span.end(); } /** @@ -172,7 +186,6 @@ export class Utils { let pathname = '/'; let origin = ''; let optionsParsed: url.URL | url.UrlWithStringQuery | RequestOptions; - if (typeof options === 'string') { optionsParsed = url.parse(options); pathname = (optionsParsed as url.UrlWithStringQuery).pathname || '/'; @@ -181,15 +194,13 @@ export class Utils { Object.assign(optionsParsed, extraOptions); } } else { - optionsParsed = options; - try { - pathname = (options as url.URL).pathname; - if (!pathname && options.path) { - pathname = url.parse(options.path).pathname || '/'; - } - origin = `${options.protocol || 'http:'}//${options.host || - `${options.hostname}:${options.port}`}`; - } catch (ignore) {} + optionsParsed = options as RequestOptions; + pathname = (options as url.URL).pathname; + if (!pathname && optionsParsed.path) { + pathname = url.parse(optionsParsed.path).pathname || '/'; + } + origin = `${optionsParsed.protocol || 'http:'}//${optionsParsed.host || + `${optionsParsed.hostname}:${optionsParsed.port}`}`; } if (Utils.hasExpectHeader(optionsParsed)) { @@ -207,4 +218,17 @@ export class Utils { return { origin, pathname, method, optionsParsed }; } + + /** + * Makes sure options is of type string or object + * @param options for the request + */ + static isValidOptionsType(options: unknown): boolean { + if (!options) { + return false; + } + + const type = typeof options; + return type === 'string' || (type === 'object' && !Array.isArray(options)); + } } diff --git a/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts b/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts index 291b11cca22..5dbf00c842e 100644 --- a/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts +++ b/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts @@ -30,7 +30,7 @@ import { } from '@opentelemetry/basic-tracer'; let server: http.Server; -const serverPort = 12345; +const serverPort = 22345; const protocol = 'http'; const hostname = 'localhost'; const pathname = '/test'; @@ -139,70 +139,67 @@ describe('HttpPlugin', () => { const isReset = memoryExporter.getFinishedSpans().length === 0; assert.ok(isReset); - await httpRequest - .get(`${protocol}://${hostname}${testPath}`) - .then(result => { - const spans = memoryExporter.getFinishedSpans(); - const reqSpan = spans[0]; - - assert.strictEqual(result.data, httpErrorCodes[i].toString()); - assert.strictEqual(spans.length, 1); - - const validations = { - hostname, - httpStatusCode: result.statusCode!, - httpMethod: 'GET', - pathname: testPath, - resHeaders: result.resHeaders, - reqHeaders: result.reqHeaders, - }; - - assertSpan(reqSpan, SpanKind.CLIENT, validations); - }); + + const result = await httpRequest.get( + `${protocol}://${hostname}${testPath}` + ); + const spans = memoryExporter.getFinishedSpans(); + const reqSpan = spans[0]; + + assert.strictEqual(result.data, httpErrorCodes[i].toString()); + assert.strictEqual(spans.length, 1); + + const validations = { + hostname, + httpStatusCode: result.statusCode!, + httpMethod: 'GET', + pathname: testPath, + resHeaders: result.resHeaders, + reqHeaders: result.reqHeaders, + }; + + assertSpan(reqSpan, SpanKind.CLIENT, validations); }); } - it('should create a child span for GET requests', done => { + it('should create a child span for GET requests', async () => { const testPath = '/outgoing/rootSpan/childs/1'; doNock(hostname, testPath, 200, 'Ok'); const name = 'TestRootSpan'; const span = tracer.startSpan(name); - return tracer.withSpan(span, () => { - httpRequest - .get(`${protocol}://${hostname}${testPath}`) - .then(result => { - span.end(); - const spans = memoryExporter.getFinishedSpans(); - const [reqSpan, localSpan] = spans; - const validations = { - hostname, - httpStatusCode: result.statusCode!, - httpMethod: 'GET', - pathname: testPath, - resHeaders: result.resHeaders, - reqHeaders: result.reqHeaders, - }; - - assert.ok(localSpan.name.indexOf('TestRootSpan') >= 0); - assert.strictEqual(spans.length, 2); - assert.ok(reqSpan.name.indexOf(testPath) >= 0); - assert.strictEqual( - localSpan.spanContext.traceId, - reqSpan.spanContext.traceId - ); - assertSpan(reqSpan, SpanKind.CLIENT, validations); - assert.notStrictEqual( - localSpan.spanContext.spanId, - reqSpan.spanContext.spanId - ); - done(); - }) - .catch(done); + return tracer.withSpan(span, async () => { + const result = await httpRequest.get( + `${protocol}://${hostname}${testPath}` + ); + span.end(); + const spans = memoryExporter.getFinishedSpans(); + const [reqSpan, localSpan] = spans; + const validations = { + hostname, + httpStatusCode: result.statusCode!, + httpMethod: 'GET', + pathname: testPath, + resHeaders: result.resHeaders, + reqHeaders: result.reqHeaders, + }; + + assert.ok(localSpan.name.indexOf('TestRootSpan') >= 0); + assert.strictEqual(spans.length, 2); + assert.ok(reqSpan.name.indexOf(testPath) >= 0); + assert.strictEqual( + localSpan.spanContext.traceId, + reqSpan.spanContext.traceId + ); + assertSpan(reqSpan, SpanKind.CLIENT, validations); + assert.notStrictEqual( + localSpan.spanContext.spanId, + reqSpan.spanContext.spanId + ); }); }); for (let i = 0; i < httpErrorCodes.length; i++) { - it(`should test a child spans for GET requests with http error ${httpErrorCodes[i]}`, done => { + it(`should test child spans for GET requests with http error ${httpErrorCodes[i]}`, async () => { const testPath = '/outgoing/rootSpan/childs/1'; doNock( hostname, @@ -212,37 +209,34 @@ describe('HttpPlugin', () => { ); const name = 'TestRootSpan'; const span = tracer.startSpan(name); - tracer.withSpan(span, () => { - httpRequest - .get(`${protocol}://${hostname}${testPath}`) - .then(result => { - span.end(); - const spans = memoryExporter.getFinishedSpans(); - const [reqSpan, localSpan] = spans; - const validations = { - hostname, - httpStatusCode: result.statusCode!, - httpMethod: 'GET', - pathname: testPath, - resHeaders: result.resHeaders, - reqHeaders: result.reqHeaders, - }; - - assert.ok(localSpan.name.indexOf('TestRootSpan') >= 0); - assert.strictEqual(spans.length, 2); - assert.ok(reqSpan.name.indexOf(testPath) >= 0); - assert.strictEqual( - localSpan.spanContext.traceId, - reqSpan.spanContext.traceId - ); - assertSpan(reqSpan, SpanKind.CLIENT, validations); - assert.notStrictEqual( - localSpan.spanContext.spanId, - reqSpan.spanContext.spanId - ); - done(); - }) - .catch(done); + return tracer.withSpan(span, async () => { + const result = await httpRequest.get( + `${protocol}://${hostname}${testPath}` + ); + span.end(); + const spans = memoryExporter.getFinishedSpans(); + const [reqSpan, localSpan] = spans; + const validations = { + hostname, + httpStatusCode: result.statusCode!, + httpMethod: 'GET', + pathname: testPath, + resHeaders: result.resHeaders, + reqHeaders: result.reqHeaders, + }; + + assert.ok(localSpan.name.indexOf('TestRootSpan') >= 0); + assert.strictEqual(spans.length, 2); + assert.ok(reqSpan.name.indexOf(testPath) >= 0); + assert.strictEqual( + localSpan.spanContext.traceId, + reqSpan.spanContext.traceId + ); + assertSpan(reqSpan, SpanKind.CLIENT, validations); + assert.notStrictEqual( + localSpan.spanContext.spanId, + reqSpan.spanContext.spanId + ); }); }); } @@ -281,5 +275,79 @@ describe('HttpPlugin', () => { assert.strictEqual(spans.length, 0); }); } + + for (const arg of ['string', '', {}, new Date()]) { + it(`should be tracable and not throw exception in http plugin when passing the following argument ${JSON.stringify( + arg + )}`, async () => { + try { + await httpRequest.get(arg); + } catch (error) { + // http request has been made + // nock throw + assert.ok(error.message.startsWith('Nock: No match for request')); + } + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1); + }); + } + + for (const arg of [true, 1, false, 0]) { + it(`should not throw exception in http plugin when passing the following argument ${JSON.stringify( + arg + )}`, async () => { + try { + // @ts-ignore + await httpRequest.get(arg); + } catch (error) { + // http request has been made + // nock throw + assert.ok( + error.stack.indexOf('/node_modules/nock/lib/intercept.js') > 0 + ); + } + const spans = memoryExporter.getFinishedSpans(); + // for this arg with don't provide trace. We pass arg to original method (http.get) + assert.strictEqual(spans.length, 0); + }); + } + + it('should have 1 ended span when request throw on bad "options" object', () => { + try { + http.request({ protocol: 'telnet' }); + } catch (error) { + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1); + } + }); + + it('should have 1 ended span when response.end throw an exception', async () => { + const testPath = '/outgoing/rootSpan/childs/1'; + doNock(hostname, testPath, 400, 'Not Ok'); + + const promiseRequest = new Promise((resolve, reject) => { + const req = http.request( + `http://${hostname}${testPath}`, + (resp: http.IncomingMessage) => { + let data = ''; + resp.on('data', chunk => { + data += chunk; + }); + resp.on('end', () => { + reject(new Error(data)); + }); + } + ); + return req.end(); + }); + + try { + await promiseRequest; + assert.fail(); + } catch (error) { + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1); + } + }); }); }); diff --git a/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts b/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts index 67e93cf80ee..c39b294163c 100644 --- a/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts +++ b/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts @@ -17,10 +17,13 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import * as url from 'url'; -import { CanonicalCode, Attributes } from '@opentelemetry/types'; +import { CanonicalCode, Attributes, SpanKind } from '@opentelemetry/types'; +import { NoopScopeManager } from '@opentelemetry/scope-base'; import { IgnoreMatcher } from '../../src/types'; import { Utils } from '../../src/utils'; import * as http from 'http'; +import { Span, BasicTracer } from '@opentelemetry/basic-tracer'; +import { AttributeNames } from '../../src'; describe('Utils', () => { describe('parseResponseStatus()', () => { @@ -199,4 +202,42 @@ describe('Utils', () => { }); }); }); + + describe('setSpanWithError()', () => { + it('should have error attributes', () => { + const span = new Span( + new BasicTracer({ + scopeManager: new NoopScopeManager(), + }), + 'test', + { spanId: '', traceId: '' }, + SpanKind.INTERNAL + ); + const errorMessage = 'test error'; + Utils.setSpanWithError(span, new Error(errorMessage)); + const attributes = span.toReadableSpan().attributes; + assert.strictEqual( + attributes[AttributeNames.HTTP_ERROR_MESSAGE], + errorMessage + ); + assert.ok(attributes[AttributeNames.HTTP_ERROR_NAME]); + }); + }); + + describe('isValidOptionsType()', () => { + ['', false, true, 1, 0, []].forEach(options => { + it(`should return false with the following value: ${JSON.stringify( + options + )}`, () => { + assert.strictEqual(Utils.isValidOptionsType(options), false); + }); + }); + for (const options of ['url', url.parse('http://url.com'), {}]) { + it(`should return true with the following value: ${JSON.stringify( + options + )}`, () => { + assert.strictEqual(Utils.isValidOptionsType(options), true); + }); + } + }); }); diff --git a/packages/opentelemetry-plugin-http/test/integrations/http-enable.test.ts b/packages/opentelemetry-plugin-http/test/integrations/http-enable.test.ts index fb2aa7d9c39..7a612e65393 100644 --- a/packages/opentelemetry-plugin-http/test/integrations/http-enable.test.ts +++ b/packages/opentelemetry-plugin-http/test/integrations/http-enable.test.ts @@ -30,7 +30,7 @@ import { SimpleSpanProcessor, } from '@opentelemetry/basic-tracer'; -const serverPort = 12345; +const serverPort = 32345; const hostname = 'localhost'; const memoryExporter = new InMemorySpanExporter(); @@ -112,55 +112,54 @@ describe('HttpPlugin Integration tests', () => { }); it('custom attributes should show up on client spans', async () => { - await httpRequest.get(`http://google.fr/`).then(result => { - const spans = memoryExporter.getFinishedSpans(); - const span = spans[0]; - const validations = { - hostname: 'google.fr', - httpStatusCode: result.statusCode!, - httpMethod: 'GET', - pathname: '/', - resHeaders: result.resHeaders, - reqHeaders: result.reqHeaders, - }; + const result = await httpRequest.get(`http://google.fr/`); + const spans = memoryExporter.getFinishedSpans(); + const span = spans[0]; + const validations = { + hostname: 'google.fr', + httpStatusCode: result.statusCode!, + httpMethod: 'GET', + pathname: '/', + resHeaders: result.resHeaders, + reqHeaders: result.reqHeaders, + }; - assert.strictEqual(spans.length, 1); - assert.ok(span.name.indexOf('GET /') >= 0); - assert.strictEqual(span.attributes['span kind'], SpanKind.CLIENT); - assertSpan(span, SpanKind.CLIENT, validations); - }); + assert.strictEqual(spans.length, 1); + assert.ok(span.name.indexOf('GET /') >= 0); + assert.strictEqual(span.attributes['span kind'], SpanKind.CLIENT); + assertSpan(span, SpanKind.CLIENT, validations); }); it('should create a span for GET requests and add propagation headers with Expect headers', async () => { - const spans = memoryExporter.getFinishedSpans(); + let spans = memoryExporter.getFinishedSpans(); assert.strictEqual(spans.length, 0); const options = Object.assign( { headers: { Expect: '100-continue' } }, url.parse('http://google.fr/') ); - await httpRequest.get(options).then(result => { - const spans = memoryExporter.getFinishedSpans(); - const span = spans[0]; - const validations = { - hostname: 'google.fr', - httpStatusCode: 301, - httpMethod: 'GET', - pathname: '/', - resHeaders: result.resHeaders, - reqHeaders: result.reqHeaders, - }; - assert.strictEqual(spans.length, 1); - assert.ok(span.name.indexOf('GET /') >= 0); + const result = await httpRequest.get(options); + spans = memoryExporter.getFinishedSpans(); + const span = spans[0]; + const validations = { + hostname: 'google.fr', + httpStatusCode: 301, + httpMethod: 'GET', + pathname: '/', + resHeaders: result.resHeaders, + reqHeaders: result.reqHeaders, + }; - try { - assertSpan(span, SpanKind.CLIENT, validations); - } catch (error) { - // temporary redirect is also correct - validations.httpStatusCode = 307; - assertSpan(span, SpanKind.CLIENT, validations); - } - }); + assert.strictEqual(spans.length, 1); + assert.ok(span.name.indexOf('GET /') >= 0); + + try { + assertSpan(span, SpanKind.CLIENT, validations); + } catch (error) { + // temporary redirect is also correct + validations.httpStatusCode = 307; + assertSpan(span, SpanKind.CLIENT, validations); + } }); for (const headers of [ { Expect: '100-continue', 'user-agent': 'http-plugin-test' }, diff --git a/packages/opentelemetry-plugin-http/test/utils/httpRequest.ts b/packages/opentelemetry-plugin-http/test/utils/httpRequest.ts index 40253fd4947..e53d09b9938 100644 --- a/packages/opentelemetry-plugin-http/test/utils/httpRequest.ts +++ b/packages/opentelemetry-plugin-http/test/utils/httpRequest.ts @@ -37,7 +37,7 @@ export const httpRequest = { }) : options; return new Promise((resolve, reject) => { - return http.get(_options, (resp: http.IncomingMessage) => { + const req = http.get(_options, (resp: http.IncomingMessage) => { const res = (resp as unknown) as http.IncomingMessage & { req: http.IncomingMessage; }; @@ -62,6 +62,10 @@ export const httpRequest = { reject(err); }); }); + req.on('error', err => { + reject(err); + }); + return req; }); }, };