dd-trace plugin-http doesn't support WHATWG URL objects

[daniel-edwards-iress]

Describe the bug
When making a request using a WHATWG URL object (node URL) dd-trace clears the request URL.

The latest Got converts urls before making the request.

Simple example

{
  "name": "test",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "dd-trace": "^0.16.3"
  }
}

require('dd-trace').default.init(
  {
    service: 'test',
    runtimeMetrics: true,
    env: 'test',
    logger: {
      debug: message => console.log,
      error: error => console.error,
    },
    analytics: true,
    logInjection: false,

  }
)
const url = 'https://wtfismyip.com/json'

require('https').request(new URL(url), (res) => {
  res.on('data', console.log)
  res.on('error', console.error)
}).end()

# node index.js 
events.js:187
      throw er; // Unhandled 'error' event
      ^

Error: connect ECONNREFUSED 127.0.0.1:443
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1128:14)
Emitted 'error' event on ClientRequest instance at:
    at ClientRequest.req.emit (/test/node_modules/dd-trace/packages/datadog-plugin-http/src/client.js:93:21)
    at TLSSocket.socketErrorListener (_http_client.js:406:9)
    at TLSSocket.emit (events.js:210:5)
    at emitErrorNT (internal/streams/destroy.js:92:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:60:3)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  errno: 'ECONNREFUSED',
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 443
}

Environment

• docker node:12
• node 12.13.0
• dd-trace 0.16.3

[ericmustin]

Had a chance to take a quick look at this on the plane.

I was able to reproduce this, thank you for the snippet @daniel-edwards-iress ! I believe what's happening is that this test case is being hit:

https://github.com/nodejs/node/blob/master/test/parallel/test-whatwg-url-custom-properties.js#L151-L166

Where, basically, there's an attempt to copy the URL Object that gets passed into the normalizeArgs helper method here:

dd-trace-js/packages/datadog-plugin-http/src/client.js

Line 161 in 1c1d389

let options = typeof inputURL === 'string' ? url.parse(inputURL) : Object.assign({}, inputURL)

Which uses Object.assign. Unfortunately it looks the data properties here are getter / setters that exist on the prototype, and so they do not get copied, and so details like the hostname get dropped. Previous to node 11(? not sure) it looks like this would've caused an error to get thrown explicitly, but now it sort've just gets swallowed and the output is useless, per this PR which added the above test nodejs/node#26226 . I suppose that's good, but made this a bit harder to pin down.

a quick fix here could be to use a for...in loop instead of Object.assign, which was similar to how the logger was recently patched

let options

if(typeof inputURL === 'string') {
  options = url.parse(inputURL)
} else {
  options = {}
  for(const key in inputUrl) {
    options[key] = inputUrl[key]
  }
}

I haven't totally thought through if there's any other implications that would occur from this though, and if this change is made it may also make sense to clean up the other uses of Object.assign in this helper function a bit further down. Thoughts?

[daniel-edwards-iress]

Only thought would be to filter out functions from the copied keys.

let options

if(typeof inputURL === 'string') {
  options = url.parse(inputURL)
} else {
  options = {}
  for(const key in inputUrl) {
    if (typeof options[key] !== 'function')
        options[key] = inputUrl[key]
  }
}

as for making the change from Object.assign more generally I think it might be good to use a replacement to wrap it but can be extended with logic like if a predicate is provided only those that pass get assigned