-
Notifications
You must be signed in to change notification settings - Fork 197
feat(kiali): add aggregated health summary to mesh graph tool #506
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
feat(kiali): add aggregated health summary to mesh graph tool #506
Conversation
38169d0 to
ad0f311
Compare
aljesusg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDTY?
pkg/kiali/get_mesh_graph.go
Outdated
| Outbound map[string]map[string]float64 `json:"outbound"` | ||
| HealthAnnotations map[string]string `json:"healthAnnotations"` | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this to a new file called types.go to keep clean this one
pkg/kiali/get_mesh_graph.go
Outdated
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this logic into a helper_calculation file or something similar? The idea is to make it easier to test and keep the call file clean.
On another note, I’m wondering whether it makes sense to keep health in the struct when MeshHealthSummary is already using it for computation. Is there any additional information in health that isn’t already included in MeshHealthSummary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I think the Health in response is not needed anymore, if more information would be needed, we can add into the MeshHealthSummary
|
Thanks @aljesusg , all comments approached. |
88bbb78 to
7455ae9
Compare
aljesusg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else looks good; only this change is needed.
Signed-off-by: Hayk Hovsepyan <[email protected]>
Signed-off-by: Hayk Hovsepyan <[email protected]>
Signed-off-by: Hayk Hovsepyan <[email protected]>
Signed-off-by: Hayk Hovsepyan <[email protected]>
Co-authored-by: Alberto Gutierrez <[email protected]> Signed-off-by: Hayk Hovsepyan <[email protected]>
0a0f3c9 to
e03219c
Compare
aljesusg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGFM
| "rateInterval": { | ||
| Type: "string", | ||
| Description: "Rate interval for fetching (e.g., '10m', '5m', '1h'). Default: '60s'", | ||
| Description: "Rate interval for fetching (e.g., '10m', '5m', '1h'). Default: '10m'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you can include a Default: api.ToRawMessage(kialiclient.DefaultRateInterval)
to improve the toolset documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great I am going to move all defaults and do it thabks !
| "rateInterval": { | ||
| Type: "string", | ||
| Description: "Rate interval for metrics (e.g., '1m', '5m'). Optional, defaults to '1m'", | ||
| Description: "Rate interval for metrics (e.g., '1m', '5m'). Optional, defaults to '10m'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manusa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx!
For 'kiali_get_mesh_graph' tool, added summary health call as well.
Original PR: #479