[receiver/vcenter] Adds vCenter VM CPU readiness metric #33608

BominRahmani · 2024-06-18T02:04:08Z

Description:

This PR adds the vcenter.vm.cpu.readiness metric.
More information on this metric can be found here.
Link to tracking Issue: #33607

Testing:
The metric was scraped from a test vCenter environment, and golden test files were updated to reflect the addition of the metric.

Documentation:
Documentation was updated according to the metadata.yaml

StefanKurek

@BominRahmani Overall looks OK. Have a few questions though. Also think it needs an integration test update.

receiver/vcenterreceiver/metadata.yaml

receiver/vcenterreceiver/testdata/metrics/expected-all-enabled.yaml

receiver/vcenterreceiver/testdata/metrics/expected.yaml

receiver/vcenterreceiver/metrics.go

StefanKurek · 2024-06-18T21:40:05Z

receiver/vcenterreceiver/metrics.go

+
+	// OverallCpuReadiness is only available in vSphere API 7.0
+	// https://vdc-repo.vmware.com/vmwb-repository/dcr-public/d1902b0e-d479-46bf-8ac9-cee0e31e8ec0/07ce8dbd-db48-4261-9b8f-c6d3ad8ba472/vim.vm.Summary.QuickStats.html
+	vsphereAPIVersion, err := version.NewVersion(v.scrapeData.apiVersion)


I went back and forth on whether I was going to comment more on this or just leave it be.

It seems like this logic could be moved to a new function (or maybe even to the client as a helper function). The client could be reworked to have a private method like "refreshVsphereAPIVersion" which stores its own version. We could make sure we call this once a collection (or on client initialization). Then we could have another method vsphereAPIVersionMeetsMin(minVsphereAPIVersion string) bool that would be used here where you just pass in "7.0.0". It could do all this logic (making use of the stored version (possibly calling refresh if it's empty) and then return a true/false for the conditional for lines 158/159. This would be reusable for other cases too.

Ultimately I think I'm fine with it as is, just thinking that it feels a little out of place here and would likely be better placed elsewhere. What do you think?

This is probably a good idea, there may be more of these metrics that require a check on the API in the future. I'll look into changing it up!

That will be a great add-on, Readiness is the most important CPU metric for a VM. I had started to worked on a PR to add it, glad I checked here again!

For the documentation you refer to, maybe we can use the one hosted by Broadcom as it will be updated there: https://developer.broadcom.com/xapis/virtual-infrastructure-json-api/latest/data-structures/VirtualMachineQuickStats/

The API version requirement (7.0.0) for overallCpuReadiness is not in it, not sure why, as there are other recommendations. Same for grantedMemory.

The others metrics that required a recent API version are not used in the receiver yet:

activeMemory

memoryTierStats

But we could probably add them at some point, especially grantedMemory, so good idea @StefanKurek for the private method.

I can check the other requirements for Host, Cluster and Datastore.

@jlg-io That JSON API is a totally different (and would require major changes to the vcenterreceiver) from how we're currently collecting. In addition, it looks like it's only been available since vCenter 8.0+. I do agree that a note in the README would be a good add.

@StefanKurek, my bad! I guess the right one is there: https://dp-downloads.broadcom.com/api-content/apis/API_VMA_001/8.0U2/html/vim.vm.Summary.QuickStats.html

It has been updated more recently than the one on VMware website, so I guess it will be the reference for the future

Thanks! I'll add in a section in the readme and also update the link in the comments.

jlg-io

Great work!

Maybe we could update vcenterreceiver/README.md to either drop support for 6.7 or add a comment that overallCpuReadiness won't work with this version?

jlg-io · 2024-06-19T17:02:10Z

receiver/vcenterreceiver/metrics.go

+
+	// OverallCpuReadiness is only available in vSphere API 7.0
+	// https://vdc-repo.vmware.com/vmwb-repository/dcr-public/d1902b0e-d479-46bf-8ac9-cee0e31e8ec0/07ce8dbd-db48-4261-9b8f-c6d3ad8ba472/vim.vm.Summary.QuickStats.html
+	vsphereAPIVersion, err := version.NewVersion(v.scrapeData.apiVersion)


That will be a great add-on, Readiness is the most important CPU metric for a VM. I had started to worked on a PR to add it, glad I checked here again!

For the documentation you refer to, maybe we can use the one hosted by Broadcom as it will be updated there: https://developer.broadcom.com/xapis/virtual-infrastructure-json-api/latest/data-structures/VirtualMachineQuickStats/

The API version requirement (7.0.0) for overallCpuReadiness is not in it, not sure why, as there are other recommendations. Same for grantedMemory.

The others metrics that required a recent API version are not used in the receiver yet:

activeMemory

memoryTierStats

But we could probably add them at some point, especially grantedMemory, so good idea @StefanKurek for the private method.

I can check the other requirements for Host, Cluster and Datastore.

receiver/vcenterreceiver/README.md

receiver/vcenterreceiver/metrics.go

receiver/vcenterreceiver/metadata.yaml

receiver/vcenterreceiver/testdata/metrics/expected.yaml

receiver/vcenterreceiver/client.go

StefanKurek

Looks good enough to move out of Draft form. Thanks!

receiver/vcenterreceiver/README.md

djaglowski · 2024-06-28T15:42:56Z

receiver/vcenterreceiver/client.go

@@ -74,6 +76,7 @@ func (vc *vcenterClient) EnsureConnection(ctx context.Context) error {
 	vc.finder = find.NewFinder(vc.vimDriver)
 	vc.pm = performance.NewManager(vc.vimDriver)
 	vc.vm = view.NewManager(vc.vimDriver)
+	vc.apiVersion = vc.getVsphereAPIVersion()


If we drop support for 6.7 we don't need this check. Perhaps there is a reasonable way to fail gracefully if the server does not support the new metric?

I think we can just drop support for 6.7 in this case. The problem with this metric is it doesn't really fail, rather it returns 0 if the server doesn't support the metric, but 0 in of itself is a valid value.

As 6.7 is "End of General Support" since October 15, 2022 (https://core.vmware.com/blog/reminder-vsphere-6567-end-general-support), it would make sense to drop the support here.

@djaglowski To drop support for a version do we need to make a new pr / add warnings anywhere? or are we allowed to just remove it from the readme.

@BominRahmani, a separate PR would be ideal but I think it's ok to do here as long as there's a separate changelog entry for it. You can just add a second changelog yaml file and note it there.

.chloggen/add_vcenter_vm_cpu_readiness_metric.yaml

github-actions bot added the receiver/vcenter label Jun 18, 2024

github-actions bot requested review from djaglowski, schmikei and StefanKurek June 18, 2024 02:04

BominRahmani force-pushed the feat/add-vcenter-vm-cpu-metric branch 2 times, most recently from 6c99f2f to f4a517e Compare June 18, 2024 03:09

StefanKurek suggested changes Jun 18, 2024

View reviewed changes

BominRahmani force-pushed the feat/add-vcenter-vm-cpu-metric branch from 9a14bb3 to 2d172a6 Compare June 18, 2024 19:18

StefanKurek reviewed Jun 18, 2024

View reviewed changes

jlg-io reviewed Jun 19, 2024

View reviewed changes

BominRahmani added 10 commits June 19, 2024 16:44

New metric added + documentation updated

6346350

Used Golden to update expected.yaml metrics

a02c299

Used Golden to update expected-all-enabled.yaml metrics

0022261

removed comments

0706198

added changelog

af8d74b

make gotidy

be98d5e

make generate

53d48f5

made checkAPI call more efficient

7f48e32

updated tests + mdatagen to include warning

a397374

Changed location of api call + added helper methods

6de60f2

BominRahmani force-pushed the feat/add-vcenter-vm-cpu-metric branch from c65c735 to 6de60f2 Compare June 19, 2024 20:50

test data update

9d968d7

StefanKurek suggested changes Jun 20, 2024

View reviewed changes

moved meetMin() logic to client

c742018

BominRahmani force-pushed the feat/add-vcenter-vm-cpu-metric branch from 55d2863 to c742018 Compare June 20, 2024 14:43

StefanKurek reviewed Jun 20, 2024

View reviewed changes

receiver/vcenterreceiver/client.go Outdated Show resolved Hide resolved

added test cases

3430d83

BominRahmani force-pushed the feat/add-vcenter-vm-cpu-metric branch from c7ca22d to 3430d83 Compare June 20, 2024 21:01

StefanKurek approved these changes Jun 21, 2024

View reviewed changes

BominRahmani marked this pull request as ready for review June 21, 2024 01:59

BominRahmani requested a review from a team June 21, 2024 01:59

github-actions bot assigned bryan-aguilar Jun 21, 2024

BominRahmani mentioned this pull request Jun 21, 2024

[receiver/vcenter] Additional metrics for vCenter receiver #33607

Closed

4 tasks

BominRahmani added 2 commits June 24, 2024 21:28

updated chlog with correct issue PR

67432e2

updated metadata warning

0ed6b92

djaglowski reviewed Jun 28, 2024

View reviewed changes

removed version check + ended support for 6.7

b6de73f

StefanKurek reviewed Jul 1, 2024

View reviewed changes

.chloggen/add_vcenter_vm_cpu_readiness_metric.yaml Outdated Show resolved Hide resolved

BominRahmani requested a review from djaglowski July 1, 2024 13:25

Updated changelog to breaking

d77a71e

BominRahmani force-pushed the feat/add-vcenter-vm-cpu-metric branch from c39b81d to d77a71e Compare July 1, 2024 13:41

Seperated changelog to include breaking

65eddd2

djaglowski approved these changes Jul 1, 2024

View reviewed changes

djaglowski merged commit 0e9dde7 into open-telemetry:main Jul 1, 2024
154 checks passed

github-actions bot added this to the next release milestone Jul 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/vcenter] Adds vCenter VM CPU readiness metric #33608

[receiver/vcenter] Adds vCenter VM CPU readiness metric #33608

BominRahmani commented Jun 18, 2024

StefanKurek left a comment

StefanKurek Jun 18, 2024 •

edited

Loading

BominRahmani Jun 18, 2024

jlg-io Jun 19, 2024

StefanKurek Jun 19, 2024

jlg-io Jun 19, 2024 •

edited

Loading

BominRahmani Jun 19, 2024

jlg-io left a comment

jlg-io Jun 19, 2024

StefanKurek left a comment

djaglowski Jun 28, 2024

BominRahmani Jun 28, 2024

jlg-io Jun 28, 2024

BominRahmani Jun 28, 2024

djaglowski Jul 1, 2024

[receiver/vcenter] Adds vCenter VM CPU readiness metric #33608

[receiver/vcenter] Adds vCenter VM CPU readiness metric #33608

Conversation

BominRahmani commented Jun 18, 2024

StefanKurek left a comment

Choose a reason for hiding this comment

StefanKurek Jun 18, 2024 • edited Loading

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

jlg-io Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlg-io left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKurek 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

StefanKurek Jun 18, 2024 •

edited

Loading

jlg-io Jun 19, 2024 •

edited

Loading