Send expected bootsplash, auth retry and Pusher teardown signals to Sentry logs instead of the error stream - #99789
Send expected bootsplash, auth retry and Pusher teardown signals to Sentry logs instead of the error stream#99789MelvinBot wants to merge 2 commits into
Conversation
…entry logs instead of the error stream The bootsplash monitor captured an event every 10s for as long as the splash stayed up, so a single stuck boot emitted unbounded error-stream volume. It now forwards a Sentry log carrying how long the gate stayed shut, capped at six reports. trackAuthenticationError sent the retryable UNABLE_TO_RETRY path as an exception even though the user is deliberately kept signed in and the retry mechanism handles it. That error type now goes to Sentry logs, keeping its tags as log attributes; every other error type still captures an exception. Pusher.subscribe captured an exception when the socket was gone, a race its own comment describes as expected during Upgrade Required teardown. It now logs instead, on both web and native. Co-authored-by: Rodrigo Lino da Costa <rlinoz@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: Rodrigo Lino da Costa <rlinoz@users.noreply.github.com>
|
Fixed and pushed — Cause: one line in the new test file was 191 characters, one over the Fix: wrapped that call's arguments the way oxfmt formats them, in The only remaining failure is Why the earlier local run missed itThe AI-test table in the PR body lists ESLint, typecheck, spellcheck, and Jest — none of which enforce oxfmt's print width, and In this run I could not execute |
Web test run: ✅ passRan this PR's testing steps on Web against the
Evidence (6)App booted to Home after bootsplash, no hang
Concierge chat and Settings/Profile navigation working normally
Force offline enabled; 'You appear to be offline' banner visible
Message sent while offline is queued in the chat
Force offline disabled, offline banner cleared
Reconnected: queued message delivered normally after coming back online
Two caveats on coverage1. "No JS console errors" was verified indirectly. The web harness has no browser-console command, so the check was zero failed HTTP requests across the whole session plus no error-boundary/crash UI in any screenshot. That is weaker than reading 2. The offline pass used the in-app simulator, not a real network cut. Settings → Troubleshoot → "Force offline" marks the app offline for the banner and the optimistic queue, but it does not drop the socket. So this run exercised the offline UX, not the Also worth noting for the record: because all three changes route signals to Sentry rather than change behavior, a passing manual web run confirms no regression but cannot confirm the signals actually landed in Sentry logs. Confirming that needs a look at the Sentry project after this deploys. |
Android test run: ✅ passRan this PR's testing steps on Android (standalone
Evidence (6)Home screen reached after boot
Concierge chat report open
Settings/Account page
Offline indicator visible in chat
Message queued while offline
Back online, message synced, offline indicator cleared
Console output worth notingTwo
Coverage noteThe offline pass here was a real network cut ( As with the web run: because all three changes only reroute signals to Sentry, a passing manual run confirms no regression but cannot confirm the signals actually landed in Sentry logs. That needs a look at the Sentry project after this deploys. Evidence (1)Recordings- [▶ Session recording of the verification run](https://melvinbot-assets.exops.io/session-33207906168-1-478c9aaf-969d-43a6-a71f-3b7aae301a98.mp4) |
2 participants
Explanation of Change
Three call sites reported conditions their own code comments describe as expected, using Sentry's error stream. Each is now a Sentry log instead, so the signal is kept but stops counting against the error quota. This implements items 2 and 4 of the audit shortlist in #99784.
1. Bootsplash monitor (
src/libs/telemetry/bootsplashTelemetry.ts)startBootsplashMonitorcalledSentry.captureMessageon a 10-second interval for as long as the splash stayed visible, with no upper bound. One stuck boot therefore emitted events for the whole session, and it was alreadylevel: 'warning'— nobody triages it as a crash.It now calls
Sentry.logger.warn, and stops after 6 reports. The cap needs to exist because the interval only clears when the splash state moves offVISIBLE; a splash that never hides would otherwise report forever. Six covers the first minute, which is where a recoverable stall resolves. Each report carriesstuckForMs, which is the thing repeated reports were actually for — how long the gate stayed shut.2. Authentication
network_retry(src/libs/telemetry/trackAuthenticationError.ts)trackAuthenticationErrorsent every error type throughSentry.captureException, includingnetwork_retry. That is theUNABLE_TO_RETRYpath inReauthentication, whose comment states the request failed on a spotty connection, the user is deliberately not logged out, and the retry mechanism takes it from there — a routine offline condition sitting in the error stream.network_retrynow goes toSentry.logger.warnwith its tags preserved as log attributes, so it stays searchable byauthentication_function/authentication_json_code. Every other error type (missing_params,auth_failure,unexpected_error) still captures an exception, unchanged. This mirrors the classificationtrackMFAFlowOutcomealready does, where routine outcomes are logged and only anomalous ones become events.3. Pusher subscribe-after-disconnect (
src/libs/Pusher/index.ts,index.native.ts)Pusher.subscribecaptured an exception whensocketwas gone in production. The comment on that branch already says this happens whendisconnect()runs during the "Upgrade Required" teardown before the deferredTransitionTrackercallback does — a teardown race, not a crash. It now logs. The__DEV__branch is untouched, so a genuine subscribe-before-init in development still rejects the promise loudly.Note on scope: this is a telemetry-routing change only. No user-facing behavior changes, and no diagnostic signal is removed — all three still report, just to Sentry logs (
enableLogs: trueis already set insetupSentry) rather than the error stream.Fixed Issues
$ #99784
PROPOSAL: #99784 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
// TODO: The human co-author must fill this out. The
network_retrypath in change 2 is reached by going offline or dropping the connection mid-reauthentication, so an offline pass is directly relevant here.QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
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
AI Tests run by MelvinBot
npm run typechecknpm run lint-changednpx eslint <changed files>eslint-seatbeltwarnings on untouched lines of the Pusher filesnpm run spell-changed -- <changed files>npm test -- --silent tests/unit/bootsplashTelemetryTest.ts tests/unit/trackAuthenticationErrorTest.tsnpm test -- --silenton existing tests covering touched code (PusherSubscribeTest,PusherSubscribeWebTest,PusherPingPongTest,ReauthenticationMiddlewareTest,forwardLogsToSentryTest,setupSentryIntegrationOrderTest)Not run, with reasons:
npm run prettier— App has noprettierscript or prettier config of its own; ESLint (witheslint-config-prettier) is the formatting gate here and it passed. Invoking prettier directly picked up an unrelated parent-directory config.npm test— only the suites covering the touched code were run, listed above. The full suite was not run in this environment.npm run storybook -- --smoke-test --ciandnpm run react-compiler-compliance-check check-changed— no components, hooks, or.tsxfiles changed.New tests added:
tests/unit/bootsplashTelemetryTest.ts— asserts the stuck splash goes toSentry.logger.warnand notcaptureMessage, thatstuckForMsgrows across reports, that reporting is capped at 6 rather than growing with session length, and that a hidden splash reports nothing.tests/unit/trackAuthenticationErrorTest.ts— assertsnetwork_retrylogs (with its tags preserved as attributes) and never captures an exception, whileauth_failureandunexpected_errorstill capture exceptions.Not covered by a new test: the Pusher change.
PusherSubscribeWebTest's harness never reaches the production!socketbranch — under Jest__DEV__is true, so that path rejects instead. The change is a one-line swap of the telemetry call with no surrounding logic change, and the existing Pusher suites still pass.