Skip to content

Conversation

@hhovsepy
Copy link
Contributor

For 'kiali_get_mesh_graph' tool, added summary health call as well.

Original PR: #479

@hhovsepy hhovsepy force-pushed the kiali_mesh_graph_summary_health branch from 38169d0 to ad0f311 Compare November 26, 2025 08:36
Copy link
Contributor

@aljesusg aljesusg left a comment

Choose a reason for hiding this comment

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

WDTY?

Outbound map[string]map[string]float64 `json:"outbound"`
HealthAnnotations map[string]string `json:"healthAnnotations"`
}

Copy link
Contributor

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

}
}
}
}
Copy link
Contributor

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?

Copy link
Contributor Author

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

@hhovsepy hhovsepy requested a review from aljesusg November 26, 2025 11:02
@hhovsepy
Copy link
Contributor Author

Thanks @aljesusg , all comments approached.

@hhovsepy hhovsepy requested a review from aljesusg November 26, 2025 16:33
@hhovsepy hhovsepy force-pushed the kiali_mesh_graph_summary_health branch from 88bbb78 to 7455ae9 Compare November 26, 2025 16:34
Copy link
Contributor

@aljesusg aljesusg left a 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.

hhovsepy and others added 5 commits November 27, 2025 11:41
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]>
@hhovsepy hhovsepy force-pushed the kiali_mesh_graph_summary_health branch from 0a0f3c9 to e03219c Compare November 27, 2025 10:41
@hhovsepy hhovsepy requested a review from aljesusg November 27, 2025 10:41
Copy link
Contributor

@aljesusg aljesusg left a comment

Choose a reason for hiding this comment

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

LGFM

@manusa manusa changed the title Added summary health for kiali_get_mesh_graph tool. feat(kiali): add aggregated health summary to mesh graph tool Nov 27, 2025
"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'",
Copy link
Member

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

Copy link
Contributor

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'",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@manusa manusa added this to the 0.1.0 milestone Nov 27, 2025
@manusa manusa merged commit 9e3645d into containers:main Nov 27, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants