Fix: http(s) instrumentation URL arg (#361)

* http requests can be constructed with a WHATWG URL type object * add handling for URLs and test argument variations
honeycombio · Jun 23, 2021 · 95070c2 · 95070c2
1 parent f91d3e4
commit 95070c2
Show file tree

Hide file tree

Showing 4 changed files with 208 additions and 4 deletions.
diff --git a/lib/instrumentation/http.js b/lib/instrumentation/http.js
@@ -3,7 +3,8 @@ const url = require("url"),
   shimmer = require("shimmer"),
   api = require("../api"),
   schema = require("../schema"),
-  traceUtil = require("./trace-util");
+  traceUtil = require("./trace-util"),
+  urlUtil = require("../url-util");
 
 let instrumentHTTP = (http, opts = {}) => {
   shimmer.wrap(http, "get", function (_original) {
@@ -41,6 +42,14 @@ let instrumentHTTP = (http, opts = {}) => {
         } else {
           callback = options;
         }
+      } else if (_url instanceof URL) {
+        combinedOptions = Object.assign({}, urlUtil.urlToHttpOptions(_url));
+        if (typeof options == "object") {
+          Object.assign(combinedOptions, options);
+          callback = cb;
+        } else {
+          callback = options;
+        }
       } else if (typeof _url === "object") {
         // the two-arg form.
         combinedOptions = Object.assign({}, _url);

diff --git a/lib/instrumentation/http.test.js b/lib/instrumentation/http.test.js
@@ -40,6 +40,168 @@ afterEach(() => {
   api._resetForTesting();
 });
 
+describe("argument permutations", () => {
+  test("(options)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    let req = http.get({ hostname: "localhost", port: 9009 });
+
+    req.on("close", () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(options, callback)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    http.get({ hostname: "localhost", port: 9009 }, () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(url-string)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    let req = http.get("http://localhost:9009");
+
+    req.on("close", () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(url-string, options)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    let req = http.get("http://what.not:8888", { hostname: "localhost", port: 9009 });
+
+    req.on("close", () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(url-string, callback)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    http.get("http://localhost:9009", () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(url-string, options, callback)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    http.get("http://what.not:9999", { hostname: "localhost", port: 9009 }, () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(url-URL)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    let req = http.get(new URL("http://localhost:9009"));
+
+    req.on("close", () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(url-URL, options)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    let req = http.get(new URL("http://what.not:9999"), { hostname: "localhost", port: 9009 });
+
+    req.on("close", () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(url-URL, callback)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    http.get(new URL("http://localhost:9009"), () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+
+  test("(url-URL, options, callback)", (done) => {
+    tracker.setTracked(newMockContext());
+
+    http.get(new URL("http://what.not:9999"), { hostname: "localhost", port: 9009 }, () => {
+      expect(api._apiForTesting().sentEvents).toMatchObject([
+        {
+          [schema.EVENT_TYPE]: "http",
+          [schema.TRACE_SPAN_NAME]: "GET",
+          url: "http://localhost:9009/",
+        },
+      ]);
+      done();
+    });
+  });
+});
+
 test("url as a string", (done) => {
   tracker.setTracked(newMockContext());
 
@@ -82,7 +244,7 @@ test("url as options", (done) => {
   );
 });
 
-test("url as options with pathname and query", done => {
+test("url as options with pathname and query", (done) => {
   tracker.setTracked(newMockContext());
 
   http.get(
@@ -93,7 +255,7 @@ test("url as options with pathname and query", done => {
       pathname: "/test",
       search: "?something=true",
     },
-    res => {
+    (res) => {
       expect(res.headers[api.TRACE_HTTP_HEADER.toLowerCase()]).toBe(
         "1;trace_id=0,parent_id=51001,context=e30="
       );

diff --git a/lib/instrumentation/https.js b/lib/instrumentation/https.js
@@ -4,7 +4,8 @@ const url = require("url"),
   api = require("../api"),
   schema = require("../schema"),
   tracker = require("../async_tracker"),
-  traceUtil = require("./trace-util");
+  traceUtil = require("./trace-util"),
+  urlUtil = require("../url-util");
 
 let instrumentHTTPS = (https, opts = {}) => {
   shimmer.wrap(https, "get", function (_original) {
@@ -42,6 +43,14 @@ let instrumentHTTPS = (https, opts = {}) => {
         } else {
           callback = options;
         }
+      } else if (_url instanceof URL) {
+        combinedOptions = Object.assign({}, urlUtil.urlToHttpOptions(_url));
+        if (typeof options == "object") {
+          Object.assign(combinedOptions, options);
+          callback = cb;
+        } else {
+          callback = options;
+        }
       } else if (typeof _url === "object") {
         // the two-arg form.
         combinedOptions = Object.assign({}, _url);

diff --git a/lib/url-util.js b/lib/url-util.js
@@ -0,0 +1,24 @@
+/* eslint-env node */
+exports.urlToHttpOptions = urlToHttpOptions;
+// https://github.com/nodejs/node/blob/e46c680bf2b211bbd52cf959ca17ee98c7f657f5/lib/internal/url.js#L1291
+function urlToHttpOptions(url) {
+  const options = {
+    protocol: url.protocol,
+    hostname:
+      typeof url.hostname === "string" && url.hostname.startsWith("[")
+        ? url.hostname.slice(1, -1)
+        : url.hostname,
+    hash: url.hash,
+    search: url.search,
+    pathname: url.pathname,
+    path: `${url.pathname || ""}${url.search || ""}`,
+    href: url.href,
+  };
+  if (url.port !== "") {
+    options.port = Number(url.port);
+  }
+  if (url.username || url.password) {
+    options.auth = `${url.username}:${url.password}`;
+  }
+  return options;
+}