Repository navigation
fix(embeddings): silence Ollama-not-running Sentry noise (OPENHUMAN-TAURI-B7) - #1546
CodeGhost21 wants to merge 1 commit into
Conversation
…AN-TAURI-B7) Transport-layer failures from the Ollama embedding provider (connection refused, DNS, timeout) almost always mean the user simply doesn't have Ollama running on `localhost:11434` — an expected environment state for anyone who hasn't installed it. The previous code funneled those failures through `report_error`, generating 226 Sentry events across 0 impacted users in a single day. Downgrade transport failures to `tracing::warn!` so callers and local logs still see them, but Sentry doesn't. Non-2xx HTTP responses keep the existing `report_error` path because those indicate Ollama is up and an actual server-side problem worth diagnosing. Fixes OPENHUMAN-TAURI-B7
📝 WalkthroughWalkthroughThe ChangesOllama Transport Error Observability
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/embeddings/ollama.rs (1)
168-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSuppress transport errors only by checking error type before skipping Sentry.
This
map_errclosure (lines 168–189) suppresses all.send()errors withtracing::warn!()but unconditionally skipsreport_error(). The comment says only connection/DNS/timeout failures should be suppressed—others like client configuration, body read, or decode errors should still alert. Useis_connect()andis_timeout()predicates to gate the suppression.Proposed fix
.send() .await .map_err(|e| { let message = format!( "ollama embed request failed (is Ollama running at {}?): {e}", self.base_url ); - // Transport failure (connection refused, DNS, timeout) almost - // always means the user simply doesn't have Ollama running - // locally — an expected environment state, not a bug. Log a - // warning but skip Sentry to avoid alerting on every user - // without Ollama installed. Non-2xx responses below still - // funnel through `report_error` because those indicate Ollama - // is up but something is wrong. - tracing::warn!( - target: "embeddings.ollama", - domain = "embeddings", - operation = "ollama_embed", - failure = "transport", - model = %self.model, - "[embeddings] {message}" - ); + if e.is_connect() || e.is_timeout() { + // Expected local-environment state (Ollama not running/reachable). + tracing::warn!( + target: "embeddings.ollama", + domain = "embeddings", + operation = "ollama_embed", + failure = "transport", + model = %self.model, + "[embeddings] {message}" + ); + } else { + crate::core::observability::report_error( + message.as_str(), + "embeddings", + "ollama_embed", + &[ + ("model", self.model.as_str()), + ("failure", "request_error"), + ], + ); + } anyhow::anyhow!(message) })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/embeddings/ollama.rs` around lines 168 - 189, The map_err closure on the Ollama embed request currently treats all send() errors as transport failures and skips report_error; change it to inspect the reqwest::Error (use error.is_connect() || error.is_timeout()) and only suppress/report differently for transport errors: for transport errors call tracing::warn! and return anyhow::anyhow!(message) without calling report_error, but for all other errors call report_error(...) (or let it propagate) so they still alert; update the closure around the send() call (the map_err attached to the request send) to branch on error.is_connect()/is_timeout() and handle each branch accordingly while keeping the existing log message and returned anyhow error for transport cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/openhuman/embeddings/ollama.rs`:
- Around line 168-189: The map_err closure on the Ollama embed request currently
treats all send() errors as transport failures and skips report_error; change it
to inspect the reqwest::Error (use error.is_connect() || error.is_timeout()) and
only suppress/report differently for transport errors: for transport errors call
tracing::warn! and return anyhow::anyhow!(message) without calling report_error,
but for all other errors call report_error(...) (or let it propagate) so they
still alert; update the closure around the send() call (the map_err attached to
the request send) to branch on error.is_connect()/is_timeout() and handle each
branch accordingly while keeping the existing log message and returned anyhow
error for transport cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 742ab4c7-abf6-4d43-b2c7-5e099a29c351
📒 Files selected for processing (1)
src/openhuman/embeddings/ollama.rs
|
No bro this is not how you handle a bug. you should not silence. you need to fix the root cause |
|
Closing this — silencing the symptom was the wrong call. The actual root cause is that the Ollama embedding provider is being selected on machines that don't have Ollama installed (or running). Tracking down the provider-selection logic and fixing that instead. Will open a follow-up PR. |
Summary
localhost:11434connection refused / DNS / timeout) no longer report to Sentry — they were generating 226 events/day across 0 impacted users, all from a perfectly normal state: the user simply hasn't installed Ollama.report_error— those remain actionable.anyhow::Errorwith the"is Ollama running at <url>?"message; only the Sentry side effect is removed and the log severity is reduced fromerrortowarn.Problem
Sentry issue
OPENHUMAN-TAURI-B7(link) was firing on every embed call for any user who didn't have Ollama running locally — 226 occurrences in a single day, 0 users affected by an actual product defect. Every embed retry generated another event, drowning out genuine alerts. Local-provider "server isn't installed" is an expected user environment state, not a bug.Solution
In
src/openhuman/embeddings/ollama.rsthe.send()failure branch previously calledcrate::core::observability::report_error(...), which is a thin wrapper aroundtracing::error!+sentry::with_scope. Switched that branch to a directtracing::warn!with the same structured fields (domain,operation,failure=transport,model). Thesentry-tracingintegration captureserror-level events;warnis logged locally but not forwarded. The non-2xx branch is unchanged.Test
embed_connection_refusedalready covers this path — the assertion on the returned error message still passes because we only removed the Sentry side effect, not theanyhow!return.Submission Checklist
embed_connection_refusedcovers the transport-failure path; assertion on the user-visible error message is unchanged.ollama.rs; transport-failure branch covered byembed_connection_refused, non-2xx branch still covered byembed_server_error_with_body/embed_server_error_empty_body.## Related— N/A: no matrix entries touched.Closes #NNN— N/A: tracked in Sentry (Fixes OPENHUMAN-TAURI-B7), not GitHub Issues.Impact
openhuman-tauriimproves substantially; one of the top noisy issues stops firing.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/ollama-embed-transport-noisec91afff7cc11779daee226467dc3639215b54b2eValidation Run
pnpm --filter openhuman-app format:check— passed in pre-push hookpnpm typecheck— passed in pre-push hookcargo test --lib openhuman::embeddings::ollama→ 23 passed, 0 failedcargo fmt -- --checkpassed;cargo checkclean (only pre-existing unrelated warnings)Validation Blocked
Behavior Changes
anyhow::Error(is Ollama running at <url>?); only a server-side telemetry side-effect is removed and the local log severity drops fromerrortowarn.Parity Contract
embed_connection_refusedassertion still passes.Duplicate / Superseded PR Handling
Summary by CodeRabbit