From ef89a7fc5a80f8945ba509d057256c6c84128451 Mon Sep 17 00:00:00 2001 From: Sung-Hyun Kang Date: Wed, 19 Aug 2026 16:29:49 -0500 Subject: [PATCH] fix: improve Data Explorer health signal accuracy Scope expected failures to their scenario and phase, preserve unexpected-failure precedence, treat hidden-page timeouts as expected, and add diagnostics and regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68ccefda-9e7d-4b01-a4db-e390810491ee --- src/Common/ErrorHandlingUtils.ts | 8 - src/Explorer/Explorer.tsx | 30 +- src/Metrics/ErrorClassification.test.ts | 12 +- src/Metrics/ErrorClassification.ts | 34 +- src/Metrics/ScenarioMonitor.test.ts | 396 ++++++++++++------------ src/Metrics/ScenarioMonitor.ts | 125 ++++++-- src/Utils/AuthorizationUtils.ts | 14 - src/hooks/useKnockoutExplorer.ts | 60 +++- 8 files changed, 407 insertions(+), 272 deletions(-) diff --git a/src/Common/ErrorHandlingUtils.ts b/src/Common/ErrorHandlingUtils.ts index 6996f19cd..5820ab8bb 100644 --- a/src/Common/ErrorHandlingUtils.ts +++ b/src/Common/ErrorHandlingUtils.ts @@ -1,8 +1,6 @@ import { stringifyError } from "Common/stringifyError"; import { MessageTypes } from "../Contracts/ExplorerContracts"; import { SubscriptionType } from "../Contracts/SubscriptionType"; -import { isExpectedError } from "../Metrics/ErrorClassification"; -import { scenarioMonitor } from "../Metrics/ScenarioMonitor"; import { userContext } from "../UserContext"; import { ARMError } from "../Utils/arm/request"; import { logConsoleError } from "../Utils/NotificationConsoleUtils"; @@ -34,12 +32,6 @@ export const handleError = ( // checks for errors caused by firewall and sends them to portal to handle sendNotificationForError(errorMessage, errorCode); - - // Mark expected failures for health metrics (auth, firewall, permissions, etc.) - // This ensures timeouts with expected failures emit healthy instead of unhealthy - if (isExpectedError(error)) { - scenarioMonitor.markExpectedFailure(); - } }; export const getErrorMessage = (error: string | Error = ""): string => { diff --git a/src/Explorer/Explorer.tsx b/src/Explorer/Explorer.tsx index bf1c5a33e..49bf696d6 100644 --- a/src/Explorer/Explorer.tsx +++ b/src/Explorer/Explorer.tsx @@ -33,6 +33,7 @@ import * as DataModels from "../Contracts/DataModels"; import { ContainerConnectionInfo, IPhoenixServiceInfo, IProvisionData, IResponse } from "../Contracts/DataModels"; import * as ViewModels from "../Contracts/ViewModels"; import { UploadDetailsRecord } from "../Contracts/ViewModels"; +import { classifyError } from "../Metrics/ErrorClassification"; import MetricScenario from "../Metrics/MetricEvents"; import { ApplicationMetricPhase } from "../Metrics/ScenarioConfig"; import { scenarioMonitor } from "../Metrics/ScenarioMonitor"; @@ -367,10 +368,19 @@ export default class Explorer { return; } - const collection: DataModels.Collection = await readCollection(databaseId, collectionId); - const resourceTokenCollection = new ResourceTokenCollection(this, databaseId, collection); - useDatabases.setState({ resourceTokenCollection }); - useSelectedNode.getState().setSelectedNode(resourceTokenCollection); + try { + const collection: DataModels.Collection = await readCollection(databaseId, collectionId); + const resourceTokenCollection = new ResourceTokenCollection(this, databaseId, collection); + useDatabases.setState({ resourceTokenCollection }); + useSelectedNode.getState().setSelectedNode(resourceTokenCollection); + } catch (error) { + scenarioMonitor.failPhase( + MetricScenario.DatabaseLoad, + ApplicationMetricPhase.DatabasesFetched, + classifyError(error), + ); + throw error; + } } public async refreshAllDatabases(): Promise { @@ -412,7 +422,11 @@ export default class Explorer { ); logConsoleError(`Error while refreshing databases: ${errorMessage}`); useDatabases.setState({ databasesFetchedSuccessfully: false }); - scenarioMonitor.failPhase(MetricScenario.DatabaseLoad, ApplicationMetricPhase.DatabasesFetched); + scenarioMonitor.failPhase( + MetricScenario.DatabaseLoad, + ApplicationMetricPhase.DatabasesFetched, + classifyError(error), + ); } } @@ -625,7 +639,11 @@ export default class Explorer { }, startKey, ); - scenarioMonitor.failPhase(MetricScenario.DatabaseLoad, ApplicationMetricPhase.CollectionsLoaded); + scenarioMonitor.failPhase( + MetricScenario.DatabaseLoad, + ApplicationMetricPhase.CollectionsLoaded, + classifyError(error), + ); } } diff --git a/src/Metrics/ErrorClassification.test.ts b/src/Metrics/ErrorClassification.test.ts index e2ab2610a..19e0d789c 100644 --- a/src/Metrics/ErrorClassification.test.ts +++ b/src/Metrics/ErrorClassification.test.ts @@ -1,7 +1,17 @@ import { ARMError } from "../Utils/arm/request"; -import { isExpectedError } from "./ErrorClassification"; +import { classifyError, ErrorCategory, isExpectedError } from "./ErrorClassification"; describe("ErrorClassification", () => { + describe("classifyError", () => { + it("returns a typed expected category", () => { + expect(classifyError({ status: 403 })).toBe(ErrorCategory.Expected); + }); + + it("returns a typed unexpected category", () => { + expect(classifyError({ status: 500 })).toBe(ErrorCategory.Unexpected); + }); + }); + describe("isExpectedError", () => { describe("ARMError with expected codes", () => { it("returns true for AuthorizationFailed code", () => { diff --git a/src/Metrics/ErrorClassification.ts b/src/Metrics/ErrorClassification.ts index 93d1a8ae3..73db7272a 100644 --- a/src/Metrics/ErrorClassification.ts +++ b/src/Metrics/ErrorClassification.ts @@ -51,8 +51,13 @@ interface HttpError { status?: number; } +export enum ErrorCategory { + Expected = "Expected", + Unexpected = "Unexpected", +} + /** - * Determines if an error is an expected failure that should not mark the scenario as unhealthy. + * Classifies whether an error is an expected failure that should not mark the scenario as unhealthy. * * Expected failures include: * - Authentication/authorization errors (user not logged in, permissions) @@ -60,20 +65,20 @@ interface HttpError { * - User-cancelled operations * * @param error - The error to classify - * @returns true if the error is expected and should not affect health metrics + * @returns the health category for the error */ -export function isExpectedError(error: unknown): boolean { +export function classifyError(error: unknown): ErrorCategory { if (!error) { - return false; + return ErrorCategory.Unexpected; } // Check ARMError code if (error instanceof ARMError && error.code !== undefined) { if (typeof error.code === "string" && EXPECTED_ARM_ERROR_CODES.has(error.code)) { - return true; + return ErrorCategory.Expected; } if (typeof error.code === "number" && EXPECTED_HTTP_STATUS_CODES.has(error.code)) { - return true; + return ErrorCategory.Expected; } } @@ -81,7 +86,7 @@ export function isExpectedError(error: unknown): boolean { const msalError = error as MsalAuthError; if (msalError.errorCode && typeof msalError.errorCode === "string") { if (EXPECTED_MSAL_ERROR_CODES.has(msalError.errorCode)) { - return true; + return ErrorCategory.Expected; } } @@ -89,21 +94,28 @@ export function isExpectedError(error: unknown): boolean { const httpError = error as HttpError; if (httpError.status && typeof httpError.status === "number") { if (EXPECTED_HTTP_STATUS_CODES.has(httpError.status)) { - return true; + return ErrorCategory.Expected; } } // Check for firewall error in message (the only message-based check) if (error instanceof Error && error.message) { if (FIREWALL_ERROR_PATTERN.test(error.message)) { - return true; + return ErrorCategory.Expected; } } // Check for string errors with firewall pattern if (typeof error === "string" && FIREWALL_ERROR_PATTERN.test(error)) { - return true; + return ErrorCategory.Expected; } - return false; + return ErrorCategory.Unexpected; +} + +/** + * Determines if an error is an expected failure that should not mark the scenario as unhealthy. + */ +export function isExpectedError(error: unknown): boolean { + return classifyError(error) === ErrorCategory.Expected; } diff --git a/src/Metrics/ScenarioMonitor.test.ts b/src/Metrics/ScenarioMonitor.test.ts index 1bd819e43..7d32bd736 100644 --- a/src/Metrics/ScenarioMonitor.test.ts +++ b/src/Metrics/ScenarioMonitor.test.ts @@ -3,11 +3,11 @@ */ import { updateUserContext } from "../UserContext"; +import { ErrorCategory } from "./ErrorClassification"; import MetricScenario, { reportMetric } from "./MetricEvents"; import { ApplicationMetricPhase, CommonMetricPhase } from "./ScenarioConfig"; import { scenarioMonitor } from "./ScenarioMonitor"; -// Mock the MetricEvents module jest.mock("./MetricEvents", () => ({ __esModule: true, default: { @@ -17,7 +17,6 @@ jest.mock("./MetricEvents", () => ({ reportMetric: jest.fn().mockResolvedValue({ ok: true }), })); -// Mock configContext jest.mock("../ConfigContext", () => ({ configContext: { platform: "Portal", @@ -32,12 +31,32 @@ jest.mock("../ConfigContext", () => ({ })); describe("ScenarioMonitor", () => { + let documentHidden = false; + + const getMetric = (scenario: MetricScenario) => { + const call = (reportMetric as jest.Mock).mock.calls.find(([event]) => event.scenario === scenario); + return call?.[0]; + }; + + const completeApplicationLoad = () => { + scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); + scenarioMonitor.startPhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); + scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); + scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, CommonMetricPhase.Interactive); + }; + + beforeAll(() => { + Object.defineProperty(document, "hidden", { + configurable: true, + get: () => documentHidden, + }); + }); + beforeEach(() => { + documentHidden = false; jest.clearAllMocks(); - // Use legacy fake timers to avoid conflicts with performance API jest.useFakeTimers({ legacyFakeTimers: true }); - // Ensure performance mock is available (setupTests.ts sets this but fake timers may override) if (typeof performance.mark !== "function") { Object.defineProperty(global, "performance", { writable: true, @@ -55,220 +74,211 @@ describe("ScenarioMonitor", () => { }); } - // Reset userContext - updateUserContext({ - apiType: "SQL", - }); - - // Reset the scenario monitor to clear any previous state + updateUserContext({ apiType: "SQL" }); scenarioMonitor.reset(); }); afterEach(() => { - // Reset scenarios before switching to real timers scenarioMonitor.reset(); jest.useRealTimers(); }); - describe("markExpectedFailure", () => { - it("sets hasExpectedFailure flag on active scenarios", () => { - // Start a scenario - scenarioMonitor.start(MetricScenario.ApplicationLoad); - - // Mark expected failure - scenarioMonitor.markExpectedFailure(); - - // Let timeout fire - should emit healthy because of expected failure - jest.advanceTimersByTime(10000); - - expect(reportMetric).toHaveBeenCalledWith( - expect.objectContaining({ - scenario: MetricScenario.ApplicationLoad, - healthy: true, - hasExpectedFailure: true, - timedOut: true, - }), - ); - }); - - it("sets flag on multiple active scenarios", () => { - // Start two scenarios - scenarioMonitor.start(MetricScenario.ApplicationLoad); - scenarioMonitor.start(MetricScenario.DatabaseLoad); - - // Mark expected failure - should affect both - scenarioMonitor.markExpectedFailure(); - - // Let timeouts fire - jest.advanceTimersByTime(10000); - - expect(reportMetric).toHaveBeenCalledTimes(2); - expect(reportMetric).toHaveBeenCalledWith(expect.objectContaining({ healthy: true, hasExpectedFailure: true })); - }); - - it("does not affect already emitted scenarios", () => { - // Start scenario - scenarioMonitor.start(MetricScenario.ApplicationLoad); - - // Complete all phases to emit (PlatformConfigured auto-started, deferred phases need start+complete) - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); - scenarioMonitor.startPhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, CommonMetricPhase.Interactive); - - // Now mark expected failure - should not change anything - scenarioMonitor.markExpectedFailure(); - - // reportMetric was called when phases completed - expect(reportMetric).toHaveBeenCalledTimes(1); - expect(reportMetric).toHaveBeenCalledWith(expect.objectContaining({ healthy: true })); - }); + afterAll(() => { + delete (document as unknown as { hidden?: boolean }).hidden; }); - describe("timeout behavior", () => { - it("emits unhealthy on timeout without expected failure", () => { - scenarioMonitor.start(MetricScenario.ApplicationLoad); + it("does not apply an ApplicationLoad expected error to DatabaseLoad", () => { + scenarioMonitor.start(MetricScenario.ApplicationLoad); + scenarioMonitor.start(MetricScenario.DatabaseLoad); - // Let timeout fire without marking expected failure - jest.advanceTimersByTime(10000); + scenarioMonitor.failPhase( + MetricScenario.ApplicationLoad, + ApplicationMetricPhase.PlatformConfigured, + ErrorCategory.Expected, + ); + jest.advanceTimersByTime(10000); - expect(reportMetric).toHaveBeenCalledWith( - expect.objectContaining({ - scenario: MetricScenario.ApplicationLoad, - healthy: false, - timedOut: true, - }), - ); - }); - - it("emits healthy on timeout with expected failure", () => { - scenarioMonitor.start(MetricScenario.ApplicationLoad); - - // Mark expected failure - scenarioMonitor.markExpectedFailure(); - - // Let timeout fire - jest.advanceTimersByTime(10000); - - expect(reportMetric).toHaveBeenCalledWith( - expect.objectContaining({ - scenario: MetricScenario.ApplicationLoad, - healthy: true, - timedOut: true, - hasExpectedFailure: true, - }), - ); - }); - - it("emits healthy even with partial phase completion and expected failure", () => { - scenarioMonitor.start(MetricScenario.ApplicationLoad); - - // Complete one non-deferred phase - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); - - // Mark expected failure - scenarioMonitor.markExpectedFailure(); - - // Let timeout fire (deferred phases and Interactive not completed) - jest.advanceTimersByTime(10000); - - expect(reportMetric).toHaveBeenCalledWith( - expect.objectContaining({ - healthy: true, - timedOut: true, - hasExpectedFailure: true, - completedPhases: expect.arrayContaining(["PlatformConfigured"]), - }), - ); - }); + expect(getMetric(MetricScenario.ApplicationLoad)).toEqual( + expect.objectContaining({ healthy: true, hasExpectedFailure: true, timedOut: true }), + ); + expect(getMetric(MetricScenario.DatabaseLoad)).toEqual( + expect.objectContaining({ healthy: false, hasExpectedFailure: false, timedOut: true }), + ); }); - describe("failPhase behavior", () => { - it("emits unhealthy immediately on unexpected failure", () => { - scenarioMonitor.start(MetricScenario.DatabaseLoad); + it("emits unhealthy when an unexpected DatabaseLoad phase fails after expected evidence", () => { + scenarioMonitor.start(MetricScenario.DatabaseLoad); + scenarioMonitor.failPhase( + MetricScenario.DatabaseLoad, + ApplicationMetricPhase.DatabasesFetched, + ErrorCategory.Expected, + ); + scenarioMonitor.startPhase(MetricScenario.DatabaseLoad, ApplicationMetricPhase.CollectionsLoaded); - // Fail a phase (simulating unexpected error) - scenarioMonitor.failPhase(MetricScenario.DatabaseLoad, ApplicationMetricPhase.DatabasesFetched); + scenarioMonitor.failPhase( + MetricScenario.DatabaseLoad, + ApplicationMetricPhase.CollectionsLoaded, + ErrorCategory.Unexpected, + ); - // Should emit unhealthy immediately, not wait for timeout - expect(reportMetric).toHaveBeenCalledWith( - expect.objectContaining({ - scenario: MetricScenario.DatabaseLoad, - healthy: false, - timedOut: false, - }), - ); - }); - - it("does not emit twice after failPhase and timeout", () => { - scenarioMonitor.start(MetricScenario.DatabaseLoad); - - // Fail a phase - scenarioMonitor.failPhase(MetricScenario.DatabaseLoad, ApplicationMetricPhase.DatabasesFetched); - - // Let timeout fire - jest.advanceTimersByTime(10000); - - // Should only have emitted once (from failPhase) - expect(reportMetric).toHaveBeenCalledTimes(1); - }); + expect(reportMetric).toHaveBeenCalledTimes(1); + expect(getMetric(MetricScenario.DatabaseLoad)).toEqual( + expect.objectContaining({ healthy: false, hasExpectedFailure: true, timedOut: false }), + ); }); - describe("completePhase behavior", () => { - it("emits healthy when all phases complete", () => { - scenarioMonitor.start(MetricScenario.ApplicationLoad); + it("emits healthy once at timeout after multiple expected failures", () => { + scenarioMonitor.start(MetricScenario.DatabaseLoad); + scenarioMonitor.failPhase( + MetricScenario.DatabaseLoad, + ApplicationMetricPhase.DatabasesFetched, + ErrorCategory.Expected, + ); + scenarioMonitor.startPhase(MetricScenario.DatabaseLoad, ApplicationMetricPhase.CollectionsLoaded); + scenarioMonitor.failPhase( + MetricScenario.DatabaseLoad, + ApplicationMetricPhase.CollectionsLoaded, + ErrorCategory.Expected, + ); - // Complete all required phases (PlatformConfigured + Interactive auto-started, deferred need start) - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); - scenarioMonitor.startPhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, CommonMetricPhase.Interactive); + jest.advanceTimersByTime(10000); - expect(reportMetric).toHaveBeenCalledWith( - expect.objectContaining({ - scenario: MetricScenario.ApplicationLoad, - healthy: true, - timedOut: false, - completedPhases: expect.arrayContaining(["PlatformConfigured", "ExplorerInitialized", "Interactive"]), - }), - ); - }); - - it("does not emit until all phases complete", () => { - scenarioMonitor.start(MetricScenario.ApplicationLoad); - - // Complete only one non-deferred phase - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); - - expect(reportMetric).not.toHaveBeenCalled(); - }); + expect(reportMetric).toHaveBeenCalledTimes(1); + expect(getMetric(MetricScenario.DatabaseLoad)).toEqual( + expect.objectContaining({ healthy: true, hasExpectedFailure: true, timedOut: true }), + ); }); - describe("scenario isolation", () => { - it("expected failure on one scenario does not affect others after completion", () => { - // Start both scenarios - scenarioMonitor.start(MetricScenario.ApplicationLoad); - scenarioMonitor.start(MetricScenario.DatabaseLoad); + it("keeps an expected-erroring required phase pending", () => { + scenarioMonitor.start(MetricScenario.ApplicationLoad); + scenarioMonitor.failPhase( + MetricScenario.ApplicationLoad, + ApplicationMetricPhase.PlatformConfigured, + ErrorCategory.Expected, + ); + scenarioMonitor.startPhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); + scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); + scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, CommonMetricPhase.Interactive); - // Complete ApplicationLoad (all phases including deferred) - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); - scenarioMonitor.startPhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.ExplorerInitialized); - scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, CommonMetricPhase.Interactive); + expect(reportMetric).not.toHaveBeenCalled(); - // Now mark expected failure - should only affect DatabaseLoad - scenarioMonitor.markExpectedFailure(); + jest.advanceTimersByTime(10000); - // Let DatabaseLoad timeout - jest.advanceTimersByTime(10000); + expect(getMetric(MetricScenario.ApplicationLoad)).toEqual( + expect.objectContaining({ + healthy: true, + timedOut: true, + completedPhases: expect.not.arrayContaining([ApplicationMetricPhase.PlatformConfigured]), + }), + ); + }); - // ApplicationLoad emitted healthy on completion - // DatabaseLoad emits healthy on timeout (expected failure) - expect(reportMetric).toHaveBeenCalledTimes(2); - // Both should be healthy - const calls = (reportMetric as jest.Mock).mock.calls; - expect(calls[0][0].healthy).toBe(true); - expect(calls[1][0].healthy).toBe(true); - }); + it("emits healthy with diagnostics when a phase succeeds after expected evidence", () => { + scenarioMonitor.start(MetricScenario.ApplicationLoad); + scenarioMonitor.markExpectedFailure(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); + + completeApplicationLoad(); + + expect(reportMetric).toHaveBeenCalledTimes(1); + expect(getMetric(MetricScenario.ApplicationLoad)).toEqual( + expect.objectContaining({ + healthy: true, + hasExpectedFailure: true, + timedOut: false, + completedPhases: expect.arrayContaining([ + ApplicationMetricPhase.PlatformConfigured, + ApplicationMetricPhase.ExplorerInitialized, + CommonMetricPhase.Interactive, + ]), + }), + ); + }); + + it("keeps an unexpected result unhealthy when expected evidence arrives later", () => { + scenarioMonitor.start(MetricScenario.DatabaseLoad); + scenarioMonitor.failPhase(MetricScenario.DatabaseLoad, ApplicationMetricPhase.DatabasesFetched); + scenarioMonitor.markExpectedFailure(MetricScenario.DatabaseLoad, ApplicationMetricPhase.CollectionsLoaded); + + jest.advanceTimersByTime(10000); + + expect(reportMetric).toHaveBeenCalledTimes(1); + expect(getMetric(MetricScenario.DatabaseLoad)).toEqual( + expect.objectContaining({ healthy: false, hasExpectedFailure: false, timedOut: false }), + ); + }); + + it("treats hidden-at-start and hidden-later timeouts as healthy expected outcomes", () => { + documentHidden = true; + scenarioMonitor.start(MetricScenario.ApplicationLoad); + + documentHidden = false; + scenarioMonitor.start(MetricScenario.DatabaseLoad); + documentHidden = true; + document.dispatchEvent(new Event("visibilitychange")); + documentHidden = false; + + jest.advanceTimersByTime(10000); + + expect(getMetric(MetricScenario.ApplicationLoad)).toEqual( + expect.objectContaining({ healthy: true, hasExpectedFailure: true, timedOut: true }), + ); + expect(getMetric(MetricScenario.DatabaseLoad)).toEqual( + expect.objectContaining({ healthy: true, hasExpectedFailure: true, timedOut: true }), + ); + }); + + it("emits unhealthy when an unexpected failure occurs while hidden", () => { + documentHidden = true; + scenarioMonitor.start(MetricScenario.DatabaseLoad); + + scenarioMonitor.failPhase(MetricScenario.DatabaseLoad, ApplicationMetricPhase.DatabasesFetched); + + expect(reportMetric).toHaveBeenCalledTimes(1); + expect(getMetric(MetricScenario.DatabaseLoad)).toEqual( + expect.objectContaining({ healthy: false, hasExpectedFailure: true, timedOut: false }), + ); + }); + + it("emits unhealthy on a visible timeout without expected evidence", () => { + scenarioMonitor.start(MetricScenario.ApplicationLoad); + + jest.advanceTimersByTime(10000); + + expect(getMetric(MetricScenario.ApplicationLoad)).toEqual( + expect.objectContaining({ healthy: false, hasExpectedFailure: false, timedOut: true }), + ); + }); + + it("does not double emit when callbacks race after timeout", () => { + scenarioMonitor.start(MetricScenario.ApplicationLoad); + scenarioMonitor.markExpectedFailure(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); + jest.advanceTimersByTime(10000); + + completeApplicationLoad(); + scenarioMonitor.failPhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); + + expect(reportMetric).toHaveBeenCalledTimes(1); + expect(getMetric(MetricScenario.ApplicationLoad)).toEqual( + expect.objectContaining({ healthy: true, timedOut: true }), + ); + }); + + it("reset removes visibility listeners and clears scenario state", () => { + const removeEventListener = jest.spyOn(document, "removeEventListener"); + scenarioMonitor.start(MetricScenario.ApplicationLoad); + scenarioMonitor.markExpectedFailure(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); + + scenarioMonitor.reset(); + + expect(removeEventListener).toHaveBeenCalledWith("visibilitychange", expect.any(Function)); + + scenarioMonitor.start(MetricScenario.ApplicationLoad); + jest.advanceTimersByTime(10000); + + expect(reportMetric).toHaveBeenCalledTimes(1); + expect(getMetric(MetricScenario.ApplicationLoad)).toEqual( + expect.objectContaining({ healthy: false, hasExpectedFailure: false, timedOut: true }), + ); + removeEventListener.mockRestore(); }); }); diff --git a/src/Metrics/ScenarioMonitor.ts b/src/Metrics/ScenarioMonitor.ts index c0fc28b2c..f13f3ca0b 100644 --- a/src/Metrics/ScenarioMonitor.ts +++ b/src/Metrics/ScenarioMonitor.ts @@ -1,9 +1,11 @@ import type { PhaseTimings, WebVitals } from "Metrics/Constants"; import { Metric, onCLS, onFCP, onINP, onLCP, onTTFB } from "web-vitals"; +import { stringifyError } from "../Common/stringifyError"; import { configContext } from "../ConfigContext"; import { Action } from "../Shared/Telemetry/TelemetryConstants"; import { traceFailure, traceMark, traceStart, traceSuccess } from "../Shared/Telemetry/TelemetryProcessor"; import { userContext } from "../UserContext"; +import { ErrorCategory } from "./ErrorClassification"; import MetricScenario, { reportMetric } from "./MetricEvents"; import { scenarioConfigs } from "./MetricScenarioConfigs"; import { MetricPhase, ScenarioConfig, ScenarioContextSnapshot } from "./ScenarioConfig"; @@ -23,12 +25,27 @@ interface InternalScenarioContext { timeoutId?: number; emitted: boolean; hasExpectedFailure: boolean; // Flag for expected failures (auth, firewall, etc.) + expectedFailurePhases: Set; + documentWasHidden: boolean; } class ScenarioMonitor { private contexts = new Map(); private vitals: WebVitals = {}; private vitalsInitialized = false; + private visibilityListenerAttached = false; + + private readonly handleVisibilityChange = () => { + if (!document.hidden) { + return; + } + + this.contexts.forEach((ctx) => { + if (!ctx.emitted) { + this.markDocumentHidden(ctx); + } + }); + }; constructor() { this.initializeVitals(); @@ -85,8 +102,16 @@ class ScenarioMonitor { phases: new Map(), emitted: false, hasExpectedFailure: false, + expectedFailurePhases: new Set(), + documentWasHidden: false, }; + this.contexts.set(scenario, ctx); + this.ensureVisibilityListener(); + if (document.hidden) { + this.markDocumentHidden(ctx); + } + // Start all required phases at scenario start time, except deferred ones const deferredSet = new Set(config.deferredPhases ?? []); config.requiredPhases.forEach((phase) => { @@ -110,12 +135,17 @@ class ScenarioMonitor { }); ctx.timeoutId = window.setTimeout(() => { + if (document.hidden) { + this.markDocumentHidden(ctx); + } const missingPhases = ctx.config.requiredPhases.filter((p) => !ctx.completed.has(p)); this.devLog( `timeout: ${scenario} | missing=[${missingPhases.join(", ")}] | completed=[${Array.from(ctx.completed).join( ", ", - )}] | documentHidden=${document.hidden} | hasExpectedFailure=${ctx.hasExpectedFailure}`, + )}] | expected=[${Array.from(ctx.expectedFailurePhases).join(", ")}] | documentHidden=${ + document.hidden + } | hasExpectedFailure=${ctx.hasExpectedFailure}`, ); traceMark(Action.MetricsScenario, { @@ -123,15 +153,15 @@ class ScenarioMonitor { scenario, missingPhases: missingPhases.join(","), completedPhases: Array.from(ctx.completed).join(","), + expectedFailurePhases: Array.from(ctx.expectedFailurePhases).join(","), documentHidden: document.hidden, hasExpectedFailure: ctx.hasExpectedFailure, }); - // If an expected failure occurred (auth, firewall, etc.), emit healthy instead of unhealthy - const healthy = ctx.hasExpectedFailure; + // Expected-only timeouts are healthy. Visible timeouts without expected evidence are unhealthy. + const healthy = ctx.hasExpectedFailure && ctx.failed.size === 0; this.emit(ctx, healthy, true); }, config.timeoutMs); - this.contexts.set(scenario, ctx); } startPhase(scenario: MetricScenario, phase: MetricPhase) { @@ -220,16 +250,14 @@ class ScenarioMonitor { this.tryEmitIfReady(ctx); } - failPhase(scenario: MetricScenario, phase: MetricPhase) { + failPhase(scenario: MetricScenario, phase: MetricPhase, category: ErrorCategory = ErrorCategory.Unexpected) { const ctx = this.contexts.get(scenario); - if (!ctx || ctx.emitted) { + if (!ctx || ctx.emitted || !ctx.config.requiredPhases.includes(phase)) { return; } - // If an expected failure was flagged (auth, firewall, etc.), treat as success. - if (ctx.hasExpectedFailure) { - this.devLog(`phase_fail: ${scenario}.${phase} — expected failure, completing as healthy`); - this.completePhase(scenario, phase); + if (category === ErrorCategory.Expected) { + this.markExpectedFailure(scenario, phase); return; } @@ -267,22 +295,61 @@ class ScenarioMonitor { /** * Marks that an expected failure occurred (auth, firewall, permissions, etc.). - * When the scenario times out with this flag set, it will emit healthy instead of unhealthy. - * This is called automatically from handleError when an expected error is detected. + * Expected evidence is scoped to one active scenario and phase. It does not complete + * the phase; the phase remains pending until it genuinely succeeds or the scenario times out. */ - markExpectedFailure() { - // Set the flag on all active (non-emitted) scenarios - this.contexts.forEach((ctx) => { - if (!ctx.emitted) { - ctx.hasExpectedFailure = true; - traceMark(Action.MetricsScenario, { - event: "expected_failure_marked", - scenario: ctx.scenario, - }); - } + markExpectedFailure(scenario: MetricScenario, phase: MetricPhase) { + const ctx = this.contexts.get(scenario); + if (!ctx || ctx.emitted || !ctx.config.requiredPhases.includes(phase)) { + return; + } + + ctx.hasExpectedFailure = true; + if (ctx.expectedFailurePhases.has(phase)) { + return; + } + ctx.expectedFailurePhases.add(phase); + this.devLog(`expected_failure: ${scenario}.${phase}`); + traceMark(Action.MetricsScenario, { + event: "expected_failure_marked", + scenario, + phase, }); } + private markDocumentHidden(ctx: InternalScenarioContext) { + if (ctx.documentWasHidden) { + return; + } + + ctx.documentWasHidden = true; + ctx.hasExpectedFailure = true; + this.devLog(`expected_failure: ${ctx.scenario} | document hidden`); + traceMark(Action.MetricsScenario, { + event: "expected_failure_marked", + scenario: ctx.scenario, + reason: "document_hidden", + }); + } + + private ensureVisibilityListener() { + if (this.visibilityListenerAttached) { + return; + } + + document.addEventListener("visibilitychange", this.handleVisibilityChange); + this.visibilityListenerAttached = true; + } + + private removeVisibilityListenerIfIdle() { + if (!this.visibilityListenerAttached || Array.from(this.contexts.values()).some((ctx) => !ctx.emitted)) { + return; + } + + document.removeEventListener("visibilitychange", this.handleVisibilityChange); + this.visibilityListenerAttached = false; + } + private tryEmitIfReady(ctx: InternalScenarioContext) { const allDone = ctx.config.requiredPhases.every((p) => ctx.completed.has(p)); if (!allDone) { @@ -348,6 +415,7 @@ class ScenarioMonitor { durationMs: finalSnapshot.durationMs, completedPhases: finalSnapshot.completed.join(","), failedPhases: finalSnapshot.failedPhases?.join(","), + expectedFailurePhases: Array.from(ctx.expectedFailurePhases).join(","), lcp: finalSnapshot.vitals?.lcp, inp: finalSnapshot.vitals?.inp, cls: finalSnapshot.vitals?.cls, @@ -384,10 +452,17 @@ class ScenarioMonitor { startTimeISO: finalSnapshot.startTimeISO, endTimeISO: finalSnapshot.endTimeISO, vitals: finalSnapshot.vitals, + }).catch((error: unknown) => { + traceFailure(Action.MetricsScenario, { + event: "scenario_report_failure", + scenario: ctx.scenario, + error: error instanceof Error ? stringifyError(error) : String(error), + }); }); // Cleanup performance entries this.cleanupPerformanceEntries(ctx); + this.removeVisibilityListenerIfIdle(); } private cleanupPerformanceEntries(ctx: InternalScenarioContext) { @@ -444,8 +519,14 @@ class ScenarioMonitor { if (ctx.timeoutId) { clearTimeout(ctx.timeoutId); } + this.cleanupPerformanceEntries(ctx); }); this.contexts.clear(); + if (this.visibilityListenerAttached) { + document.removeEventListener("visibilitychange", this.handleVisibilityChange); + this.visibilityListenerAttached = false; + } + this.vitals = {}; } } diff --git a/src/Utils/AuthorizationUtils.ts b/src/Utils/AuthorizationUtils.ts index 0938c6f33..092364ed5 100644 --- a/src/Utils/AuthorizationUtils.ts +++ b/src/Utils/AuthorizationUtils.ts @@ -9,8 +9,6 @@ import * as Logger from "../Common/Logger"; import { configContext } from "../ConfigContext"; import { DatabaseAccount } from "../Contracts/DataModels"; import * as ViewModels from "../Contracts/ViewModels"; -import { isExpectedError } from "../Metrics/ErrorClassification"; -import { scenarioMonitor } from "../Metrics/ScenarioMonitor"; import { trace, traceFailure, traceStart, traceSuccess } from "../Shared/Telemetry/TelemetryProcessor"; import { UserContext, userContext } from "../UserContext"; @@ -158,10 +156,6 @@ export async function acquireMsalTokenForAccount( errorMessage: stringifyError(error), }); traceFailure(Action.AcquireMsalToken, { error: stringifyError(error) }, msalStartKey); - // Mark expected failure for health metrics so timeout emits healthy - if (isExpectedError(error)) { - scenarioMonitor.markExpectedFailure(); - } throw error; } } else { @@ -205,10 +199,6 @@ export async function acquireTokenWithMsal( acquireTokenType: "interactive", errorMessage: JSON.stringify(interactiveError), }); - // Mark expected failure for health metrics so timeout emits healthy - if (isExpectedError(interactiveError)) { - scenarioMonitor.markExpectedFailure(); - } throw interactiveError; } } else { @@ -217,10 +207,6 @@ export async function acquireTokenWithMsal( acquireTokenType: "silent", errorMessage: JSON.stringify(silentError), }); - // Mark expected failure for health metrics so timeout emits healthy - if (isExpectedError(silentError)) { - scenarioMonitor.markExpectedFailure(); - } throw silentError; } } diff --git a/src/hooks/useKnockoutExplorer.ts b/src/hooks/useKnockoutExplorer.ts index 41bec9797..508ca3169 100644 --- a/src/hooks/useKnockoutExplorer.ts +++ b/src/hooks/useKnockoutExplorer.ts @@ -47,6 +47,7 @@ import { HostedExplorerChildFrame, ResourceToken, } from "../HostedExplorerChildFrame"; +import { classifyError } from "../Metrics/ErrorClassification"; import MetricScenario from "../Metrics/MetricEvents"; import { ApplicationMetricPhase } from "../Metrics/ScenarioConfig"; import { scenarioMonitor } from "../Metrics/ScenarioMonitor"; @@ -83,6 +84,8 @@ export function useKnockoutExplorer(platform: Platform): Explorer { useEffect(() => { const effect = async () => { if (platform) { + scenarioMonitor.start(MetricScenario.ApplicationLoad); + //Updating phoenix feature flags for MPAC based of config context if (configContext.isPhoenixEnabled === true) { userContext.features.phoenixNotebooks = true; @@ -101,7 +104,11 @@ export function useKnockoutExplorer(platform: Platform): Explorer { explorer = await configureFabric(); } } catch (error) { - scenarioMonitor.failPhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); + scenarioMonitor.failPhase( + MetricScenario.ApplicationLoad, + ApplicationMetricPhase.PlatformConfigured, + classifyError(error), + ); throw error; } scenarioMonitor.completePhase(MetricScenario.ApplicationLoad, ApplicationMetricPhase.PlatformConfigured); @@ -363,6 +370,11 @@ async function configureHostedWithAAD(config: AAD): Promise { authority: `${configContext.AAD_ENDPOINT}${cachedTenantId}`, }); } catch (authError) { + scenarioMonitor.failPhase( + MetricScenario.ApplicationLoad, + ApplicationMetricPhase.PlatformConfigured, + classifyError(authError), + ); logConsoleError("Failed to acquire authorization token: " + authError); } } @@ -682,14 +694,18 @@ export async function fetchAndUpdateKeys(subscriptionId: string, resourceGroup: }); let keys; try { - keys = await listKeys(subscriptionId, resourceGroup, account); - Logger.logInfo(`Keys fetched for ${userContext.apiType} account ${account}`, "Explorer/fetchAndUpdateKeys"); - updateUserContext({ - masterKey: keys.primaryMasterKey, - }); - traceSuccess(Action.FetchAccountKeys, { accountName: account }, startKey); - } catch (error) { - if (error.code === "AuthorizationFailed") { + try { + keys = await listKeys(subscriptionId, resourceGroup, account); + Logger.logInfo(`Keys fetched for ${userContext.apiType} account ${account}`, "Explorer/fetchAndUpdateKeys"); + updateUserContext({ + masterKey: keys.primaryMasterKey, + }); + traceSuccess(Action.FetchAccountKeys, { accountName: account }, startKey); + } catch (error) { + if (error.code !== "AuthorizationFailed") { + throw error; + } + keys = await getReadOnlyKeys(subscriptionId, resourceGroup, account); Logger.logInfo( `Read only Keys fetched for ${userContext.apiType} account ${account}`, @@ -699,15 +715,20 @@ export async function fetchAndUpdateKeys(subscriptionId: string, resourceGroup: masterKey: keys.primaryReadonlyMasterKey, }); traceSuccess(Action.FetchAccountKeys, { accountName: account, fallbackToReadOnly: true }, startKey); - } else { - logConsoleError(`Error occurred fetching keys for the account." ${error.message}`); - Logger.logError( - `Error during fetching keys or updating user context: ${error} for ${userContext.apiType} account ${account}`, - "Explorer/fetchAndUpdateKeys", - ); - traceFailure(Action.FetchAccountKeys, { accountName: account, error: error.message }, startKey); - throw error; } + } catch (error) { + logConsoleError(`Error occurred fetching keys for the account." ${error.message}`); + Logger.logError( + `Error during fetching keys or updating user context: ${error} for ${userContext.apiType} account ${account}`, + "Explorer/fetchAndUpdateKeys", + ); + traceFailure(Action.FetchAccountKeys, { accountName: account, error: error.message }, startKey); + scenarioMonitor.failPhase( + MetricScenario.ApplicationLoad, + ApplicationMetricPhase.PlatformConfigured, + classifyError(error), + ); + throw error; } } @@ -810,6 +831,11 @@ async function configurePortal(): Promise { updateUserContext({ aadToken: aadToken }); useDataPlaneRbac.setState({ aadTokenUpdated: true }); } catch (authError) { + scenarioMonitor.failPhase( + MetricScenario.ApplicationLoad, + ApplicationMetricPhase.PlatformConfigured, + classifyError(authError), + ); Logger.logWarning( `Failed to silently acquire authorization token from MSAL: ${authError} for ${userContext.apiType} account ${account}`, "Explorer/configurePortal",