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.

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

fix: error filtering of non-errors #1811

Merged
merged 7 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/analytics-js-common/src/constants/errors.ts
saikumarrs marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const FAILED_REQUEST_ERR_MSG_PREFIX = 'The request failed';
const ERROR_MESSAGES_TO_BE_FILTERED = [FAILED_REQUEST_ERR_MSG_PREFIX];

Check warning on line 2 in packages/analytics-js-common/src/constants/errors.ts

View check run for this annotation

Codecov / codecov/patch

packages/analytics-js-common/src/constants/errors.ts#L1-L2

Added lines #L1 - L2 were not covered by tests

export { FAILED_REQUEST_ERR_MSG_PREFIX, ERROR_MESSAGES_TO_BE_FILTERED };

Check warning on line 4 in packages/analytics-js-common/src/constants/errors.ts

View check run for this annotation

Codecov / codecov/patch

packages/analytics-js-common/src/constants/errors.ts#L4

Added line #L4 was not covered by tests
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
getBugsnagErrorEvent,
getErrorDeliveryPayload,
getConfigForPayloadCreation,
isAllowedToBeNotified,
} from '../../src/errorReporting/utils';

jest.mock('@rudderstack/analytics-js-common/utilities/uuId', () => ({
Expand Down Expand Up @@ -548,4 +549,31 @@ describe('Error Reporting utilities', () => {
});
});
});

describe('ErrorHandler - isAllowedToBeNotified', () => {
it('should return true for Error argument value', () => {
const result = isAllowedToBeNotified(new Error('dummy error'));
expect(result).toBeTruthy();
});

it('should return true for Error argument value', () => {
const result = isAllowedToBeNotified(new Error('The request failed'));
expect(result).toBeFalsy();
});

it('should return true for ErrorEvent argument value', () => {
const result = isAllowedToBeNotified(new ErrorEvent('dummy error'));
expect(result).toBeTruthy();
});

it('should return true for PromiseRejectionEvent argument value', () => {
const result = isAllowedToBeNotified(new PromiseRejectionEvent('dummy error'));
expect(result).toBeTruthy();
});

it('should return true for PromiseRejectionEvent argument value', () => {
const result = isAllowedToBeNotified(new PromiseRejectionEvent('The request failed'));
expect(result).toBeFalsy();
});
});
});
81 changes: 15 additions & 66 deletions packages/analytics-js-plugins/src/errorReporting/event/event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,71 +64,22 @@ const hasNecessaryFields = (error: any) =>
(typeof error.name === 'string' || typeof error.errorClass === 'string') &&
(typeof error.message === 'string' || typeof error.errorMessage === 'string');

const normaliseError = (
maybeError: any,
tolerateNonErrors: boolean,
component: string,
logger?: ILogger,
) => {
const normaliseError = (maybeError: any, logger?: ILogger) => {
let error;
saikumarrs marked this conversation as resolved.
Show resolved Hide resolved
let internalFrames = 0;

const createAndLogInputError = (reason: string) => {
const verb = component === 'error cause' ? 'was' : 'received';
if (logger) logger.warn(`${component} ${verb} a non-error: "${reason}"`);
const err = new Error(
`${component} ${verb} a non-error. See "${component}" tab for more detail.`,
);
err.name = 'InvalidError';
return err;
};

// In some cases:
//
// - the promise rejection handler (both in the browser and node)
// - the node uncaughtException handler
//
// We are really limited in what we can do to get a stacktrace. So we use the
// tolerateNonErrors option to ensure that the resulting error communicates as
// such.
if (!tolerateNonErrors) {
if (isError(maybeError)) {
error = maybeError;
} else {
error = createAndLogInputError(typeof maybeError);
internalFrames += 2;
}
if (maybeError !== null && isError(maybeError)) {
saikumarrs marked this conversation as resolved.
Show resolved Hide resolved
error = maybeError;
} else if (maybeError !== null && hasNecessaryFields(maybeError)) {
saikumarrs marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we check if it is an object first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

error = new Error(maybeError.message || maybeError.errorMessage);
error.name = maybeError.name || maybeError.errorClass;
internalFrames += 1;
} else {
switch (typeof maybeError) {
case 'string':
case 'number':
case 'boolean':
error = new Error(String(maybeError));
internalFrames += 1;
break;
case 'function':
error = createAndLogInputError('function');
internalFrames += 2;
break;
case 'object':
if (maybeError !== null && isError(maybeError)) {
error = maybeError;
} else if (maybeError !== null && hasNecessaryFields(maybeError)) {
error = new Error(maybeError.message || maybeError.errorMessage);
error.name = maybeError.name || maybeError.errorClass;
internalFrames += 1;
} else {
error = createAndLogInputError(maybeError === null ? 'null' : 'unsupported object');
internalFrames += 2;
}
break;
default:
error = createAndLogInputError('nothing');
internalFrames += 2;
}
if (logger) logger.warn(error);
saikumarrs marked this conversation as resolved.
Show resolved Hide resolved
error = undefined;
}

if (!hasStack(error)) {
if (error && !hasStack(error)) {
// in IE10/11 a new Error() doesn't have a stacktrace until you throw it, so try that here
try {
throw error;
Expand All @@ -137,7 +88,7 @@ const normaliseError = (
error = e;
// if the error only got a stacktrace after we threw it here, we know it
// will only have one extra internal frame from this function, regardless
// of whether it went through createAndLogInputError() or not
// of whether it went through logInputError() or not
saikumarrs marked this conversation as resolved.
Show resolved Hide resolved
internalFrames = 1;
}
}
Expand All @@ -160,12 +111,10 @@ class ErrorFormat implements IErrorFormat {
errorFramesToSkip = 0,
logger?: ILogger,
) {
const [error, internalFrames] = normaliseError(
maybeError,
tolerateNonErrors,
component,
logger,
);
const [error, internalFrames] = normaliseError(maybeError, logger);
if (!error) {
return undefined;
}
let event;
try {
const stacktrace = getStacktrace(
Expand Down
6 changes: 5 additions & 1 deletion packages/analytics-js-plugins/src/errorReporting/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
isRudderSDKError,
getBugsnagErrorEvent,
getErrorDeliveryPayload,
isAllowedToBeNotified,
} from './utils';
import { REQUEST_TIMEOUT_MS } from './constants';
import { ErrorFormat } from './event/event';
Expand Down Expand Up @@ -73,6 +74,9 @@
const { component, tolerateNonErrors, errorFramesToSkip, normalizedError } =
getConfigForPayloadCreation(error, errorState?.severityReason.type as string);

if (!isAllowedToBeNotified(error)) {
saikumarrs marked this conversation as resolved.
Show resolved Hide resolved
return;

Check warning on line 78 in packages/analytics-js-plugins/src/errorReporting/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/analytics-js-plugins/src/errorReporting/index.ts#L78

Added line #L78 was not covered by tests
}
// Generate the error payload
const errorPayload = ErrorFormat.create(
normalizedError,
Expand All @@ -84,7 +88,7 @@
);

// filter errors
if (!isRudderSDKError(errorPayload.errors[0])) {
if (!errorPayload || !isRudderSDKError(errorPayload.errors[0])) {
return;
}

Expand Down
22 changes: 22 additions & 0 deletions packages/analytics-js-plugins/src/errorReporting/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {
import { generateUUID } from '@rudderstack/analytics-js-common/utilities/uuId';
import { METRICS_PAYLOAD_VERSION } from '@rudderstack/analytics-js-common/constants/metrics';
import { stringifyWithoutCircular } from '@rudderstack/analytics-js-common/utilities/json';
import { ERROR_MESSAGES_TO_BE_FILTERED } from '@rudderstack/analytics-js-common/constants/errors';
saikumarrs marked this conversation as resolved.
Show resolved Hide resolved
import {
APP_STATE_EXCLUDE_KEYS,
DEV_HOSTS,
Expand Down Expand Up @@ -142,6 +143,26 @@ const getBugsnagErrorEvent = (
],
});

/**
* A function to determine whether the error should be promoted to notify or not
* @param {Error} error
* @returns
*/
const isAllowedToBeNotified = (error: SDKError) => {
if ((error instanceof Error || error instanceof ErrorEvent) && error.message) {
return !ERROR_MESSAGES_TO_BE_FILTERED.some(e => error.message.includes(e));
}
if (error instanceof PromiseRejectionEvent && typeof error.reason === 'string') {
return !ERROR_MESSAGES_TO_BE_FILTERED.some(e => error.reason.includes(e));
}
return true;
};

/**
* A function to determine if the error is from Rudder SDK
* @param {Error} event
* @returns
*/
const isRudderSDKError = (event: any) => {
const errorOrigin = event.stacktrace?.[0]?.file;

Expand Down Expand Up @@ -188,4 +209,5 @@ export {
isRudderSDKError,
getErrorDeliveryPayload,
getErrorContext,
isAllowedToBeNotified,
};
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('Config Manager Common Utilities', () => {

updateReportingState(mockSourceConfig, mockLogger);

// expect(state.reporting.isErrorReportingEnabled.value).toBe(true); // TODO: uncomment this line when error reporting is enabled
expect(state.reporting.isErrorReportingEnabled.value).toBe(true);
expect(state.reporting.isMetricsReportingEnabled.value).toBe(true);
expect(mockLogger.warn).not.toHaveBeenCalled();
});
Expand All @@ -133,7 +133,7 @@ describe('Config Manager Common Utilities', () => {

updateReportingState(mockSourceConfig, mockLogger);

// expect(state.reporting.isErrorReportingEnabled.value).toBe(true); // TODO: uncomment this line when error reporting is enabled
expect(state.reporting.isErrorReportingEnabled.value).toBe(true);
expect(state.reporting.isMetricsReportingEnabled.value).toBe(true);
expect(mockLogger.warn).not.toHaveBeenCalled();
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
processError,
getNormalizedErrorForUnhandledError,
isAllowedToBeNotified,
} from '../../../src/services/ErrorHandler/processError';

jest.mock('../../../src/components/utilities/event', () => {
Expand Down Expand Up @@ -95,7 +94,7 @@ describe('ErrorHandler - getNormalizedErrorForUnhandledError', () => {
expect(normalizedError).toBeUndefined();
});

it.skip('should return error instance for Event argument value with SDK script target', () => {
it('should return error instance for Event argument value with SDK script target', () => {
const event = new Event('dummyError');
const targetElement = document.createElement('script');
targetElement.dataset.loader = 'RS_JS_SDK';
Expand All @@ -116,30 +115,3 @@ describe('ErrorHandler - getNormalizedErrorForUnhandledError', () => {
);
});
});

describe('ErrorHandler - isAllowedToBeNotified', () => {
it('should return true for Error argument value', () => {
const result = isAllowedToBeNotified(new Error('dummy error'));
expect(result).toBeTruthy();
});

it('should return true for Error argument value', () => {
const result = isAllowedToBeNotified(new Error('The request failed'));
expect(result).toBeFalsy();
});

it('should return true for ErrorEvent argument value', () => {
const result = isAllowedToBeNotified(new ErrorEvent('dummy error'));
expect(result).toBeTruthy();
});

it('should return true for PromiseRejectionEvent argument value', () => {
const result = isAllowedToBeNotified(new PromiseRejectionEvent('dummy error'));
expect(result).toBeTruthy();
});

it('should return true for PromiseRejectionEvent argument value', () => {
const result = isAllowedToBeNotified(new PromiseRejectionEvent('The request failed'));
expect(result).toBeFalsy();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,8 @@ const getSDKUrl = (): string | undefined => {
* @param logger Logger instance
*/
const updateReportingState = (res: SourceConfigResponse): void => {
state.reporting.isErrorReportingEnabled.value = false;
// TODO: Enable this once the error reporting is tested properly
// state.reporting.isErrorReportingEnabled.value =
// isErrorReportingEnabled(res.source.config) && !isSDKRunningInChromeExtension();
state.reporting.isErrorReportingEnabled.value =
isErrorReportingEnabled(res.source.config) && !isSDKRunningInChromeExtension();
state.reporting.isMetricsReportingEnabled.value = isMetricsReportingEnabled(res.source.config);
};

Expand Down
8 changes: 0 additions & 8 deletions packages/analytics-js/src/constants/errors.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,7 @@ import {
import { state } from '../../state';
import { defaultPluginEngine } from '../PluginEngine';
import { defaultLogger } from '../Logger';
import {
isAllowedToBeNotified,
getNormalizedErrorForUnhandledError,
processError,
} from './processError';
import { getNormalizedErrorForUnhandledError, processError } from './processError';

/**
* A service to handle errors
Expand Down Expand Up @@ -204,7 +200,7 @@ class ErrorHandler implements IErrorHandler {
* @param {Error} error Error instance from handled error
*/
notifyError(error: SDKError, errorState: ErrorState) {
if (this.pluginEngine && this.httpClient && isAllowedToBeNotified(error)) {
if (this.pluginEngine && this.httpClient) {
try {
this.pluginEngine.invokeSingle(
'errorReporting.notify',
Expand Down
Loading