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 all 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
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,21 @@ describe('Error Reporting utilities', () => {
});
});
});

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

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

it('should return true if message is not defined', () => {
const result = isAllowedToBeNotified({ name: 'dummy error' });
expect(result).toBeTruthy();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const REQUEST_TIMEOUT_MS = 10 * 1000; // 10 seconds
const NOTIFIER_NAME = 'RudderStack JavaScript SDK Error Notifier';
const SDK_GITHUB_URL = 'https://github.com/rudderlabs/rudder-sdk-js';
const SOURCE_NAME = 'js';
const ERROR_REPORTING_PLUGIN = 'ErrorReportingPlugin';

export {
SDK_FILE_NAME_PREFIXES,
Expand All @@ -31,4 +32,5 @@ export {
NOTIFIER_NAME,
SDK_GITHUB_URL,
SOURCE_NAME,
ERROR_REPORTING_PLUGIN,
};
86 changes: 19 additions & 67 deletions packages/analytics-js-plugins/src/errorReporting/event/event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ import type { ErrorState } from '@rudderstack/analytics-js-common/types/ErrorHan
import type { ILogger } from '@rudderstack/analytics-js-common/types/Logger';
import ErrorStackParser from 'error-stack-parser';
import type { Exception, Stackframe } from '@rudderstack/analytics-js-common/types/Metrics';
import { stringifyWithoutCircular } from '@rudderstack/analytics-js-common/utilities/json';
import type { FrameType, IErrorFormat } from '../types';
import { hasStack, isError } from './utils';
import { ERROR_REPORTING_PLUGIN } from '../constants';

const normaliseFunctionName = (name: string) =>
/^global code$/i.test(name) ? 'global code' : name;
Expand Down Expand Up @@ -64,80 +66,32 @@ 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, component: string, 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 (isError(maybeError)) {
error = maybeError;
} else if (typeof maybeError === 'object' && hasNecessaryFields(maybeError)) {
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;
}
logger?.warn(
`${ERROR_REPORTING_PLUGIN}:: ${component} received a non-error: ${stringifyWithoutCircular(error)}`,
);
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;
} catch (e) {
if (hasStack(e)) {
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
// will only have one extra internal frame from this function
internalFrames = 1;
}
}
Expand All @@ -160,12 +114,10 @@ class ErrorFormat implements IErrorFormat {
errorFramesToSkip = 0,
logger?: ILogger,
) {
const [error, internalFrames] = normaliseError(
maybeError,
tolerateNonErrors,
component,
logger,
);
const [error, internalFrames] = normaliseError(maybeError, component, logger);
if (!error) {
return undefined;
}
let event;
try {
const stacktrace = getStacktrace(
Expand Down
5 changes: 5 additions & 0 deletions 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 @@ -83,6 +84,10 @@
logger,
);

if (!errorPayload || !isAllowedToBeNotified(errorPayload.errors[0])) {
return;

Check warning on line 88 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#L88

Added line #L88 was not covered by tests
}

// filter errors
if (!isRudderSDKError(errorPayload.errors[0])) {
return;
Expand Down
20 changes: 20 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,24 @@ const getBugsnagErrorEvent = (
],
});

/**
* A function to determine whether the error should be promoted to notify or not
* @param {Error} error
* @returns
*/
const isAllowedToBeNotified = (event: any) => {
const errorMessage = event.message;
if (errorMessage && typeof errorMessage === 'string') {
return !ERROR_MESSAGES_TO_BE_FILTERED.some(e => errorMessage.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 +207,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