From c941f3d78b0c0cb4ce7790d34f65d9b71ffeaa70 Mon Sep 17 00:00:00 2001 From: Max Mykal Date: Tue, 5 Mar 2024 13:19:21 -0800 Subject: [PATCH] improve error display for failed plan --- .../components/editor/EditorPreview.tsx | 1 - .../src/library/components/plan/Plan.tsx | 5 +- .../components/plan/PlanApplyStageTracker.tsx | 202 +++++++++++------- .../src/library/components/plan/help.ts | 5 +- .../pages/root/context/notificationCenter.tsx | 19 +- web/server/api/endpoints/commands.py | 31 ++- web/server/api/endpoints/plan.py | 150 ++++++++----- web/server/console.py | 51 +++-- web/server/utils.py | 14 +- 9 files changed, 308 insertions(+), 170 deletions(-) diff --git a/web/client/src/library/components/editor/EditorPreview.tsx b/web/client/src/library/components/editor/EditorPreview.tsx index e28597649d..d599330169 100644 --- a/web/client/src/library/components/editor/EditorPreview.tsx +++ b/web/client/src/library/components/editor/EditorPreview.tsx @@ -126,7 +126,6 @@ export default function EditorPreview({ includes( [ EnumErrorKey.Fetchdf, - EnumErrorKey.General, EnumErrorKey.EvaluateModel, EnumErrorKey.RenderQuery, EnumErrorKey.ColumnLineage, diff --git a/web/client/src/library/components/plan/Plan.tsx b/web/client/src/library/components/plan/Plan.tsx index 99b130e4a0..e62350eedf 100644 --- a/web/client/src/library/components/plan/Plan.tsx +++ b/web/client/src/library/components/plan/Plan.tsx @@ -50,10 +50,10 @@ function Plan(): JSX.Element { function reset(): void { setPlanAction(new ModelPlanAction({ value: EnumPlanAction.Run })) - resetPlanTrackers() clearPlanApply() - cleanUp() + + setTimeout(() => resetPlanTrackers(), 500) } function cancel(): void { @@ -78,7 +78,6 @@ function Plan(): JSX.Element { function apply(): void { setPlanAction(new ModelPlanAction({ value: EnumPlanAction.Applying })) - resetPlanTrackers() clearPlanApply() dispatch([{ type: EnumPlanActions.ResetTestsReport }]) diff --git a/web/client/src/library/components/plan/PlanApplyStageTracker.tsx b/web/client/src/library/components/plan/PlanApplyStageTracker.tsx index 8fdf2ea27c..1b8f0c8df0 100644 --- a/web/client/src/library/components/plan/PlanApplyStageTracker.tsx +++ b/web/client/src/library/components/plan/PlanApplyStageTracker.tsx @@ -1,9 +1,6 @@ import { Disclosure, Transition } from '@headlessui/react' -import { - MinusCircleIcon, - PlusCircleIcon, - ExclamationCircleIcon, -} from '@heroicons/react/24/solid' +import { MinusCircleIcon, PlusCircleIcon } from '@heroicons/react/24/solid' +import { ExclamationCircleIcon } from '@heroicons/react/24/outline' import { CheckIcon } from '@heroicons/react/20/solid' import clsx from 'clsx' import { useEffect, useMemo, useRef, useState } from 'react' @@ -16,6 +13,8 @@ import { isNil, toDateFormat, isFalse, + includes, + isFalseOrNil, } from '../../../utils' import { EnumPlanChangeType, usePlan } from './context' import { getPlanOverviewDetails } from './help' @@ -45,11 +44,8 @@ export default function PlanApplyStageTracker(): JSX.Element { const planCancel = useStorePlan(s => s.planCancel) const planAction = useStorePlan(s => s.planAction) - const { plan_options } = getPlanOverviewDetails( - planApply, - planOverview, - planCancel, - ) + const { plan_options, hasChanges, hasBackfills, isFailed } = + getPlanOverviewDetails(planApply, planOverview, planCancel) const hasTestsDetails = isNotNil(tests) && Boolean(tests.total) const showTestsMessage = @@ -60,6 +56,11 @@ export default function PlanApplyStageTracker(): JSX.Element { isFalse(planAction.isProcessing) && isFalse(planAction.isDone) && isFalse(planCancel.isFinished) + const showFailedMessage = + isFailed && + [hasFailedTests, hasTestsDetails, hasChanges, hasBackfills].every( + isFalseOrNil, + ) return ( - {isTrue(plan_options?.skip_tests) ? ( + {showFailedMessage ? ( - - Tests Skipped + + Plan Failed - ) : hasTestsDetails ? ( - - ) : showTestsMessage ? ( - ) : ( - - - No Tests - - )} - {isFalse(hasFailedTests) && ( <> - - - - {planApply.shouldShowEvaluation && ( - + + Plan Failed + + )} + {isTrue(plan_options?.skip_tests) ? ( + - - - - - + + + Tests Skipped + + + ) : hasTestsDetails ? ( + + ) : showTestsMessage ? ( + + ) : ( + + + No Tests + + )} + {isFalse(hasFailedTests) && ( + <> + + + + {planApply.shouldShowEvaluation && ( + + + + + + + )} + )} )} @@ -136,7 +162,9 @@ function StageChanges({ isOpen = false }: { isOpen?: boolean }): JSX.Element { planCancel, ) const tempMeta = stageChanges?.meta ?? meta - const showChanges = tempMeta?.status === Status.init || isTrue(hasChanges) + const showChanges = + includes([Status.init, Status.fail], stageChanges?.meta?.status) || + isTrue(hasChanges) return showChanges ? ( + ) : (
- {isNotNil(start) && ( - <> - - Evaluation started at{' '} - {toDateFormat(new Date(start), 'yyyy-mm-dd hh-mm-ss')} - - - - )} + + Evaluation started at{' '} + {toDateFormat(new Date(start), 'yyyy-mm-dd hh-mm-ss')} + +
{children}
{isNotNil(end) && ( <> @@ -319,9 +345,9 @@ function StageEvaluate({ function StageCreation({ isOpen }: { isOpen?: boolean }): JSX.Element { const planApply = useStorePlan(s => s.planApply) - if (isNil(planApply.stageCreation)) return <> - - return ( + return isNil(planApply.stageCreation) ? ( + <> + ) : ( + const showStageBackfill = isNotNil(stageBackfill) && isNotNil(environment) - return ( + return showStageBackfill ? ( + ) : ( + <> ) } @@ -515,12 +543,19 @@ function StageVirtualUpdate(): JSX.Element { const planApply = useStorePlan(s => s.planApply) const planOverview = useStorePlan(s => s.planOverview) + const planCancel = useStorePlan(s => s.planCancel) + + const { isFailed } = getPlanOverviewDetails( + planApply, + planOverview, + planCancel, + ) const isVirtualUpdate = planApply.overview?.isVirtualUpdate ?? planOverview.isVirtualUpdate const isUpdated = isTrue(planApply.isFinished) - return isVirtualUpdate ? ( + return isVirtualUpdate && isFalse(isFailed) ? ( s.planOverview) const planApply = useStorePlan(s => s.planApply) - const hasChildren = isNotNil(panel) || isNotNil(children) + const hasChildren = + (meta?.status !== Status.fail && isNotNil(panel)) || isNotNil(children) const [open, setOpen] = useState(isOpen) @@ -665,19 +701,21 @@ function Stage({ setOpen(isOpen && hasChildren) }, [isOpen, hasChildren]) - const variant = - meta?.status === Status.success - ? EnumVariant.Success - : meta?.status === Status.fail - ? EnumVariant.Danger - : EnumVariant.Info + const isStatusFail = meta?.status === Status.fail + const isStatusSuccess = meta?.status === Status.success + const isStatusInit = meta?.status === Status.init + + const variant = isStatusSuccess + ? EnumVariant.Success + : isStatusFail + ? EnumVariant.Danger + : EnumVariant.Info const [titleSuccess, titleFail, titleDefault] = states - const text = - meta?.status === Status.success - ? titleSuccess - : meta?.status === Status.fail - ? titleFail - : titleDefault + const text = isStatusSuccess + ? titleSuccess + : isStatusFail + ? titleFail + : titleDefault return ( {isNil(trigger) ? ( <> - {meta?.status === Status.init && ( + {isStatusInit && ( )} - {meta?.status !== Status.init && ( + {isFalse(isStatusInit) && (
- {showDetails ? ( + {showDetails && hasChildren ? ( <> {open ? ( diff --git a/web/client/src/library/components/plan/help.ts b/web/client/src/library/components/plan/help.ts index 8e2a752ee9..4a96d6f26a 100644 --- a/web/client/src/library/components/plan/help.ts +++ b/web/client/src/library/components/plan/help.ts @@ -24,7 +24,9 @@ type PlanOverviewDetails = Pick< | 'stageValidation' | 'stageChanges' | 'stageBackfills' -> +> & { + isFailed: boolean +} export function getPlanOverviewDetails( planApply: ModelPlanApplyTracker, @@ -56,5 +58,6 @@ export function getPlanOverviewDetails( stageValidation: plan.stageValidation, stageBackfills: plan.stageBackfills, stageChanges: plan.stageChanges, + isFailed: overview.isFailed || planCancel.isFailed || planApply.isFailed, } } diff --git a/web/client/src/library/pages/root/context/notificationCenter.tsx b/web/client/src/library/pages/root/context/notificationCenter.tsx index 7e88c1038c..e6f9d32397 100644 --- a/web/client/src/library/pages/root/context/notificationCenter.tsx +++ b/web/client/src/library/pages/root/context/notificationCenter.tsx @@ -1,5 +1,5 @@ import { type ApiExceptionPayload } from '@api/client' -import { isString, uid } from '@utils/index' +import { uid } from '@utils/index' import { createContext, type ReactNode, useState, useContext } from 'react' export const EnumErrorKey = { @@ -89,14 +89,15 @@ export default function NotificationCenterProvider({ } function removeError(error: ErrorIDE | ErrorKey): void { - setErrors( - errors => - new Set( - Array.from(errors).filter(e => - isString(error) ? e.key !== error : e !== error, - ), - ), - ) + setErrors(errors => { + errors.forEach(err => { + if (err === error || err.key === error) { + errors.delete(err) + } + }) + + return new Set(errors) + }) } function clearErrors(): void { diff --git a/web/server/api/endpoints/commands.py b/web/server/api/endpoints/commands.py index de1ecde70e..7f24e58460 100644 --- a/web/server/api/endpoints/commands.py +++ b/web/server/api/endpoints/commands.py @@ -41,7 +41,7 @@ async def initiate_apply( request.app.state.circuit_breaker.clear() request.app.state.task = asyncio.create_task( run_in_executor( - _run_plan_apply, + run_plan_apply, context, environment, plan_options, @@ -187,6 +187,33 @@ async def test( ) +def run_plan_apply( + context: Context, + environment: t.Optional[str] = None, + plan_options: t.Optional[models.PlanOptions] = None, + plan_dates: t.Optional[models.PlanDates] = None, + categories: t.Optional[t.Dict[str, SnapshotChangeCategory]] = None, + circuit_breaker: t.Optional[t.Callable[[], bool]] = None, +) -> None: + try: + _run_plan_apply( + context, + environment, + plan_options, + plan_dates, + categories, + circuit_breaker, + ) + except ApiException as e: + raise e + except Exception as e: + raise ApiException( + message="Unable to apply a plan", + origin="API -> plan -> initiate_apply", + ) from e + return None + + def _run_plan_apply( context: Context, environment: t.Optional[str] = None, @@ -212,7 +239,7 @@ def _run_plan_apply( api_console.stop_plan_tracker(tracker_apply, success=False) raise ApiException( message=str(e), - origin="API -> commands -> apply", + origin="API -> commands -> _run_plan_apply", ) return None diff --git a/web/server/api/endpoints/plan.py b/web/server/api/endpoints/plan.py index c7aab068fe..569c8905b0 100644 --- a/web/server/api/endpoints/plan.py +++ b/web/server/api/endpoints/plan.py @@ -81,66 +81,15 @@ def get_plan_builder( plan_dates: t.Optional[models.PlanDates] = None, categories: t.Optional[t.Dict[str, SnapshotChangeCategory]] = None, ) -> PlanBuilder: - tracker = models.PlanOverviewStageTracker(environment=environment, plan_options=plan_options) - api_console.start_plan_tracker(tracker) - tracker_stage_validate = models.PlanStageValidation() - tracker.add_stage(stage=models.PlanStage.validation, data=tracker_stage_validate) try: - plan_builder = context.plan_builder( - environment=environment, - include_unmodified=plan_options.include_unmodified, - start=plan_dates.start if plan_dates else None, - end=plan_dates.end if plan_dates else None, - create_from=plan_options.create_from, - skip_tests=plan_options.skip_tests, - restate_models=plan_options.restate_models, - no_gaps=plan_options.no_gaps, - skip_backfill=plan_options.skip_backfill, - forward_only=plan_options.forward_only, - no_auto_categorization=plan_options.no_auto_categorization, - ) - plan = plan_builder.build() - tracker.start = plan.start - tracker.end = plan.end - if categories: - for new, _ in plan.context_diff.modified_snapshots.values(): - if plan.is_new_snapshot(new) and new.name in categories: - plan_builder.set_choice(new, categories[new.name]) - plan = plan_builder.build() - tracker_stage_validate.stop(success=True) - api_console.log_event_plan_overview() - except Exception: - tracker_stage_validate.stop(success=False) - tracker.stop(success=False) - api_console.log_event_plan_overview() + return _get_plan_builder(context, plan_options, environment, plan_dates, categories) + except ApiException as e: + raise e + except Exception as e: raise ApiException( message="Unable to run a plan", - origin="API -> plan -> run_plan", - ) - - tracker_stage_changes = models.PlanStageChanges() - tracker.add_stage(stage=models.PlanStage.changes, data=tracker_stage_changes) - if plan.context_diff.has_changes: - changes = _get_plan_changes(context, plan) - tracker_stage_changes.update( - { - "added": changes.added, - "removed": changes.removed, - "modified": changes.modified, - } - ) - tracker_stage_changes.stop(success=True) - api_console.log_event_plan_overview() - tracker_stage_backfills = models.PlanStageBackfills() - tracker.add_stage(stage=models.PlanStage.backfills, data=tracker_stage_backfills) - if plan.requires_backfill: - tracker_stage_backfills.update( - _get_plan_backfills(context, plan), - ) - tracker_stage_backfills.stop(success=True) - api_console.log_event_plan_overview() - api_console.stop_plan_tracker(tracker) - return plan_builder + origin="API -> plan -> initiate_plan", + ) from e def _get_plan_changes(context: Context, plan: Plan) -> models.PlanChanges: @@ -206,3 +155,90 @@ def _get_plan_backfills(context: Context, plan: Plan) -> t.Dict[str, t.Any]: for interval in plan.missing_intervals ] } + + +def _get_plan_builder( + context: Context, + plan_options: models.PlanOptions, + environment: t.Optional[str] = None, + plan_dates: t.Optional[models.PlanDates] = None, + categories: t.Optional[t.Dict[str, SnapshotChangeCategory]] = None, +) -> PlanBuilder: + tracker = models.PlanOverviewStageTracker(environment=environment, plan_options=plan_options) + api_console.start_plan_tracker(tracker) + tracker_stage_validate = models.PlanStageValidation() + tracker.add_stage(stage=models.PlanStage.validation, data=tracker_stage_validate) + try: + plan_builder = context.plan_builder( + environment=environment, + include_unmodified=plan_options.include_unmodified, + start=plan_dates.start if plan_dates else None, + end=plan_dates.end if plan_dates else None, + create_from=plan_options.create_from, + skip_tests=plan_options.skip_tests, + restate_models=plan_options.restate_models, + no_gaps=plan_options.no_gaps, + skip_backfill=plan_options.skip_backfill, + forward_only=plan_options.forward_only, + no_auto_categorization=plan_options.no_auto_categorization, + ) + plan = plan_builder.build() + tracker.start = plan.start + tracker.end = plan.end + if categories: + for new, _ in plan.context_diff.modified_snapshots.values(): + if plan.is_new_snapshot(new) and new.name in categories: + plan_builder.set_choice(new, categories[new.name]) + plan = plan_builder.build() + tracker_stage_validate.stop(success=True) + api_console.log_event_plan_overview() + except Exception: + tracker_stage_validate.stop(success=False) + tracker.stop(success=False) + api_console.log_event_plan_overview() + raise ApiException( + message="Unable to run a plan", + origin="API -> plan -> run_plan", + ) + + tracker_stage_changes = models.PlanStageChanges() + tracker.add_stage(stage=models.PlanStage.changes, data=tracker_stage_changes) + try: + if plan.context_diff.has_changes: + changes = _get_plan_changes(context, plan) + tracker_stage_changes.update( + { + "added": changes.added, + "removed": changes.removed, + "modified": changes.modified, + } + ) + except Exception: + tracker_stage_changes.stop(success=False) + tracker.stop(success=False) + api_console.log_event_plan_overview() + raise ApiException( + message="Unable to get plan changes", + origin="API -> plan -> run_plan", + ) + tracker_stage_changes.stop(success=True) + api_console.log_event_plan_overview() + tracker_stage_backfills = models.PlanStageBackfills() + tracker.add_stage(stage=models.PlanStage.backfills, data=tracker_stage_backfills) + try: + if plan.requires_backfill: + tracker_stage_backfills.update( + _get_plan_backfills(context, plan), + ) + except Exception: + tracker_stage_backfills.stop(success=False) + tracker.stop(success=False) + api_console.log_event_plan_overview() + raise ApiException( + message="Unable to get plan backfills", + origin="API -> plan -> run_plan", + ) + tracker_stage_backfills.stop(success=True) + api_console.log_event_plan_overview() + api_console.stop_plan_tracker(tracker) + return plan_builder diff --git a/web/server/console.py b/web/server/console.py index 44b503a165..d2fa5fa9d5 100644 --- a/web/server/console.py +++ b/web/server/console.py @@ -37,7 +37,7 @@ def start_plan_evaluation(self, plan: Plan) -> None: def stop_plan_evaluation(self) -> None: if self.plan_apply_stage_tracker: - self.stop_plan_tracker(tracker=self.plan_apply_stage_tracker, success=True) + self.stop_plan_tracker(self.plan_apply_stage_tracker, True) def start_creation_progress( self, @@ -210,17 +210,20 @@ def stop_plan_tracker( success: bool = True, ) -> None: if isinstance(tracker, models.PlanApplyStageTracker) and self.plan_apply_stage_tracker: - self.plan_apply_stage_tracker.stop(success=success) + self.stop_plan_tracker_stages(self.plan_apply_stage_tracker, False) + self.plan_apply_stage_tracker.stop(success) self.log_event_plan_apply() self.plan_apply_stage_tracker = None elif ( isinstance(tracker, models.PlanOverviewStageTracker) and self.plan_overview_stage_tracker ): - self.plan_overview_stage_tracker.stop(success=success) + self.stop_plan_tracker_stages(self.plan_overview_stage_tracker, False) + self.plan_overview_stage_tracker.stop(success) self.log_event_plan_overview() elif isinstance(tracker, models.PlanCancelStageTracker) and self.plan_cancel_stage_tracker: - self.plan_cancel_stage_tracker.stop(success=success) + self.stop_plan_tracker_stages(self.plan_cancel_stage_tracker, False) + self.plan_cancel_stage_tracker.stop(success) self.log_event_plan_cancel() self.plan_cancel_stage_tracker = None @@ -289,24 +292,46 @@ def log_event_plan_cancel(self) -> None: data=self.plan_cancel_stage_tracker.dict() if self.plan_cancel_stage_tracker else {}, ) - def log_exception(self) -> None: + def log_exception(self, exception: ApiException) -> None: self.log_event( event=models.EventName.ERRORS, - data=ApiException( - message="Tasks failed to run", - origin="API -> console -> log_exception", - ).to_dict(), + data=exception.to_dict(), ) if self.plan_overview_stage_tracker: - self.stop_plan_tracker(tracker=self.plan_overview_stage_tracker, success=False) + self.stop_plan_tracker(self.plan_overview_stage_tracker, False) if self.plan_apply_stage_tracker: - self.stop_plan_tracker(tracker=self.plan_apply_stage_tracker, success=False) + self.stop_plan_tracker(self.plan_apply_stage_tracker, False) def is_cancelling_plan(self) -> bool: return bool(self.plan_cancel_stage_tracker and not self.plan_cancel_stage_tracker.meta.done) + def stop_plan_tracker_stages( + self, + tracker: t.Optional[ + t.Union[ + models.PlanApplyStageTracker, + models.PlanCancelStageTracker, + models.PlanOverviewStageTracker, + ] + ], + success: bool = True, + ) -> None: + if not tracker: + return + + stages = ( + [attr for attr in tracker.__fields__ if not attr.startswith("__")] if tracker else [] + ) + + for key in stages: + stage = getattr(tracker, key) + if isinstance(stage, models.Trackable) and stage.meta and not stage.meta.done: + stage.stop(success) + + tracker.stop(success) + def finish_plan_cancellation(self) -> None: cancel_tracker = self.plan_cancel_stage_tracker @@ -332,8 +357,8 @@ def _is_stage_done(stage: str) -> bool: is_apply_tracker_completed = all([_is_stage_done(stage) for stage in stages]) if apply_tracker and not apply_tracker.meta.done and is_apply_tracker_completed: - self.stop_plan_tracker(tracker=apply_tracker, success=False) - self.stop_plan_tracker(tracker=cancel_tracker, success=True) + self.stop_plan_tracker(apply_tracker, False) + self.stop_plan_tracker(cancel_tracker, True) api_console = ApiConsole() diff --git a/web/server/utils.py b/web/server/utils.py index d706c0dd1e..0a23977739 100644 --- a/web/server/utils.py +++ b/web/server/utils.py @@ -10,7 +10,7 @@ import pyarrow as pa # type: ignore from fastapi import Depends, HTTPException from starlette.responses import StreamingResponse -from starlette.status import HTTP_404_NOT_FOUND +from starlette.status import HTTP_404_NOT_FOUND, HTTP_422_UNPROCESSABLE_ENTITY from sqlmesh.core.context import Context from web.server.console import api_console @@ -33,8 +33,18 @@ async def run_in_executor(func: t.Callable[..., R], *args: t.Any) -> R: def func_wrapper() -> R: try: return func(*args) + except ApiException as e: + api_console.log_exception(e) + raise e except Exception as e: - api_console.log_exception() + api_console.log_exception( + ApiException( + message="An unexpected error occurred", + origin="API -> utils -> run_in_executor", + trigger="An unexpected error occurred", + status_code=HTTP_422_UNPROCESSABLE_ENTITY, + ) + ) raise e loop = asyncio.get_running_loop()