Remove HealthStatus defaulting to healthy when there is no value from a health check

[adamint]

Description

The proposed changes are

• give resources a null health status until health reports start to be received
• give resources a healthy status if they do not have health checks and are running
• create initial health reports with a null status so that the dashboard immediately knows which health checks will be running and that they have not returned data yet (make HealthReportSnapshot.Status nullable
• remove the "Waiting for data..." text and immediately render the grid of health reports, with an indeterminate loading circle while we wait for health check data

@davidfowl @drewnoakes

Fixes #6125

Checklist

• Is this feature complete?
  • Yes. Ready to ship.
  • No. Follow-up changes expected.
• Are you including unit tests for the changes and scenario tests if relevant?
  • Yes
  • No
• Did you add public API?
  • Yes
    • If yes, did you have an API Review for it?
      • Yes
      • No
    • Did you add <remarks /> and <code /> elements on your triple slash comments?
      • Yes
      • No
  • No
• Does the change make any security assumptions or guarantees?
  • Yes
    • If yes, have you done a threat model and had a security review?
      • Yes
      • No
  • No
• Does the change require an update in our Aspire docs?
  • Yes
    • Link to aspire-docs issue:
  • No

Microsoft Reviewers: Open in CodeFlow

[adamint]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[davidfowl]

I think the tests are failing

[JamesNK]

Needs test. Could probably add an assert on health status to an existing test that looks at custom resource snapshot properties after publish.

[drewnoakes]

I'm not sure the approach here is correct. With the changes here, I expect the UI to show "Running (Unhealthy)" for resources that don't have health checks.

In main, a null value is used so that the dashboard's state column shows a resource as unhealthy (i.e. "Running (Unhealthy)") when we are expecting health reports to arrive. The null applies only when a health check exists, and so carries that information.

This distinction was added as a fix for the following scenario:

• A resource that has a health check starts, but we have no health reports.
  • It appears in the UI as "Running" and green.
• A health report arrives showing that the resource is unhealthy.
  • It changes to show as "Running (Unhealthy)" with a white icon.
• The resource becomes healthy.
  • It changes to "Running" and green again.

The null value allows us to prevent that first state, which incorrectly suggests the resource is healthy.

[davidfowl]

I'm not sure the approach here is correct. With the changes here, I expect the UI to show "Running (Unhealthy)" for resources that don't have health checks.

No, it should show Running, not unhealthy. If we don't have health checks then we're in a "we don't know any better and I have to assume the resource is ready" state.

[drewnoakes]

No, it should show Running, not unhealthy. If we don't have health checks then we're in a "we don't know any better and I have to assume the resource is ready" state.

Sorry I wasn't clear. I agree with you, and that's what happens today. I should have written:

I'm not sure the approach here is correct. With the changes here, I expect the UI to show "Running (Unhealthy)" for resources that have health checks but don't yet have health reports.

[adamint]

I think the tests are failing

It is a helix issue

[adamint]

I'm not sure the approach here is correct. With the changes here, I expect the UI to show "Running (Unhealthy)" for resources that don't have health checks.

In main, a null value is used so that the dashboard's state column shows a resource as unhealthy (i.e. "Running (Unhealthy)") when we are expecting health reports to arrive. The null applies only when a health check exists, and so carries that information.

That's a confusing distinction. It carries the meaning of "no current health status available." This meaning also applies to resources that do not contain health checks, because we do not have a health status available for them. So when resources aren't defaulting to healthy, null can be simplified to "no current health status available" for all resources, regardless of health checks being present.

A resource that has a health check starts, but we have no health reports. It appears in the UI as "Running" and green.

With the current text, we don't need to make that distinction between

• has health checks but does not yet have a report
• does not have health checks

If we do want to indicate that there are health checks present, but that a result has not come back yet, we may want to consider adding a different status, maybe Running (waiting for health check)

[adamint]

Needs test. Could probably add an assert on health status to an existing test that looks at custom resource snapshot properties after publish.

Could you clarify? I also added a null assert in ResourceNotificationTests but I'm pretty sure you mean something else.

[JamesNK]

Needs test. Could probably add an assert on health status to an existing test that looks at custom resource snapshot properties after publish.

Could you clarify? I also added a null assert in ResourceNotificationTests but I'm pretty sure you mean something else.

Write a test for each scenario that people are talking about here and assert the health status. That draws a line in the sand for how each scenario would work.

I see:

• Resource doesn't have health checks.
• Resource has health checks but no result yet.
• Resource has health checks and it has a healthy result.

Maybe others? I didn't look at each comment closely.

[davidfowl]

This doesn't need to run every publish, it never changes. Can the caller do this instead?

[adamint]

It becomes much cheaper to call if the first condition is previousState.HealthStatus is not HealthStatus.Healthy - would you have concerns about that?

[drewnoakes]

The new HasAnnotationIncludingAncestorsOfType<T> method in #6357 would make this cheaper to call.

[adamint]

It becomes much cheaper to call if the first condition is previousState.HealthStatus is not HealthStatus.Healthy - changed

[JamesNK]

FYI I have some changes to make in the next hour. Nothing too contraversial.

In the interest of time, I'm going to add them directly to this branch.

[JamesNK]

Changes:

• Moved most of StateColumnDisplay logic into the view model.
• Always display state tooltip. If there is no custom tooltip then fallback to state text.
• Moved tests from components to dashboard. The actual control isn't being tested with BUnit (just static methods on the control) so it didn't fit in components. Now they're on a view model, a better place for them, they definitely belong in dashboard tests.
• Changed state tests to use a shared localizer. It's a bit more robust because you don't have to declare the translation again. Previously if a formatted param was added or removed, the translation in tests could get out of sync.
• Treating RuntimeUnhealthy state as stopped produced a sub-optimal tooltip:
  
  Now there is a tooltip that explains the most common problem (container runtime isn't running) and includes a URL for more info.

[JamesNK]

Thumbs up for dashboard changes

[JamesNK]

I tested more and found a situation that doesn't look right to me.

messaging hasn't reported a health status but the state is Running and the icon is green:

I would expect the resource it to have a state like Running (Waiting for health) with the same icon as an unhealthy running resource.

[drewnoakes]

There's a regression in the status column.

The status for the unhealthy and degraded resources should read Running (unhealthy) and Running (degraded) respectively.

[davidfowl]

Maybe we can start writing dashboard view model tests

[JamesNK]

I tested and the problem is in ResourceNotificationService (@davidfowl how dare you blame the dashboard 😋 )

aspire/src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs

Lines 398 to 402 in f19600f

	// A resource is also healthy if it has no health check annotations and is in the running state.
	if (previousState.HealthStatus is not HealthStatus.Healthy && !resource.TryGetAnnotationsIncludingAncestorsOfType<HealthCheckAnnotation>(out _) && previousState.State?.Text == KnownResourceStates.Running)
	{
	return previousState with { HealthStatus = HealthStatus.Healthy };
	}

There isn't a HealthCheckAnnotation so it is marked as healthy. The health report with a degraded status isn't taken into account.

I'm guessing this problem is unique to the health checks sandbox test app because the health report is manually specified in the snapshot and isn't driven by the annotation like typical usage.

aspire/playground/HealthChecks/HealthChecksSandbox.AppHost/Program.cs

Lines 43 to 49 in f19600f

	.WithInitialState(new()
	{
	ResourceType = "Test Resource",
	State = "Starting",
	Properties = [],
	HealthReports = [new HealthReportSnapshot($"{name}_check", status, description, exception)]
	})

It seems like the snapshot health reports is the better place to calculate the health status from. But this is the first time I've looked at health checks so someone with more knowledge here should decide.

[drewnoakes]

@adamint will be opening a PR shortly

[drewnoakes]

#6367

[adamint]

I tested and the problem is in ResourceNotificationService (@davidfowl how dare you blame the dashboard 😋 )

aspire/src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs

Lines 398 to 402 in f19600f

	// A resource is also healthy if it has no health check annotations and is in the running state.
	if (previousState.HealthStatus is not HealthStatus.Healthy && !resource.TryGetAnnotationsIncludingAncestorsOfType<HealthCheckAnnotation>(out _) && previousState.State?.Text == KnownResourceStates.Running)
	{
	return previousState with { HealthStatus = HealthStatus.Healthy };
	}

There isn't a HealthCheckAnnotation so it is marked as healthy. The health report with a degraded status isn't taken into account.

I'm guessing this problem is unique to the health checks sandbox test app because the health report is manually specified in the snapshot and isn't driven by the annotation like typical usage.

aspire/playground/HealthChecks/HealthChecksSandbox.AppHost/Program.cs

Lines 43 to 49 in f19600f

	.WithInitialState(new()
	{
	ResourceType = "Test Resource",
	State = "Starting",
	Properties = [],
	HealthReports = [new HealthReportSnapshot($"{name}_check", status, description, exception)]
	})

It seems like the snapshot health reports is the better place to calculate the health status from. But this is the first time I've looked at health checks so someone with more knowledge here should decide.

The bug is a little more generalized than just the health check test app. The root cause is that there should be a check to see if a resource has a ResourceSnapshotAnnotation; if it does, we should not attempt to override the health status. This is correct because sans a health check, there is nothing else that could modify that initial HealthStatus - @JamesNK #6367

[JamesNK]

This is correct because sans a health check, there is nothing else that could modify that initial HealthStatus

I don't think that's true. ResourceNotificationService is public. Someone could add a health report after creating the resource.

[davidfowl]

maybe we make this property internal. The only thing publishing health state would be via the annotations