From 78e76b5aac995651c870485165024b43aa157474 Mon Sep 17 00:00:00 2001 From: Dat Vu Date: Thu, 9 Apr 2026 09:18:45 +0700 Subject: [PATCH] docs: ADR-0076 - Reduce Sentry Noise with Targeted Error Reporting (#4433) --- ...try-noise-with-targeted-error-reporting.md | 99 ++++++++++++++ docs/adr/0077-sentry-request-data-privacy.md | 50 +++++++ docs/adr/0078-app-logger-handler-pipeline.md | 123 ++++++++++++++++++ 3 files changed, 272 insertions(+) create mode 100644 docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md create mode 100644 docs/adr/0077-sentry-request-data-privacy.md create mode 100644 docs/adr/0078-app-logger-handler-pipeline.md diff --git a/docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md b/docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md new file mode 100644 index 000000000..94cf44af5 --- /dev/null +++ b/docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md @@ -0,0 +1,99 @@ +# 0076 - Reduce Sentry Event Volume + +Date: 2026-04-02 + +## Status + +Proposed — needs ADR-0078 to be implemented first + +## Context + +Sentry is receiving too many low-value events, making it hard to triage real errors and draining quota. Four root causes were identified: + +1. **`logTrace` sends Sentry events.** `_shouldReportToSentry` includes `Level.trace`, which is called on every scroll, cache hit, and token validation — high-frequency diagnostics with no actionable value. + +2. **`RemoteExceptionThrower` calls `logError` before re-throwing.** Every `DioException` — including expected ones like no internet, connection timeout, and HTTP 401 — is sent to Sentry before being re-thrown as a typed exception. + +3. **`SendEmailExceptionThrower` calls `logError` for network loss.** Loss of connectivity is a normal user-facing condition, not an application bug. + +4. **`tracesSampleRate` and `profilesSampleRate` are set to 1.0 (100%).** Given JMAP's high request frequency (push, sync, email fetch), 100% sampling rapidly consumes quota. + +## Decision + +### 1. Remove `Level.trace` from Sentry reporting + +`logTrace` calls will no longer create Sentry events. Trace context is preserved as Sentry Breadcrumbs — stored locally and attached automatically to the next error event (see ADR-0078 for the implementation mechanism). + +### 2. Log expected network errors as warnings, not errors + +In `RemoteExceptionThrower`, replace the blanket `logError` at the top of `throwException` with per-branch logging: + +- `NoNetworkError`, `ConnectionTimeout`, `ConnectionError` → `logWarning` +- HTTP 401 (`BadCredentialsException`) → no log (handled by auth retry flow) +- HTTP 403, 404, 429 and other unlisted 4xx → `logWarning` (client errors, not application bugs) +- HTTP 500, 502 and other 5xx → `logWarning` +- Unknown HTTP status or unrecognised error → `logError` (genuinely unexpected; should appear in Sentry) + +In `SendEmailExceptionThrower`, replace `logError` for no realtime network with `logWarning`. + +### 3. Add `ignoredExceptions` as a safety net + +Configure the Sentry SDK to drop expected exception types at the SDK level. This acts as a second line of defence if a call site accidentally logs them as errors: + +```dart +options.addIgnoredExceptionType(NoNetworkError); +options.addIgnoredExceptionType(ConnectionTimeout); +options.addIgnoredExceptionType(ConnectionError); +options.addIgnoredExceptionType(SocketError); +``` + +### 4. Drop HTTP 4xx and 5xx events in `beforeSend` + +Extend `_beforeSendHandler` to return `null` for HTTP 4xx and 5xx events by default. + +This is a second line of defence: Decision 2 already routes these to `logWarning` (which never creates Sentry events), but `beforeSend` filtering guards against future regressions where a call site accidentally uses `logError` for an expected HTTP error. + +**Exception:** authentication-critical 4xx events must be retained. Specifically, HTTP 400 responses from the refresh-token and retry-flow paths in `AuthorizationInterceptors` are genuine failure signals that require developer visibility. These call sites must tag the event so `beforeSend` allows them through: + +```dart +logError( + 'Refresh token request failed', + exception: exception, + extras: {'auth_critical': true}, +); +``` + +The `beforeSend` handler checks for `extras['auth_critical'] == true` and skips the filter for those events. This allowlist must be documented in code alongside the `beforeSend` implementation so future maintainers do not inadvertently suppress or re-add these events. + +Note: the `AuthorizationInterceptors` call sites described in the **Unchanged** section below require a one-line update to add this tag — they are not truly unchanged, just unchanged in log *level*. + +### 5. Reduce performance sampling rates + +Lower `tracesSampleRate` and `profilesSampleRate` from `1.0` to `0.1` in `SentryConfig`. 10% sampling is sufficient to detect performance regressions without exhausting quota. + +## Consequences + +**Benefits:** +- Sentry events reflect genuine, actionable bugs — not connectivity noise or routine diagnostics. +- Quota savings from both event filtering and reduced sampling. +- Trace context (from `logTrace`) remains available in Sentry, attached to actual error events. + +**Trade-offs:** +- Routine network errors (`NoNetworkError`, `ConnectionTimeout`) are no longer visible in Sentry. Use `logWarning` + LogTracking export for analysis if needed. +- New `logError` call sites must be intentional — only use when the error requires developer action. + +**Unchanged:** +- `FlutterError.onError`, `PlatformDispatcher.instance.onError`, and `runZonedGuarded` continue to use `logError` — unhandled errors must always be monitored. +- `AuthorizationInterceptors` `logError` for refresh token failure (HTTP 400) and retry failure remains. + +## Logging Guidelines + +| Situation | Log function | Sentry result | +|-----------|--------------|---------------| +| App crash / unhandled exception | `logError` / `logCritical` | ✅ Event | +| Refresh token failure, retry failure | `logError` + `auth_critical: true` tag | ✅ Event (allowlisted in `beforeSend`) | +| Unknown HTTP status or unrecognised error | `logError` | ✅ Event | +| HTTP 4xx (excl. auth-critical), network loss, timeout | `logWarning` | ❌ Not sent | +| HTTP 5xx (known: 500, 502; unknown: other 5xx) | `logWarning` | ❌ Not sent | +| Diagnostic info (scroll, cache, counts) | `logTrace` | 🔶 Breadcrumb only | +| Normal flow information | `logInfo` | ❌ Not sent | diff --git a/docs/adr/0077-sentry-request-data-privacy.md b/docs/adr/0077-sentry-request-data-privacy.md new file mode 100644 index 000000000..e6896fb8e --- /dev/null +++ b/docs/adr/0077-sentry-request-data-privacy.md @@ -0,0 +1,50 @@ +# 0077 - Sentry Request Data Privacy + +Date: 2026-04-03 + +## Status + +Proposed + +## Context + +The `sentry_dio` integration automatically captures HTTP request metadata — headers, query string, and body — and attaches it to Sentry events. In Twake Mail, these fields can contain sensitive data: + +- **Headers:** OAuth bearer tokens, session cookies, and API keys appear in `Authorization`, `Cookie`, and custom `X-Token` headers on every JMAP request. +- **Query string:** OIDC callback URLs carry `code` (authorization code) and `state` (anti-CSRF token) as query parameters. These are short-lived but still sensitive. +- **Body:** JMAP request bodies may contain message content or personal data. + +The Sentry SDK also captures the current URL of the page (on web), which could include OAuth callback parameters. + +## Decision + +### 1. Strip sensitive headers + +Remove headers whose names match known sensitive patterns before the event is sent. Matching is case-insensitive substring: + +`authorization`, `cookie`, `set-cookie`, `x-auth`, `x-token`, `api-key`, `x-api-key`, `apikey`, `secret`, `bearer`, `session`, `password`, `token` + +Implemented in `SentryInitializer._sanitizeHeaders()`, called from `_sanitizeRequest()` inside `beforeSend`. + +### 2. Keep query string + +The `queryString` field is retained in Sentry request events. For JMAP API calls — which form the vast majority of requests — query parameters carry no sensitive credentials and provide useful context for triaging errors. OAuth callback parameters (`code`, `state`) are short-lived and appear only in the narrow OIDC login flow; the debugging benefit of retaining query context across all other requests outweighs this limited risk. + +### 3. Drop request body + +`data: null` in `_sanitizeRequest`. The body is never sent regardless of `maxRequestBodySize`. + +### 4. Attach app URL as a tag + +The current page URL is useful context for triaging errors — it shows which screen the user was on and, where relevant, which query parameters were active. The URL is attached as an `app.url` tag using **path and query string** (no fragment), consistent with the decision to retain query parameters in section 2. + +Only applies on web (`http`/`https`). On mobile/desktop, `Uri.base` returns a `file://` path with no diagnostic value and is skipped. + +## Consequences + +**Benefits:** +- OAuth tokens and session credentials cannot leak into Sentry, even if the HTTP client is misconfigured. +- `app.url` tag gives screen context for every error event on web without exposing sensitive URL parameters. + +**Trade-offs:** +- Query parameters are retained in Sentry for request/app URL context; this carries a limited, accepted risk (notably short-lived OIDC `code`/`state` in a narrow flow), mitigated by header/body sanitization and the short lifetime of those tokens. diff --git a/docs/adr/0078-app-logger-handler-pipeline.md b/docs/adr/0078-app-logger-handler-pipeline.md new file mode 100644 index 000000000..e079e8f5b --- /dev/null +++ b/docs/adr/0078-app-logger-handler-pipeline.md @@ -0,0 +1,123 @@ +# 0078 - app_logger Handler Pipeline + +Date: 2026-04-03 + +## Status + +Proposed + +## Context + +`app_logger.dart` has one function, `_internalLog`, that handles five distinct concerns: formatting messages, printing to the console, deciding whether to send to Sentry, calling `SentryManager.captureException`, and providing the public API. This leads to three problems: + +- **Not extensible:** Adding a new log destination (e.g., Sentry Breadcrumbs, file logger) requires modifying `_internalLog` directly. +- **Invisible coupling:** Whether `logTrace` sends a Sentry event is not visible at call sites — it was added silently to `_shouldReportToSentry`, which is how ADR-0076 root cause #1 occurred. +- **Untestable:** `_internalLog` depends on `SentryManager.instance` and `html.window.console` directly, with no injection point. + +ADR-0076 requires `logTrace` to add a Sentry Breadcrumb instead of a full event. +This change cannot be made cleanly without an extension point in the logger. + +The intended behaviour is: each `logTrace()` call stores a breadcrumb locally inside Sentry's buffer; when `logError()` is eventually called, Sentry automatically attaches all accumulated breadcrumbs to that error event, providing the full call trail leading up to the failure — without sending any quota-consuming events for the trace calls themselves. + +``` +logTrace("fetching mailbox list") ──► [Breadcrumb #1 stored in Sentry buffer] +logTrace("cache miss, going remote") ──► [Breadcrumb #2 stored in Sentry buffer] +logError("JMAP request failed") ──► [Sentry Event] + └─ breadcrumbs: [#1, #2] ← attached automatically +``` + +## Decision + +Refactor `app_logger.dart` to a **handler pipeline**. Each log destination becomes a `LogHandler` implementation registered at startup. The public API (`logError`, `logTrace`, etc.) is unchanged. + +### Core abstractions + +```dart +// log_handler.dart +abstract interface class LogHandler { + bool handles(Level level); + void handle(LogRecord record); +} +``` + +`AppLoggerRegistry` holds a list of handlers and dispatches each `LogRecord` to all handlers where `handles()` returns `true`. The `_shouldReportToSentry` function is deleted — each handler owns its own filter rule. + +### Handlers + +| Handler | `handles()` | Behaviour | +|---------|-------------|-----------| +| `ConsoleLogHandler` | all levels | `print()` or `html.window.console.*` (filtered by debug mode / `webConsoleEnabled`) | +| `SentryEventHandler` | `error`, `critical` | `captureException` if exception present; `captureMessage` otherwise | +| `SentryBreadcrumbHandler` | `trace` | `Sentry.addBreadcrumb()` — zero quota cost; attached to next error event | + +`captureMessage` is not assigned to a dedicated level. `SentryEventHandler` selects between `captureException` and `captureMessage` based on whether the `LogRecord` contains an exception object. This keeps `logError` as a single API regardless of whether the caller has an exception. + +### File structure + +``` +core/lib/utils/ +├── app_logger.dart ← public API only; delegates to AppLoggerRegistry +└── logging/ + ├── log_record.dart ← data class + ├── log_handler.dart ← abstract interface + ├── app_logger_registry.dart ← dispatch orchestrator + ├── handlers/ + │ ├── console_log_handler.dart + │ ├── sentry_event_handler.dart + │ └── sentry_breadcrumb_handler.dart + └── formatters/ + ├── log_formatter.dart + ├── web_console_formatter.dart + └── mobile_console_formatter.dart +``` + +### Registration + +Handlers are registered once at app startup in `app_runner.dart`. Sentry handlers check `isSentryAvailable` internally, so they can be registered before Sentry initialises. + +```dart +AppLoggerRegistry.instance + ..registerHandler(ConsoleLogHandler(formatter: ...)) + ..registerHandler(SentryBreadcrumbHandler(SentryManager.instance)) + ..registerHandler(SentryEventHandler(SentryManager.instance)); +``` + +**Idempotency and duplicate prevention:** + +`registerHandler` checks handler identity by runtimeType before adding. Registering the same handler type a second time (e.g., due to a hot restart or repeated bootstrap call) is a no-op — the existing registration is kept and no duplicate is added. This prevents double console output and duplicate Sentry captures without requiring callers to guard the registration site. + +For test isolation, `AppLoggerRegistry.resetForTesting()` clears all registered handlers. Tests that need a clean registry must call this in `setUp` / `tearDown`. Production code must never call `resetForTesting`. + +## Implementation + +| Phase | Scope | Notes | +|-------|-------|-------| +| 1 + 2 | Skeleton + ConsoleLogHandler | One PR. Zero behavior change. `_internalLog` delegates to registry; console output restored via handler. | +| 3 | SentryEventHandler + SentryBreadcrumbHandler | Behavior-changing PR. `logTrace` → breadcrumb. `_shouldReportToSentry` deleted. | +| 4 | ADR-0076 call-site fixes | Parallel to Phase 3. `RemoteExceptionThrower`, `SendEmailExceptionThrower`, `SentryInitializer`. | +| 5 | Tests | `AppLoggerRegistry.resetForTesting()`, unit tests per handler, dispatch integration test. | +| 6 (optional) | `FileLogHandler` for `LogTracking` | Wraps the existing file logger; no changes to `LogTracking` itself. | + +> ⚠️ Phase 1 must not be merged to production without Phase 2 — the registry starts empty, causing all logs to be dropped. + +## Consequences + +**Benefits:** +- Adding a new log destination requires only a new `LogHandler` and one registration line — no changes to existing code. +- Each handler is independently testable via constructor injection. +- The public API (`logError`, `logTrace`, etc.) is stable across all 188+ call sites. + +**Trade-offs:** +- Slightly more indirection for a straightforward log call. Acceptable given the extensibility gain. +- `AppLoggerRegistry` uses a static singleton (not GetX) because the logger must be available before GetX initialises. + +**Behavior change: `logError` without an exception object** + +The current `app_logger.dart` always calls `captureException(exception ?? rawMessage, ...)` — passing the raw message string as the exception value when no exception object is present. + +`SentryEventHandler` will instead call `captureMessage` when the `LogRecord` contains no exception. This means: + +- **Before:** `logError('something went wrong')` → appears in Sentry as an *exception* with `rawMessage` as the exception value. +- **After:** `logError('something went wrong')` → appears in Sentry as a *message* event. + +This is semantically correct — a string-only log is not an exception — but it is a visible change: Sentry dashboards, issue grouping, and alerts filtered by event type (exception vs. message) may need to be reviewed after rollout.