-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(sentry): drop dev-server fetch noise from Tauri shell events (OPENHUMAN-TAURI-V) #1545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ba51459
aea9789
5bbdab0
83ebf72
480a081
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1142,6 +1142,25 @@ pub fn run() { | |
| environment: Some(std::borrow::Cow::Owned(resolve_sentry_environment())), | ||
| send_default_pii: false, | ||
| before_send: Some(std::sync::Arc::new(|mut event| { | ||
| // Drop "dev-server fetch failed" noise: the vendored | ||
| // `tauri-runtime-cef` dev proxy | ||
| // (vendor/tauri-cef/crates/tauri/src/protocol/tauri.rs) calls | ||
| // `log::error!("Failed to request {url}: {err}")` whenever the | ||
| // CEF webview asks for an asset on `http://localhost:1420` (the | ||
| // Vite dev URL baked into `tauri.conf.json`). That `log::error!` | ||
| // is bridged into `tracing` and picked up by the sentry-tracing | ||
| // layer as an Event — see `src/core/logging.rs::sentry_tracing_layer`. | ||
| // In packaged staging/production builds Vite isn't running, so | ||
| // the request correctly fails — but the failure is noise we | ||
| // don't want in Sentry (issue OPENHUMAN-TAURI-V, 66+ events). | ||
| // See [sentry-localhost-filter] log line below for diagnostics. | ||
| if event_is_localhost_dev_fetch_noise(&event) { | ||
| log::debug!( | ||
| "[sentry-localhost-filter] dropping dev-server fetch noise event: {:?}", | ||
| event.message.as_deref().unwrap_or("<no message>") | ||
| ); | ||
| return None; | ||
| } | ||
| // Strip server_name (hostname) to avoid leaking machine identity. | ||
| event.server_name = None; | ||
| event.user = None; | ||
|
|
@@ -2159,6 +2178,56 @@ pub fn run_core_from_args(args: &[String]) -> Result<(), String> { | |
| /// every surface (React frontend, core sidecar, Tauri shell) group under the | ||
| /// same release in Sentry and benefit from the same source-map / debug-info | ||
| /// upload. | ||
| /// Return `true` when the Sentry event is a "Failed to request | ||
| /// http://localhost:…" message originating from the vendored | ||
| /// `tauri-runtime-cef` dev-server proxy. | ||
| /// | ||
| /// The proxy logs this message via `log::error!` (see | ||
| /// `app/src-tauri/vendor/tauri-cef/crates/tauri/src/protocol/tauri.rs`) | ||
| /// every time the CEF webview asks for an asset on the Vite dev URL | ||
| /// (`http://localhost:1420` per `tauri.conf.json`). In packaged | ||
| /// staging/production builds Vite isn't running, so the request fails — | ||
| /// but the failure is benign and shouldn't be reported. | ||
| /// | ||
| /// The match is conservative: it checks the exact `Failed to request ` + | ||
| /// `http://localhost` / `http://127.0.0.1` prefix that only the dev-proxy | ||
| /// emits. Production HTTP errors from elsewhere in the shell or core use | ||
| /// different message shapes and won't be filtered. | ||
| fn event_is_localhost_dev_fetch_noise(event: &sentry::protocol::Event<'_>) -> bool { | ||
| // sentry-tracing 0.47 (with default `attach_stacktrace=false`) stores the | ||
| // log message in `event.message`. Check there first; fall back to the | ||
| // last exception's `value` for the (currently unused) stacktrace-enabled | ||
| // path so the filter stays correct if attach_stacktrace ever flips. | ||
| let direct = event.message.as_deref(); | ||
| let from_exception = event.exception.last().and_then(|e| e.value.as_deref()); | ||
| [direct, from_exception] | ||
| .into_iter() | ||
| .flatten() | ||
| .any(message_is_localhost_dev_fetch_noise) | ||
| } | ||
|
|
||
| /// Pure prefix check, separated from `event_is_localhost_dev_fetch_noise` | ||
| /// so the matching rule can be unit-tested without constructing a full | ||
| /// Sentry `Event`. | ||
| fn message_is_localhost_dev_fetch_noise(message: &str) -> bool { | ||
| // The tauri-cef dev proxy formats the message as: | ||
| // `Failed to request {url}: {err}` | ||
| // so anchoring on `Failed to request http://localhost` / `127.0.0.1` is | ||
| // sufficient and avoids matching unrelated "Failed to request …" errors | ||
| // elsewhere in the codebase that target real hosts. | ||
| // | ||
| // Note: no `[::1]` (IPv6 loopback) entry — the vendored tauri-cef dev | ||
| // proxy resolves `localhost` to IPv4 via reqwest's default resolver, so | ||
| // dev-server fetches always surface as `http://localhost:` or | ||
| // `http://127.0.0.1:`. Add an `[::1]` prefix if that ever changes | ||
| // (per graycyrus note on PR #1545). | ||
| const PREFIXES: &[&str] = &[ | ||
| "Failed to request http://localhost:", | ||
| "Failed to request http://127.0.0.1:", | ||
| ]; | ||
| PREFIXES.iter().any(|p| message.starts_with(p)) | ||
| } | ||
|
|
||
| fn build_sentry_release_tag() -> String { | ||
| let version = env!("CARGO_PKG_VERSION"); | ||
| let sha = option_env!("OPENHUMAN_BUILD_SHA").unwrap_or("").trim(); | ||
|
|
@@ -2527,4 +2596,123 @@ mod tests { | |
| } | ||
| assert_eq!(env, "production"); | ||
| } | ||
|
|
||
| // ── Sentry before_send filter: drop "Failed to request http://localhost:…" | ||
| // noise emitted by the vendored tauri-runtime-cef dev proxy in packaged | ||
| // builds (issue OPENHUMAN-TAURI-V). Tests target the pure | ||
| // `message_is_localhost_dev_fetch_noise` helper so the rule can be | ||
| // asserted without standing up a Sentry client. | ||
|
|
||
| #[test] | ||
| fn localhost_dev_fetch_noise_drops_vite_dev_url_1420() { | ||
| // The exact message shape reported by the latest event tag in Sentry | ||
| // (URL repeated by reqwest's `error sending request for url (…)`). | ||
| let msg = "Failed to request http://localhost:1420/components/skills/SkillCard.tsx: \ | ||
| error sending request for url (http://localhost:1420/components/skills/SkillCard.tsx)"; | ||
| assert!( | ||
| message_is_localhost_dev_fetch_noise(msg), | ||
| "expected Vite dev-server fetch failure to be filtered" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn localhost_dev_fetch_noise_drops_127_0_0_1_dev_url() { | ||
| // Some environments resolve `localhost` to 127.0.0.1 at the reqwest | ||
| // layer; the formatted message can carry either spelling. | ||
| let msg = "Failed to request http://127.0.0.1:1420/index.html: \ | ||
| error sending request for url (http://127.0.0.1:1420/index.html)"; | ||
| assert!( | ||
| message_is_localhost_dev_fetch_noise(msg), | ||
| "expected 127.0.0.1 dev-server fetch failure to be filtered" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn localhost_dev_fetch_noise_passes_production_url_through() { | ||
| // Real upstream failures (e.g. backend API errors surfaced via the | ||
| // same `Failed to request …` wording elsewhere) must NOT be filtered — | ||
| // they're the high-signal events Sentry exists for. | ||
| let msg = "Failed to request https://api.openhuman.ai/v1/skills: \ | ||
| error sending request for url (https://api.openhuman.ai/v1/skills)"; | ||
| assert!( | ||
| !message_is_localhost_dev_fetch_noise(msg), | ||
| "production API errors must NOT be filtered out" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn localhost_dev_fetch_noise_passes_unrelated_localhost_messages() { | ||
| // The filter is anchored on the dev-proxy's exact prefix to avoid | ||
| // accidentally dropping any error that happens to mention localhost | ||
| // (e.g. core-sidecar transport errors logged from coreRpcClient). | ||
| let msg = | ||
| "[core_rpc] transport error: error sending request for url (http://localhost:7788/rpc)"; | ||
| assert!( | ||
| !message_is_localhost_dev_fetch_noise(msg), | ||
| "non-tauri-cef localhost errors must NOT be filtered" | ||
| ); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] Consider adding 1-2 event-level tests for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
|
|
||
| #[test] | ||
| fn event_filter_uses_message_field() { | ||
| // event-level coverage: when sentry-tracing populates | ||
| // `event.message` (default with `attach_stacktrace=false`), the | ||
| // filter should see the noise payload through the primary read | ||
| // path. Per graycyrus on PR #1545. | ||
| let mut event = sentry::protocol::Event::new(); | ||
| event.message = Some("Failed to request http://localhost:1420/foo: timeout".into()); | ||
| assert!( | ||
| event_is_localhost_dev_fetch_noise(&event), | ||
| "event.message read path must catch noise messages" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn event_filter_falls_back_to_last_exception_value() { | ||
| // event-level coverage: if `attach_stacktrace` is ever turned on, | ||
| // sentry-tracing populates `event.exception` instead of (or in | ||
| // addition to) `event.message`. Filter must still see the noise | ||
| // payload through the exception fallback. Per graycyrus on PR #1545. | ||
| let mut event = sentry::protocol::Event::new(); | ||
| event.message = None; | ||
| event.exception.values.push(sentry::protocol::Exception { | ||
| ty: "log".into(), | ||
| value: Some("Failed to request http://localhost:1420/foo: timeout".into()), | ||
| ..Default::default() | ||
| }); | ||
| assert!( | ||
| event_is_localhost_dev_fetch_noise(&event), | ||
| "exception fallback must catch noise messages when event.message is absent" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn event_filter_passes_through_when_neither_field_matches() { | ||
| // Negative event-level case: no noise prefix in either field → | ||
| // event must NOT be filtered. | ||
| let mut event = sentry::protocol::Event::new(); | ||
| event.message = Some("genuine production error".into()); | ||
| event.exception.values.push(sentry::protocol::Exception { | ||
| ty: "log".into(), | ||
| value: Some("connection refused (10061)".into()), | ||
| ..Default::default() | ||
| }); | ||
| assert!( | ||
| !event_is_localhost_dev_fetch_noise(&event), | ||
| "legitimate production events must pass through" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn localhost_dev_fetch_noise_anchors_to_message_start() { | ||
| // CodeRabbit (PR #1545) caught that the predicate used | ||
| // `contains` rather than `starts_with`. Regression: a message | ||
| // that merely embeds the dev-proxy prefix later in its text | ||
| // must NOT be filtered — only messages that *begin* with it. | ||
| let msg = "User report: `Failed to request http://localhost:1420/foo` was logged earlier"; | ||
| assert!( | ||
| !message_is_localhost_dev_fetch_noise(msg), | ||
| "messages that merely contain the dev-proxy prefix must NOT be filtered" | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] No
[::1](IPv6 loopback) coverage. Likely fine since the vendored proxy resolves localhost to IPv4 via reqwest, but worth a one-line comment noting the omission so future readers dont wonder if it was forgotten.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in
83ebf723— added inline note explaining the absence of[::1](tauri-cef dev proxy resolves localhost to IPv4 via reqwest default resolver, so dev-server fetches always surface as IPv4).