Add Sentry monitoring for OnyxDerived recompute loops - #98133
Add Sentry monitoring for OnyxDerived recompute loops#98133TMisiukiewicz wants to merge 8 commits into
Conversation
|
@abzokhattab @mountiny One of you needs to 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] |
|
reviewing 👀 |
|
@codex @MelvinBot review |
|
🤖 Code review — this is a clean, well-scoped telemetry addition. The core logic is correct, it follows the existing 1. False-positive tuning — worth watching on rollout (not a blocker). The threshold is 50 recomputes / 10s (5/s sustained). Because 2. Nit: at What I checked
Overall LGTM pending green CI. 👍 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef42694a75
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-10.at.19.10.35.mov |
|
i found the following bug while testing:
const k = 'cards_TEST_LOOP';
for (let i = 0; i < 60; i++) { await Onyx.merge(k, {[`card${i}`]: {cardID: i}}); }
await Onyx.set(k, null);
Expected: the alert appears again. Actual: nothing is logged or sent to Sentry, for the rest of that session. Step 5 also reproduces via Settings → Troubleshoot → Clear cache and restart Screen.Recording.2026-08-10.at.19.15.56.mov |
…' into feature/onyx-derived-loop-monitoring
|
This PR adds a new |
|
@abzokhattab mind checking again? |
luacmartins
left a comment
There was a problem hiding this comment.
Left a few comments. In general, can we de-AI the comments please?
|
@luacmartins mind checking again? |
Explanation of Change
Adds a rate tripwire that detects runaway Onyx derived-value recompute loops and reports them once per derived key per session.
detectOnyxDerivedLoop(src/libs/telemetry/detectOnyxDerivedLoop.ts): per-derived-key rolling 10s window (WINDOW_MS) of recompute timestamps; trips at 50 recomputes (RECOMPUTE_THRESHOLD, ~5/s sustained). On trip it reports[OnyxDerived] recompute loop detected for <key>to Sentry viacaptureMessagewithfingerprint: ['onyx-derived-loop', <key>]plusextra.dependencyCounts(a per-dependency count showing which dependency drives the churn), and mirrors the same message to the server log viaLog.alert(no stack trace) so it lands in VictoriaLogs next to the surrounding[OnyxDerived]lines. Latched once per key per session; window cleared on trip so a tripped key retains nothing.flushRecompute, right after the existingtriggeredKeysset is built — so it counts flushes, not per-dependency callbacks, and costs one Map lookup + one small push in steady state.getSpan(SPAN_APP_STARTUP)), since dependencies legitimately hydrate in bursts during startup.captureMessagerather than a span deliberately:startSpanearly-returns unlessAppState === active, so span-based detection is blind exactly when a backgrounded client is spinning.Fixed Issues
$ #97711
PROPOSAL:
Tests
[alrt] [OnyxDerived] recompute loop detected for...showed up in the consoleOffline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari