Define structs for CPU and Memory stats #149

KlwntSingh · 2024-01-08T07:41:19Z

Description:
1. Create new structs to represent CPU and memory stats in RawMetric. This removes RawMetric dependency on Kubelet CPU and memory stats.
2. Refactor existing RawMetric struct to use new CPU and memory stats.
3. Add more unit tests for extractorhelpers
4. This refactor helped avoid adding nil verification checks in extractors.
5. This refactoring extends RawMetrics and existing extractors to work for HNSSHIM API responses.

Testing:

Unit tests are passing locally.
Pod CPU and memory utilization working after refactoring.

Documentation:

1. Create new structs to represent CPU and memory stats in RawMetric. This removes RawMetric dependency on Kubelet CPU and memory stats. 2. Refactor existing RawMetric struct to use new CPU and memory stats. 3. Add more unit tests for extractorhelpers

nathalapooja

Any specific reason to introduce new structs for CPU and memory stats? What are we achieving by doing this?

KlwntSingh · 2024-01-08T21:19:34Z

Any specific reason to introduce new structs for CPU and memory stats? What are we achieving by doing this?

Yes, there are multiple reasons to do this.

Currently RawMetric depends on structs from k8s stats. RawMetric struct should support HNS summary structs as well. That's why i added new structs in RawMetric to make it compatible with both kubelet and HNS. Now, translators/convertors will convert summary structs from kubelet/HNS to RawMetric.
I am now assigning values individually from kubelet/HNS struct to RawMetric structs, this allows me to validate nil values in translators/convertors rather than doing it in extractors.

chadpatel · 2024-01-08T21:28:02Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

 	stats "k8s.io/kubelet/pkg/apis/stats/v1alpha1"
 )

+// convertCPUStats Convert kubelet CPU stats to Raw CPU stats
+func convertCPUStats(kubeletCPUStat *stats.CPUStats) *CPUStat {
+	var cpuStat CPUStat


while we are null checking... should we null check the kubeletCPUStat pointer?

yes, I am already checking null checks on kubeletCPUStat before func is called.

the function should be self contained, we shouldn't rely on the calling context.

My two cents - avoid pointers when possible and just convert to/from pointers at the usage boundary. It will also reduce our need for null checking. I get that the kubeletCPUStat function has pointer member variables so we still need to check for nil before dereferencing. What I am proposing is

func convertCPUStats(kubeletCPUStat stats.CPUStats) CPUStat {

and

func convertMemoryStats(kubeletMemoryStat stats.MemoryStats) MemoryStat {

Performance wise, copying this struct is not expensive.
https://daryl-ng.medium.com/why-you-should-avoid-pointers-in-golang-998f7778c311

Addressed. I have removed pointer references.
Thanks, this will definitely help avoid nil checks.

chadpatel · 2024-01-08T21:29:29Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

+	if kubeletCPUStat.UsageNanoCores != nil {
+		cpuStat.UsageNanoCores = *kubeletCPUStat.UsageNanoCores
+	}
+	return &cpuStat


Why do the helpers need to return a pointer?
I wonder why they even need to take in pointers

I realize the software around it expects pointers but maybe we should do the & reference at higher scope

Hi chad, I thought about it.
Even though we can avoid using pointers here but I see benefit of using pointers.
These structs are supposed to be large in the sense that they will have many fields, if we start passing these structs by value, it's will be very heavy in processing since go will have to copy whole struct when passing them as arguments in func calls. It is better to have them pass by reference since it will go processing.

what do you think? if you still think pointers are unnecessary, then i remove pointers.

This is not a hot path. This code will get called once every 15 seconds or every 60 seconds or whatever. Go is designed to pass by value. Passing by reference is an exception IMO. These structs are not big, maybe 100 bytes

Sounds good, I have removed pointers references.

chadpatel · 2024-01-08T23:17:23Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

 	stats "k8s.io/kubelet/pkg/apis/stats/v1alpha1"
 )

+// convertCPUStats Convert kubelet CPU stats to Raw CPU stats
+func convertCPUStats(kubeletCPUStat *stats.CPUStats) *CPUStat {
+	var cpuStat CPUStat


the function should be self contained, we shouldn't rely on the calling context.

My two cents - avoid pointers when possible and just convert to/from pointers at the usage boundary. It will also reduce our need for null checking. I get that the kubeletCPUStat function has pointer member variables so we still need to check for nil before dereferencing. What I am proposing is

func convertCPUStats(kubeletCPUStat stats.CPUStats) CPUStat {

and

func convertMemoryStats(kubeletMemoryStat stats.MemoryStats) MemoryStat {

Performance wise, copying this struct is not expensive.
https://daryl-ng.medium.com/why-you-should-avoid-pointers-in-golang-998f7778c311

chadpatel · 2024-01-08T23:18:48Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

+	if kubeletCPUStat.UsageNanoCores != nil {
+		cpuStat.UsageNanoCores = *kubeletCPUStat.UsageNanoCores
+	}
+	return &cpuStat


This is not a hot path. This code will get called once every 15 seconds or every 60 seconds or whatever. Go is designed to pass by value. Passing by reference is an exception IMO. These structs are not big, maybe 100 bytes

chadpatel · 2024-01-08T23:20:08Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

+
+// ConvertContainerToRaw Converts Kubelet Container stats per Pod to RawMetric.
+func ConvertContainerToRaw(containerStat *stats.ContainerStats, podStat *stats.PodStats) *RawMetric {
+	var rawMetic *RawMetric


same feedback. I think the pointer return type just adds complexity to the code. And the pointer input types compel us to null check before dereferencing

Addressed, I have removed pointer references.

chadpatel · 2024-01-09T17:45:40Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

-	rawMetic = &RawMetric{}
+func ConvertPodToRaw(podStat stats.PodStats) RawMetric {
+	var rawMetic RawMetric
+	//rawMetic = RawMetric{}


extraneous comment

removed the comment.

chadpatel · 2024-01-09T17:46:07Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

+// ConvertContainerToRaw Converts Kubelet Container stats per Pod to RawMetric.
+func ConvertContainerToRaw(containerStat stats.ContainerStats, podStat stats.PodStats) RawMetric {
+	var rawMetic RawMetric
+	//rawMetic = RawMetric{}


extraneous comment

removed the comment

chadpatel · 2024-01-09T17:48:56Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/cpu_extractor.go

-func (c *CPUMetricExtractor) HasValue(rawMetric *RawMetric) bool {
-	if rawMetric.CPUStats != nil {
+func (c *CPUMetricExtractor) HasValue(rawMetric RawMetric) bool {
+	if !rawMetric.Time.IsZero() {


Using time is clever here, good idea

nathalapooja · 2024-01-10T02:44:51Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

-	rawMetic = &RawMetric{}
+func ConvertPodToRaw(podStat stats.PodStats) RawMetric {
+	var rawMetic RawMetric
+
 	rawMetic.Id = podStat.PodRef.UID
 	rawMetic.Name = podStat.PodRef.Name
 	rawMetic.Namespace = podStat.PodRef.Namespace

 	if podStat.CPU != nil {
 		rawMetic.Time = podStat.CPU.Time.Time


Shouldn't we null check the Time as well?podStat.CPU.Time?

Time is not pointer in podStat.CPU so it will always have default value or zero value

nathalapooja · 2024-01-10T02:45:06Z

receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractorhelpers.go

 	}

 	if podStat.Memory != nil {
-		rawMetic.MemoryStats = podStat.Memory
+		if rawMetic.Time.IsZero() {
+			rawMetic.Time = podStat.Memory.Time.Time


Shouldn't we null check the Time as well?podStat.Memory.Time?

same comment, time is not pointer in podStat.Memory, it will have default value or zero value

* Define structs for CPU and Memory and stats 1. Create new structs to represent CPU and memory stats in RawMetric. This removes RawMetric dependency on Kubelet CPU and memory stats. 2. Refactor existing RawMetric struct to use new CPU and memory stats. 3. Add more unit tests for extractorhelpers * Refactor: Remove parameters passing by reference in extractors * Remove extra comments in error

… commits (#168) Add support for Container Insights on Windows for EKS * Add kubelet summary API for Windows (#142) * CPU extractors with unit tests (#146) * Add memory extractors for pod and node level (#147) * Define structs for CPU and Memory stats (#149) * Add Container level metrics for CPU and memory resources. (#150) * Add storage metrics for container and node level (#151) * Add network metrics (#152) * Enable awscontainerinsights receiver to run inside Host Process container (#153) * Add HCS shim api as alternative source for metric provider (#154) * Add check for host process container before reading from hcshim (#156) * Fix CPU utilization percentage for Windows nodes (#161) * Add List of Metrics for Windows + Design (#166) * fix fstype (#164)

* Define structs for CPU and Memory and stats 1. Create new structs to represent CPU and memory stats in RawMetric. This removes RawMetric dependency on Kubelet CPU and memory stats. 2. Refactor existing RawMetric struct to use new CPU and memory stats. 3. Add more unit tests for extractorhelpers * Refactor: Remove parameters passing by reference in extractors * Remove extra comments in error # Conflicts: # receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/cpu_extractor.go # receiver/awscontainerinsightreceiver/internal/k8swindows/extractors/extractor.go # receiver/awscontainerinsightreceiver/internal/k8swindows/kubelet.go

* Add kubelet summary API for Windows (#142) * CPU extractors with unit tests (#146) * Add memory extractors for pod and node level (#147) * Define structs for CPU and Memory stats (#149) * Add Container level metrics for CPU and memory resources. (#150) * Add storage metrics for container and node level (#151) * Add network metrics (#152) * Enable awscontainerinsights receiver to run inside Host Process container (#153) * Add HCS shim api as alternative source for metric provider (#154) * Add check for host process container before reading from hcshim (#156) * Fix CPU utilization percentage for Windows nodes (#161) * Add List of Metrics for Windows + Design (#166) * fix fstype (#164) * Add windows build tag to fix building cw agent on Windows * Downgrade internal/aws/containerinsight from 0.92 0.89 * Separate unit tests in util.go specific for Windows * Fix util unit tests applicable for Windows * Fix goporto issue * Fix lint issue * Fix regression in unit tests caused due to rebasing mainline * Add unit tests for k8s Windows * Add unit test for kubelet client on Windows * Run DCGM scrapper only for CW agent on Linux * Separate out node Volume unit tests for Windows * Add changelog for Container Insights on Windows

nathalapooja reviewed Jan 8, 2024

View reviewed changes

chadpatel reviewed Jan 8, 2024

View reviewed changes

KlwntSingh force-pushed the raw_metric_refactor branch from 55d6287 to 9915664 Compare January 8, 2024 22:13

chadpatel reviewed Jan 8, 2024

View reviewed changes

KlwntSingh force-pushed the raw_metric_refactor branch from d6e986f to 9915664 Compare January 9, 2024 17:23

Refactor: Remove parameters passing by reference in extractors

bd5b551

chadpatel reviewed Jan 9, 2024

View reviewed changes

Remove extra comments in error

fb757ff

chadpatel approved these changes Jan 9, 2024

View reviewed changes

nathalapooja reviewed Jan 10, 2024

View reviewed changes

nathalapooja approved these changes Jan 10, 2024

View reviewed changes

KlwntSingh merged commit dfc7c15 into amazon-contributing:aws-cwa-ciwindows Jan 10, 2024
55 of 67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define structs for CPU and Memory stats #149

Define structs for CPU and Memory stats #149

KlwntSingh commented Jan 8, 2024 •

edited

Loading

nathalapooja left a comment

KlwntSingh commented Jan 8, 2024

chadpatel Jan 8, 2024

KlwntSingh Jan 8, 2024

chadpatel Jan 8, 2024

KlwntSingh Jan 9, 2024

chadpatel Jan 8, 2024

KlwntSingh Jan 8, 2024

chadpatel Jan 8, 2024

KlwntSingh Jan 9, 2024

chadpatel Jan 8, 2024

chadpatel Jan 8, 2024

chadpatel Jan 8, 2024

KlwntSingh Jan 9, 2024

chadpatel Jan 9, 2024

KlwntSingh Jan 9, 2024

chadpatel Jan 9, 2024

KlwntSingh Jan 9, 2024

chadpatel Jan 9, 2024

nathalapooja Jan 10, 2024

KlwntSingh Jan 10, 2024

nathalapooja Jan 10, 2024

KlwntSingh Jan 10, 2024

Define structs for CPU and Memory stats #149

Define structs for CPU and Memory stats #149

Conversation

KlwntSingh commented Jan 8, 2024 • edited Loading

nathalapooja left a comment

Choose a reason for hiding this comment

KlwntSingh commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KlwntSingh commented Jan 8, 2024 •

edited

Loading