refactor: reduce duplication in the error-collector (#2323)

* Refactored _processUserErrors, _processTransactionExceptions, and _processTransactionErrors into a single function: _processErrors * Created _processErrrors test
newrelic · Jul 2, 2024 · 10581bf · 10581bf
1 parent 3a910ef
commit 10581bf
Show file tree

Hide file tree

Showing 2 changed files with 125 additions and 59 deletions.
diff --git a/lib/errors/error-collector.js b/lib/errors/error-collector.js
@@ -89,75 +89,62 @@ class ErrorCollector {
   }
 
   /**
-   * Helper method for processing errors that are created with .noticeError()
+   * Gets the iterable property from the transaction based on the error type
    *
    * @param {Transaction} transaction the collected exception's transaction
-   * @param {number} collectedErrors the number of errors we've successfully .collect()-ed
-   * @param {number} expectedErrors the number of errors marked as expected in noticeError
-   * @returns {Array.<number>} the updated [collectedErrors, expectedErrors] numbers post-processing
+   * @param {string} errorType the type of error: "user", "transactionException", "transaction"
+   * @returns {object[]} the iterable property from the transaction based on the error type
    */
-  _processUserErrors(transaction, collectedErrors, expectedErrors) {
-    if (transaction.userErrors.length) {
-      for (let i = 0; i < transaction.userErrors.length; i++) {
-        const exception = transaction.userErrors[i]
-        if (this.collect(transaction, exception)) {
-          ++collectedErrors
-          // if we could collect it, then check if expected
-          if (
-            urltils.isExpectedError(this.config, transaction.statusCode) ||
-            errorHelper.isExpectedException(transaction, exception, this.config, urltils)
-          ) {
-            ++expectedErrors
-          }
-        }
-      }
+  _getIterableProperty(transaction, errorType) {
+    let iterableProperty = null
+    if (errorType === 'user') {
+      iterableProperty = transaction.userErrors
     }
-
-    return [collectedErrors, expectedErrors]
+    if (errorType === 'transactionException') {
+      iterableProperty = transaction.exceptions
+    }
+    return iterableProperty
   }
 
   /**
-   * Helper method for processing exceptions on the transaction (transaction.exceptions array)
+   * Helper method for processing errors that are created with .noticeError(), exceptions
+   * on the transaction (transaction.exceptions array), and inferred errors based on Transaction metadata.
    *
-   * @param {Transaction} transaction the transaction being processed
-   * @param {number} collectedErrors the number of errors successfully .collect()-ed
-   * @param {number} expectedErrors the number of collected errors that were expected
-   * @returns {Array.<number>} the updated [collectedErrors, expectedErrors] numbers post-processing
+   * @param {Transaction} transaction the collected exception's transaction
+   * @param {number} collectedErrors the number of errors we've successfully .collect()-ed
+   * @param {number} expectedErrors the number of errors marked as expected in noticeError
+   * @param {string} errorType the type of error to be processed; "user", "transactionException", "transaction"
+   * @returns {Array.<number>} the updated [collectedErrors, expectedErrors] numbers post processing
    */
-  _processTransactionExceptions(transaction, collectedErrors, expectedErrors) {
-    for (let i = 0; i < transaction.exceptions.length; i++) {
-      const exception = transaction.exceptions[i]
-      if (this.collect(transaction, exception)) {
-        ++collectedErrors
-        // if we could collect it, then check if expected
-        if (
-          urltils.isExpectedError(this.config, transaction.statusCode) ||
-          errorHelper.isExpectedException(transaction, exception, this.config, urltils)
-        ) {
-          ++expectedErrors
+  _processErrors(transaction, collectedErrors, expectedErrors, errorType) {
+    const iterableProperty = this._getIterableProperty(transaction, errorType)
+    if (iterableProperty === null && errorType === 'transaction') {
+      if (this.collect(transaction)) {
+        collectedErrors++
+        if (urltils.isExpectedError(this.config, transaction.statusCode)) {
+          expectedErrors++
         }
       }
+      return [collectedErrors, expectedErrors]
     }
 
-    return [collectedErrors, expectedErrors]
-  }
+    if (iterableProperty === null) {
+      return [collectedErrors, expectedErrors]
+    }
 
-  /**
-   * Helper method for processing an inferred error based on Transaction metadata
-   *
-   * @param {Transaction} transaction the transaction being processed
-   * @param {number} collectedErrors the number of errors successfully .collect()-ed
-   * @param {number} expectedErrors the number of collected errors that were expected
-   * @returns {Array.<number>} the updated [collectedErrors, expectedErrors] numbers post-processing
-   */
-  _processTransactionErrors(transaction, collectedErrors, expectedErrors) {
-    if (this.collect(transaction)) {
-      ++collectedErrors
-      if (urltils.isExpectedError(this.config, transaction.statusCode)) {
-        ++expectedErrors
+    for (let i = 0; i < iterableProperty.length; i++) {
+      const exception = iterableProperty[i]
+      if (!this.collect(transaction, exception)) {
+        continue
+      }
+      collectedErrors++
+      if (
+        urltils.isExpectedError(this.config, transaction.statusCode) ||
+        errorHelper.isExpectedException(transaction, exception, this.config, urltils)
+      ) {
+        expectedErrors++
       }
     }
-
     return [collectedErrors, expectedErrors]
   }
 
@@ -183,27 +170,30 @@ class ErrorCollector {
 
     // errors from noticeError are currently exempt from
     // ignore and exclude rules
-    ;[collectedErrors, expectedErrors] = this._processUserErrors(
+    ;[collectedErrors, expectedErrors] = this._processErrors(
       transaction,
       collectedErrors,
-      expectedErrors
+      expectedErrors,
+      'user'
     )
 
     const isErroredTransaction = urltils.isError(this.config, transaction.statusCode)
     const isIgnoredErrorStatusCode = urltils.isIgnoredError(this.config, transaction.statusCode)
 
     // collect other exceptions only if status code is not ignored
     if (transaction.exceptions.length && !isIgnoredErrorStatusCode) {
-      ;[collectedErrors, expectedErrors] = this._processTransactionExceptions(
+      ;[collectedErrors, expectedErrors] = this._processErrors(
         transaction,
         collectedErrors,
-        expectedErrors
+        expectedErrors,
+        'transactionException'
       )
     } else if (isErroredTransaction) {
-      ;[collectedErrors, expectedErrors] = this._processTransactionErrors(
+      ;[collectedErrors, expectedErrors] = this._processErrors(
         transaction,
         collectedErrors,
-        expectedErrors
+        expectedErrors,
+        'transaction'
       )
     }
 

diff --git a/test/unit/errors/error-collector.test.js b/test/unit/errors/error-collector.test.js
@@ -2370,3 +2370,79 @@ test('When using the async listener', (t) => {
     })
   }
 })
+
+tap.test('_processErrors', (t) => {
+  t.beforeEach((t) => {
+    t.context.agent = helper.loadMockedAgent({
+      attributes: {
+        enabled: true
+      }
+    })
+
+    const transaction = new Transaction(t.context.agent)
+    transaction.url = '/'
+    t.context.transaction = transaction
+    t.context.errorCollector = t.context.agent.errors
+  })
+
+  t.afterEach((t) => {
+    helper.unloadAgent(t.context.agent)
+  })
+
+  t.test('invalid errorType should return no iterableProperty', (t) => {
+    const { errorCollector, transaction } = t.context
+    const errorType = 'invalid'
+    const result = errorCollector._getIterableProperty(transaction, errorType)
+
+    t.equal(result, null)
+    t.end()
+  })
+
+  t.test('if errorType is transaction, should return no iterableProperty', (t) => {
+    const { errorCollector, transaction } = t.context
+    const errorType = 'transaction'
+    const result = errorCollector._getIterableProperty(transaction, errorType)
+
+    t.equal(result, null)
+    t.end()
+  })
+
+  t.test('if type is user, return an array of objects', (t) => {
+    const { errorCollector, transaction } = t.context
+    const errorType = 'user'
+    const result = errorCollector._getIterableProperty(transaction, errorType)
+
+    t.same(result, [])
+    t.end()
+  })
+
+  t.test('if type is transactionException, return an array of objects', (t) => {
+    const { errorCollector, transaction } = t.context
+    const errorType = 'transactionException'
+    const result = errorCollector._getIterableProperty(transaction, errorType)
+
+    t.same(result, [])
+    t.end()
+  })
+
+  t.test(
+    'if iterableProperty is null and errorType is not transaction, do not modify collectedErrors or expectedErrors',
+    (t) => {
+      const { errorCollector, transaction } = t.context
+      const errorType = 'error'
+      const collectedErrors = 0
+      const expectedErrors = 0
+      const result = errorCollector._processErrors(
+        transaction,
+        collectedErrors,
+        expectedErrors,
+        errorType
+      )
+
+      t.same(result, [collectedErrors, expectedErrors])
+      t.end()
+    }
+  )
+
+  t.end()
+})