instrumentation of http{s}.request() has a few edge case issues

[trentm]

In lib/instrumentation/http-shared.js the traceOutgoingRequest function that handles instrumenting http.request and https.request has a few issues:

      var options = {}
      var newArgs = [options]
      for (const arg of args) {
        if (typeof arg === 'function') {
          newArgs.push(arg)
        } else {
          Object.assign(options, ensureUrl(arg))
        }
      }

1. That iteration through all args changes the signature of http.request() such that you can pass any number of objects and URLs to it -- before and after a callback function. Not a biggie.
2. That ensureUrl uses a formatURL function that is meant to be (or should be) an equivalent to node core's handling (https://github.com/nodejs/node/blob/8d9d8236b79ea91640f973cc8c1423603694b482/lib/internal/url.js#L1288-L1311) but misses a few things:

• it doesn't restore options.auth, so users of APM and http.request and HTTP Basic auth in a first URL arg are being broken
• it handles "port" slightly differently that could surprise, I think
• it doesn't have changes for a couple node core issues over the years: url: handle quasi-WHATWG URLs in urlToOptions() nodejs/node#26226 url: make urlToOptions() handle IPv6 literals nodejs/node#19720

---

I wonder if it would be possible to avoid full processing of the original args, only try to sniff out and update an "options.headers" object. Then the agent doesn't have to try to track change made to node's internal "urlToHttpOptions" handling.

One side-effect of ^^ is that the potential handling for extracting the "url", see #2039, may have to change to handle inspecting those original args itself. I believe that would be preferable to changing the behaviour of http.request.

[astorm]

I wonder if it would be possible to avoid full processing of the original args, only try to sniff out and update an "options.headers" object. Then the agent doesn't have to try to track change made to node's internal "urlToHttpOptions" handling.

My thinking is similar.

Looking at the code here, it appears that that only property of the extracted options variable that our function uses is options.headers. It is not, to my knowledge, possible to extract a headers value from a URL string or object. In other words, it's not necessary to merge the URL data to get the value we want (options.headers). Sniffing out the options object, and then passing the original arguments (with a modified options object) seems like the best course of action, with one caveat.

Here's all the method signatures of http.request

  // Three Argument:
  // http.request(string|String|URL url, object options, function callback)
  //
  // Two Argument:
  // http.request(string|String|URL url, object options)
  // http.request(string|String|URL url, function callback)
  // http.request(object options, function callback)
  //
  // One Argument:
  // http.request(string|String|URL url)
  // http.request(object options)
  // http.request(function callback)

  // No Arguments:
  // http.request()

There's two that don't have an options object.

// http.request(string|String|URL url)
// http.request(string|String|URL url, function callback)

For these method signatures we'll need to generate an empty options object and then either push or splice it onto the original args.

[trentm]

This issue has existed since v2.17.0. It was introduced in commit dd60b12 when switching directly from url.parse() to new URL(). Recent versions of node have a url.urlToHttpOptions() to help with this conversion.