fix: span url hostname not ip

CHANGELOG.asciidoc

@@ -62,6 +62,9 @@ variable), then it should be removed.
 [float]
 ===== Bug fixes
 
+* Fixed bug where the URL property for outgoing HTTP request spans was set
+  with the server's IP address rather than its hostname. The Agent now sets
+  this property with the actual URL requested by Node.js.
 * Fixed bug where external services were not listed under Dependencies on the
   APM Service Overview page due to the trace-context propagated `sample_rate`
   value not being set on either transactions or spans.

lib/instrumentation/http-shared.js

@@ -1,9 +1,7 @@
 'use strict'
 
 var url = require('url')
-
 var endOfStream = require('end-of-stream')
-var httpRequestToUrl = require('http-request-to-url')
 
 var { parseUrl } = require('../parsers')
 var { getHTTPDestination } = require('./context')
@@ -191,13 +189,11 @@ exports.traceOutgoingRequest = function (agent, moduleName, method) {
         return emit.apply(req, arguments)
       }
 
-      let url, statusCode
-      httpRequestToUrl(req).then(_url => {
-        url = _url
-      }).catch(() => {
+      const url = getUrlFromRequestAndOptions(req, options, moduleName + ':')
+      if (!url) {
         agent.logger.warn('unable to identify http.ClientRequest url %o', { id: id })
-      })
-
+      }
+      let statusCode
       return req
 
       // In case the request is ended prematurely
@@ -260,3 +256,41 @@ function isAWSSigned (opts) {
   const auth = opts.headers && (opts.headers.Authorization || opts.headers.authorization)
   return typeof auth === 'string' ? auth.startsWith('AWS4-') : false
 }
+
+// Creates a sanitized URL suitable for the span's HTTP context
+//
+// This function reconstructs a URL using the request object's properties
+// where it can (node versions v14.5.0, v12.19.0 and later) , and falling
+// back to the options where it can not.  This function also strips any
+// authentication information provided with the hostname.  In other words
+//
+// http://username:password@example.com/foo
+//
+// becomes http://example.com/foo
+//
+// NOTE: The options argument may not be the same options that are passed
+// to http.request if the caller uses the the http.request(url,options,...)
+// method signature. The agent normalizes the url and options into a single
+// options array. This function expects those pre-normalized options.
+//
+// @param {ClientRequest} req
+// @param {object} options
+// @param {string} fallbackProtocol
+// @return string|undefined
+function getUrlFromRequestAndOptions (req, options, fallbackProtocol) {
+  if (!req) {
+    return undefined
+  }
+  options = options || {}
+  req = req || {}
+  req.agent = req.agent || {}
+
+  const port = options.port ? `:${options.port}` : ''
+  // req.host and req.protocol are node versions v14.5.0/v12.19.0 and later
+  const host = req.host || options.hostname || options.host || 'localhost'
+  const protocol = req.protocol || req.agent.protocol || fallbackProtocol
+
+  return `${protocol}//${host}${port}${req.path}`
+}
+
+exports.getUrlFromRequestAndOptions = getUrlFromRequestAndOptions

package.json

@@ -93,7 +93,6 @@
     "escape-string-regexp": "^4.0.0",
     "fast-safe-stringify": "^2.0.7",
     "http-headers": "^3.0.2",
-    "http-request-to-url": "^1.0.0",
     "is-native": "^1.0.1",
     "measured-reporting": "^1.51.1",
     "monitor-event-loop-delay": "^1.0.0",

test/instrumentation/modules/http/outgoing.js

@@ -172,15 +172,15 @@ function echoTest (type, opts, handler) {
         t.deepEqual(data.spans[0].context.http, {
           method: 'GET',
           status_code: 200,
-          url: `${type}://127.0.0.1:${port}/`
+          url: `${type}://localhost:${port}/`
         })
         t.deepEqual(data.spans[0].context.destination, {
           service: {
-            name: `${type}://127.0.0.1:${port}`,
-            resource: `127.0.0.1:${port}`,
+            name: `${type}://localhost:${port}`,
+            resource: `localhost:${port}`,
             type: data.spans[0].type
           },
-          address: '127.0.0.1',
+          address: 'localhost',
           port: Number(port)
         })
         t.end()
@@ -224,7 +224,7 @@ function abortTest (type, handler) {
       const httpContext = {
         method: 'GET',
         status_code: undefined,
-        url: undefined
+        url: `http://localhost:${port}/`
       }
 
       resetAgent({}, data => {
@@ -241,11 +241,11 @@ function abortTest (type, handler) {
         if (httpContext.url) {
           t.deepEqual(data.spans[0].context.destination, {
             service: {
-              name: `${type}://127.0.0.1:${port}`,
-              resource: `127.0.0.1:${port}`,
+              name: `${type}://localhost:${port}`,
+              resource: `localhost:${port}`,
               type: data.spans[0].type
             },
-            address: '127.0.0.1',
+            address: 'localhost',
             port: Number(port)
           })
         }
@@ -262,7 +262,7 @@ function abortTest (type, handler) {
 
       req.on('response', () => {
         httpContext.status_code = 200
-        httpContext.url = `${type}://127.0.0.1:${port}/`
+        httpContext.url = `${type}://localhost:${port}/`
       })
 
       // NOTE: Don't use an arrow function here

test/instrumentation/modules/http/request-to-url.js

@@ -0,0 +1,136 @@
+const tape = require('tape')
+const http = require('http')
+const { getUrlFromRequestAndOptions } = require('../../../../lib/instrumentation/http-shared')
+
+// Creates a ClientRequest from options
+//
+// Creates and request an immediatly aborts/destroys it.
+// This allows us to test with real ClientRequest objects
+// and ensure their underlying properties are stable/consistant
+// across versions.
+//
+// @param {options} options
+// @return {ClientRequest}
+function requestFromOptions (options) {
+  const req = http.request(options)
+  req.on('error', function () {})
+  req.destroy()
+  return req
+}
+
+tape('getUrlFromRequestAndOptions tests', function (suite) {
+  suite.test('options with host', function (t) {
+    const options = {
+      host: 'example.com'
+    }
+    const req = requestFromOptions(options)
+
+    const url = getUrlFromRequestAndOptions(req, options)
+    t.equals(url, 'http://example.com/', 'url rendered as expected')
+    t.end()
+  })
+
+  suite.test('options with host and path', function (t) {
+    const options = {
+      host: 'example.com',
+      path: '/foo'
+    }
+    const req = requestFromOptions(options)
+
+    const url = getUrlFromRequestAndOptions(req, options)
+    t.equals(url, 'http://example.com/foo', 'url rendered as expected')
+    t.end()
+  })
+
+  suite.test('options with host, path, port, and a query string', function (t) {
+    const options = {
+      host: 'example.com',
+      path: '/foo?fpp=bar',
+      port: 32
+    }
+    const req = requestFromOptions(options)
+
+    const url = getUrlFromRequestAndOptions(req, options)
+    t.equals(url, 'http://example.com:32/foo?fpp=bar', 'url rendered as expected')
+    t.end()
+  })
+
+  suite.test('options with host, path, port, query string, and a username/password', function (t) {
+    const options = {
+      host: 'example.com',
+      path: '/foo?fpp=bar',
+      auth: 'username:password',
+      port: 32
+    }
+    const req = requestFromOptions(options)
+
+    const url = getUrlFromRequestAndOptions(req, options)
+    t.equals(url, 'http://example.com:32/foo?fpp=bar', 'url rendered as expected')
+    t.equals(url.indexOf('username'), -1, 'no auth information in url')
+    t.equals(url.indexOf('password'), -1, 'no auth information in url')
+    t.end()
+  })
+
+  suite.test('options with host and hostname', function (t) {
+    const options = {
+      host: 'two.example.com',
+      hostname: 'one.example.com',
+      path: '/bar',
+      auth: 'username:password'
+    }
+    const req = requestFromOptions(options)
+    const url = getUrlFromRequestAndOptions(req, options)
+    t.equals(url, 'http://one.example.com/bar', 'url rendered as expected (hostname wins)')
+    t.equals(url.indexOf('username'), -1, 'no auth information in url')
+    t.equals(url.indexOf('password'), -1, 'no auth information in url')
+    t.end()
+  })
+
+  suite.test('does not crash with unexpected data', function (t) {
+    const options = {
+      host: 'two.example.com',
+      hostname: 'one.example.com',
+      path: '/bar'
+    }
+    const req = requestFromOptions(options)
+
+    const url1 = getUrlFromRequestAndOptions(null, null)
+    const url2 = getUrlFromRequestAndOptions(req, null)
+    const url3 = getUrlFromRequestAndOptions(null, options)
+
+    t.equal(url1, undefined, 'no url returned')
+    t.ok(url2, 'URL returned')
+    t.equal(url3, undefined, 'no url returned')
+    t.end()
+  })
+
+  suite.test('port 80 makes it through', function (t) {
+    const options = {
+      host: 'two.example.com',
+      port: 80
+    }
+    const req = requestFromOptions(options)
+
+    const url = getUrlFromRequestAndOptions(req, options)
+    t.equals(url, 'http://two.example.com:80/', 'port 80 made it thorugh')
+    t.end()
+  })
+
+  suite.test('missing protocol', function (t) {
+    const options = {
+      hostname: 'localhost',
+      path: '/get',
+      // A custom agent that implements the minimum to pass muster, but does
+      // *not* define `agent.protocol`.
+      agent: {
+        addRequest () {}
+      }
+    }
+    const req = requestFromOptions(options)
+
+    const url = getUrlFromRequestAndOptions(req, options, 'http:')
+    t.equals(url, 'http://localhost/get', 'protocol falls back correctly')
+    t.end()
+  })
+  suite.end()
+})