Skip to content

Add sentry support#250

Merged
lukevalenta merged 7 commits into
mainfrom
mendess/add-sentry-support
Jul 15, 2026
Merged

Add sentry support#250
lukevalenta merged 7 commits into
mainfrom
mendess/add-sentry-support

Conversation

@mendess

@mendess mendess commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Add sentry support to all workers. This requires a bump of the worker version to
support unwinding in wasm

@mendess mendess self-assigned this Jul 7, 2026
@mendess mendess requested review from a team and lbaquerofierro as code owners July 7, 2026 12:04
@mendess mendess marked this pull request as draft July 7, 2026 12:30
lukevalenta
lukevalenta reviewed Jul 7, 2026
View reviewed changes
Comment thread crates/generic_log_worker/src/obs/sentry.rs
dsn.project_id(),
);
let auth = format!(
"Sentry sentry_key={}, sentry_version=7, sentry_client=azul/{}",

@lukevalenta lukevalenta Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from sentry_client=azul line, it seems like this sentry functionality would be pretty generally useful for Rust-based Workers. Worth considering publishing as a standalone worker-sentry crate, possibly in the https://github.com/cloudflare/workers-rs repo.

@lukevalenta

lukevalenta commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Since Bonk seems to be having trouble lately, here's some feedback from a local AI review:

Review summary of PR #250
CI: All three integration jobs are currently failing — not a compile error, but wrangler dev failed to start in time. The wasm build got slower (--panic-unwind, dropping strip = true, worker 0.7→0.8, plus sentry/getrandom deps) and likely blew past the ~60s startup window. Needs confirming whether to bump the timeout or if the build actually hangs.
Bugs / issues found:

  1. Uuid::nil() on all panic events (Medium) — Sentry dedupes by event_id, so every panic sharing the all-zero ID may be dropped after the first. Since panic capture is the headline feature, this undermines it. Fix: derive a getrandom-free varying UUID (hash of message + file:line:col).
  2. Cron handlers don't catch panics (Low–Medium) — the scheduled handlers capture Err but aren't wrapped in catch_unwind_and_flush, so a panic buffers an event that never flushes. Inconsistent with the DO/frontend paths.
  3. witness_worker only partially integrated (Low) — gets --panic-unwind + RefUnwindSafe but no init_sentry/capture wrappers, despite "all workers" in the description.
    Non-blocking: scope tags persist across requests in a DO isolate (use with_scope); DO errors reported at Fatal (defensible); trailing flush skipped on frontend ? error path (matches pre-existing behavior).

@lukevalenta

lukevalenta commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

/bonk review

@ask-bonk

ask-bonk Bot commented Jul 7, 2026

Copy link
Copy Markdown

APIError: Invalid Anthropic API Key

github run

2 similar comments
@ask-bonk

ask-bonk Bot commented Jul 7, 2026

Copy link
Copy Markdown

APIError: Invalid Anthropic API Key

github run

@ask-bonk

ask-bonk Bot commented Jul 7, 2026

Copy link
Copy Markdown

APIError: Invalid Anthropic API Key

github run

@ask-bonk

ask-bonk Bot commented Jul 7, 2026

Copy link
Copy Markdown

@lukevalenta Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@mendess mendess force-pushed the mendess/add-sentry-support branch 5 times, most recently from c8c8e82 to b581f55 Compare July 13, 2026 15:05
mendess added 5 commits July 14, 2026 12:18
@mendess
Add sentry support
2a54873
@mendess
Update integration test wait time
9319184
@mendess
Add panic-unwind to CI
fc69608
@mendess
Force legacy Wasm EH in --panic-unwind builds
ad0674c 
Since nightly >=2026-05-09 (rust-lang/rust#156061), wasm targets emit
modern exnref exception-handling opcodes by default under
-Cpanic=unwind.  The wasm-bindgen 0.2.126 CLI bundles wasmparser
0.245.1, which rejects these opcodes with:

    type mismatch: catch_all_ref label must be a subtype of (ref exn)

Work around this by passing -Cllvm-args=-wasm-use-legacy-eh via
RUSTFLAGS at every worker-build --panic-unwind invocation site.
worker-build reads existing RUSTFLAGS and appends -Cpanic=unwind,
so the flag survives into the cargo subprocess.  Placing it in
.cargo/config.toml would not work: Cargo treats RUSTFLAGS env and
target.<triple>.rustflags as mutually exclusive sources, and the
env var (always set by worker-build) wins.

This can be removed once wasm-bindgen ships a wasmparser that
understands exnref (tracked in wasm-bindgen/wasm-bindgen#5219).
@mendess
Add getrandom_03 wasm_js feature to witness_worker
5ae3d03 
witness_worker transitively depends on getrandom 0.3.4 via
generic_log_worker -> sentry-core -> rand 0.9 -> rand_core 0.9.
Without the wasm_js feature, getrandom 0.3.4 emits a compile_error!
on wasm32-unknown-unknown.

ct_worker and bootstrap_mtc_worker already carry this dependency
for feature unification; witness_worker was missing it.
@mendess mendess force-pushed the mendess/add-sentry-support branch from 0460a55 to 5ae3d03 Compare July 14, 2026 11:18
@mendess mendess requested a review from lukevalenta July 14, 2026 13:40
@mendess mendess marked this pull request as ready for review July 14, 2026 13:40
@lukevalenta

lukevalenta commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

/bonk review

@ask-bonk

ask-bonk Bot commented Jul 14, 2026

Copy link
Copy Markdown

PR #250 – Sentry integration review

Findings (ordered by severity)

  1. Deterministic panic UUIDs – The panic‑hook now builds a deterministic UUID from the panic message and location (lines 134‑155 in crates/generic_log_worker/src/obs/sentry.rs). This eliminates the previous Uuid::nil() deduplication problem. ✅ No change needed.

  2. Cron handlers capture panics – All scheduled jobs (ccadb_roots_cron.rs in both ct_worker and bootstrap_mtc_worker) wrap their async body with generic_log_worker::obs::sentry::catch_unwind_and_flush (lines 33‑60). Panics are therefore reported and flushed before the isolate exits. ✅ No change needed.

  3. Witness worker fully integrated –

    • init_sentry is called at the start of the fetch handler (line 80 in crates/witness_worker/src/frontend_worker.rs).
    • The fetch handler uses catch_unwind_and_flush (lines 83‑97).
    • The DO methods (witness_state_do.rs) also invoke init_sentry before any work.
      → Sentry is active for both request‑path errors and panics. ✅ No change needed.

  4. Scope‑tag isolation – capture_error_and_flush scopes tags via sentry_core::with_scope (lines 313‑321). This prevents tag leakage across requests in long‑lived DO isolates. ✅ No change needed.

  5. CI build timeout – The CI comment notes that the wasm build with --panic-unwind and the added sentry/getrandom deps sometimes exceeds the ~60 s startup window. This is a CI configuration issue (timeout value) rather than a code defect. No code changes are required; consider raising the workflow timeout or optimizing the build if the problem persists.

Overall assessment – The Sentry support is correctly wired across all workers, panic events are uniquely identified, and error handling respects DO isolation semantics. No functional regressions are apparent. The only actionable item is to address the CI timeout (outside the codebase).

github run

lukevalenta
lukevalenta approved these changes Jul 14, 2026
View reviewed changes

@lukevalenta lukevalenta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Use of nightly features seems OK.

Comment thread .github/workflows/integration.yml

# RUSTFLAGS: force legacy Wasm EH so wasm-bindgen 0.2.126's wasmparser
# accepts the module (modern exnref opcodes from nightly >=2026-05-09
# are not yet supported).

@lukevalenta lukevalenta Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the commit message, looks like wasm-bindgen/wasm-bindgen#5219 tracks support being added to wasm-bindgen without the flag? Could you link that here?

@lukevalenta lukevalenta merged commit 056fe96 into main Jul 15, 2026
9 checks passed
@lukevalenta lukevalenta deleted the mendess/add-sentry-support branch July 15, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukevalenta lukevalenta lukevalenta approved these changes
@bwesterb bwesterb Awaiting requested review from bwesterb bwesterb is a code owner
@cjpatton cjpatton Awaiting requested review from cjpatton cjpatton is a code owner
@lbaquerofierro lbaquerofierro Awaiting requested review from lbaquerofierro lbaquerofierro is a code owner

Assignees

@mendess mendess

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mendess @lukevalenta