Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NEWRELIC-5279 refactored Error Collector to reduce complexity #1431

Merged
merged 1 commit into from
Nov 23, 2022
Merged

NEWRELIC-5279 refactored Error Collector to reduce complexity #1431

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
179 changes: 124 additions & 55 deletions lib/errors/error-collector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@

'use strict'

/* eslint sonarjs/cognitive-complexity: ["error", 25] -- TODO: https://issues.newrelic.com/browse/NEWRELIC-5252 */

const errorsModule = require('./index')

const logger = require('../logger').child({ component: 'error_tracer' })
const urltils = require('../util/urltils')
const Exception = require('../errors').Exception
const Transaction = require('../transaction')
const errorHelper = require('./helper')
const createError = errorsModule.createError
const createEvent = errorsModule.createEvent
Expand Down Expand Up @@ -75,6 +74,7 @@ class ErrorCollector {
*
* @param {?Transaction} transaction -
* @param {Error} exception - The error to be checked.
* @returns {boolean} whether or not the exception has already been tracked
*/
_haveSeen(transaction, exception) {
const txId = transaction ? transaction.id : 'Unknown'
Expand Down Expand Up @@ -109,12 +109,78 @@ class ErrorCollector {
return false
}

/**
* Helper method for processing errors that are created with .noticeError()
*
* @param {Transaction} transaction the collected exception's transaction
* @param {number} collectedErrors the number of errors we've successfully .collect()-ed
* @returns {number} the new number of errors successfully .collect()-ed
*/
_processUserErrors(transaction, collectedErrors) {
if (transaction.userErrors.length) {
for (let i = 0; i < transaction.userErrors.length; i++) {
const exception = transaction.userErrors[i]
if (this.collect(transaction, exception)) {
++collectedErrors
}
}
}

return collectedErrors
}

/**
* Helper method for processing exceptions on the transaction (transaction.exceptions array)
*
* @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
*/
_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.error, this.config, urltils)
) {
++expectedErrors
}
}
}

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
}
}

return [collectedErrors, expectedErrors]
}

/**
* Every finished transaction goes through this handler, so do as little as
* possible.
*
* @param {Transaction} transaction
* @returns {number} The number of unexpected errors
* TODO: Prob shouldn't do any work if errors fully disabled.
*
* @param {Transaction} transaction the completed transaction
*/
onTransactionFinished(transaction) {
if (!transaction) {
Expand All @@ -124,48 +190,30 @@ class ErrorCollector {
return
}

// TODO: Prob shouldn't do any work if errors fully disabled.

// collect user errors even if status code is ignored
let collectedErrors = 0
let expectedErrors = 0

// errors from noticeError are currently exempt from
// ignore and exclude rules
if (transaction.userErrors.length) {
for (let i = 0; i < transaction.userErrors.length; i++) {
const exception = transaction.userErrors[i]
if (this.collect(transaction, exception)) {
++collectedErrors
}
}
}
collectedErrors = this._processUserErrors(transaction, collectedErrors)

const isErroredTransaction = urltils.isError(this.config, transaction.statusCode)
const isIgnoredErrorStatusCode = urltils.isIgnoredError(this.config, transaction.statusCode)

const isExpectedErrorStatusCode = urltils.isExpectedError(this.config, transaction.statusCode)

// collect other exceptions only if status code is not ignored
if (transaction.exceptions.length && !isIgnoredErrorStatusCode) {
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 (
isExpectedErrorStatusCode ||
errorHelper.isExpectedException(transaction, exception.error, this.config, urltils)
) {
++expectedErrors
}
}
}
} else if (isErroredTransaction && this.collect(transaction)) {
++collectedErrors
if (isExpectedErrorStatusCode) {
++expectedErrors
}
;[collectedErrors, expectedErrors] = this._processTransactionExceptions(
transaction,
collectedErrors,
expectedErrors
)
} else if (isErroredTransaction) {
;[collectedErrors, expectedErrors] = this._processTransactionErrors(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these semicolons doing here? Did eslint put them there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we do not use semi-colons, this is necessary for destructuring of let variables. The AST parser does not know how to handle it without it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I don't think this is necessary. It's only needed when object destructuring

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is an AST parser issue but how ESLint parses code without semi colons

transaction,
collectedErrors,
expectedErrors
)
}

const unexpectedErrors = collectedErrors - expectedErrors
Expand Down Expand Up @@ -226,9 +274,8 @@ class ErrorCollector {
* NOTE: this interface is unofficial and may change in future.
*
* @param {?Transaction} transaction Transaction associated with the error.
* @param {Exception} exception The Exception to be traced.
* @param error
* @param customAttributes
* @param {*} error The error passed into `API#noticeError()`
* @param {object} customAttributes custom attributes to add to the error
*/
addUserError(transaction, error, customAttributes) {
if (!error) {
Expand Down Expand Up @@ -263,13 +310,38 @@ class ErrorCollector {
*
* @param {?Transaction} transaction Transaction associated with the error.
* @param {?Exception} exception The Exception object to be traced.
* @returns {bool} True if the error was collected.
* @returns {boolean} True if the error was collected.
*/
collect(transaction, exception) {
if (!exception) {
exception = new Exception({})
collect(transaction, exception = new Exception({})) {
if (!this._isValidException(exception, transaction)) {
return false
}

const errorTrace = createError(transaction, exception, this.config)
this._maybeRecordErrorMetrics(errorTrace, transaction)

// defaults true in config/index. can be modified server-side
if (this.config.collect_errors) {
this.traceAggregator.add(errorTrace)
}

if (this.config.error_collector.capture_events === true) {
const priority = (transaction && transaction.priority) || Math.random()
const event = createEvent(transaction, errorTrace, exception.timestamp, this.config)
this.eventAggregator.add(event, priority)
}

return true
}

/**
* Helper method for ensuring that a collected exception/transaction combination can be collected
*
* @param {object} exception the exception to validate
* @param {Transaction} transaction the Transaction to validate, if exception is malformed we'll try to fallback to transaction data
* @returns {boolean} whether or not the exception/transaction combo has everything needed for processing
*/
_isValidException(exception, transaction) {
if (exception.error) {
if (this._haveSeen(transaction, exception.error)) {
return false
Expand All @@ -288,10 +360,20 @@ class ErrorCollector {

if (exception.error) {
logger.trace(exception.error, 'Got exception to trace:')
} else {
logger.trace(transaction, 'Got transaction error to trace:')
bizob2828 marked this conversation as resolved.
Show resolved Hide resolved
}

const errorTrace = createError(transaction, exception, this.config)
return true
}

/**
* Helper method for recording metrics about errors depending on the type of error that happened
*
* @param {Array} errorTrace list of error information
* @param {Transaction} transaction the transaction associated with the trace
*/
_maybeRecordErrorMetrics(errorTrace, transaction) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 384 and 385 can be combined right? I thought we had a rule eslint rule to catch that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we do have a rule, but it only kicks in if the logic doesn't change from the combining, in this case, the logic would change in that we would start logging "other" error metrics if the transaction doesn't exist. All that being said, I think it would be safe to combine regardless. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never mind. this should stay. other has a special meaning within a transaction

const isExpectedError = true === errorTrace[4].intrinsics['error.expected']

if (isExpectedError) {
Expand All @@ -307,19 +389,6 @@ class ErrorCollector {
}
}
}

// defaults true in config/index. can be modified server-side
if (this.config.collect_errors) {
this.traceAggregator.add(errorTrace)
}

if (this.config.error_collector.capture_events === true) {
const priority = (transaction && transaction.priority) || Math.random()
const event = createEvent(transaction, errorTrace, exception.timestamp, this.config)
this.eventAggregator.add(event, priority)
}

return true
}

// TODO: ideally, this becomes unnecessary
Expand Down