WIP: Add Sentry

ryansquared commented

2026-10-05 06:26:38 +00:00

(Migrated from codeberg.org)

No description provided.

vkobel commented

2026-10-06 09:26:18 +00:00

(Migrated from codeberg.org)

Sentry review: 6 issues to fix before merge. I've left out the smaller ones; I'm happy to share them separately.

src/sentry-middleware/src/lib.rs:247: an empty or invalid SENTRY_DSN panics at startup. ClientOptions::dsn calls expect (sentry-core clientoptions.rs:311), so a blank SENTRY_DSN= in an env file takes down api, gateway and metering. The doc comment says it returns None in that case. Fix: use dsn.parse::<Dsn>().ok() and treat empty or invalid as disabled.
lib.rs:393: breadcrumbs leak across requests and users. Arc::new(Hub::current()) returns the worker thread's long-lived hub, not a new one. add_breadcrumb adds to it permanently, so a later 5xx on the same thread includes earlier requests' breadcrumbs, such as other users' emails and IDs. Fix: Hub::new_from_top(Hub::current()).
src/api/src/main.rs:790: removing err from all 343 #[tracing::instrument] attributes drops root causes. The only replacement is one error = %error line in SentryLayer. It logs only the top-level error message, without the underlying cause, and only for 5xx. It also logs under the sentry_middleware target, which the default filters hide. SENTRY_DSN isn't set anywhere, so errors such as sqlx failures are now logged nowhere. Fix: restore err, or log the full error chain in the middleware.
api/src/main.rs:3115: deploy failures are invisible. deploy_logic runs in a tokio::spawn behind a 200 streaming response, and its Err branch only streams client_response() to the client. There's no Sentry event and no ERROR log, and the spawned task doesn't inherit REQUEST_ID. Fix: add an explicit log and capture_error in that branch.
lib.rs:132: PII is sent to Sentry through breadcrumbs. Every INFO/WARN message and its fields go out without scrubbing, which bypasses send_default_pii=false. That includes emails (onboarding.rs:305, webhooks.rs:1173), client IPs (rate_limit.rs:115), and backend URLs inside reqwest errors (proxy.rs:299). Fix: a field allowlist or before_breadcrumb scrubbing.
lib.rs:366: the global BREADCRUMBS map leaks memory. Cleanup runs only after inner.call finishes. A client disconnect, cancellation or panic leaves up to 100 breadcrumbs per request in the map forever. Fix: a drop guard, or keep the buffer in the task-local instead.

Suggestion: Sentry already ships most of this middleware: sentry::integrations::tower::{NewSentryLayer, SentryHttpLayer} plus sentry::integrations::tracing::layer(). They give each request its own hub, add request context and capture ERROR events. That fixes 2 and 6, and also removes the global lock and the ERROR events that currently go missing. It may be simpler than patching the custom middleware.

Severity	Issue	Impact	Fix effort
High	1. invalid DSN panics	All services down at startup	One line
High	2. shared hub	Breadcrumbs leak across users	One line
High	3. `err` removed	Root causes no longer logged	Mechanical revert
High	4. deploy errors invisible	No log, no Sentry event	Small
High	5. PII in breadcrumbs	Compliance	Medium
Medium	6 breadcrumb map leak	Unbounded memory growth	Small (free with Sentry's integrations)

**Sentry review: 6 issues to fix before merge.** I've left out the smaller ones; I'm happy to share them separately. 1. **`src/sentry-middleware/src/lib.rs:247`: an empty or invalid `SENTRY_DSN` panics at startup.** `ClientOptions::dsn` calls `expect` (sentry-core `clientoptions.rs:311`), so a blank `SENTRY_DSN=` in an env file takes down api, gateway and metering. The doc comment says it returns `None` in that case. Fix: use `dsn.parse::<Dsn>().ok()` and treat empty or invalid as disabled. 2. **`lib.rs:393`: breadcrumbs leak across requests and users.** `Arc::new(Hub::current())` returns the worker thread's long-lived hub, not a new one. `add_breadcrumb` adds to it permanently, so a later 5xx on the same thread includes earlier requests' breadcrumbs, such as other users' emails and IDs. Fix: `Hub::new_from_top(Hub::current())`. 3. **`src/api/src/main.rs:790`: removing `err` from all 343 `#[tracing::instrument]` attributes drops root causes.** The only replacement is one `error = %error` line in `SentryLayer`. It logs only the top-level error message, without the underlying cause, and only for 5xx. It also logs under the `sentry_middleware` target, which the default filters hide. `SENTRY_DSN` isn't set anywhere, so errors such as sqlx failures are now logged nowhere. Fix: restore `err`, or log the full error chain in the middleware. 4. **`api/src/main.rs:3115`: deploy failures are invisible.** `deploy_logic` runs in a `tokio::spawn` behind a 200 streaming response, and its `Err` branch only streams `client_response()` to the client. There's no Sentry event and no ERROR log, and the spawned task doesn't inherit `REQUEST_ID`. Fix: add an explicit log and `capture_error` in that branch. 5. **`lib.rs:132`: PII is sent to Sentry through breadcrumbs.** Every INFO/WARN message and its fields go out without scrubbing, which bypasses `send_default_pii=false`. That includes emails (`onboarding.rs:305`, `webhooks.rs:1173`), client IPs (`rate_limit.rs:115`), and backend URLs inside reqwest errors (`proxy.rs:299`). Fix: a field allowlist or `before_breadcrumb` scrubbing. 6. **`lib.rs:366`: the global `BREADCRUMBS` map leaks memory.** Cleanup runs only after `inner.call` finishes. A client disconnect, cancellation or panic leaves up to 100 breadcrumbs per request in the map forever. Fix: a drop guard, or keep the buffer in the task-local instead. **Suggestion:** Sentry already ships most of this middleware: `sentry::integrations::tower::{NewSentryLayer, SentryHttpLayer}` plus `sentry::integrations::tracing::layer()`. They give each request its own hub, add request context and capture ERROR events. That fixes 2 and 6, and also removes the global lock and the ERROR events that currently go missing. It may be simpler than patching the custom middleware. | Severity | Issue | Impact | Fix effort | | -------- | -------------------------- | ----------------------------- | --------------------------------------- | | High | 1. invalid DSN panics | All services down at startup | One line | | High | 2. shared hub | Breadcrumbs leak across users | One line | | High | 3. `err` removed | Root causes no longer logged | Mechanical revert | | High | 4. deploy errors invisible | No log, no Sentry event | Small | | High | 5. PII in breadcrumbs | Compliance | Medium | | Medium | 6 breadcrumb map leak | Unbounded memory growth | Small (free with Sentry's integrations) |

ryansquared commented

2026-10-08 05:47:46 +00:00

(Migrated from codeberg.org)

Posting here in addition to elsewhere: Sentry's integrations are not enough, as they aren't Tokio task aware. In Sentry, breadcrumbs are tracked thread_local, but tasks are Send + Sync, which means that, at any point after a .await, you might accidentally gain, lose, or whatever else a bunch of breadcrumbs. That's the reason I went with a custom middleware. I don't think we care about leaking BREADCRUMBS at this point in time, and if we hit memory issues, we can add a cleaner in the future.

Posting here in addition to elsewhere: Sentry's integrations are not enough, as they aren't Tokio task aware. In Sentry, breadcrumbs are tracked thread_local, but tasks are Send + Sync, which means that, at any point after a `.await`, you might accidentally gain, lose, or whatever else a bunch of breadcrumbs. That's the reason I went with a custom middleware. I don't think we care about leaking BREADCRUMBS *at this point in time*, and if we hit memory issues, we can add a cleaner in the future.

ryansquared commented

2026-10-08 06:13:55 +00:00

(Migrated from codeberg.org)

We don't have automata for Sentry events yet so I'm not sure how concerned we should be about PII. If we don't have that, we could lose critical information about what's happened. Requesting feedback on what's acceptable there.

ryansquared commented

2026-10-08 06:14:16 +00:00

(Migrated from codeberg.org)

The rebase includes a fix for deploy failures.

ryansquared commented

2026-10-08 06:15:16 +00:00

(Migrated from codeberg.org)

We can add #[tracing::instrument(err(Debug))] on handlers to print logs of all errors in a function. Debug repr prints out sources, so we get a full stack. However, sending the error through Sentry should capture the entire chain.

We can add `#[tracing::instrument(err(Debug))]` on handlers to print logs of all errors in a function. Debug repr prints out sources, so we get a full stack. However, sending the error through Sentry should capture the entire chain.

ryansquared commented

2026-10-08 06:15:36 +00:00

(Migrated from codeberg.org)

Arc::new(Hub::current()) returns the worker thread's long-lived hub, not a new one.

Fixed.

> Arc::new(Hub::current()) returns the worker thread's long-lived hub, not a new one. Fixed.

ryansquared commented

2026-10-08 06:16:11 +00:00

(Migrated from codeberg.org)

src/sentry-middleware/src/lib.rs:247: an empty or invalid SENTRY_DSN panics at startup.

Expected. What if it's not set properly? We're not gonna notice it in Sentry.

> src/sentry-middleware/src/lib.rs:247: an empty or invalid SENTRY_DSN panics at startup. Expected. What if it's not set properly? We're not gonna notice it in Sentry.

vkobel commented

2026-10-08 08:02:46 +00:00

(Migrated from codeberg.org)

Thanks, the rest LGTM. A few notes:

Tokio / thread-local hubs (not a blocker): I don't think that's right. sentry-tower attaches the per-request hub to the request future (Hub::run + fut.bind_hub(hub), sentry-tower 0.49.3 lib.rs:277-280). SentryFuture::poll swaps that hub into the thread-local on every poll (sentry-core futures.rs:34-41). sentry-tracing breadcrumbs go to the current hub (layer/mod.rs:277), so they follow the task across .awaits and threads. Fine to keep the custom middleware for now. Two real caveats:
- tokio::spawn needs an explicit .bind_hub(), same as your REQUEST_ID re-scope in deploy.
- new_from_top() copies the worker thread's hub, so breadcrumbs from unbound background work can leak in. A closure provider returning a hub with an empty Scope::default() avoids that.
PII: let's scrub emails/IPs and keep only UUIDs.
- The messages are interpolated (onboarding.rs:305, rate_limit.rs:115), so it needs a regex over the message and field values, replacing them with [email] / [ip].
- A before_breadcrumb in init_from_env is enough. Hub::add_breadcrumb runs it (sentry-core hub.rs:255), so it only costs anything when an error is reported, and regex is already a workspace dep.
- The same regex in before_send covers exception messages too.
- If it gets messy, ship as-is and we'll follow up.
Deploy failures: the Err branch (api/src/main.rs:3131) reports to Sentry but doesn't log anything. With no SENTRY_DSN, which is the default everywhere, a failed deploy still leaves no trace. Add tracing::error!(error = ?error, "deploy failed").
DSN: failing fast on an invalid DSN is fine. Two small fixes:
- The doc comment (sentry-middleware/src/lib.rs:243) still says it returns None when the DSN is invalid.
- Better to return a typed error from init_from_env than to panic inside sentry with "invalid value for DSN".

Thanks, the rest LGTM. A few notes: 1. **Tokio / thread-local hubs (not a blocker):** I don't think that's right. `sentry-tower` attaches the per-request hub to the request future (`Hub::run` + `fut.bind_hub(hub)`, sentry-tower 0.49.3 `lib.rs:277-280`). `SentryFuture::poll` swaps that hub into the thread-local on every poll (sentry-core `futures.rs:34-41`). `sentry-tracing` breadcrumbs go to the current hub (`layer/mod.rs:277`), so they follow the task across `.await`s and threads. Fine to keep the custom middleware for now. Two real caveats: - `tokio::spawn` needs an explicit `.bind_hub()`, same as your `REQUEST_ID` re-scope in deploy. - `new_from_top()` copies the worker thread's hub, so breadcrumbs from unbound background work can leak in. A closure provider returning a hub with an empty `Scope::default()` avoids that. 2. **PII:** let's scrub emails/IPs and keep only UUIDs. - The messages are interpolated (`onboarding.rs:305`, `rate_limit.rs:115`), so it needs a regex over the message and field values, replacing them with `[email]` / `[ip]`. - A `before_breadcrumb` in `init_from_env` is enough. `Hub::add_breadcrumb` runs it (sentry-core `hub.rs:255`), so it only costs anything when an error is reported, and `regex` is already a workspace dep. - The same regex in `before_send` covers exception messages too. - If it gets messy, ship as-is and we'll follow up. 3. **Deploy failures:** the `Err` branch (`api/src/main.rs:3131`) reports to Sentry but doesn't log anything. With no `SENTRY_DSN`, which is the default everywhere, a failed deploy still leaves no trace. Add `tracing::error!(error = ?error, "deploy failed")`. 4. **DSN:** failing fast on an invalid DSN is fine. Two small fixes: - The doc comment (`sentry-middleware/src/lib.rs:243`) still says it returns `None` when the DSN is invalid. - Better to return a typed error from `init_from_env` than to panic inside sentry with "invalid value for DSN".

vincent referenced this pull request

2026-10-15 12:02:53 +00:00

WIP: Migrate Platform public sources to git.caution.co #479

ryan force-pushed ryansquared/add-sentry from d17307e281 to 391ba5502a

2026-10-16 06:40:49 +00:00

Compare

ryan force-pushed ryansquared/add-sentry from 391ba5502a to 84d75c7c77

2026-10-16 06:44:26 +00:00

Compare

public manually merged commit e87ff01794 into main

2026-10-16 06:44:50 +00:00

Rows
Columns

WIP: Add Sentry #469