Skip to content

Commit

Permalink
🏗 Simplify error reporting thresholds (#39975)
Browse files Browse the repository at this point in the history
* Replace magic numbers with consts

* Fix incorrect comparison for non-AMP JS threshold

* Reduce reporting threshold
  • Loading branch information
danielrozenberg authored Apr 29, 2024
1 parent 20cc74f commit 0df5aa4
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 14 deletions.
33 changes: 22 additions & 11 deletions src/error-reporting.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,20 +44,35 @@ const ABORTED = 'AbortError';
* them, but we'd still like to report the rough number.
* @const {number}
*/
const NON_ACTIONABLE_ERROR_THROTTLE_THRESHOLD = 0.001;
const NON_ACTIONABLE_ERROR_THROTTLE_THRESHOLD = 0.0001;

/**
* The threshold for errors throttled because nothing can be done about
* them, but we'd still like to report the rough number.
* @const {number}
*/
const USER_ERROR_THROTTLE_THRESHOLD = 0.1;
const USER_ERROR_THROTTLE_THRESHOLD = 0.01;

/**
* Chance to post to the new error reporting endpoint.
* The threshold for reporting errors to the beta error reporting endpoint
* instead of the production endpoint.
* @const {number}
*/
const BETA_ERROR_REPORT_URL_FREQ = 0.1;
const REPORT_ERROR_TO_BETA_ENDPOINT_THRESHOLD = 0.1;

/**
* The threshold for errors on pages with non-AMP JS. These errors can almost
* never be acted upon, but spikes such as due to buggy browser extensions may
* be helpful to notify authors.
* @const {number}
*/
const NON_AMP_JS_ERROR_THRESHOLD = 0.01;

/**
* Throttles reports for the Stable version.
* @const {number}
*/
const THROTTLE_STABLE_THRESHOLD = 0.9;

/**
* Collects error messages, so they can be included in subsequent reports.
Expand Down Expand Up @@ -319,10 +334,7 @@ function onError(message, filename, line, col, error) {
} catch (ignore) {
// Ignore errors during error report generation.
}
if (hasNonAmpJs && Math.random() > 0.01) {
// Only report 1% of errors on pages with non-AMP JS.
// These errors can almost never be acted upon, but spikes such as
// due to buggy browser extensions may be helpful to notify authors.
if (hasNonAmpJs && Math.random() < NON_AMP_JS_ERROR_THRESHOLD) {
return;
}
const data = getErrorReportData(
Expand Down Expand Up @@ -357,7 +369,7 @@ function onError(message, filename, line, col, error) {
* @return {string} error reporting endpoint URL.
*/
function chooseReportingUrl_() {
return Math.random() < BETA_ERROR_REPORT_URL_FREQ
return Math.random() < REPORT_ERROR_TO_BETA_ENDPOINT_THRESHOLD
? urls.betaErrorReporting
: urls.errorReporting;
}
Expand All @@ -373,8 +385,7 @@ export function reportErrorToServerOrViewer(win, data) {
// to the viewer is exactly the same as the data passed to the server
// below.

// Throttle reports from Stable by 90%.
if (data['pt'] && Math.random() < 0.9) {
if (data['pt'] && Math.random() < THROTTLE_STABLE_THRESHOLD) {
return Promise.resolve();
}

Expand Down
6 changes: 3 additions & 3 deletions test/unit/test-error-reporting.js
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ describes.sandboxed('getErrorReportData', {}, (env) => {
});

it('should report throttled load errors at threshold', () => {
nextRandomNumber = 1e-3;
nextRandomNumber = 1e-4;
const e = new Error('Failed to load:');
const data = getErrorReportData(
undefined,
Expand All @@ -560,7 +560,7 @@ describes.sandboxed('getErrorReportData', {}, (env) => {
});

it('should report throttled Script errors at threshold', () => {
nextRandomNumber = 1e-3;
nextRandomNumber = 1e-4;
const e = new Error('Script error.');
const data = getErrorReportData(
undefined,
Expand All @@ -574,7 +574,7 @@ describes.sandboxed('getErrorReportData', {}, (env) => {
});

it('should report throttled load errors under threshold', () => {
nextRandomNumber = 1e-3 - 1e-4;
nextRandomNumber = 1e-4 - 1e-5;
const e = new Error('Failed to load:');
const data = getErrorReportData(
undefined,
Expand Down

0 comments on commit 0df5aa4

Please sign in to comment.