fix(plugin-http): improve formatting for url attribute (open-telemetr…

…y#317) closes open-telemetry#316 Signed-off-by: Olivier Albertini <[email protected]>
srikanthccv · Sep 23, 2019 · cee42d8 · cee42d8
1 parent cc8a27d
commit cee42d8
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 20 deletions.
diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts
@@ -179,13 +179,17 @@ export class HttpPlugin extends BasePlugin<Http> {
               response.req && response.req.method
                 ? response.req.method.toUpperCase()
                 : 'GET';
-            const headers = options.headers;
-            const userAgent = headers ? headers['user-agent'] : null;
+            const headers = options.headers || {};
+            const userAgent = headers['user-agent'];
 
             const host = options.hostname || options.host || 'localhost';
 
             const attributes: Attributes = {
-              [AttributeNames.HTTP_URL]: `${options.protocol}//${options.hostname}${options.path}`,
+              [AttributeNames.HTTP_URL]: Utils.getAbsoluteUrl(
+                options,
+                headers,
+                `${HttpPlugin.component}:`
+              ),
               [AttributeNames.HTTP_HOSTNAME]: host,
               [AttributeNames.HTTP_METHOD]: method,
               [AttributeNames.HTTP_PATH]: options.path || '/',
@@ -286,9 +290,10 @@ export class HttpPlugin extends BasePlugin<Http> {
           const userAgent = headers['user-agent'];
 
           const attributes: Attributes = {
-            [AttributeNames.HTTP_URL]: Utils.getUrlFromIncomingRequest(
+            [AttributeNames.HTTP_URL]: Utils.getAbsoluteUrl(
               requestUrl,
-              headers
+              headers,
+              `${HttpPlugin.component}:`
             ),
             [AttributeNames.HTTP_HOSTNAME]: hostname,
             [AttributeNames.HTTP_METHOD]: method,

diff --git a/packages/opentelemetry-plugin-http/src/utils.ts b/packages/opentelemetry-plugin-http/src/utils.ts
@@ -20,8 +20,9 @@ import {
   IncomingMessage,
   ClientRequest,
   IncomingHttpHeaders,
+  OutgoingHttpHeaders,
 } from 'http';
-import { IgnoreMatcher, Err } from './types';
+import { IgnoreMatcher, Err, ParsedRequestOptions } from './types';
 import { AttributeNames } from './enums/AttributeNames';
 import * as url from 'url';
 
@@ -30,20 +31,30 @@ import * as url from 'url';
  */
 export class Utils {
   /**
-   * return an absolute url
+   * Get an absolute url
    */
-  static getUrlFromIncomingRequest(
-    requestUrl: url.UrlWithStringQuery | null,
-    headers: IncomingHttpHeaders
+  static getAbsoluteUrl(
+    requestUrl: ParsedRequestOptions | null,
+    headers: IncomingHttpHeaders | OutgoingHttpHeaders,
+    fallbackProtocol = 'http:'
   ): string {
-    if (!requestUrl) {
-      return `http://${headers.host || 'localhost'}/`;
+    const reqUrlObject = requestUrl || {};
+    const protocol = reqUrlObject.protocol || fallbackProtocol;
+    const port = (reqUrlObject.port || '').toString();
+    const path = reqUrlObject.path || '/';
+    let host =
+      headers.host || reqUrlObject.hostname || headers.host || 'localhost';
+
+    // if there is no port in host and there is a port
+    // it should be displayed if it's not 80 and 443 (default ports)
+    if (
+      (host as string).indexOf(':') === -1 &&
+      (port && port !== '80' && port !== '443')
+    ) {
+      host += `:${port}`;
     }
 
-    return requestUrl.href && requestUrl.href.startsWith('http')
-      ? `${requestUrl.protocol}//${requestUrl.hostname}${requestUrl.path}`
-      : `${requestUrl.protocol || 'http:'}//${headers.host ||
-          'localhost'}${requestUrl.path || '/'}`;
+    return `${protocol}//${host}${path}`;
   }
   /**
    * Parse status code from HTTP response.

diff --git a/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts b/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts
@@ -165,21 +165,28 @@ describe('Utils', () => {
     });
   });
 
-  describe('getUrlFromIncomingRequest()', () => {
+  describe('getAbsoluteUrl()', () => {
     it('should return absolute url with localhost', () => {
       const path = '/test/1';
-      const result = Utils.getUrlFromIncomingRequest(url.parse(path), {});
+      const result = Utils.getAbsoluteUrl(url.parse(path), {});
       assert.strictEqual(result, `http://localhost${path}`);
     });
     it('should return absolute url', () => {
       const absUrl = 'http://www.google/test/1?query=1';
-      const result = Utils.getUrlFromIncomingRequest(url.parse(absUrl), {});
+      const result = Utils.getAbsoluteUrl(url.parse(absUrl), {});
       assert.strictEqual(result, absUrl);
     });
     it('should return default url', () => {
-      const result = Utils.getUrlFromIncomingRequest(null, {});
+      const result = Utils.getAbsoluteUrl(null, {});
       assert.strictEqual(result, 'http://localhost/');
     });
+    it("{ path: '/helloworld', port: 8080 } should return http://localhost:8080/helloworld", () => {
+      const result = Utils.getAbsoluteUrl(
+        { path: '/helloworld', port: 8080 },
+        {}
+      );
+      assert.strictEqual(result, 'http://localhost:8080/helloworld');
+    });
   });
   describe('setSpanOnError()', () => {
     it('should call span methods when we get an error event', done => {