-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(health): recover stuck loads and stop premature healthy state #91351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import '@testing-library/jest-dom' | ||
|
|
||
| import { cleanup, render, screen } from '@testing-library/react' | ||
|
|
||
| import { HealthEmptyState } from './HealthEmptyState' | ||
|
|
||
| describe('HealthEmptyState', () => { | ||
| afterEach(cleanup) | ||
|
|
||
| it('points a project with no events at install instead of declaring it healthy', () => { | ||
| render(<HealthEmptyState hasIngestedEvents={false} />) | ||
|
|
||
| expect(screen.getByText('Health checks have not run yet')).toBeInTheDocument() | ||
| // LemonBanner renders the action twice for its responsive layout, so there is at least one. | ||
| expect(screen.getAllByText('Install PostHog').length).toBeGreaterThan(0) | ||
| expect(screen.queryByText('All systems healthy')).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('declares a project with events healthy when no issues are found', () => { | ||
| render(<HealthEmptyState hasIngestedEvents={true} />) | ||
|
|
||
| expect(screen.getByText('All systems healthy')).toBeInTheDocument() | ||
| expect(screen.queryByText('Install PostHog')).not.toBeInTheDocument() | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import { LemonBanner } from '@posthog/lemon-ui' | ||
|
|
||
| import { urls } from 'scenes/urls' | ||
|
|
||
| import { ProductKey } from '~/queries/schema/schema-general' | ||
| import { OnboardingStepKey } from '~/types' | ||
|
|
||
| export function HealthEmptyState({ hasIngestedEvents }: { hasIngestedEvents: boolean }): JSX.Element { | ||
| // Without any ingested events the checks have nothing to run against, so treat an empty result | ||
| // set as "not set up yet" and point the user at install rather than claiming everything is fine. | ||
| if (!hasIngestedEvents) { | ||
| return ( | ||
| <LemonBanner | ||
| type="info" | ||
| action={{ | ||
| to: urls.onboarding({ | ||
| productKey: ProductKey.PRODUCT_ANALYTICS, | ||
| stepKey: OnboardingStepKey.INSTALL, | ||
| }), | ||
| children: 'Install PostHog', | ||
| 'data-attr': 'health-empty-install', | ||
| }} | ||
| > | ||
| <p className="font-semibold mb-0">Health checks have not run yet</p> | ||
| <p className="text-sm mt-1 mb-0"> | ||
| Health checks start once your project receives data. Install PostHog to send your first events. | ||
| </p> | ||
| </LemonBanner> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <LemonBanner type="success"> | ||
| <p className="font-semibold mb-0">All systems healthy</p> | ||
| <p className="text-sm mt-1 mb-0">No active health issues found for your project.</p> | ||
| </LemonBanner> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,11 +8,17 @@ import { sceneConfigurations } from 'scenes/scenes' | |
| import { Scene } from 'scenes/sceneTypes' | ||
| import { teamLogic } from 'scenes/teamLogic' | ||
|
|
||
| import { Breadcrumb } from '~/types' | ||
| import { Breadcrumb, TeamPublicType, TeamType } from '~/types' | ||
|
|
||
| import { CATEGORY_ORDER, HEALTH_CATEGORY_CONFIG, categoryForKind } from './healthCategories' | ||
| import type { CategoryHealthSummary, HealthIssue, HealthIssueSeverity } from './types' | ||
| import { REFRESH_COOLDOWN_MS, REFRESH_POLL_COUNT, REFRESH_POLL_INTERVAL_MS, SEVERITY_ORDER } from './types' | ||
| import { | ||
| HEALTH_ISSUES_LOAD_TIMEOUT_MS, | ||
| REFRESH_COOLDOWN_MS, | ||
| REFRESH_POLL_COUNT, | ||
| REFRESH_POLL_INTERVAL_MS, | ||
| SEVERITY_ORDER, | ||
| } from './types' | ||
|
|
||
| export interface HealthIssuesResponse { | ||
| results: HealthIssue[] | ||
|
|
@@ -23,9 +29,11 @@ export interface HealthIssuesResponse { | |
|
|
||
| // Generated by kea-typegen. Update if you're an agent, ignore if you're human. | ||
| export interface healthSceneLogicValues { | ||
| currentTeam: TeamPublicType | TeamType | null // teamLogic | ||
| currentTeamIdStrict: number | string // teamLogic | ||
| breadcrumbs: Breadcrumb[] | ||
| categorySummaries: CategoryHealthSummary[] | ||
| hasIngestedEvents: boolean | ||
| healthIssues: HealthIssuesResponse | null | ||
| healthIssuesLoading: boolean | ||
| isManualRefresh: boolean | ||
|
|
@@ -88,6 +96,7 @@ export interface healthSceneLogicMeta { | |
| __keaTypeGenInternalSelectorTypes: { | ||
| issues: (healthIssues: HealthIssuesResponse | null) => HealthIssue[] | ||
| totalCount: (healthIssues: HealthIssuesResponse | null) => number | ||
| hasIngestedEvents: (currentTeam: TeamPublicType | TeamType | null) => boolean | ||
| categorySummaries: (issues: HealthIssue[]) => CategoryHealthSummary[] | ||
| } | ||
| } | ||
|
|
@@ -102,7 +111,7 @@ export type healthSceneLogicType = MakeLogicType< | |
| export const healthSceneLogic = kea<healthSceneLogicType>([ | ||
| path(['scenes', 'health', 'healthSceneLogic']), | ||
| connect({ | ||
| values: [teamLogic, ['currentTeamIdStrict']], | ||
| values: [teamLogic, ['currentTeamIdStrict', 'currentTeam']], | ||
|
posthog[bot] marked this conversation as resolved.
|
||
| }), | ||
| actions({ | ||
| setShowDismissed: (show: boolean) => ({ show }), | ||
|
|
@@ -156,7 +165,7 @@ export const healthSceneLogic = kea<healthSceneLogicType>([ | |
| const queryString = new URLSearchParams(params).toString() | ||
| const url = `api/environments/${values.currentTeamIdStrict}/health_issues/?${queryString}` | ||
|
|
||
| return await api.get(url) | ||
| return await api.get(url, { signal: AbortSignal.timeout(HEALTH_ISSUES_LOAD_TIMEOUT_MS) }) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The category detail loader can still hang foreverWhy we think it's a valid issue
Issue descriptionThe timeout protects only the overview loader. Suggested fixMove the bounded request into a shared function and use it from both loaders. Add a failure state and retry action to the category detail scene. Prompt to fix with AI (copy-paste)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed this is a real, still-present defect — but it belongs in a separate follow-up, not this PR. The category detail loader ( I did not fix it here because both of those files are outside this PR's diff — this change only touches the overview scene (the empty-state, issue list, and scene logic). The fix the reviewer suggests (extract the bounded request into a shared helper used by both loaders, and add a failure state + retry action to the detail scene) would edit sibling files and add new UI to a scene this PR never touched, which is beyond its intent. That's a deliberate scope boundary, not a disagreement that the bug is worth fixing. For a human to decide: track this as a follow-up PR that gives the category-detail loader the same |
||
| }, | ||
| }, | ||
| ], | ||
|
|
@@ -170,6 +179,12 @@ export const healthSceneLogic = kea<healthSceneLogicType>([ | |
| (s) => [s.healthIssues], | ||
| (healthIssues: HealthIssuesResponse | null): number => healthIssues?.count ?? 0, | ||
| ], | ||
| // A project that has never ingested an event has no data for any check to run against, so an | ||
| // empty result set there means "checks have not run yet", not "everything is healthy". | ||
| hasIngestedEvents: [ | ||
| (s) => [s.currentTeam], | ||
| (currentTeam: TeamPublicType | TeamType | null): boolean => !!currentTeam?.ingested_event, | ||
| ], | ||
|
Comment on lines
+182
to
+187
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An ingested event does not prove that health checks ranWhy we think it's a valid issue
Issue descriptionHealth checks run from daily or weekly Temporal schedules, and schedule creation sets Suggested fixExpose a backend signal that records whether the relevant checks completed, and require it before showing the success banner. Until that signal exists, use a neutral empty state such as “No active health issues” instead of claiming that all checks passed. Prompt to fix with AI (copy-paste)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and worth addressing, but it needs a human product call rather than an unattended change. The issue is real: The clean fix is the one you suggested first: a backend signal that records whether the relevant checks completed, gating the success banner on it. That's a new API field plus Temporal changes, which is out of scope for this frontend-only PR, so I'm leaving it for the owning team to decide. The only in-scope alternative is neutral copy (e.g. "No active health issues" instead of "All systems healthy"). I didn't make that change unattended because it removes the green healthy affordance for every project, including genuinely-healthy mature ones where the banner is accurate and reassuring — that's a product tradeoff someone owning the Health page should weigh, not a mechanical fix. For what it's worth this isn't a regression (master showed the same banner for any empty set) and it self-corrects at the next check run. Decision needed: build the backend "checks completed" signal to gate the success state, or accept the frontend-only compromise of neutral copy that drops the "All systems healthy" state for all projects. |
||
| categorySummaries: [ | ||
| (s) => [s.issues], | ||
| (issues: HealthIssue[]): CategoryHealthSummary[] => { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.