CPU extractors with unit tests #146

KlwntSingh · 2023-12-27T02:35:35Z

Description:
Add CPU extractors from kubelet summary API

1. Make cadvisor helper func's public to be used in k8swindows extractor
2. Add CPU extractor and add utilization fields
3. Add unit test for CPU extractor.
4. Add unit test data for kubelet summary API
5. Add helper func to convert Pod and Node summary stats to RawMetric

Testing:

Unit tests passed in local

1. Make cadvisor helper func's public to be used in k8swindows extractor 2. Add CPU extractor and add utilization fields 3. Add unit test for CPU extractor. 4. Add unit test data for kubelet summary API 5. Add helper func to convert Pod and Node summary stats to RawMetric

chadpatel · 2023-12-27T15:29:22Z

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

+import (
+	ci "github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/containerinsight"
+	awsmetrics "github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/metrics"
+	cExtractor "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/awscontainerinsightreceiver/internal/cadvisor/extractors"


is there a better import name than cExtractor?

IIRC our linter will want the import names to be all lower case, i.e. awsmetrics

Changed cExtractor to cextractor.
I saw import following similar pattern in cadvisor code where import was alias as cInfo but for now i changed it to cextractor

got it, I didn't realize it as following an established pattern. If there was no pattern I would say "containerextractor" is more clear and avoids non-obvious abbreviations. If there is an established pattern than cExtractor seems fine.

Approving this rev, curious what the other agents team review thinks. This can be changed in a later PR

Replaced cextractor back to cExtractor

chadpatel · 2023-12-27T15:31:47Z

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

+
+// ConvertNodeToRaw Converts Kubelet Node stats to RawMetric.
+func ConvertNodeToRaw(nodeStat *stats.NodeStats) *RawMetric {
+	var rawMetic *RawMetric


is there any risk of null pointers when dereferencing here?

Addressed this issue by adding nil checks

1. Changed cExtractor to cextractors 2. Add nil checks to avoid panic during pointer deferences

nathalapooja

Added some nit comments. Everything else looks good

nathalapooja · 2023-12-29T16:06:51Z

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

Why did we change this file name?

It was extractorhelpers_test before, I was not able to import helper funcs from test file in file in k8swindows package. That's why i changed it to extractorhelpers.

nathalapooja · 2023-12-29T16:12:20Z

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

+	MemoryStats *stats.MemoryStats
+}
+
+type MetricExtractor interface {


Can we implement similar to MetricExtractor interface which has method HasValue using the CPUstats?

type MetricExtractor interface {
HasValue(*cinfo.ContainerInfo) bool
GetValue(info *cinfo.ContainerInfo, mInfo CPUMemInfoProvider, containerType string) []*CAdvisorMetric
Shutdown() error
}

I planned to add this func in future PRs but i have added this func now. Thanks!

nathalapooja · 2023-12-29T16:18:18Z

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

The name of the file need to be corrected to extractor_helpers_test.go to match cAdvisor

actually cAdvisor file is extractorhelpers, I am keeping file names without snake pattern in name to keep consistent with cAdvisor.

nathalapooja · 2023-12-29T16:18:37Z

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

Name of the file can be corrected to extractor_helpers.go

I add s at last but i am still keeping extractorhelpers to keep consistent with cAdvisor and other pakcages where name doesn't follow snake pattern.

1. Added missing HasValue func in extractors 2. Replaced cextractor with cExtractor 3. Corrected extractorhelper name with missing characters

KlwntSingh

Addressed some of comments and provided explanation for other comments

KlwntSingh · 2023-12-29T16:27:25Z

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

It was extractorhelpers_test before, I was not able to import helper funcs from test file in file in k8swindows package. That's why i changed it to extractorhelpers.

KlwntSingh · 2023-12-29T16:29:23Z

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

+	MemoryStats *stats.MemoryStats
+}
+
+type MetricExtractor interface {


I planned to add this func in future PRs but i have added this func now. Thanks!

KlwntSingh · 2023-12-29T16:32:23Z

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

actually cAdvisor file is extractorhelpers, I am keeping file names without snake pattern in name to keep consistent with cAdvisor.

KlwntSingh · 2023-12-29T16:33:37Z

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

I add s at last but i am still keeping extractorhelpers to keep consistent with cAdvisor and other pakcages where name doesn't follow snake pattern.

KlwntSingh · 2023-12-29T16:52:49Z

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

+import (
+	ci "github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/containerinsight"
+	awsmetrics "github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/metrics"
+	cExtractor "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/awscontainerinsightreceiver/internal/cadvisor/extractors"


Replaced cextractor back to cExtractor

* Add CPU extractors from kubelet summary API 1. Make cadvisor helper func's public to be used in k8swindows extractor 2. Add CPU extractor and add utilization fields 3. Add unit test for CPU extractor. 4. Add unit test data for kubelet summary API 5. Add helper func to convert Pod and Node summary stats to RawMetric * Refactor code 1. Changed cExtractor to cextractors 2. Add nil checks to avoid panic during pointer deferences * Refactored code 1. Added missing HasValue func in extractors 2. Replaced cextractor with cExtractor 3. Corrected extractorhelper name with missing characters

… 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)

* Add CPU extractors from kubelet summary API 1. Make cadvisor helper func's public to be used in k8swindows extractor 2. Add CPU extractor and add utilization fields 3. Add unit test for CPU extractor. 4. Add unit test data for kubelet summary API 5. Add helper func to convert Pod and Node summary stats to RawMetric * Refactor code 1. Changed cExtractor to cextractors 2. Add nil checks to avoid panic during pointer deferences * Refactored code 1. Added missing HasValue func in extractors 2. Replaced cextractor with cExtractor 3. Corrected extractorhelper name with missing characters # Conflicts: # receiver/awscontainerinsightreceiver/internal/cadvisor/extractors/cpu_extractor.go # receiver/awscontainerinsightreceiver/internal/cadvisor/extractors/diskio_extractor.go # receiver/awscontainerinsightreceiver/internal/cadvisor/extractors/extractor.go # receiver/awscontainerinsightreceiver/internal/cadvisor/extractors/mem_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

KlwntSingh requested a review from mxiamxia as a code owner December 27, 2023 02:35

KlwntSingh changed the base branch from main to aws-cwa-ciwindows December 27, 2023 02:36

chadpatel reviewed Dec 27, 2023

View reviewed changes

Refactor code

3af42c8

1. Changed cExtractor to cextractors 2. Add nil checks to avoid panic during pointer deferences

chadpatel previously approved these changes Dec 29, 2023

View reviewed changes

nathalapooja reviewed Dec 29, 2023

View reviewed changes

Refactored code

d637042

1. Added missing HasValue func in extractors 2. Replaced cextractor with cExtractor 3. Corrected extractorhelper name with missing characters

KlwntSingh dismissed chadpatel’s stale review via d637042 December 29, 2023 16:49

nathalapooja approved these changes Dec 29, 2023

View reviewed changes

chadpatel approved these changes Dec 29, 2023

View reviewed changes

KlwntSingh commented Dec 29, 2023

View reviewed changes

KlwntSingh merged commit 1ad51be into amazon-contributing:aws-cwa-ciwindows Dec 29, 2023
55 of 67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPU extractors with unit tests #146

CPU extractors with unit tests #146

KlwntSingh commented Dec 27, 2023

chadpatel Dec 27, 2023

chadpatel Dec 27, 2023

KlwntSingh Dec 29, 2023

chadpatel Dec 29, 2023

KlwntSingh Dec 29, 2023

chadpatel Dec 27, 2023

KlwntSingh Dec 29, 2023

nathalapooja left a comment

nathalapooja Dec 29, 2023

KlwntSingh Dec 29, 2023

nathalapooja Dec 29, 2023

KlwntSingh Dec 29, 2023

nathalapooja Dec 29, 2023

KlwntSingh Dec 29, 2023

nathalapooja Dec 29, 2023

KlwntSingh Dec 29, 2023

KlwntSingh left a comment

KlwntSingh Dec 29, 2023

KlwntSingh Dec 29, 2023

KlwntSingh Dec 29, 2023

KlwntSingh Dec 29, 2023

KlwntSingh Dec 29, 2023

CPU extractors with unit tests #146

CPU extractors with unit tests #146

Conversation

KlwntSingh commented Dec 27, 2023

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

nathalapooja left a 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 left a 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