Notice when the ingest API refuses an event - #825
Merged
Conversation
The client never asked the ingest API whether it *accepted* an event, so it never read the status field: a rejected event came back HTTP 200 and was reported as a success. A malformed property would have failed silently, forever, and the only way to find out was to go and look in Mixpanel. `verbose: true` is the whole client-side fix. What it needs beside it is a decision about what a failure deserves, because turning detection on without one would trade silence for noise. **A refusal and an unreachable API are different facts.** A refusal is our bug — a bad token, a property the API will not take — and it fails *every* event until someone fixes it, so it must be visible. Nothing reachable is somebody's wifi; reporting those would fill the crash reporter with other people's tunnels and bury the refusals among them. `DeliveryFailure::classify` splits them per variant rather than by category, and a failure that never reached the network at all counts as ours by definition. **A refusal is reported once per run.** It fails every event alike, so the second report says nothing the first did not — without this, one bad token becomes one crash report per user action. Same shape as the login-episode rule: count the problem, not each discovery of it. The subtlety worth a test: a network failure must not *spend* the one refusal report, or the first flaky tunnel masks a real misconfiguration for the rest of the run. Classifying before deciding is what avoids that. This is only observable at all because #823 gave fault reporting a seam — before it, "did we report the refusal" was unanswerable. just lint 0, cargo fmt --check 0, 317 quilt-sync tests, workspace green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fiskus
added a commit
that referenced
this pull request
Aug 5, 2026
Review caught a real defect, and it reintroduced the exact thing #825 added a test for. The timeout borrowed `TelemetryError::Serialize` for want of a better variant. `classify` treats anything that is not the client's own error as a refusal — sound for a payload that never left the process, wrong for a timeout, which *did* reach the network and simply got no verdict. So a slow request filed a false fault and **spent the one refusal report a run is allowed**, swallowing any genuine refusal later in the same run. The per-variant tests passed throughout, because the variant they exercised was not the one the timeout used. That is the more useful lesson than the bug: the coverage was per-error-value, and the defect was in which value got chosen. - A `SendTimeout` variant of its own, since classification follows from the variant. - `classify` is now exhaustive over our own errors as well as the client's, so a new one cannot inherit an answer by falling through — which is precisely how this happened. - The existing refusal-report test now runs both kinds of not-reaching-the-API, so the property is pinned against the shape that broke it rather than only the shape I thought of first. just lint 0, cargo fmt --check 0, 322 quilt-sync tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fifth code unit of quiltdata/quilt-specs#37 (
w-delivery-observable), after #820, #822, #823 and #824.Problem
The client never asked the ingest API whether it accepted an event, so it never read the status field: a rejected event came back HTTP 200 and was reported as a success. A malformed property would fail silently, forever, and the only way to find out was to go and look in Mixpanel.
verbose: trueis the whole client-side fix. What it needs beside it is a decision about what a failure deserves — turning detection on without one would trade silence for noise.A refusal and an unreachable API are different facts
DeliveryFailure::classifysplits them per variant rather than by category, so a new client error has to be placed deliberately. A failure that never reached the network at all — a payload we could not serialize — counts as ours by definition.A refusal is reported once per run
It fails every event alike, so the second report says nothing the first did not. Without this, one bad token becomes one crash report per user action. Same shape as the login-episode rule in #824: count the problem, not each discovery of it.
The subtlety worth its own test: a network failure must not spend the one refusal report, or the first flaky tunnel masks a real misconfiguration for the whole run. Classifying before deciding is what avoids that.
Note
This is observable at all only because #823 gave fault reporting a seam. Before it, "did we report the refusal?" was unanswerable — which is also why the previous telemetry change's post-release verification could confirm that events arrive but never that none are dropped.
Verification
just lintexit 0,cargo fmt --checkexit 0, 317 quilt-sync tests, workspace green (55 / 458 / 317 / 84 / 1).Five new tests: the classification per variant (three refusal kinds, four unreachable kinds), a serialization failure counting as ours, a refusal reported exactly once across five occurrences, an unreachable API never reported, and a network failure not spending the refusal report.
🤖 Generated with Claude Code
Greptile Summary
This PR enables verbose Mixpanel responses so rejected events become observable, then separates systematic refusals from transient connectivity failures.
Confidence Score: 5/5
The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issue established.
The new path detects rejected events, reports systematic failures once per process, and keeps transient network failures out of crash reporting, with tests covering the intended classification and deduplication behavior.
Important Files Changed
Sequence Diagram
sequenceDiagram participant App participant Telemetry participant Mixpanel participant Sentry App->>Telemetry: track(event) Telemetry->>Mixpanel: "track(verbose=true)" alt Event accepted Mixpanel-->>Telemetry: success else Event refused Mixpanel-->>Telemetry: refusal error Telemetry->>Telemetry: classify Refused opt First refusal this run Telemetry->>Sentry: report error end else API unreachable Mixpanel-->>Telemetry: transport/server error Telemetry->>Telemetry: classify Unreachable Telemetry->>Telemetry: write warning endReviews (1): Last reviewed commit: "Notice when the ingest API refuses an ev..." | Re-trigger Greptile
Context used: