logging-and-error-reporting

# logging-and-error-reporting Warp has two related ways to surface what happened at runtime: - **`log::*`** (`error!`/`warn!`/`info!`/`debug!`/`trace!`) — local diagnostics written to the terminal/log file and, on crash-reporting builds, uploaded to Sentry as **breadcrumbs** (context attached to the next captured event). - **`report_error!`** — captures a **structured Sentry event** (an actual issue) for errors worth engineering attention. ## How logs reach Sentry (important) On crash-reporting builds a `SentryLogger` wraps the logger (`warp_logging`). The filter: - `Error` / `Warn` / `Info` → **breadcrumb** only (not their own Sentry issue). - `Debug` / `Trace` → **dropped** from Sentry entirely (local-only). - The `Error`-level line emitted by `report_error!` itself → **ignored** by Sentry (the macro already captured a structured event; the log line would double-report). - A few noisy targets (wgpu, `panic`, redraw-frame, the crash-reporting module) are dropped. Consequences: - **`log::error!` does NOT create a Sentry issue** — it's only a breadcrumb. If a failure should be tracked in Sentry, use `report_error!`. Only `report_error!` and panics create Sentry events. - Breadcrumbs (Info and above) are uploaded, so they must never contain secrets or PII — see "Sensitive data: safe_* macros" below. ## Choosing: `report_error!` vs. `log::error!` vs. `log::warn!` Pick based on **whose fault the failure is** and **what it means for the user**, not on how bad it feels. Remember only `report_error!` reaches Sentry (see above) — the two `log` levels are local/breadcrumb-only, so choosing between them is about log severity, not about paging anyone. - **`report_error!` — actionable; an engineer should fix something.** Use it when the failure means our code is wrong: an invariant was violated, an assumption that should always hold didn't, or execution reached a state that shouldn't be possible. Also use it when the cause isn't our bug but the user-facing impact is severe enough that we need to build a workaround — this covers failures in a **critical subsystem** (a core startup/lifecycle path, or a fallback that itself guards a critical path), where even an externally-caused failure is worth an engineer's eyes. **One further exception:** when we're programming against an **external system whose behavior is uncertain** — its quirks aren't fully understood, or a failure might actually mean *we're* using it wrong — prefer `report_error!` over a plain `log`, since the "external" failure may be our bug in disguise. This is the only form that becomes a Sentry issue, so reserve it for failures worth an engineer's attention. - **`log::error!` — a genuine failure that is not our code's fault.** The operation we intended couldn't be completed, but the cause is external (the environment or an external system), not a bug we can fix. It's a real failure, so it's error severity — but there's nothing to act on, so it does not page us. (Exception: if it's a critical subsystem, or the external system's behavior is uncertain enough that the failure might be our fault, escalate to `report_error!` per the bullet above.) - **`log::warn!` — non-ideal but largely expected.** The app can still proceed with the functionality generally intact, perhaps with a degraded experience. Recoverable/handled conditions, fallbacks, retries, and skipped work belong here. For everything else — lifecycle/state info, expected or handled conditions, and diagnostic "loud logs" (e.g. listing valid options after a lookup miss) — use plain `log::*` at the level that fits (see "Log levels"). If a function's name/doc says it *logs* (or it emits a header line plus one entry per item), it should use `log::*`, not `report_error!`. Don't signal the same failure twice. Report it once, at the sink where it stops propagating; `log::*` breadcrumbs on the way there are useful context (see "Report once, at the sink"). ## Log levels The default filter is `Info`, so `debug!`/`trace!` are off unless `RUST_LOG` enables them. - **`error!`** — a genuine failure whose cause is **external, not a bug in our code**: the environment or an external system prevented the operation we intended. Nothing for us to fix, so it's not `report_error!`; still a real failure, so it's error severity. Only a breadcrumb — if it turns out to be actionable, use `report_error!` instead. - **`warn!`** — non-ideal but largely expected: the app proceeds with the functionality generally intact (possibly degraded). Recoverable/handled paths, retries, fallbacks, skipped work. - **`info!`** — coarse lifecycle/state milestones (startup, connection up/down, feature enabled). On by default and uploaded as breadcrumbs, so keep it low-volume and free of per-item spam. - **`debug!`** — verbose diagnostics for local development; off by default; never sent to Sentry. - **`trace!`** — very fine-grained / hot-path detail; off by default; never sent to Sentry. Guidance: - Hot loops, per-frame render paths, and per-message handlers must log at `debug!`/`trace!` (never `info!`+), or they flood the log file and breadcrumb buffer. - Prefer static, greppable message prefixes with structured `key=value` detail (e.g. `"[Remote codebase indexing] … repo_path={} state={:?}"`), matching the surrounding module. - Use inline format args (`log::warn!("… {err:#}")`) per the workspace clippy config; format an error chain with `{err:#}`. ## Sensitive data: `safe_*` macros Never log secrets, tokens, or credentials at any level. For messages whose *useful* detail is sensitive-ish — file paths, response payloads, user-generated content — that you want locally but must NOT ship in release-channel logs (which get bundled and become breadcrumbs), use the `safe_*` macros instead of `log::*`. `safe_error!`, `safe_warn!`, `safe_info!`, `safe_debug!` (plus `safe_anyhow!` to build an `anyhow::Error`, and `safe_eprintln!`) each take a `safe:` and a `full:` arm. Dogfood builds log `full:`; release channels log `safe:`: ```rust use warp_core::safe_error; safe_error!( safe: ("Remote server unexpected response for Initialize"), full: ("Remote server unexpected response for Initialize: response={other:?}") ); ``` - The `safe:` arm must stand on its own and contain no sensitive/verbose detail; put those bits only in `full:`. --- # Reporting errors with `report_error!` `report_error!` is the explicit way to send an error to Sentry as a structured event. Reserve it for **actionable** failures — an invariant or always-true assumption was violated, or the code reached a state it shouldn't, i.e. a bug we should FIX — or for cases where the cause isn't our bug but the user-facing impact is severe enough to warrant a workaround. A failure that's merely an external error (nothing to fix) or an expected/degraded condition belongs at `log::error!` / `log::warn!` instead (see "Choosing" above). At runtime it checks `err.is_actionable()`: - **actionable** → captured to Sentry AND logged at `Error` level. - **not actionable** (e.g. registered network errors: reqwest connect/5xx/429, tokio `JoinError` cancelled) → logged at `Warn` level only, never sent to Sentry. This classification only works if the *typed error* reaches the macro. Sentry buckets events into issues by a **fingerprint** (its grouping key); because these events are stacktrace-less, Sentry derives that fingerprint from the **message text**. Interpolating instance-specific data (ids, paths, counts, a stringified error) into the message changes the fingerprint on every occurrence, so one logical error fragments into countless separate issues (and burns through quota). Keep the message a **static string** so the fingerprint stays stable; per-instance data goes in the error chain (via `.context()`) or a structured `extra:` block — neither of which affects the fingerprint — never interpolated into the message. Bad (fragments Sentry grouping into one group per distinct value, and — when it stringifies a typed error — hides it from `is_actionable`): ```rust report_error!("Failed to read persisted data: {err}"); ``` Good (canonical in-tree example, `app/src/persistence/sqlite.rs`): ```rust report_error!(anyhow::Error::new(err).context("Failed to read persisted data")); ``` ## Prefer typed error enums over `anyhow` when it makes sense Don't default to `anyhow`. The conventional Rust guidance is "`anyhow` for applications, `thiserror` for libraries," but the sharper question is: **does anything downstream need to tell the failures apart?** If yes, define a typed error enum (`thiserror`), `impl ErrorExt`, and `register_error!` it. In Warp that "downstream" includes the Sentry reporting layer, not just calling code — so typed errors pay off more often than the plain app-vs-library rule suggests. Reach for a typed error enum when these hold: - **A caller branches on the failure** — it `match`es to recover, retry, fall back, render a specific message/state, or map to a status. This is the classic reason and the strongest signal: an `anyhow::Error` is opaque, so callers can essentially only print it. - **Mixed actionability** — some variants are real bugs worth a Sentry issue and others are expected/environmental (network, auth-expired, user-cancelled, not-found). Per-variant `is_actionable()` reports the bugs and stays silent on the noise; `anyhow` is all-or-nothing. - **A fixed, known set of failure modes** worth naming — it makes a function's failure surface visible in its signature and gives Sentry stable, meaningful groups (one per variant) instead of one catch-all bucket. - **The same failure recurs across many call sites** — define the message and classification once on the type, then report once at the sink. Default to `anyhow` when these hold: - The error is only **propagated** (`?` / `.context("…")`) up to a sink that logs/reports/displays it — nobody matches on its kind. - The failure modes are open-ended or not worth enumerating. - A static `.context("…")` string carries enough for a human reading the Sentry event or log. - It's leaf/glue code, not an API boundary other code depends on. It's not either/or: a typed enum can keep an `anyhow` escape hatch for the genuinely-unexpected case (an `Unexpected(#[from] anyhow::Error)` variant), classifying the known failures precisely while still absorbing the rest. Group variants by **failure mode** (what went wrong / what the caller does about it), not by which crate produced the error. Real example (`UserAuthenticationError`, `crates/warp_server_client/src/auth/mod.rs`): ```rust #[derive(thiserror::Error, Debug)] pub enum UserAuthenticationError { #[error("Firebase returned a token error when fetching an ID token")] DeniedAccessToken(FirebaseError), #[error("unexpected error occurred when fetching an ID token: {0:#}")] Unexpected(#[from] anyhow::Error), // … } impl ErrorExt for UserAuthenticationError { fn is_actionable(&self) -> bool { match self { UserAuthenticationError::DeniedAccessToken(_) => false, UserAuthenticationError::Unexpected(e) => e.is_actionable(), // … } } } register_error!(UserAuthenticationError); ``` ## Choosing the form (variable data out of the grouped message) **Never stringify an already-typed error.** `report_error!(anyhow::anyhow!("{e}"))` flattens `e` to a `String`, which erases the typed source chain and defeats `is_actionable()` (so registered non-actionable network errors get over-reported). Reserve `anyhow!("…")` for values that are genuinely *not* errors (see rule 4). Rules, in priority order: 1. **The error IS the payload — report it as an error, never demote it into `extra:`.** `extra:` is only for genuinely incidental data (ids, paths, counts, durations). 2. **Prefer Result-level `.context()` whenever a `Result` is in hand.** This is the **preferred, most succinct style** — reach for it before an `anyhow::Error::new(..)` / `anyhow!(..)` wrapper whenever practical. Works for any `Result<_, E: std::error::Error>` and for `anyhow::Result`; add `use anyhow::Context` for the trait method. Don't add an `anyhow::Error::new(..)` / `anyhow!(..)` wrapper when `.context()` on the `Result` will do. Avoid the UFCS form `anyhow::Context::context(result, "msg")` — it reads poorly; import the trait, or if you'd rather not import it, restructure to own the error and use the inherent method: `if let Err(e) = some_call() { report_error!(e.context("msg")); }`. ```rust let data = some_call().context("Failed to load data")?; ``` 3. **Wrap a bare error value only when there is no `Result` to hang `.context()` on** (closure/callback params, `match` arms that special-case other variants): - `e` is a `std::error::Error` but not `anyhow`: `report_error!(anyhow::Error::new(e).context("msg"))`. - `e` is already an `anyhow::Error`: `report_error!(e.context("msg"))`. - `e` is a *registered* error (`register_error!`): pass it directly — `report_error!(e)` — which keeps it fully typed. - Error type differs by feature/config (sometimes `anyhow`, sometimes `StdError`): use the Result-level `.context()` above, or `report_error!(anyhow::Error::from(e).context("msg"))` which compiles for both (do NOT use `Error::from` where `e` is unconditionally `anyhow` — clippy flags it as a useless conversion; use `e.context()` there). 4. **Non-`Error` payload** — a `String`/`&str` message, a const, `format_args!`, or an opaque value that isn't `std::error::Error` — has no typed chain to preserve, so `anyhow::anyhow!("{value}")` (or `"static", extra: { .. }`) is correct here. 5. **The error must be reused or returned, so it can't be consumed** — but first treat this as a **smell**. The **sink** (where the error stops propagating) should normally be the one reporting, and it should *own* the error so it can move it into `report_error!` fully typed and with `.context()`. Needing to report a *borrowed* error usually means you're reporting away from the true sink, or reporting an error you also return (see "Report once, at the sink") — prefer restructuring so the owner reports. When you genuinely can't take ownership, report it **borrowed**, which still keeps it typed: - `e` is an `anyhow::Error` or a *registered* error → `report_error!(&e)` (optionally `report_error!(&e, extra: { .. })`). Note this drops any static `.context("…")` message, so `&e` groups by the error's own message. - If the later use is itself a **borrow** (building a `format!`/detail string, calling a `&e` method), reorder so that borrow runs first and then **move** `e` into the report last — `report_error!(anyhow::Error::new(e).context("msg"))` (StdError) or `report_error!(e.context("msg"))` (anyhow). This keeps the typed chain AND the static context. - `inspect_err(|e| report_error!(..))` only hands you `&e` (a borrow), which forces the stringified `anyhow!("{e}")` form. When you're reporting-and-swallowing (`.ok()` / `.ok()?`) or otherwise discarding the error, switch to `map_err(|e| report_error!(anyhow::Error::new(e).context("msg")))` so `e` is **owned** and stays typed — the closure returns `()`, which composes with a trailing `.ok()`/`?`. (Clippy's `manual_inspect` only fires when a `map_err` closure returns `e` unchanged; returning `()` is fine.)