From 10581bf8cdad5c61c25dc1309ad97ca36d58cf79 Mon Sep 17 00:00:00 2001 From: Amy Chisholm Date: Tue, 2 Jul 2024 10:25:00 -0700 Subject: [PATCH] refactor: reduce duplication in the error-collector (#2323) * Refactored _processUserErrors, _processTransactionExceptions, and _processTransactionErrors into a single function: _processErrors * Created _processErrrors test --- lib/errors/error-collector.js | 108 ++++++++++------------- test/unit/errors/error-collector.test.js | 76 ++++++++++++++++ 2 files changed, 125 insertions(+), 59 deletions(-) diff --git a/lib/errors/error-collector.js b/lib/errors/error-collector.js index 88379d3aac..c5b674abbe 100644 --- 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.} 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.} 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.} 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.} 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,10 +170,11 @@ 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) @@ -194,16 +182,18 @@ class ErrorCollector { // 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 index bc89a1cb8b..b35b92e1db 100644 --- 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() +})