blueslip: Only show in dev mode, or report to Sentry.

This removes the production reporting to `/json/report/error` upon
`blueslip.error`, and replaces it with reporting to Sentry, if
enabled.  Sentry provides better reporting and grouping for exceptions
than the email- and `#errors`-reporting provided by the
`/json/report/error` endpoint.

The development behaviour of rendering `blueslip.error` messages and
stacktraces immediately, and stopping execution, is preserved.

To better chain exception information, the whole previous exception is
passed to `blueslip.error`, not just the stack, and the second
parameter is formalized to be an object to map to Sentry's "context"
concept.

docs/subsystems/logging.md

@@ -179,45 +179,41 @@ See `./scripts/log-search --help` for complete documentation.
 
 ## Blueslip frontend error reporting
 
-We have a custom library, called `blueslip` (named after the form used
-at MIT to report problems with the facilities), that takes care of
-reporting JavaScript errors. In production, this means emailing the
-server administrators (though the setting controlling this,
-`BROWSER_ERROR_REPORTING`, is disabled by default, since most problems
-are unlikely to be addressable by a system administrator, and it's
-very hard to make JavaScript errors not at least somewhat spammy due
-to the variety of browser versions and sets of extensions that someone
-might use). In development, this means displaying a highly visible
-overlay over the message view area, to make exceptions in testing a
-new feature hard to miss.
+We have a custom library, called `blueslip` (named after the form used at MIT to
+report problems with the facilities), that logs and reports JavaScript errors in
+the browser. In production, this uses Sentry (if configured) to report and
+aggregate errors. In development, this means displaying a highly visible overlay
+over the message view area, to make exceptions in testing a new feature hard to
+miss.
 
 - Blueslip is implemented in `web/src/blueslip.ts`.
-- In order to capture essentially any error occurring in the browser,
-  Blueslip listens for the `error` event on `window`, and has methods
-  for being manually triggered by Zulip JavaScript code for warnings
-  and assertion failures.
-- Blueslip keeps a log of all the notices it has received during a
-  browser session, and includes them in reports to the server, so that
-  one can see cases where exceptions chained together. You can print
-  this log from the browser console using
+- In order to capture essentially any error occurring in the browser, in
+  development mode Blueslip listens for the `error` event on `window`.
+- It also has methods for being manually triggered by Zulip JavaScript code for
+  warnings and assertion failures. Explicit `blueslip.error` calls are sent to
+  Sentry, if configured.
+- Blueslip keeps a log of all the notices it has received during a browser
+  session, so that one can see cases where exceptions chained together. You can
+  print this log from the browser console using
   `blueslip = require("./src/blueslip"); blueslip.get_log()`.
 
 Blueslip supports several error levels:
 
-- `throw new Error(…)`: For fatal errors that cannot be easily
-  recovered from. We try to avoid using it, since it kills the
-  current JS thread, rather than returning execution to the caller.
-- `blueslip.error`: For logging of events that are definitely caused
-  by a bug and thus sufficiently important to be reported, but where
-  we can handle the error without creating major user-facing problems
-  (e.g. an exception when handling a presence update).
-- `blueslip.warn`: For logging of events that are a problem but not
-  important enough to send an email about in production. They are,
-  however, highlighted in the JS console in development.
-- `blueslip.log` (and `blueslip.info`): Logged to the JS console in
-  development and also in the blueslip log in production. Useful for
-  data that might help discern what state the browser was in during an
-  error (e.g. whether the user was in a narrow).
+- `throw new Error(…)`: For fatal errors that cannot be easily recovered
+  from. We try to avoid using it, since it kills the current JS thread, rather
+  than returning execution to the caller.
+- `blueslip.error`: For logging of events that are definitely caused by a bug
+  and thus sufficiently important to be reported, but where we can handle the
+  error without creating major user-facing problems (e.g. an exception when
+  handling a presence update).
+- `blueslip.warn`: For logging of events that are a problem but not important
+  enough to log an error to Sentry in production. They are, however, highlighted
+  in the JS console in development, and appear as breadcrumb logs in Sentry in
+  production.
+- `blueslip.log` (and `blueslip.info`): Logged to the JS console in development
+  and also in the Sentry breadcrumb log in production. Useful for data that
+  might help discern what state the browser was in during an error (e.g. whether
+  the user was in a narrow).
 - `blueslip.debug`: Similar to `blueslip.log`, but are not printed to
   the JS console in development.
 

web/src/blueslip.ts

@@ -6,6 +6,7 @@
 // in order to be able to report exceptions that occur during their
 // execution.
 
+import * as Sentry from "@sentry/browser";
 import $ from "jquery";
 
 import * as blueslip_stacktrace from "./blueslip_stacktrace";
@@ -54,105 +55,6 @@ export function get_log(): string[] {
     return logger.get_log();
 }
 
-const reported_errors = new Set<string>();
-const last_report_attempt = new Map<string, number>();
-
-function report_error(
-    msg: string,
-    stack = "No stacktrace available",
-    more_info?: unknown,
-): void {
-    if (page_params.development_environment) {
-        // In development, we display blueslip errors in the web UI,
-        // to make them hard to miss.
-        void blueslip_stacktrace.display_stacktrace(msg, stack);
-    }
-
-    const key = ":" + msg + stack;
-    const last_report_time = last_report_attempt.get(key);
-    if (
-        reported_errors.has(key) ||
-        (last_report_time !== undefined &&
-            // Only try to report a given error once every 5 minutes
-            Date.now() - last_report_time <= 60 * 5 * 1000)
-    ) {
-        return;
-    }
-
-    last_report_attempt.set(key, Date.now());
-
-    // TODO: If an exception gets thrown before we set up ajax calls
-    // to include the CSRF token, our ajax call will fail.  The
-    // elegant thing to do in that case is to either wait until that
-    // setup is done or do it ourselves and then retry.
-    //
-    // Important: We don't use channel.js here so that exceptions
-    // always make it to the server even if reload_state.is_in_progress.
-    void $.ajax({
-        type: "POST",
-        url: "/json/report/error",
-        dataType: "json",
-        data: {
-            web_version: ZULIP_VERSION,
-            message: msg,
-            stacktrace: stack,
-            more_info: JSON.stringify(more_info),
-            href: window.location.href,
-            user_agent: window.navigator.userAgent,
-            log: logger.get_log().join("\n"),
-        },
-        timeout: 3 * 1000,
-        success() {
-            reported_errors.add(key);
-        },
-        error() {
-        },
-    });
-}
-
-class BlueslipError extends Error {
-    override name = "BlueslipError";
-    more_info?: unknown;
-    constructor(msg: string, more_info?: unknown) {
-        super(msg);
-        if (more_info !== undefined) {
-            this.more_info = more_info;
-        }
-    }
-}
-
-export function exception_msg(
-    ex: Error & {
-        // Unsupported properties available on some browsers
-        fileName?: string;
-        lineNumber?: number;
-    },
-): string {
-    let message = ex.message;
-    if (ex.fileName !== undefined) {
-        message += " at " + ex.fileName;
-        if (ex.lineNumber !== undefined) {
-            message += `:${ex.lineNumber}`;
-        }
-    }
-    return message;
-}
-
-$(window).on("error", (event) => {
-    const {originalEvent} = event;
-    if (!(originalEvent instanceof ErrorEvent)) {
-        return;
-    }
-
-    const ex = originalEvent.error;
-    if (!ex || ex instanceof BlueslipError) {
-        return;
-    }
-
-    const message = exception_msg(ex);
-    report_error(message, ex.stack);
-});
-
 function build_arg_list(msg: string, more_info?: unknown): [string, string?, unknown?] {
     const args: [string, string?, unknown?] = [msg];
     if (more_info !== undefined) {
@@ -184,15 +86,78 @@ export function warn(msg: string, more_info?: unknown): void {
     }
 }
 
-export function error(msg: string, more_info?: unknown, stack = new Error("dummy").stack): void {
+class BlueslipError extends Error {
+    override name = "BlueslipError";
+    more_info?: unknown;
+    constructor(msg: string, more_info?: object | undefined) {
+        super(msg);
+        if (more_info !== undefined) {
+            this.more_info = more_info;
+        }
+    }
+}
+
+export function error(
+    msg: string,
+    more_info?: object | undefined,
+    original_error?: unknown | undefined,
+): void {
+    // original_error could be of any type, because you can "raise"
+    // any type -- something we do see in practice with the error
+    // object being "dead": https://github.com/zulip/zulip/issues/18374
+    let exception: string | Error = msg;
+    if (original_error !== undefined && original_error instanceof Error) {
+        original_error.message = msg;
+        exception = original_error;
+    }
+
+    // Log the Sentry error before the console warning, so we don't
+    // end up with a doubled message in the Sentry logs.
+    Sentry.setContext("more_info", more_info === undefined ? null : more_info);
+    Sentry.getCurrentHub().captureException(exception);
+
     const args = build_arg_list(msg, more_info);
     logger.error(...args);
-    report_error(msg, stack, more_info);
 
+    // Throw an error in development; this will show a dialog (see below).
     if (page_params.development_environment) {
         throw new BlueslipError(msg, more_info);
     }
-
     // This function returns to its caller in production!  To raise a
     // fatal error even in production, use throw new Error(…) instead.
 }
+
+export function exception_msg(
+    ex: Error & {
+        // Unsupported properties available on some browsers
+        fileName?: string;
+        lineNumber?: number;
+    },
+): string {
+    let message = ex.message;
+    if (ex.fileName !== undefined) {
+        message += " at " + ex.fileName;
+        if (ex.lineNumber !== undefined) {
+            message += `:${ex.lineNumber}`;
+        }
+    }
+    return message;
+}
+
+// Install a window-wide onerror handler in development to display the stacktraces, to make them
+// hard to miss
+if (page_params.development_environment) {
+    $(window).on("error", (event: JQuery.TriggeredEvent) => {
+        const {originalEvent} = event;
+        if (!(originalEvent instanceof ErrorEvent)) {
+            return;
+        }
+
+        const ex = originalEvent.error;
+        if (!ex) {
+            return;
+        }
+
+        void blueslip_stacktrace.display_stacktrace(exception_msg(ex), ex.stack);
+    });
+}

web/src/channel.js

@@ -86,7 +86,7 @@ function call(args) {
                 blueslip.error(
                     "Unexpected 403 response from server",
                     {xhr: xhr.responseText, args},
-                    error.stack,
+                    error,
                 );
             }
         }

web/src/deprecated_feature_notice.js

@@ -24,7 +24,7 @@ export function maybe_show_deprecation_notice(key) {
     } else if (key === "*") {
         message = get_hotkey_deprecation_notice("*", isCmdOrCtrl + " + s");
     } else {
-        blueslip.error("Unexpected deprecation notice for hotkey:", key);
+        blueslip.error("Unexpected deprecation notice for hotkey:", {key});
         return;
     }
 

web/src/drafts.js

@@ -408,7 +408,7 @@ export function format_draft(draft) {
             {
                 draft_content: draft.content,
             },
-            error.stack,
+            error,
         );
         return undefined;
     }

web/src/favicon.ts

@@ -63,12 +63,6 @@ export function update_favicon(new_message_count: number, pm_count: number): voi
 
         load_and_set_favicon(rendered_favicon);
     } catch (error) {
-        // Error must be a type of Error in order to access error.stack
-        // The undetermined error was mentioned in this issue:
-        // https://github.com/zulip/zulip/issues/18374
-        if (!(error instanceof Error)) {
-            throw error;
-        }
-        blueslip.error("Failed to update favicon", undefined, error.stack);
+        blueslip.error("Failed to update favicon", undefined, error);
     }
 }

web/src/people.js

@@ -1258,7 +1258,7 @@ export function add_active_user(person) {
 
 export const is_person_active = (user_id) => {
     if (!people_by_user_id_dict.has(user_id)) {
-        blueslip.error("No user found.", user_id);
+        blueslip.error(`No user ${user_id} found.`);
     }
 
     if (cross_realm_dict.has(user_id)) {

web/src/portico/integrations.js

@@ -196,10 +196,11 @@ function hide_catalog_show_integration() {
         dataType: "html",
         success: hide_catalog,
         error(err) {
-            blueslip.error(
-                "Integration documentation for '" + state.integration + "' not found.",
-                err,
-            );
+            blueslip.error(`Integration documentation for '${state.integration}' not found.`, {
+                readyState: err.readyState,
+                status: err.status,
+                responseText: err.responseText,
+            });
         },
     });
 }

web/src/reload.js

@@ -228,7 +228,7 @@ function do_reload_app(send_after_reload, save_pointer, save_narrow, save_compos
         try {
             preserve_state(send_after_reload, save_pointer, save_narrow, save_compose);
         } catch (error) {
-            blueslip.error("Failed to preserve state", undefined, error.stack);
+            blueslip.error("Failed to preserve state", undefined, error);
         }
     }
 
@@ -257,7 +257,7 @@ function do_reload_app(send_after_reload, save_pointer, save_narrow, save_compos
     try {
         server_events.cleanup_event_queue();
     } catch (error) {
-        blueslip.error("Failed to clean up before reloading", undefined, error.stack);
+        blueslip.error("Failed to clean up before reloading", undefined, error);
     }
 
     window.location.reload(true);

web/src/server_events.js

@@ -39,11 +39,7 @@ function get_events_success(events) {
         try {
             get_events_params.last_event_id = Math.max(get_events_params.last_event_id, event.id);
         } catch (error) {
-            blueslip.error(
-                "Failed to update last_event_id",
-                {event: clean_event(event)},
-                error.stack,
-            );
+            blueslip.error("Failed to update last_event_id", {event: clean_event(event)}, error);
         }
     }
 
@@ -93,11 +89,7 @@ function get_events_success(events) {
         try {
             dispatch_event(event);
         } catch (error) {
-            blueslip.error(
-                "Failed to process an event\n" + blueslip.exception_msg(error),
-                {event: clean_event(event)},
-                error.stack,
-            );
+            blueslip.error("Failed to process an event", {event: clean_event(event)}, error);
         }
     }
 
@@ -123,11 +115,7 @@ function get_events_success(events) {
                 message_events.insert_new_messages(messages, sent_by_this_client);
             }
         } catch (error) {
-            blueslip.error(
-                "Failed to insert new messages\n" + blueslip.exception_msg(error),
-                undefined,
-                error.stack,
-            );
+            blueslip.error("Failed to insert new messages", undefined, error);
         }
     }
 
@@ -139,11 +127,7 @@ function get_events_success(events) {
         try {
             message_events.update_messages(update_message_events);
         } catch (error) {
-            blueslip.error(
-                "Failed to update messages\n" + blueslip.exception_msg(error),
-                undefined,
-                error.stack,
-            );
+            blueslip.error("Failed to update messages", undefined, error);
         }
     }
 
@@ -215,11 +199,7 @@ function get_events({dont_block = false} = {}) {
 
                 get_events_success(data.events);
             } catch (error) {
-                blueslip.error(
-                    "Failed to handle get_events success\n" + blueslip.exception_msg(error),
-                    undefined,
-                    error.stack,
-                );
+                blueslip.error("Failed to handle get_events success", undefined, error);
             }
             get_events_timeout = setTimeout(get_events, 0);
         },
@@ -259,11 +239,7 @@ function get_events({dont_block = false} = {}) {
                     hide_ui_connection_error();
                 }
             } catch (error) {
-                blueslip.error(
-                    "Failed to handle get_events error\n" + blueslip.exception_msg(error),
-                    undefined,
-                    error.stack,
-                );
+                blueslip.error("Failed to handle get_events error", undefined, error);
             }
             const retry_sec = Math.min(90, Math.exp(get_events_failures / 2));
             get_events_timeout = setTimeout(get_events, retry_sec * 1000);

web/src/settings_users.js

@@ -165,7 +165,7 @@ function failed_listing_users() {
     loading.destroy_indicator($("#subs_page_loading_indicator"));
     const status = get_status_field();
     const user_id = people.my_current_user_id();
-    blueslip.error("Error while listing users for user_id " + user_id, status);
+    blueslip.error(`Error while listing users for user_id ${user_id}`, {status});
 }
 
 function populate_users() {

web/tests/lib/zblueslip.js

@@ -112,10 +112,6 @@ function make_zblueslip() {
         };
     }
 
-    lib.exception_msg = function (ex) {
-        return ex.message;
-    };
-
     return lib;
 }
 

web/tests/people.test.js

@@ -411,7 +411,7 @@ test_people("check_active_non_active_users", () => {
     active_users = people.get_realm_users();
     assert.equal(active_users.length, 5);
     // Invalid ID
-    blueslip.expect("error", "No user found.");
+    blueslip.expect("error", "No user 1000001 found.");
     people.is_person_active(1000001);
     assert.equal(people.is_person_active(maria.user_id), true);
     assert.equal(people.is_person_active(linus.user_id), true);

web/tests/server_events.test.js

@@ -94,7 +94,7 @@ run_test("event_dispatch_error", () => {
         options.success(data);
     };
 
-    blueslip.expect("error", "Failed to process an event\nsubs update error");
+    blueslip.expect("error", "Failed to process an event");
 
     server_events.restart_get_events();
 
@@ -114,7 +114,7 @@ run_test("event_new_message_error", () => {
         options.success(data);
     };
 
-    blueslip.expect("error", "Failed to insert new messages\ninsert error");
+    blueslip.expect("error", "Failed to insert new messages");
 
     server_events.restart_get_events();
 
@@ -129,7 +129,7 @@ run_test("event_edit_message_error", () => {
     channel.get = (options) => {
         options.success(data);
     };
-    blueslip.expect("error", "Failed to update messages\nupdate error");
+    blueslip.expect("error", "Failed to update messages");
 
     server_events.restart_get_events();