[No QA] Add advance-vs-stall telemetry to reconnect requests and alert on the older loop-breaker - #97585
Conversation
…lder loop-breaker A repeated GetMissingOnyxMessages call is only readable with the range it asked for: a range that advances is a client catching up on a busy account, a range that repeats is a client stuck in a loop. The span carried neither, so classifying the two shapes meant inferring from response sizes. Stamp updateIDFrom/updateIDTo onto the outgoing request span via Sentry's beforeOutgoingRequestSpan hook. It reads the values out of the request body, so it lands on the same http.client span that existing Sentry queries match, which no middleware or call-site span can reach. Only GetMissingOnyxMessages and incremental ReconnectApp send those fields, so nothing else is stamped. Also switch the older loop-breaker in validateAndApplyDeferredUpdates from Log.info to Log.alert. Info stays on the device, so we could not tell whether that guard has ever fired in the field, while the newer guard in OnyxUpdateManager already alerts and is countable. No change to reconnect or gap-detection behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
The Sentry `beforeOutgoingRequestSpan` approach could not work on native. It reads the request body out of `hint.input`, and only the fetch path populates that field. React Native's tracing integration sets `traceFetch: false` and `traceXHR: true`, so on iOS and Android the hook fired with no body to read and stamped nothing — on exactly the platforms where reconnect storms hurt most. The hook also fires at request start, so it can never answer the question the telemetry exists for: did this call advance the client. That is a comparison against the response. Sentry search compares a field to a literal, not to another field, so `last_update_id <= update_id_from` is not expressible as a query either. The verdict has to be computed at write time and stored. `SentryServerTiming` already runs for every command, already sees both the request and the response, and its own comment says to extend `TRACKED_COMMAND_GROUPS` rather than add a middleware. Reconnect commands get a group there, with `update_id_from` from the request and `response_advanced` from the response. Two details worth naming: `update_id_from` is absent on a full ReconnectApp, which refetches everything, so `response_advanced` is left off those spans too — there is nothing to advance past. That also keeps the attribute off the write-command spans in the other group. The check is `=== undefined` rather than truthiness because `updateIDFrom` is legitimately 0 for GetMissingOnyxMessages. Span ids no longer key off `requestIndex`. That field is only set on persisted write requests, so every reconnect span shared one key, and starting the next span cancelled the one still in flight — a collision in precisely the overlapping-burst case this measures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
adhorodyski
changed the title
The floor alone cannot separate two different stalls. When update_id_from holds still and update_id_to keeps rising, the client is falling behind a server that keeps producing updates. When both hold still, the client is stuck asking for a range the server will not serve. `response_advanced` reads false in both cases. The earlier reasoning for dropping this attribute was that Sentry cannot subtract two attributes, so it cannot report backlog size. True, but the useful query needs no subtraction: group by user.id, then plot count_unique(update_id_from) against count_unique(update_id_to). A unique floor of 1 against a rising target is the falling-behind case. The value is real on every GetMissingOnyxMessages call, never the 0 default: it arrives as lastUpdateIDFromServer, previousUpdateIDFromServer, or latestMissingUpdateID. ReconnectApp has no target, so the attribute stays off those spans. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`prepareRequest` stamps `requestIndex: requestIndex++` on every request it builds (API/index.ts:137), and `makeRequestWithSideEffects` goes through it, so GetMissingOnyxMessages and ReconnectApp already carry a unique monotonic index. The counter added earlier solved nothing. The field reads as optional on the Request type because persisted requests restored from disk can predate it, not because side-effect requests lack it. Also drops the last two comments. Both restated what the code says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"refetches" is not in the dictionary; "fetches everything again" says the same thing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
didResponseAdvance defaulted a missing lastUpdateID to 0, so an unreadable answer compared as 0 > updateIDFrom and stamped response_advanced: false — indistinguishable from a client genuinely re-asking for the same range. Return undefined instead, so an absent verdict means "could not tell". The presence of update_id_from still separates that from a full reconnect, which has nothing to compare against in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@mkhutornyi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@MelvinBot review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
🤖 Review: LGTM — small, well-tested, low-risk telemetry change. It adds a I confirmed the reconnect semantics line up with how the requests are actually built:
Web smoke test (web platform): app loads, Inbox renders, and open-chat → back navigates cleanly with no regression (screenshots below). Console-level span inspection isn't reachable through the web agent tooling, and no reconnect fired in the short session — so the span-line assertions in the test plan (Tests 1–3) still need a human pass with DevTools + network throttling. Two minor, non-blocking notes: 1.
|
|
Please pull main. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Looks good, there is one pending comment, can you check it please?
Number(null) and Number('') are both a finite 0, so Number.isFinite let
them through as update ID 0: a response carrying no lastUpdateID read as
a stalled client, and a request with no updateIDTo stamped a target of 0.
Both sites now share one reader that rejects those two shapes first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🚧 blimpich has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.52-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site content under No help site changes are required. This PR is purely internal observability and changes no customer-facing behavior:
The help site documents user-facing product features, settings, workspaces, integrations, and billing — none of which are touched here. There is no UI, setting, label, or workflow change to document, so no draft help site PR was created and there is nothing to review. @adhorodyski, if you believe a help site update is nonetheless warranted, let me know what behavior should be documented and I'll open a draft PR. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.52-11 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
Sentry shows a reconnect request as an opaque
POST .../api/GetMissingOnyxMessagesorReconnectAppspan with no way to tell a client that's genuinely advancing from one that's stuck re-asking for the same range.This PR adds a new
ManualReconnectServerResponsespan aroundReconnectAppandGetMissingOnyxMessagesrequests in theSentryServerTimingmiddleware, tagged withupdate_id_from,update_id_toandresponse_advanced.response_advancedis stamped only when the request asked from an update ID and the response returned one. An absentresponse_advancedmeans the middleware could not tell. A response with nolastUpdateIDis never counted as a stalled client.Fixed Issues
$ #92541
PROPOSAL:
Tests
Test 1: incremental ReconnectApp
npm run web. Sign in. Wait for the app to load.[Sentry][ManualReconnectServerResponse.Ending spanline. The line showscommand: ReconnectAppandjson_code: 200.update_id_fromandresponse_advanced. The line shows noupdate_id_to. A client that already loaded sends an incremental reconnect, soReconnect.reconnect()passeslastUpdateIDAppliedToClientasupdateIDFrom.reconnectAppsends noupdateIDTo.[OnyxUpdates] App reconnecting with updateIDFrom: Xline.Xequalsupdate_id_fromon the span.response_advancedisfalse. Nothing changed on the account during the 5 seconds offline.response_advancedon theReconnectAppline is nowtrue.Test 2: full ReconnectApp
subscribeToFullReconnect.Full reconnect triggered, then[OnyxUpdates] App reconnecting with updateIDFrom: 0.Ending spanline. The line showscommand: ReconnectAppandjson_code: 200.update_id_from, noupdate_id_to, and noresponse_advanced. A full reconnect fetches all data again, soreconnectAppsends noupdateIDFrom. The middleware has nothing to compare.Test 3: GetMissingOnyxMessages
GetMissingOnyxMessages.Ending spanline. The line showscommand: GetMissingOnyxMessages,json_code: 200,update_id_from,update_id_to, andresponse_advanced: true.[OnyxUpdates] Fetching missing updates updateIDFrom: X and updateIDTo: Yline.Xequalsupdate_id_fromon the span.Yequalsupdate_id_toon the span.Offline tests
QA Steps
An explicit no QA label.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A — this PR only adds Sentry span attributes and upgrades an internal log call to an alert; there is no UI change to capture.
Android: mWeb Chrome
N/A — this PR only adds Sentry span attributes and upgrades an internal log call to an alert; there is no UI change to capture.
iOS: Native
N/A — this PR only adds Sentry span attributes and upgrades an internal log call to an alert; there is no UI change to capture.
iOS: mWeb Safari
N/A — this PR only adds Sentry span attributes and upgrades an internal log call to an alert; there is no UI change to capture.
MacOS: Chrome / Safari
N/A — this PR only adds Sentry span attributes and upgrades an internal log call to an alert; there is no UI change to capture.