refactor: improve handling of http.request arguments

Author: Stephen Belanger

lib/instrumentation/http-shared.js

@@ -75,39 +75,51 @@ function isRequestBlacklisted (agent, req) {
   return false
 }
 
+// NOTE: This will also stringify and parse URL instances
+// to a format which can be mixed into the options object.
+function ensureUrl (v) {
+  if (typeof v === 'string' || v instanceof url.URL) {
+    return url.parse(String(v))
+  } else {
+    return v
+  }
+}
+
 exports.traceOutgoingRequest = function (agent, moduleName) {
   var spanType = 'ext.' + moduleName + '.http'
   var ins = agent._instrumentation
 
   return function (orig) {
-    return function (reqUrl, options, callback) {
+    return function (...args) {
       var span = agent.buildSpan()
       var id = span && span.transaction.id
 
       agent.logger.debug('intercepted call to %s.request %o', moduleName, { id: id })
 
-      if (!span) return orig.apply(this, arguments)
-
-      if (typeof options === 'function') {
-        callback = options
-        options = {}
-      }
-
-      if (typeof reqUrl === 'object' && !(reqUrl instanceof url.URL)) {
-        options = reqUrl
-      } else {
-        if (!(reqUrl instanceof url.URL)) {
-          reqUrl = new url.URL(reqUrl)
+      var options = {}
+      var newArgs = [ options ]
+      for (let arg of args) {
+        if (typeof arg === 'function') {
+          newArgs.push(arg)
+        } else {
+          Object.assign(options, ensureUrl(arg))
         }
-        options = Object.assign({}, options, reqUrl)
       }
 
       if (!options.headers) options.headers = {}
 
-      // Use span context as traceparent header
-      options.headers['Elastic-APM-Traceparent'] = span.context.toString()
+      // Attempt to use the span context as a traceparent header.
+      // If the transaction is unsampled the span will not exist,
+      // however a traceparent header must still be propagated
+      // to indicate requested services should not be sampled.
+      // Use the transaction context as the parent, in this case.
+      var parent = span || agent.currentTransaction
+      if (parent && parent.context) {
+        options.headers['Elastic-APM-Traceparent'] = parent.context.toString()
+      }
 
-      var req = orig.call(this, options, callback)
+      var req = orig.apply(this, newArgs)
+      if (!span) return req
 
       if (req._headers.host === agent._conf.serverHost) {
         agent.logger.debug('ignore %s request to intake API %o', moduleName, { id: id })

test/instrumentation/modules/http/outgoing.js

@@ -2,21 +2,115 @@
 
 var agent = require('../../_agent')()
 
+var http = require('http')
+var https = require('https')
+var url = require('url')
+
+var semver = require('semver')
 var test = require('tape')
 
 var echoServer = require('./_echo_server_util').echoServer
 var mockClient = require('../../../_mock_http_client')
 var TraceContext = require('.././../../../lib/instrumentation/trace-context')
 
-var transports = [['http', require('http')], ['https', require('https')]]
+//
+// http
+//
+test('http.request(options)', echoTest('http', (port, cb) => {
+  var options = { port }
+  var req = http.request(options)
+  req.on('response', cb)
+  return req
+}))
+
+test('http.request(options, callback)', echoTest('http', (port, cb) => {
+  var options = { port }
+  return http.request(options, cb)
+}))
+
+test('http.request(urlString)', echoTest('http', (port, cb) => {
+  var urlString = `http://localhost:${port}`
+  var req = http.request(urlString)
+  req.on('response', cb)
+  return req
+}))
+
+test('http.request(urlString, callback)', echoTest('http', (port, cb) => {
+  var urlString = `http://localhost:${port}`
+  return http.request(urlString, cb)
+}))
+
+if (url.URL) {
+  test('http.request(urlObject)', echoTest('http', (port, cb) => {
+    var urlString = `http://localhost:${port}`
+    var urlObject = new url.URL(urlString)
+    var req = http.request(urlObject)
+    req.on('response', cb)
+    return req
+  }))
+
+  test('http.request(urlObject, callback)', echoTest('http', (port, cb) => {
+    var urlString = `http://localhost:${port}`
+    var urlObject = new url.URL(urlString)
+    return http.request(urlObject, cb)
+  }))
+}
+
+//
+// https
+//
+test('https.request(options)', echoTest('https', (port, cb) => {
+  var options = { port, rejectUnauthorized: false }
+  var req = https.request(options)
+  req.on('response', cb)
+  return req
+}))
+
+test('https.request(options, callback)', echoTest('https', (port, cb) => {
+  var options = { port, rejectUnauthorized: false }
+  return https.request(options, cb)
+}))
 
-transports.forEach(function (tuple) {
-  var name = tuple[0]
-  var transport = tuple[1]
+test('https.request(urlString, options)', echoTest('https', (port, cb) => {
+  var urlString = `https://localhost:${port}`
+  var options = { rejectUnauthorized: false }
+  var req = https.request(urlString, options)
+  req.on('response', cb)
+  return req
+}))
 
-  test(name + '.request', function (t) {
-    echoServer(name, function (cp, port) {
-      resetAgent(function (data, ...args) {
+if (semver.satisfies(process.version, '>=10.9')) {
+  test('https.request(urlString, options, callback)', echoTest('https', (port, cb) => {
+    var urlString = `https://localhost:${port}`
+    var options = { rejectUnauthorized: false }
+    var req = https.request(urlString, options, cb)
+    return req
+  }))
+}
+
+if (url.URL && semver.satisfies(process.version, '>=10.9')) {
+  test('https.request(urlObject, options)', echoTest('https', (port, cb) => {
+    var urlString = `https://localhost:${port}`
+    var urlObject = new url.URL(urlString)
+    var options = { rejectUnauthorized: false }
+    var req = https.request(urlObject, options)
+    req.on('response', cb)
+    return req
+  }))
+
+  test('https.request(urlObject, options, callback)', echoTest('https', (port, cb) => {
+    var urlString = `https://localhost:${port}`
+    var urlObject = new url.URL(urlString)
+    var options = { rejectUnauthorized: false }
+    var req = https.request(urlObject, options, cb)
+    return req
+  }))
+}
+
+function echoTest (type, handler) {
+  return function (t) {
+    echoServer(type, (cp, port) => {
+      resetAgent(data => {
         t.equal(data.transactions.length, 1, 'has one transaction')
         t.equal(data.spans.length, 1, 'has one span')
         t.equal(data.spans[0].name, 'GET localhost:' + port + '/', 'has expected span name')
@@ -25,22 +119,24 @@ transports.forEach(function (tuple) {
       })
 
       var trans = agent.startTransaction()
-      var req = transport.request({ port: port, rejectUnauthorized: false }, function (res) {
+      var req = handler(port, res => {
         res.on('end', function () {
           agent.endTransaction()
         })
         res.resume()
       })
-      const expected = TraceContext.fromString(trans.context.toString())
-      const received = TraceContext.fromString(req.getHeader('Elastic-APM-traceparent'))
+
+      var expected = TraceContext.fromString(trans.context.toString())
+      var received = TraceContext.fromString(req.getHeader('Elastic-APM-traceparent'))
       t.equal(received.version, expected.version, 'traceparent header has matching version')
       t.equal(received.traceId, expected.traceId, 'traceparent header has matching traceId')
       t.ok(/^[\da-f]{16}$/.test(expected.id), 'traceparent header has valid id')
       t.equal(received.flags, expected.flags, 'traceparent header has matching flags')
+
       req.end()
     })
-  })
-})
+  }
+}
 
 function resetAgent (cb) {
   agent._instrumentation.currentTransaction = null