Add validateStatus option to HTTP plugin

The [OpenTracing spec][1] states that a span should be tagged as
an error "if and only if the application considers the operation
represented by the Span to have failed".

The `dd-trace-js` HTTP plugin allows you to automatically
instrument outgoing HTTP requests.

Currently, it only tags a span as an error when the request
emits an `error` event, which only happens in the case of "hard
failures" like a timeout or connection problem.

https://github.com/DataDog/dd-trace-js/blob/34c168555f44a961a61ede95a39341d7c527701a/src/plugins/http.js#L59-L67

This means that requests that return responses never are
considered errors, even if a 4XX or 5XX response is received.

Having spans tagged as errors is useful, because it means they
are flagged as such in the Datadog APM interface, useful for
reporting.

This commit adds support for a new `validateStatus` option for
the HTTP plugin which allows the user to supply a
`validateStatus` function which, when a response is received, is
passed the response status and expected to return `false` if the
response should be considered an error, or `true` if not. If it
returns `false`, the span will be marked as an error.

By default, we will consider requests that return `4XX` responses
to be errors, since a `4XX` indicates a failure which is within
the user's control.

Fixes #297.

[1]: https://github.com/opentracing/specification/blob/master/semantic_conventions.md

docs/API.md

@@ -214,8 +214,9 @@ query HelloWorld {
 
 | Option           | Default          | Description                            |
 |------------------|------------------|----------------------------------------|
-| service          | http-client      | The service name for this integration. |
-| splitByDomain    | false            | Use the remote endpoint host as the service name instead of the default. |
+| service          | http-client                           | The service name for this integration. |
+| splitByDomain    | false                                 | Use the remote endpoint host as the service name instead of the default. |
+| validateStatus   | `code => code < 400 || code >= 500`   | Callback function to determine if an HTTP response should be recorded as an error. It should take a status code as its only parameter and return `true` for success or `false` for errors.
 
 <h3 id="ioredis">ioredis</h3>
 

src/plugins/http.js

@@ -3,11 +3,13 @@
 const url = require('url')
 const opentracing = require('opentracing')
 const semver = require('semver')
+const log = require('../log')
 
 const Tags = opentracing.Tags
 const FORMAT_HTTP_HEADERS = opentracing.FORMAT_HTTP_HEADERS
 
 function patch (http, methodName, tracer, config) {
+  config = normalizeConfig(config)
   this.wrap(http, methodName, fn => makeRequestTrace(fn))
 
   function makeRequestTrace (request) {
@@ -53,6 +55,10 @@ function patch (http, methodName, tracer, config) {
       req.on('response', res => {
         span.setTag(Tags.HTTP_STATUS_CODE, res.statusCode)
 
+        if (!config.validateStatus(res.statusCode)) {
+          span.setTag('error', 1)
+        }
+
         res.on('end', () => span.finish())
       })
 
@@ -148,6 +154,23 @@ function unpatch (http) {
   this.unwrap(http, 'get')
 }
 
+function getStatusValidator (config) {
+  if (typeof config.validateStatus === 'function') {
+    return config.validateStatus
+  } else if (config.hasOwnProperty('validateStatus')) {
+    log.error('Expected `validateStatus` to be a function.')
+  }
+  return code => code < 400 || code >= 500
+}
+
+function normalizeConfig (config) {
+  const validateStatus = getStatusValidator(config)
+
+  return Object.assign({}, config, {
+    validateStatus
+  })
+}
+
 module.exports = [
   {
     name: 'http',

test/plugins/http.spec.js

@@ -454,7 +454,7 @@ describe('Plugin', () => {
           })
         })
 
-        it('should handle errors', done => {
+        it('should handle connection errors', done => {
           getPort().then(port => {
             agent
               .use(traces => {
@@ -471,6 +471,56 @@ describe('Plugin', () => {
           })
         })
 
+        it('should not record HTTP 5XX responses as errors by default', done => {
+          const app = express()
+
+          app.get('/user', (req, res) => {
+            res.status(500).send()
+          })
+
+          getPort().then(port => {
+            agent
+              .use(traces => {
+                expect(traces[0][0]).to.have.property('error', 0)
+              })
+              .then(done)
+              .catch(done)
+
+            appListener = server(app, port, () => {
+              const req = http.request(`${protocol}://localhost:${port}/user`, res => {
+                res.on('data', () => { })
+              })
+
+              req.end()
+            })
+          })
+        })
+
+        it('should record HTTP 4XX responses as errors by default', done => {
+          const app = express()
+
+          app.get('/user', (req, res) => {
+            res.status(400).send()
+          })
+
+          getPort().then(port => {
+            agent
+              .use(traces => {
+                expect(traces[0][0]).to.have.property('error', 1)
+              })
+              .then(done)
+              .catch(done)
+
+            appListener = server(app, port, () => {
+              const req = http.request(`${protocol}://localhost:${port}/user`, res => {
+                res.on('data', () => { })
+              })
+
+              req.end()
+            })
+          })
+        })
+
         it('should only record a request once', done => {
           // Make sure both plugins are loaded, which could cause double-counting.
           require('http')
@@ -555,6 +605,47 @@ describe('Plugin', () => {
         })
       })
 
+      describe('with validateStatus configuration', () => {
+        let config
+
+        beforeEach(() => {
+          config = {
+            validateStatus: status => status < 500
+          }
+
+          return agent.load(plugin, 'http', config)
+            .then(() => {
+              http = require(protocol)
+              express = require('express')
+            })
+        })
+
+        it('should use the supplied function to decide if a response is an error', done => {
+          const app = express()
+
+          app.get('/user', (req, res) => {
+            res.status(500).send()
+          })
+
+          getPort().then(port => {
+            agent
+              .use(traces => {
+                expect(traces[0][0]).to.have.property('error', 1)
+              })
+              .then(done)
+              .catch(done)
+
+            appListener = server(app, port, () => {
+              const req = http.request(`${protocol}://localhost:${port}/user`, res => {
+                res.on('data', () => { })
+              })
+
+              req.end()
+            })
+          })
+        })
+      })
+
       describe('with splitByDomain configuration', () => {
         let config