Make resource HealthStatus computed from HealthReports #6368

adamint · 2024-10-18T04:45:53Z

Description

The purpose of this PR is to make the HealthStatus field of a resource no longer settable, and instead have it computed from the HealthReports and State fields.

HealthStatus is computed as

null, if the resource is not Running
unhealthy, if the resource is running, and has a health check that has not returned a value yet
healthy, if there are no health checks
otherwise whatever the min of the returned health check values is

Fixes # (issue)

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

src/Aspire.Dashboard/Model/ResourceViewModel.cs

src/Aspire.Dashboard/ResourceService/Partials.cs

src/Aspire.Hosting/ApplicationModel/CustomResourceSnapshot.cs

src/Aspire.Dashboard/Model/ResourceViewModel.cs

src/Aspire.Hosting/ApplicationModel/CustomResourceSnapshot.cs

src/Aspire.Hosting/Dashboard/proto/resource_service.proto

src/Aspire.Dashboard/Model/KnownPropertyLookup.cs

JamesNK · 2024-10-18T05:50:06Z

I tested your branch on health checks sandbox and got this error:

Known state propertiy could be set before health reports.

Could fix by defaulting it to an empty array:

private readonly ImmutableArray<HealthReportViewModel> _healthReports = [];

Edit: nevermind, you've fixed this

JamesNK · 2024-10-18T05:56:28Z

Health status is inconsistently being shown in resource details:

Visible:

Not visible:

JamesNK · 2024-10-18T05:58:16Z

While you're making changes in this area, I noticed the Health State casing is inconsistent with other properties. It should be Health state

… returned

adamint · 2024-10-18T06:11:39Z

Health status is inconsistently being shown in resource details:

Visible:

Not visible:

I was just thinking about this! The property does not appear when the resource has a health report with a null value. We encode that assumption in ResourceStateViewModel but it should really be encoded in HealthStatus itself, which fixes the bug. Made this change

adamint · 2024-10-18T06:18:20Z

src/Aspire.Dashboard/Model/ResourceStateViewModel.cs

@@ -62,12 +62,6 @@ internal static ResourceStateViewModel GetStateViewModel(ResourceViewModel resou
            icon = new Icons.Filled.Size16.Circle();
            color = Color.Info;
        }
-        else if (resource.HealthStatus is null)


This condition is redundant

JamesNK

Looking pretty good. Needs tests.

src/Aspire.Dashboard/Model/ResourceViewModel.cs

adamint · 2024-10-18T06:28:41Z

Looking pretty good. Needs tests.

ComputeHealthStatus should be tested but otherwise it is pretty covered

src/Aspire.Hosting/Dashboard/DashboardServiceData.cs

playground/HealthChecks/HealthChecksSandbox.AppHost/Program.cs

src/Aspire.Hosting/Health/ResourceHealthCheckService.cs

JamesNK · 2024-10-22T02:08:46Z

src/Aspire.Hosting/Dashboard/proto/resource_service.proto

@@ -210,6 +210,7 @@ message Resource {

    // The aggregate health state of the resource. Generally reflects data from health_reports, but may differ.
    optional HealthStatus health_status = 16;


Was there offline discussion about adding this back? I'm not against it, just want to make sure that was an agreed decision.

No we want to remove it

I know I said before I didn't want it, but sending the calculated value does reduce duplication. It lets the dashboard be more dumb and accept whatever status the service decides 😄

I haven't got a hard opinion either way, but the current code looks fine to me.

@davidfowl I had re-added it to the proto to do what james said - avoid re-calculating the status in the dashboard and avoid duplication

Remove it, let the dashboard recalculate it.

Re-removed :) is everything else ok?

JamesNK · 2024-10-22T02:20:43Z

Looks good. I have some questions, nits and changes to the sandbox.

adamint · 2024-10-22T04:32:22Z

Is this now in a merge-able state?

davidfowl · 2024-10-22T06:29:09Z

@adamint I suggest you do a squash and a manual backport. This has 36 commits.

davidfowl

Looks great

JamesNK · 2024-10-22T08:40:50Z

@adamint I suggest you do a squash and a manual backport. This has 36 commits.

+1.

If you don't know how to squash, squash merge in GH does it automatically. Then just create a new branch from release/9.0 and cherry pick the squashed commit.

…interactions of multiple reports

…heck runs, even if the overall status has not

adamint · 2024-10-22T18:46:32Z

There was a health check scenario that has never been accounted for -

Resource has multiple health checks
Resource checks health, one or more reports return a new status but the overall health status has not changed (ie, Unhealthy+Healthy=Unhealthy -> Unhealthy+Degraded=Unhealthy)
The report health is the same as the prior snapshot health status
Resource health reports are not updated, even though the status of one of them changed.

I have updated the sandbox and ResourceHealthCheckService to account for this.

# Conflicts: # src/Shared/StringComparers.cs

davidfowl · 2024-10-22T19:52:04Z

Good catch @adamint !

* Make resource HealthStatus computed from HealthReports * Update health reports when they have changed but the status has not --------- Co-authored-by: Adam Ratzman <adamratzman@microsoft.com> Co-authored-by: Mitch Denny <midenn@microsoft.com> (cherry picked from commit a4ef97a)

#### AI description (iteration 1) #### PR Classification Merge branch `release/9.0` into `internal/release/9.0` to integrate recent changes and address specific work items. #### PR Summary This pull request integrates changes from the `release/9.0` branch into `internal/release/9.0`, addressing several work items related to Azure storage and network isolation. - `/tests/Aspire.Hosting.Azure.Tests/AzureContainerAppsTests.cs`: Added a new test `ConfigureCustomDomainsMutatesIngress` to validate custom domain configuration for Azure Container Apps. - `/src/Aspire.Hosting.Azure.AppContainers/ContainerAppExtensions.cs`: Introduced a new extension method `ConfigureCustomDomain` to simplify the process of assigning custom domains to Azure Container Apps. Related work items: dotnet#6368, dotnet#6391, dotnet#6433, dotnet#6458, dotnet#6467

Make resource HealthStatus computed from HealthReports

43ac440

davidfowl requested review from JamesNK, drewnoakes and mitchdenny October 18, 2024 04:48

davidfowl reviewed Oct 18, 2024

View reviewed changes

src/Aspire.Dashboard/Model/ResourceViewModel.cs Outdated Show resolved Hide resolved

davidfowl reviewed Oct 18, 2024

View reviewed changes

src/Aspire.Dashboard/ResourceService/Partials.cs Show resolved Hide resolved

davidfowl reviewed Oct 18, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/CustomResourceSnapshot.cs Outdated Show resolved Hide resolved

change HealthStatus back to property, fix test

8b3406a

adamint requested a review from davidfowl October 18, 2024 05:25

JamesNK reviewed Oct 18, 2024

View reviewed changes

src/Aspire.Dashboard/Model/ResourceViewModel.cs Outdated Show resolved Hide resolved

JamesNK reviewed Oct 18, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/CustomResourceSnapshot.cs Outdated Show resolved Hide resolved

Update HealthStatus when state changes

c608065

JamesNK reviewed Oct 18, 2024

View reviewed changes

src/Aspire.Hosting/Dashboard/proto/resource_service.proto Show resolved Hide resolved

JamesNK reviewed Oct 18, 2024

View reviewed changes

src/Aspire.Dashboard/Model/KnownPropertyLookup.cs Outdated Show resolved Hide resolved

Adam Ratzman added 3 commits October 18, 2024 01:39

Return null HealthStatus when not running, use constant

d8556f4

add 16 to reserved fields

8f605c9

add default argument to _healthReports, re-add property

d4d6849

Say resource is unhealthy if it is running but a health check has not…

5955d5f

… returned

Change Health State -> Health state

a29cc72

adamint commented Oct 18, 2024

View reviewed changes

Remove redundant condition

e817af7

JamesNK reviewed Oct 18, 2024

View reviewed changes

src/Aspire.Dashboard/Model/ResourceViewModel.cs Outdated Show resolved Hide resolved

src/Aspire.Dashboard/Model/ResourceViewModel.cs Outdated Show resolved Hide resolved

Adam Ratzman added 2 commits October 18, 2024 02:29

clean up

20dd730

add test for ComputeHealthStatus

b864dad

JamesNK approved these changes Oct 18, 2024

View reviewed changes

remove HealthAnnotationsInitialized

b3ecfeb

build-analysis bot mentioned this pull request Oct 21, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

JamesNK reviewed Oct 22, 2024

View reviewed changes

src/Aspire.Hosting/Dashboard/DashboardServiceData.cs Show resolved Hide resolved

playground/HealthChecks/HealthChecksSandbox.AppHost/Program.cs Show resolved Hide resolved

src/Aspire.Hosting/Health/ResourceHealthCheckService.cs Show resolved Hide resolved

JamesNK reviewed Oct 22, 2024

View reviewed changes

add comment

c38a5ff

re-add health status computation in dashboard

2bb8e07

adamint requested a review from JamesNK October 22, 2024 05:49

Adam Ratzman added 2 commits October 22, 2024 01:50

forgot newline

feb1dc7

fix test

5145c38

JamesNK approved these changes Oct 22, 2024

View reviewed changes

davidfowl approved these changes Oct 22, 2024

View reviewed changes

Adam Ratzman added 3 commits October 22, 2024 14:29

Add a healthy check resources in to health check sandbox to showcase …

b88279a

…interactions of multiple reports

Update resource health reports if any have changed after the health c…

5b91f84

…heck runs, even if the overall status has not

move logic into above condition

69abad0

Adam Ratzman added 4 commits October 22, 2024 15:25

extract to local static method

f7127eb

use string comparers

0129991

return true if health check name is not found in existing health reports

8a02eb5

Merge branch 'main' into dev/adamint/remove-aggregated-healthstatus

2799a2a

# Conflicts: # src/Shared/StringComparers.cs

adamint enabled auto-merge (squash) October 22, 2024 20:02

adamint merged commit a4ef97a into dotnet:main Oct 22, 2024
9 checks passed

adamint mentioned this pull request Oct 23, 2024

[release/9.0] Make resource HealthStatus computed from HealthReports (#6368) #6458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make resource HealthStatus computed from HealthReports #6368

Make resource HealthStatus computed from HealthReports #6368

adamint commented Oct 18, 2024 •

edited

Loading

JamesNK commented Oct 18, 2024 •

edited

Loading

JamesNK commented Oct 18, 2024

JamesNK commented Oct 18, 2024 •

edited

Loading

adamint commented Oct 18, 2024

adamint Oct 18, 2024

JamesNK left a comment

adamint commented Oct 18, 2024

JamesNK Oct 22, 2024

davidfowl Oct 22, 2024

JamesNK Oct 22, 2024

adamint Oct 22, 2024

davidfowl Oct 22, 2024

adamint Oct 22, 2024

JamesNK commented Oct 22, 2024

adamint commented Oct 22, 2024

davidfowl commented Oct 22, 2024

davidfowl left a comment

JamesNK commented Oct 22, 2024

adamint commented Oct 22, 2024 •

edited

Loading

davidfowl commented Oct 22, 2024

		@@ -210,6 +210,7 @@ message Resource {

		// The aggregate health state of the resource. Generally reflects data from health_reports, but may differ.
		optional HealthStatus health_status = 16;

Make resource HealthStatus computed from HealthReports #6368

Make resource HealthStatus computed from HealthReports #6368

Conversation

adamint commented Oct 18, 2024 • edited Loading

Description

Checklist

Microsoft Reviewers: Open in CodeFlow

JamesNK commented Oct 18, 2024 • edited Loading

JamesNK commented Oct 18, 2024

JamesNK commented Oct 18, 2024 • edited Loading

adamint commented Oct 18, 2024

adamint Oct 18, 2024

Choose a reason for hiding this comment

JamesNK left a comment

Choose a reason for hiding this comment

adamint commented Oct 18, 2024

JamesNK Oct 22, 2024

Choose a reason for hiding this comment

davidfowl Oct 22, 2024

Choose a reason for hiding this comment

JamesNK Oct 22, 2024

Choose a reason for hiding this comment

adamint Oct 22, 2024

Choose a reason for hiding this comment

davidfowl Oct 22, 2024

Choose a reason for hiding this comment

adamint Oct 22, 2024

Choose a reason for hiding this comment

JamesNK commented Oct 22, 2024

adamint commented Oct 22, 2024

davidfowl commented Oct 22, 2024

davidfowl left a comment

Choose a reason for hiding this comment

JamesNK commented Oct 22, 2024

adamint commented Oct 22, 2024 • edited Loading

davidfowl commented Oct 22, 2024

adamint commented Oct 18, 2024 •

edited

Loading

JamesNK commented Oct 18, 2024 •

edited

Loading

JamesNK commented Oct 18, 2024 •

edited

Loading

adamint commented Oct 22, 2024 •

edited

Loading