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

fix: add decimal precision to CPU and memory metrics in pod info #2221

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

adarsh0728
Copy link
Contributor

@adarsh0728 adarsh0728 commented Nov 15, 2024

Fixes #2225

Pod Info Box:

  • Format CPU millicores with 2 decimal places
  • Format memory Mi with 2 decimal places

Changes: Only Backend changes are done in this PR as logic is completely handled in the API ( pods-info )

Screenshot 2024-11-15 at 10 09 51 PM

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 64.04%. Comparing base (c5afc90) to head (66dadc1).

Files with missing lines Patch % Lines
server/apis/v1/handler.go 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2221      +/-   ##
==========================================
+ Coverage   63.91%   64.04%   +0.13%     
==========================================
  Files         338      338              
  Lines       41085    41101      +16     
==========================================
+ Hits        26259    26325      +66     
+ Misses      13756    13713      -43     
+ Partials     1070     1063       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@adarsh0728 adarsh0728 self-assigned this Nov 15, 2024
@adarsh0728
Copy link
Contributor Author

adarsh0728 commented Nov 18, 2024

Currently, Container Info's CPU/Mem usage and HexagonHeatMap(container level usage) comes from pods API. Pod Info's CPU/Mem usage comes from pods-info API. In future, we would have all the CPU/Mem data from pods-info API. Changes in UI code(only) would be required, as pods-info API already send container level data.

@adarsh0728 adarsh0728 marked this pull request as ready for review November 18, 2024 14:11
@vigith vigith requested a review from mshakira November 18, 2024 16:01
@@ -1684,3 +1684,24 @@ func (h *handler) getContainerStatus(state corev1.ContainerState) string {
return "Unknown"
}
}

func cpuToMillicores(quantityStr string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't take Quantity directly so that there's no need to parse?

Copy link
Contributor Author

@adarsh0728 adarsh0728 Nov 19, 2024

Choose a reason for hiding this comment

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

Quantity is like:
quantity::= signedNumber|suffix. To get the suffix and calculate accordingly we're parsing the string.
For eg: 2847159n this is the cpuQuantity, cpuQuantity.MilliValue() will give us 3, but we want 2.84m as our final result. For that we divide 2847159/1e6

Screenshot 2024-11-19 at 12 22 08 PM


// Parse the quantity string
if _, err := fmt.Sscanf(quantityStr, "%f%s", &value, &format); err != nil {
fmt.Println("Error parsing cpu quantity string:", err)
Copy link
Member

Choose a reason for hiding this comment

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

If there's a need for error handling, return it and handle it in upper layer.

@whynowy
Copy link
Member

whynowy commented Nov 18, 2024

Do we need to keep 2 decimals? I think no decimal is okay.

@adarsh0728
Copy link
Contributor Author

Do we need to keep 2 decimals? I think no decimal is okay.

We already have decimals in container info box and hexagonal heat map. This is done to bring parity. Also, sometimes a pod can have only one container - in pod info we are showing lets say 3m but in container box it's 2.7m (these should be equal)

@adarsh0728 adarsh0728 marked this pull request as draft November 19, 2024 06:37
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.

Add decimal precision to CPU and memory metrics in pod info
2 participants