fix(http-plugin): ensure exceptions are handled (open-telemetry#273)

* fix(http-plugin): ensure exceptions are handled

refactor(test): remove unnecessary code
add tests
closes open-telemetry#222

Signed-off-by: Olivier Albertini <[email protected]>

Author: OlivierAlbertini

packages/opentelemetry-plugin-http/src/http.ts

@@ -276,7 +276,9 @@ export class HttpPlugin extends BasePlugin<Http> {
           // 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<Http> {
           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<Http> {
       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<Http> {
         .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<Http> {
       .startSpan(name, options)
       .setAttribute(AttributeNames.COMPONENT, HttpPlugin.component);
   }
+
+  private _safeExecute<T extends (...args: unknown[]) => ReturnType<T>>(
+    span: Span,
+    execute: T
+  ): ReturnType<T> {
+    try {
+      return execute();
+    } catch (error) {
+      Utils.setSpanWithError(span, error);
+      throw error;
+    }
+  }
 }
 
 export const plugin = new HttpPlugin(

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;
+}

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));
+  }
 }