Repository navigation
Fix Data Explorer health scenarios reporting false unhealthy loads - #2590
Dmitrii Shilov (bk201-) wants to merge 7 commits into
Conversation
ScenarioMonitor.completePhase() silently no-oped when the phase had not been started yet, and the production call ordering always hits that case: - DatabaseTreeRendered is started in Explorer.tsx after the last databaseTreeNodes change, but the effect that completes it only re-fires on such a change and databaseTreeNodes is memoised, so the phase never closed. - Interactive is completed by a one-shot effect whose deps are stable, so it is lost entirely when ResourceTree mounts before refreshExplorer starts the scenario. Over three days DatabaseTreeRendered was missing in 1624 of 1627 DatabaseLoad timeouts (99.8%) and Interactive in 588, driving DataExplorerHealthV2 to report loads as unhealthy that had in fact completed. completePhase now self-starts a required phase that is completed before startPhase, and completions reported before the scenario exists are buffered and replayed on start(). A timeout while the tab is backgrounded no longer counts against health either: browsers throttle timers and suspend rAF there, so phase completion is unreliable and the elapsed time is not the user's experience. documentHidden is still reported so those can be sliced out in telemetry. Separately, configurePortal() resolved only from the iframe message handler with no timeout. When the portal never posts the init message the promise stayed pending forever, leaving the user on LoadingExplorer with no diagnostic while ApplicationLoad timed out mute. It now rejects after 30s with a ConfigurePortal failure trace, and the caller handles the rejection instead of leaving it unhandled. Related: IcM 865096261
Playwright tests ❌ failed
📁 Report:
|
Keep confirmation popovers above adjacent accordion headers. Target the actual switch, mock destination role APIs with ARM response shapes, and synchronize PITR and role assignment completion. Fix initialization hook formatting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Playwright tests ✅ passed
📁 Report: |
1. OverviewReducing false-unhealthy emissions is the right goal, and neither this PR nor the companion portal handshake fix needs to eliminate every transient failure. However, the reviewed patch also permits false-healthy outcomes and can prevent late initialization from recovering. Those are code-correctness issues to address before merge. Deciding which remaining failures warrant an IcM is separate monitoring work. The concern is that some changes make the observed timeout patterns disappear without establishing that the load finished. A lower unhealthy percentage alone would not demonstrate that the fix worked. 2. What needs to be fixed in this PR
These are source-level reproductions, and the relevant behavior is unchanged at reviewed head 3. What belongs in monitor and alerting changes
|
The previous revision removed the missing-phase signatures, but three of those changes could report a load as healthy without establishing that it finished, and one could prevent a slow initialization from recovering. Preserve late initialization recovery. configurePortal() raced the iframe handshake against a 30s reject while leaving the message listener active, so a late init message would build an Explorer that the rejected caller never received — turning a slow load into an unrecoverable one. The timeout is now a watchdog that traces the stuck handshake and lets the caller keep waiting, so recovery behaves as it did before. Initialization work inside the message handler is wrapped so a throw reports and still yields a shell, instead of leaving the promise pending forever. Do not accept an earlier render as proof the loaded tree is ready. Phase auto-start let a render of the databases-only tree complete DatabaseTreeRendered with a zero-duration measurement, before collections had loaded. The producer ordering is fixed instead: Explorer publishes a ready revision once the load has produced the data the tree should show, and ResourceTree completes the phase only for a render carrying that revision, acknowledging each revision once. Stale callbacks from a superseded load, accounts with no databases and unchanged trees are covered by tests. A completion for a phase that was never opened is now refused and reported as phase_complete_unstarted rather than backdated, and the early-completion buffer no longer accepts deferred phases, which must be opened by their producer. Do not turn unfinished background loads into successes. A timeout in a hidden tab emitted healthy=true even with no phases completed. The emitted outcome again reflects what actually happened; documentHidden continues to be reported so alerting can apply a background policy without the load being relabelled. Related: IcM 865096261
Playwright tests ✅ passed
📁 Report: |
The ready signal was an incrementing counter, so its correctness rested on an argument about scale rather than a property of the value. Overflow was not reachable in a page session — nine quadrillion refreshes — and the failure mode was fail-closed, leaving the phase open rather than reporting a load as healthy. It was still a bound that had to be argued rather than one that could not be exceeded. The signal is now a fresh object published per load and compared by reference. There is no value to exhaust or wrap, and the comparison is identity rather than ordering, so a store reset cannot leave the consumer permanently ahead of the producer either. Related: IcM 865096261
Playwright tests ❌ failed
📁 Report:
|
Playwright tests ✅ passed
📁 Report: |
# Conflicts: # test/sql/containercopy/permissionsScreen.spec.ts
Playwright tests ✅ passed
📁 Report: |
Playwright tests ✅ passed
📁 Report: |
This branch has not been deployed
2 participants
ScenarioMonitor.completePhase()silently no-oped when the phase had not been started yet, and the production call ordering inExplorer.tsx/ResourceTree.tsxalways hits that case.DataExplorerHealthV2therefore reported loads as unhealthy that had in fact completed.Evidence
Over three days,
scenario_timeoutevents:DatabaseLoadDatabaseTreeRenderedApplicationLoadPlatformConfigured+ExplorerInitializeddocumentHiddenwas true in 692 of 2276 (30%) — a contributing factor, not the cause; the same pattern dominates in foreground tabs.Root cause
DatabaseTreeRenderedis started atExplorer.tsx:451, immediately aftercompletePhase(CollectionsLoaded). The effect that completes it (useMetricPhases.ts:55-59) re-fires only on adatabaseTreeNodeschange, anddatabaseTreeNodesis memoised (ResourceTree.tsx:54). The last change happens before line 451, so the effect never re-fires and the phase stays open until the 10s timeout. This is a deterministic ordering inversion, not a race — which is why it is 99.8% and not 50%.Interactiveis completed by a one-shot effect whose deps ([scenario, enabled]) are stable. IfResourceTreemounts beforeExplorer.tsx:577starts the scenario, the completion is dropped and never retried. Missing in 588 of 1627.ApplicationLoadnever got pastconfigurePortal(), which resolves only from the iframemessagehandler with no timeout and no reject. When the portal does not post the init message the promise stays pending forever — the user sits on<LoadingExplorer />and the scenario times out with no diagnostic. Both phases hang off the sameawait, which is why they are always missing together.Changes
completePhaseself-starts a required phase that is completed beforestartPhase, instead of dropping the completion.start().documentHiddenis still reported, so these remain sliceable in telemetry.configurePortal()rejects after 30s with aConfigurePortalfailure trace — deliberately well above the 10s scenario budget so only genuinely stuck handshakes fail. The caller now handles the rejection instead of leaving it unhandled.Tests
src/Metrics/ScenarioMonitor.phaseOrdering.test.tsreplays the exact production call ordering and asserts it now completes healthy, plus: an early completion beforestart(), a genuine stall still reporting unhealthy, and a backgrounded-tab timeout not counting against health.Locally
src/Metrics/is 53 passed (4 new + 49 existing), andtsc --noEmitreports no errors insrc/.Notes for reviewers
cheeriofrom1.0.0-rc.12to1.2.0, andenzyme@3.11.0requirescheerio/lib/utils, which no longer exists. Sincejest.config.js:133loadsenzyme-to-json/serializerfor every suite, no test in the repo currently runs on a clean install. I validated with a temporary config that drops the enzyme serializer; that config is not part of this PR.canvasdoes not build andjsdomfails to load. The repo has noallowScriptsfield inpackage.jsonyet — worth adding separately.<LoadingExplorer />. That needs new localized strings and is better as its own change.The sending half of this handshake is fixed separately in
CosmosDB-portal(PR 2294596):_fetchMasterKey/_getAuthorizationTokenhad no rejection handler, so the init message was never posted.Related: IcM 865096261