docs: ADR-0076 - Reduce Sentry Noise with Targeted Error Reporting#4433
docs: ADR-0076 - Reduce Sentry Noise with Targeted Error Reporting#4433
Conversation
WalkthroughThe pull request adds three new ADRs: 0076, 0077, and 0078. ADR-0076 defines a multi-step Sentry noise-reduction strategy (drop 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md
Outdated
Show resolved
Hide resolved
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md
Outdated
Show resolved
Hide resolved
…r Reporting Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/adr/0078-app-logger-handler-pipeline.md (1)
76-83: Document idempotent handler registration semantics.Please specify whether repeated bootstrap calls are allowed and how duplicates are prevented (
registerHandlerdedupe,initializeOnce, orresetForTesting). Without this, duplicate console output and duplicate Sentry captures are easy to introduce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0078-app-logger-handler-pipeline.md` around lines 76 - 83, The ADR currently omits whether repeated bootstrap calls are safe and how duplicate handlers are prevented; update the document to state the registry's idempotency semantics: describe whether AppLoggerRegistry.instance.registerHandler performs deduping (by handler type/equality) or whether app startup uses an initializeOnce pattern, and document any test helpers like resetForTesting that clear handlers between runs; explicitly state the behavior for ConsoleLogHandler, SentryBreadcrumbHandler, and SentryEventHandler (e.g., "registerHandler dedupes by runtime type" or "bootstrap must be called once via initializeOnce; use resetForTesting in tests") so callers know how duplicate console output and duplicate Sentry captures are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md`:
- Around line 49-52: The ADR currently contradicts itself: the
`_beforeSendHandler` rule says drop all HTTP 4xx events but later sections still
record refresh-token HTTP 400 failures; fix by explicitly defining a carve-out
or changing the guideline text so behavior is consistent—update the “Drop HTTP
4xx events in `beforeSend`” paragraph to state that all 4xx events are dropped
except explicitly listed exceptions (e.g., refresh-token HTTP 400), or
alternatively remove the refresh-token example from the Sentry list and update
the “Unchanged/Guidelines” section to reflect the chosen policy; ensure
`_beforeSendHandler` and the refresh-token handling text reference the same rule
set so implementers have no conflicting instructions.
In `@docs/adr/0077-sentry-request-data-privacy.md`:
- Around line 49-50: The "Trade-offs" bullet claiming "Query parameters are
unavailable in Sentry" contradicts the ADR's earlier decision to retain query
values in request data and app.url; update the "Trade-offs" section (the
**Trade-offs:** bullet) to state that query parameters are retained with bounded
risk and that sensitive parameters must be redacted or limited via the
request-data retention policy and explicit logging via extras (refer to the
decision points about retaining query values in request data and app.url).
Ensure the wording consistently reflects that query-string retention is allowed
but constrained, rather than unavailable.
---
Nitpick comments:
In `@docs/adr/0078-app-logger-handler-pipeline.md`:
- Around line 76-83: The ADR currently omits whether repeated bootstrap calls
are safe and how duplicate handlers are prevented; update the document to state
the registry's idempotency semantics: describe whether
AppLoggerRegistry.instance.registerHandler performs deduping (by handler
type/equality) or whether app startup uses an initializeOnce pattern, and
document any test helpers like resetForTesting that clear handlers between runs;
explicitly state the behavior for ConsoleLogHandler, SentryBreadcrumbHandler,
and SentryEventHandler (e.g., "registerHandler dedupes by runtime type" or
"bootstrap must be called once via initializeOnce; use resetForTesting in
tests") so callers know how duplicate console output and duplicate Sentry
captures are avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 869a9fe6-049e-43ee-90a3-f503cd411fb7
📒 Files selected for processing (3)
docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.mddocs/adr/0077-sentry-request-data-privacy.mddocs/adr/0078-app-logger-handler-pipeline.md
docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md
Outdated
Show resolved
Hide resolved
…ed Error Reporting Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md`:
- Around line 49-56: The ADR omits how the allowlist tag is set and where to
update call sites; update the doc and code to specify using the existing
logError extras parameter (e.g., extras['auth_critical'] = true) as the
canonical tag mechanism, ensure _beforeSendHandler checks event.extra or
event.tags for auth_critical === true to allow the event through, and update the
AuthorizationInterceptors' logError calls for refresh-token and retry-flow
failure paths to include extras: {'auth_critical': true}; also add an inline
example and a brief comment next to the _beforeSendHandler implementation
documenting this allowlist convention so future maintainers know to set the tag
via logError extras.
- Around line 49-56: Update the ADR and the before-send behavior to remove
ambiguity about 5xx handling: explicitly state in the Decision section whether
HTTP 5xx events should be dropped by _beforeSendHandler (i.e., "drop all 5xx and
4xx by default except allowlisted auth_critical") or that only 4xx are dropped
and unexpected 5xx logged as errors are forwarded; then implement the chosen
behavior in the documentation and inline comment next to the
beforeSend/_beforeSendHandler, referencing the AuthorizationInterceptors call
sites and the logError usage and the allowlist tag (e.g., auth_critical: true)
so maintainers know to exempt those specific auth-related 400s.
- Around line 29-35: Update the ADR to explicitly document how unlisted HTTP
statuses are treated: in the RemoteExceptionThrower -> throwException mapping,
add entries for "Other 4xx codes (e.g., 403, 404, 429) → logWarning" and "Other
5xx codes (e.g., 599) → logWarning" and clarify that BadCredentialsException
(HTTP 401) remains no-log; then update the beforeSend section to state that the
filter currently drops all 4xx events by default and indicate whether 5xx should
also be filtered (make it consistent with the Decision section by either keeping
only 4xx filtered or changing it to filter 5xx as well), referencing
RemoteExceptionThrower.throwException and the beforeSend handler so readers can
reconcile logging vs event-filtering behavior.
In `@docs/adr/0078-app-logger-handler-pipeline.md`:
- Around line 47-54: Update the ADR to explicitly document the behavioral change
in SentryEventHandler: note that unlike the current implementation in which
SentryEventHandler calls captureException(exception ?? rawMessage) (treating
string-only errors as exceptions), the proposed behavior will call
captureMessage when no exception object is present; add a "Behavior change"
bullet to the Consequences section stating that error logs without an exception
(e.g., logError('message')) will now appear as Sentry messages rather than
exceptions and mention the potential impact on dashboards/alerts that filter by
event type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c7d4c1e-953a-4d81-8b5b-671e4653a76c
📒 Files selected for processing (3)
docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.mddocs/adr/0077-sentry-request-data-privacy.mddocs/adr/0078-app-logger-handler-pipeline.md
✅ Files skipped from review due to trivial changes (1)
- docs/adr/0077-sentry-request-data-privacy.md
docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md
Outdated
Show resolved
Hide resolved
… Targeted Error Reporting Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/adr/0078-app-logger-handler-pipeline.md (2)
22-27: Add language specifier to fenced code block.The code block illustrating the trace-to-error breadcrumb flow should specify a language for proper rendering and syntax highlighting.
📝 Suggested fix
-``` +```text 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</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/adr/0078-app-logger-handler-pipeline.mdaround lines 22 - 27, The
fenced code block showing the trace-to-error breadcrumb flow (the lines
containing logTrace("fetching mailbox list"), logTrace("cache miss, going
remote") and logError("JMAP request failed")) should include a language
specifier (e.g., changetotext) so the block renders with proper
formatting/syntax highlighting; update the Markdown fenced block accordingly.</details> --- `57-72`: **Add language specifier to directory structure code block.** The file structure tree should specify a language (e.g., `text` or `tree`) for proper rendering. <details> <summary>📝 Suggested fix</summary> ```diff -``` +```text 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 ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/adr/0078-app-logger-handler-pipeline.mdaround lines 57 - 72, The
fenced directory tree in the ADR file is missing a language tag; edit the code
block in docs/adr/0078-app-logger-handler-pipeline.md that contains the
core/lib/utils/ tree and change the opening triple-backtick to include a
language (e.g., replace "" with "text" or "```tree") so the directory
structure renders correctly; keep the block content unchanged.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@docs/adr/0078-app-logger-handler-pipeline.md:
- Around line 22-27: The fenced code block showing the trace-to-error breadcrumb
flow (the lines containing logTrace("fetching mailbox list"), logTrace("cache
miss, going remote") and logError("JMAP request failed")) should include a
language specifier (e.g., changetotext) so the block renders with
proper formatting/syntax highlighting; update the Markdown fenced block
accordingly.- Around line 57-72: The fenced directory tree in the ADR file is missing a
language tag; edit the code block in
docs/adr/0078-app-logger-handler-pipeline.md that contains the core/lib/utils/
tree and change the opening triple-backtick to include a language (e.g., replace
"" with "text" or "```tree") so the directory structure renders correctly;
keep the block content unchanged.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `9d4784a0-28db-4bbe-bd70-a0e18cd7c63f` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7795c3d74ebdf8813780e9b03d9f71682a69626f and 0be61cdc2480a984a64abe2f2a0615913d91ff7d. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/adr/0076-reduce-sentry-noise-with-targeted-error-reporting.md` * `docs/adr/0078-app-logger-handler-pipeline.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
3 participants
Summary by CodeRabbit