errors: changed array based book keeping to hashtable based

lib/errors/aggregator.js

@@ -6,6 +6,7 @@ var logger = require('../logger').child({component: 'error_tracer'})
 var NAMES = require('../metrics/names')
 var errorsModule = require('./index')
 var Reservoir = require('../reservoir.js')
+var WeakMap = global.WeakMap
 
 
 var createError = errorsModule.createError
@@ -31,7 +32,12 @@ function ErrorAggregator(config) {
   this.webTransactionErrorCount = 0
   this.otherTransactionErrorCount = 0
   this.errors = []
-  this.seen = []
+  if (WeakMap) {
+    this.seenObjects = new WeakMap()
+  } else {
+    this.seenObjects = []
+  }
+  this.seenStrings = {}
 
   // reservoir used for error events
   this.events = new Reservoir(this.config.error_collector.max_event_samples_stored)
@@ -138,6 +144,41 @@ ErrorAggregator.prototype.addUserError = function addUserError(transaction, exce
   }
 }
 
+/**
+ *
+ * This function takes an exception and determines whether the exception
+ * has been seen before by this aggregator.  This function mutates the
+ * book keeping structures to reflect the exception has been seen.
+ *
+ * @param {Error} exception  The error to be checked.
+ *
+ */
+
+ErrorAggregator.prototype.haveSeen = function haveSeen(exception) {
+  if (typeof exception === 'object') {
+    if (WeakMap) {
+      if (this.seenObjects.has(exception)) {
+        return true
+      }
+
+      this.seenObjects.set(exception, true)
+    } else {
+      if (this.seenObjects.indexOf(exception) !== -1) {
+        return true
+      }
+
+      this.seenObjects.push(exception)
+    }
+  } else { // typeof exception !== 'object'
+    if (this.seenStrings[exception]) {
+      return true
+    }
+
+    this.seenStrings[exception] = true
+  }
+  return false
+}
+
 /**
  * Collects the error and also creates the error event.
  * This function uses an array of seen exceptions to ensure errors don't get
@@ -160,7 +201,10 @@ ErrorAggregator.prototype._collect = _collect
 
 function _collect(transaction, exception, customParameters, timestamp) {
   if (exception) {
-    if (this.seen.indexOf(exception) !== -1) return
+    if (this.haveSeen(exception)) {
+      return
+    }
+
     if (typeof exception !== 'string' && !exception.message && !exception.stack) {
       logger.trace(exception,
         "Got error that is not an instance of Error or string.")
@@ -192,7 +236,6 @@ function _collect(transaction, exception, customParameters, timestamp) {
 
   if (exception) {
     logger.trace(exception, "Got exception to trace:")
-    this.seen.push(exception)
   }
 
   var error = createError(transaction, exception, customParameters, this.config)
@@ -307,7 +350,12 @@ ErrorAggregator.prototype.clearEvents = function clearEvents() {
 
 ErrorAggregator.prototype.clearErrors = function clearErrors() {
   this.errors = []
-  this.seen = []
+  this.seenStrings = {}
+  if (WeakMap) {
+    this.seenObjects = new WeakMap()
+  } else {
+    this.seenObjects = []
+  }
   this.errorCount = 0
   this.webTransactionErrorCount = 0
   this.otherTransactionErrorCount = 0

lib/instrumentation/core/http.js

@@ -8,7 +8,6 @@ var cat = require('../../util/cat.js')
 var instrumentOutbound = require('../../transaction/tracer/instrumentation/outbound.js')
 var util = require('util')
 var url = require('url')
-var urltils = require('../../util/urltils')
 var semver = require('semver')
 
 /*
@@ -175,18 +174,19 @@ function wrapListener(agent, listener) {
           transaction.addAgentAttribute('httpResponseMessage', response.statusMessage)
         }
 
-        var headers = urltils.getHeadersFromHeaderString(response._header)
-        if (headers['Content-Length'] !== undefined) {
+        var contentLength = response.getHeader('content-length')
+        if (contentLength) {
           transaction.addAgentAttribute(
             'response.headers.contentLength',
-            parseInt(headers['Content-Length'], 10)
+            parseInt(contentLength, 10)
           )
         }
 
-        if (headers['Content-Type'] !== undefined) {
+        var contentType = response.getHeader('content-type')
+        if (contentType) {
           transaction.addAgentAttribute(
             'response.headers.contentType',
-            headers['Content-Type']
+            contentType
           )
         }
       }

lib/util/urltils.js

@@ -64,34 +64,6 @@ module.exports = {
     return path
   },
 
-  /**
-   * Takes a header string and parses it into an object with the
-   * header's key/value pairs.
-   */
-  getHeadersFromHeaderString: function getHeadersFromHeaderString(headerString) {
-    var res = {}
-    var currentString = ''
-    var currentKey = ''
-    for (var i = 0; i < headerString.length; ++i) {
-      var char = headerString[i]
-      if (char === '\r') {
-        currentString = currentString.replace(/^\s|\s$/g, '')
-        currentKey = currentKey.replace(/^\s|\s$/g, '')
-        if (currentKey.length !== 0 && currentString.length !== 0) {
-          res[currentKey] = currentString
-        }
-        currentString = ''
-        currentKey = ''
-      } else if (char === ':') {
-        currentKey = currentString
-        currentString = ''
-      } else {
-        currentString += char
-      }
-    }
-    return res
-  },
-
   /**
    * Extract query parameters, dealing with bare parameters and parameters with
    * no value as appropriate:

test/integration/instrumentation/hapi/capture-params.js

@@ -37,6 +37,8 @@ function runTests(hapi, createServer) {
             "request.headers.accept" : "application/json",
             "request.headers.host" : "localhost:8089",
             "request.method" : "GET",
+            "response.headers.contentLength" : 15,
+            "response.headers.contentType" : "application/json; charset=utf-8",
             "response.status" : 200,
             "httpResponseCode": "200",
             "httpResponseMessage": "OK",
@@ -48,6 +50,8 @@ function runTests(hapi, createServer) {
             "request.headers.host" : "localhost:8089",
             "request.method" : "GET",
             "response.status" : 200,
+            "response.headers.contentLength" : 15,
+            "response.headers.contentType" : "application/json; charset=utf-8",
             "httpResponseCode": "200",
             "request_uri": "/test/"
           }, 'parameters should only have request/response params')
@@ -96,6 +100,8 @@ function runTests(hapi, createServer) {
             "request.headers.accept" : "application/json",
             "request.headers.host" : "localhost:8089",
             "request.method" : "GET",
+            "response.headers.contentLength" : 15,
+            "response.headers.contentType" : "application/json; charset=utf-8",
             "response.status" : 200,
             "httpResponseCode": "200",
             "httpResponseMessage": "OK",
@@ -108,6 +114,8 @@ function runTests(hapi, createServer) {
             "request.headers.host" : "localhost:8089",
             "request.method" : "GET",
             "response.status" : 200,
+            "response.headers.contentLength" : 15,
+            "response.headers.contentType" : "application/json; charset=utf-8",
             "httpResponseCode": "200",
             "id" : "1337",
             "request_uri": "/test/1337/"
@@ -157,6 +165,8 @@ function runTests(hapi, createServer) {
             "request.headers.host" : "localhost:8089",
             "request.method" : "GET",
             "response.status" : 200,
+            "response.headers.contentLength" : 15,
+            "response.headers.contentType" : "application/json; charset=utf-8",
             "httpResponseCode": "200",
             "httpResponseMessage": "OK",
             "name" : "hapi",
@@ -168,6 +178,8 @@ function runTests(hapi, createServer) {
             "request.headers.host" : "localhost:8089",
             "request.method" : "GET",
             "response.status" : 200,
+            "response.headers.contentLength" : 15,
+            "response.headers.contentType" : "application/json; charset=utf-8",
             "httpResponseCode": "200",
             "name" : "hapi",
             "request_uri": "/test/"
@@ -217,6 +229,8 @@ function runTests(hapi, createServer) {
             "request.headers.host" : "localhost:8089",
             "request.method" : "GET",
             "response.status" : 200,
+            "response.headers.contentLength" : 15,
+            "response.headers.contentType" : "application/json; charset=utf-8",
             "httpResponseCode": "200",
             "httpResponseMessage": "OK",
             "id" : "1337",
@@ -229,6 +243,8 @@ function runTests(hapi, createServer) {
             "request.headers.host" : "localhost:8089",
             "request.method" : "GET",
             "response.status" : 200,
+            "response.headers.contentLength" : 15,
+            "response.headers.contentType" : "application/json; charset=utf-8",
             "httpResponseCode": "200",
             "id" : "1337",
             "name" : "hapi",

test/unit/errors/aggregator.test.js

@@ -240,6 +240,52 @@ describe('ErrorAggregator', function () {
     config.collect_errors = true
   })
 
+  it('should gather the same error in two transactions', function () {
+    var error = new Error('this happened once')
+    expect(tracer.errorCount).equal(0)
+    expect(tracer.errors.length).equal(0)
+
+    tracer.add(null, error)
+
+    expect(tracer.errorCount).equal(1)
+    expect(tracer.errors.length).equal(1)
+
+    tracer.clearErrors()
+
+    expect(tracer.errorCount).equal(0)
+    expect(tracer.errors.length).equal(0)
+
+    tracer.add(null, error)
+
+    expect(tracer.errorCount).equal(1)
+    expect(tracer.errors.length).equal(1)
+  })
+
+  it('should not gather the same error twice', function () {
+    var error = new Error('this happened once')
+    expect(tracer.errorCount).equal(0)
+    expect(tracer.errors.length).equal(0)
+
+    tracer.add(null, error)
+    tracer.add(null, error)
+
+    expect(tracer.errorCount).equal(1)
+    expect(tracer.errors.length).equal(1)
+  })
+
+  it('should not break on read only objects', function () {
+    var error = new Error('this happened once')
+    Object.freeze(error)
+    expect(tracer.errorCount).equal(0)
+    expect(tracer.errors.length).equal(0)
+
+    tracer.add(null, error)
+    tracer.add(null, error)
+
+    expect(tracer.errorCount).equal(1)
+    expect(tracer.errors.length).equal(1)
+  })
+
   it('should retain a maximum of 20 errors to send', function () {
     for (var i = 0; i < 5; i++) tracer.add(null, new Error('filling the queue'))
     expect(tracer.errors.length).equal(5)

test/unit/urltils.test.js

@@ -150,21 +150,6 @@ describe('NR URL utilities', function () {
     })
   })
 
-  describe('getHeadersFromHeaderString', function () {
-    it('should return an object of header name and value pairs from a header string', function () {
-      var exampleInput = 'HTTP/1.1 404 Not Found\r\nX-Powered-By: Express\r\nContent-Type: text/html; charset=utf-8\r\nContent-Length: 45\r\nETag: W/"2d-+O+7jmB3UqTatBnw6I80rQ"\r\nDate: Tue, 23 Feb 2016 19:52:05 GMT\r\nConnection: keep-alive\r\n\r\n'
-      var output = urltils.getHeadersFromHeaderString(exampleInput)
-      var expectedOutput = { '52': '05 GMT',
-        'X-Powered-By': 'Express',
-        'Content-Type': 'text/html; charset=utf-8',
-        'Content-Length': '45',
-        ETag: 'W/"2d-+O+7jmB3UqTatBnw6I80rQ"',
-        Connection: 'keep-alive'
-      }
-      expect(output).deep.equal(expectedOutput)
-    })
-  })
-
   describe('isIgnoredError', function() {
     var config = {error_collector : {ignore_status_codes : []}}
 

test/versioned/hapi-7.x/hapi-vhost.tap.js

@@ -35,6 +35,8 @@ test("Hapi vhost support", function (t) {
           "request.headers.host" : "localhost:8089",
           "request.method" : "GET",
           "response.status" : 200,
+          "response.headers.contentLength" : 15,
+          "response.headers.contentType" : "application/json; charset=utf-8",
           "httpResponseCode": "200",
           "httpResponseMessage": "OK",
           "request_uri" : "/test/2"
@@ -45,6 +47,8 @@ test("Hapi vhost support", function (t) {
           "request.headers.host" : "localhost:8089",
           "request.method" : "GET",
           "response.status" : 200,
+          "response.headers.contentLength" : 15,
+          "response.headers.contentType" : "application/json; charset=utf-8",
           "httpResponseCode": "200",
           "request_uri" : "/test/2"
         }, 'parameters should only have request/response params')

test/versioned/hapi-8.x/hapi-vhost.tap.js

@@ -39,6 +39,8 @@ test("Hapi vhost support", function (t) {
           "request.headers.host" : "localhost:8089",
           "request.method" : "GET",
           "response.status" : 200,
+          "response.headers.contentLength" : 15,
+          "response.headers.contentType" : "application/json; charset=utf-8",
           "httpResponseCode": "200",
           "httpResponseMessage": "OK",
           "id" : "1337",
@@ -51,6 +53,8 @@ test("Hapi vhost support", function (t) {
           "request.headers.host" : "localhost:8089",
           "request.method" : "GET",
           "response.status" : 200,
+          "response.headers.contentLength" : 15,
+          "response.headers.contentType" : "application/json; charset=utf-8",
           "httpResponseCode": "200",
           "id" : "1337",
           "name" : "hapi",

test/versioned/hapi-latest/hapi-vhost.tap.js

@@ -35,6 +35,8 @@ test("Hapi vhost support", function (t) {
           "request.headers.host" : "localhost:8089",
           "request.method" : "GET",
           "response.status" : 200,
+          "response.headers.contentLength" : 15,
+          "response.headers.contentType" : "application/json; charset=utf-8",
           "httpResponseCode": "200",
           "httpResponseMessage": "OK",
           "id" : "1337",