Skip to content
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

Determine app health status for plugin architecture #5454

Merged
merged 7 commits into from
Dec 27, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions pkg/app/pipedv1/livestatereporter/livestatereporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@
Kind: app.Kind,
ApplicationLiveState: res.GetApplicationLiveState(),
}
// TODO: Implement DetermineAppHealthStatus for the case of plugin architecture.
snapshot.DetermineAppHealthStatus()
snapshot.DetermineApplicationHealthStatus()

Check warning on line 145 in pkg/app/pipedv1/livestatereporter/livestatereporter.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/livestatereporter/livestatereporter.go#L145

Added line #L145 was not covered by tests

if _, err := pr.apiClient.ReportApplicationLiveState(ctx, &pipedservice.ReportApplicationLiveStateRequest{
Snapshot: snapshot,
Expand Down
21 changes: 21 additions & 0 deletions pkg/model/application_live_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,24 @@ func (s *ApplicationLiveStateSnapshot) determineLambdaAppHealthStatus() {
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
}

// DetermineApplicationHealthStatus updates the health status of the application based on the health status of its resources.
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if app == nil {
return
}
if app == nil {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}

We should trickly assign the UNKNOWN stage here. The current logic looks like if this live state is nill, then the state will be kept "as is", which is unclearly UNKNOWN since UNKOWN is 0 in the enum. We should make it clear to avoid misunderstanding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIxed at f3bbedd


for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}

if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[IMO] How about prioritizing UNHEALTHY?

In your code, the app's health status depends on resource order.
(e.g. If an Unknown resource is found earlier than an Unhealthy resource, then it's Unknown)

In the following code,
1. If any Unhealthy is found, then the app is Unhealthy.
2. If no Unhealthy was found and any Unknown is found, then the app is Unknown

Suggested change
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-kikuc
Thanks, I agree with it.
Fixed at b723484

s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
khanhtc1202 marked this conversation as resolved.
Show resolved Hide resolved
}
450 changes: 227 additions & 223 deletions pkg/model/application_live_state.pb.go

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions pkg/model/application_live_state.proto
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ message ApplicationLiveStateSnapshot {
UNKNOWN = 0;
HEALTHY = 1;
OTHER = 2;
UNHEALTHY = 3;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to use UNHEALTHY instead of OTHER in pipedv1

}
string application_id = 1 [(validate.rules).string.min_len = 1];
string piped_id = 3 [(validate.rules).string.min_len = 1];
Expand Down
62 changes: 62 additions & 0 deletions pkg/model/application_live_state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,65 @@ func TestApplicationLiveStateSnapshot_DetermineAppHealthStatus(t *testing.T) {
})
}
}
func TestApplicationLiveStateSnapshot_DetermineApplicationHealthStatus(t *testing.T) {
testcases := []struct {
name string
snapshot *ApplicationLiveStateSnapshot
want ApplicationLiveStateSnapshot_Status
}{
{
name: "healthy: all resources are healthy",
snapshot: &ApplicationLiveStateSnapshot{
ApplicationLiveState: &ApplicationLiveState{
Resources: []*ResourceState{
{HealthStatus: ResourceState_HEALTHY},
{HealthStatus: ResourceState_HEALTHY},
},
},
},
want: ApplicationLiveStateSnapshot_HEALTHY,
},
{
name: "healthy: no resources",
snapshot: &ApplicationLiveStateSnapshot{
ApplicationLiveState: &ApplicationLiveState{},
},
want: ApplicationLiveStateSnapshot_HEALTHY,
Comment on lines +191 to +195
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happen if ApplicationLiveState = nil, not a dummy pointer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khanhtc1202
The case is ApplicationLiveStateSnapshot_UNKNOWN as the last of the testcases, similar to the current implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

},
{
name: "unhealthy: one of the resources is unhealthy",
snapshot: &ApplicationLiveStateSnapshot{
ApplicationLiveState: &ApplicationLiveState{
Resources: []*ResourceState{
{HealthStatus: ResourceState_HEALTHY},
{HealthStatus: ResourceState_UNHEALTHY},
},
},
},
want: ApplicationLiveStateSnapshot_UNHEALTHY,
},
{
name: "unknown: one of the resources is unknown",
snapshot: &ApplicationLiveStateSnapshot{
ApplicationLiveState: &ApplicationLiveState{
Resources: []*ResourceState{
{HealthStatus: ResourceState_HEALTHY},
{HealthStatus: ResourceState_UNKNOWN},
},
},
},
want: ApplicationLiveStateSnapshot_UNKNOWN,
},
{
name: "unknown: nil application live state",
snapshot: &ApplicationLiveStateSnapshot{},
want: ApplicationLiveStateSnapshot_UNKNOWN,
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
tc.snapshot.DetermineApplicationHealthStatus()
assert.Equal(t, tc.want, tc.snapshot.HealthStatus)
})
}
}
1 change: 1 addition & 0 deletions web/model/application_live_state_pb.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export namespace ApplicationLiveStateSnapshot {
UNKNOWN = 0,
HEALTHY = 1,
OTHER = 2,
UNHEALTHY = 3,
}
}

Expand Down
3 changes: 2 additions & 1 deletion web/model/application_live_state_pb.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading