perf(scrub): hot-path guards, recall gaps, and corrected benchmark claims

Summary

Split out of !26 (closed), which bundled a performance regression with work that has none. This is the half with no regression and no open questions: two recall gaps, and a hot-path cleanup that makes the scrubber measurably faster than main. No new value patterns, no behavior change to any existing redaction — nothing here can redact text that main wouldn't, except via key names.

Everything with a trade-off attached went to !28, which stacks on this branch. That includes the string-headed-list fix that was originally here: three fresh-context reviewers turned up two genuine judgment calls in it, so it moved rather than hold this up.

Commits

  1. creds key token + errors section walk — unchanged from !26 (closed). creds is a more common variable name than credentials and was matched by neither list. errors is persisted verbatim into IssueEvent.data and needs the same recursive walk as every other section.

  2. Perf: guard the value patterns, stop cloning on no-match — unchanged from !26 (closed). CARD_RE ran unconditionally over every string in every scrubbed section; each pattern now has a byte-level precondition that is a strict superset of what its regex can match. Separately, replace_all returns Cow::Borrowed on no-match and the old code called .into_owned() regardless.

  3. Docs: correct three claims review proved wrong — new, documentation only.

What review found

Three fresh-context reviewers re-derived this from the diff alone. The guards — the part that would be a security bug if wrong — came back clean, and the verification was thorough enough to be worth recording:

  • A full 0..=0x10FFFF sweep confirming every char the regex crate's \d matches also satisfies char::is_numeric(), so the non-ASCII fallback in has_min_digits is a genuine superset of Nd.
  • ~1.1M differential cases of guarded vs unguarded scrub_string, including a placeholder:"" config that can splice two digit runs into a new 13-digit run after PEM substitution. Zero divergence.
  • Cow::Borrowed is returned only on the genuine no-match path, so no replacement can be dropped.

Three things this file asserted did not survive, and commit 3 corrects them:

The errors rationale was backwards. The comment claimed the worker regenerates its entries from the already-scrubbed payload. It does not — EventProcessingError(type, name, value=v) is built during schema validation, before scrubbing, and stores the entire unvalidated field in value. The fields wrapped that way are exactly the PII-bearing ones.

The benchmark cannot measure what it appears to measure. The fixture has no @ anywhere, and its only ≥13-digit runs sit under the key session, which is key-redacted before scrub_string is reached. Instrumented, CARD_RE and EMAIL_RE execute zero times across all 385 strings per event. The speedup is real and reproduces in both run orders — −24% on defaults, −36% with emails, plus −62%/−75% fewer transient bytes allocated, which is the stronger north-star argument and the commit didn't even claim it — but it is a best case, and is now labelled as such in the fixture doc.

Two benchmark comments were wrong. The fixture is ~16 KB, not ~8 KB. Cloning it costs ~70 µs against a ~78–85 µs scrub — comparable, not the claimed order of magnitude.

One correction to the earlier numbers: the Cow change contributes 0 ns and 0 allocations on this fixture, because the guards prevent replace_all from being called at all. Ablation builds put guards-only and guards-plus-Cow within noise of each other. The Cow change is still right — it pays off on strings that pass the guard and don't match — but the −24%/−36% should be attributed to the guards alone, not read as additive.

Known follow-up, not in this MR

is_sensitive_key is the real hot path — ~55.6 µs of the ~78–85 µs scrub, roughly 4× everything this MR touches. normalize_key allocates twice for every key including the 82% that are already plain lowercase ASCII. A prototyped allocation-free fast path measured −43% allocations/event and −13.9% wall clock with all tests passing. Deliberately left for its own MR.

Risk

Confined to crates/gt-ingest/src/scrub.rs; nothing outside the crate constructs ScrubConfig. Per the exposure table in AGENTS.md the ingest pipeline is mothballed and GLITCHTIP_RUST_INGEST still defaults to False, so none of this is live in any deployment yet.

cargo fmt, cargo clippy --all-targets -D warnings and 139 tests clean.


AI disclosure: Claude Opus 5 (Claude Code) split this branch out of !26 (closed), wrote commit 3, drafted this description, and ran three independent fresh-context review passes whose findings are summarised above. Commits 1 and 2 are byte-identical to their reviewed form in !26 (closed). Reviewed by @bufke before publishing.

Edited by David Burke

Merge request reports

Loading
Loading