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 3 commits
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
24 changes: 24 additions & 0 deletions pkg/model/application_live_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,27 @@ 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
}
}

for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}
khanhtc1202 marked this conversation as resolved.
Show resolved Hide resolved

s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
khanhtc1202 marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Member

@khanhtc1202 khanhtc1202 Dec 26, 2024

Choose a reason for hiding this comment

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

Suggested change
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
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
}
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
}
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
return
}
unhealthy := false
unknown := false
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
unhealthy = true
}
if r.HealthStatus == ResourceState_UNKNOWN {
unknown = true
}
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
if unhealthy {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
}
if unknown {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
}
}

@ffjlabo @t-kikuc ok, let's me suggest a clear version of what you both trying to do 😄

Copy link
Member

@khanhtc1202 khanhtc1202 Dec 26, 2024

Choose a reason for hiding this comment

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

Suggested change
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
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
}
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
}
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
return
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
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
}
}
}

Or another version at least could be this to avoid miss set health status.

Copy link
Member Author

@ffjlabo ffjlabo Dec 26, 2024

Choose a reason for hiding this comment

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

Thanks both @khanhtc1202 @t-kikuc
I fixed it to reduce one for statement.
5aec9a7

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
75 changes: 75 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,78 @@ 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: "unhealthy: prioritize unhealthy over unknown",
snapshot: &ApplicationLiveStateSnapshot{
ApplicationLiveState: &ApplicationLiveState{
Resources: []*ResourceState{
{HealthStatus: ResourceState_UNKNOWN},
{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.

Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const useStyles = makeStyles((theme) => ({
other: {
color: theme.palette.info.main,
},
unhealthy: {
color: theme.palette.info.main,
},
}));

export interface ApplicationHealthStatusIconProps {
Expand All @@ -31,6 +34,8 @@ export const ApplicationHealthStatusIcon: FC<ApplicationHealthStatusIconProps> =
return <FavoriteIcon className={classes.healthy} />;
case ApplicationLiveStateSnapshot.Status.OTHER:
return <OtherIcon className={classes.other} />;
case ApplicationLiveStateSnapshot.Status.UNHEALTHY:
return <OtherIcon className={classes.unhealthy} />;
Comment on lines +37 to +38
Copy link
Member Author

Choose a reason for hiding this comment

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

Use OtherIcon similar to OTHER's one as the UNHEALTHY icon for now.

This is a fix for lint/web.

}
}
);
1 change: 1 addition & 0 deletions web/src/constants/health-status-text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ export const APPLICATION_HEALTH_STATUS_TEXT: Record<
[ApplicationLiveStateSnapshot.Status.UNKNOWN]: "Unknown",
[ApplicationLiveStateSnapshot.Status.HEALTHY]: "Healthy",
[ApplicationLiveStateSnapshot.Status.OTHER]: "Other",
[ApplicationLiveStateSnapshot.Status.UNHEALTHY]: "Unhealthy",
};
Loading