[receiver/vcenterreceiver] Optimize vCenter VM network calls #32201

Caleb-Hurshman · 2024-04-05T18:01:04Z

Description:

Changes the method for collecting VMs used by the vccenterreceiver to the more time-efficient CreateContainerView method. This is the first step to addressing the issue linked below.

Link to tracking Issue: #31837

Testing: These changes were tested on an environment with 200+ virtual machines. The original collection time was ~80 seconds. Collection times with these changes are ~40 seconds.

Documentation: N/A

StefanKurek · 2024-04-08T15:58:55Z

receiver/vcenterreceiver/client.go

@@ -89,6 +93,7 @@ func (vc *vcenterClient) Datacenters(ctx context.Context) ([]*object.Datacenter,
 	return datacenters, nil
 }

+// Computes returns the ComputeResources (Hosts & Clusters) of the vSphere SDK for a given datacenter


I know this is my original comment, but I would modify the parenthesis to "(and ClusterComputeResources)"

StefanKurek · 2024-04-08T16:33:03Z

.chloggen/feat_faster-vcenter-vm-network-calls.yaml

+component: vcenterreceiver
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: "Changes the method for collecting VMs used by the `vccenterreceiver` to the more time efficient `CreateContainerView` method."


I would modify this a bit. really we are using a container view's "retrieve" method now

StefanKurek · 2024-04-08T16:35:19Z

receiver/vcenterreceiver/scraper.go

-			errs.AddPartial(1, err)
+		hostRef := vm.Summary.Runtime.Host
+		if hostRef == nil {
+			errs.AddPartial(1, errors.New("VM doesn't have a HostSystem"))


This should actually be more specific and include what VM has this issue.

StefanKurek · 2024-04-08T16:42:27Z

receiver/vcenterreceiver/scraper.go

@@ -349,12 +342,6 @@ func (v *vcenterMetricScraper) collectVMs(
 			}
 		}

-		hostname, err := vmHost.ObjectName(ctx)


minor and wouldn't hold up PR: since hostname is references a few times, it's still probably worth it to make a variable out of it after creating the hwSum variable

StefanKurek

From what I can see, there are fewer VMs being returned than before with this logic. Did you manually check this?

StefanKurek · 2024-04-08T20:22:41Z

From what I can see, there are fewer VMs being returned than before with this logic. Did you manually check this?

@Caleb-Hurshman ignore this. I was looking at a wrong metric which only reported when VMs were active

StefanKurek · 2024-04-10T04:49:57Z

With a few additional changes, collection times in our environment dropped from 90s to 23s-30s.

Rather than make a single call to get "shell" VM objects, and then making additional network calls for each VM's properties and performance metrics, we now are up front making a single network call for all VMs along with only the specific properties that we care about as well as a single network call for all of the performance metrics for all VMs. This data is stored and matched up as we cycle through each VM to build the various VM metrics/resources.

A follow up PR will be made to follow a similar pattern for Hosts/Clusters/Resource Pools, & Datastores

djaglowski · 2024-04-15T20:18:19Z

receiver/vcenterreceiver/testdata/metrics/expected.yaml

+        - key: vcenter.resource_pool.name
+          value:
+            stringValue: Resources
+        - key: vcenter.resource_pool.inventory_path
+          value:
+            stringValue: /Datacenter/host/Cluster/Resources


Is there a bug fix or other change here, in addition to the optimization? If not, why would the expected result change?

@djaglowski I think I would argue that I improved the tests here by having better XML responses being returned (which resulted in updated expected test results). Now the tests show the new resource pool attributes being added to VMs.

Thanks for explaining. Would it be possible to submit the changes to the mock server separately? I'm sure they're good changes but it would be helpful to validate the refactoring independently.

@djaglowski I can do this.

I do want to call out that the reason that these changes are there are because I think they pertain to the testing of this block of work (the VM code).

I made changes, the tests were broken.
So I started to update the tests, with new recordings from my new requests. But then I noticed that the expected results didn't seem to match what the code was doing (which was what I WAS expecting....if that makes any sense 😄 ).

Like I said, I can work around this and modify this PR to remove the expected results modifications. But it will make the tests in a less than optimal state IMO.

Will the test changes work without the other changes in this PR? I was thinking that we would first update tests, then rebase this PR so it is only refactoring for performance.

@djaglowski let me give that a try. I think that should work and not be too difficult to implement. 👍

@djaglowski rebased off the main which contained the merged in fixes from the previous PR

Makes a few small refactors for the vcenterreceiver scraper and metrics files. Fixes unit tests. Makes a single performance metrics network call for VMs rather than one per VM.

github-actions bot added the receiver/vcenter label Apr 5, 2024

github-actions bot requested review from djaglowski and schmikei April 5, 2024 18:03

StefanKurek suggested changes Apr 8, 2024

View reviewed changes

Caleb-Hurshman requested a review from StefanKurek April 9, 2024 18:01

StefanKurek force-pushed the feat/faster-vcenter-vm-network-calls branch from 4c5e92e to 4c0c6e1 Compare April 10, 2024 04:48

StefanKurek force-pushed the feat/faster-vcenter-vm-network-calls branch 2 times, most recently from 7618c4c to 94c08f9 Compare April 10, 2024 05:46

djaglowski reviewed Apr 15, 2024

View reviewed changes

Caleb-Hurshman and others added 5 commits April 16, 2024 15:17

change vm collection method

c2d3e62

remove destroy ctx

532a62d

fix client test

2b64fe9

pr feedback tweaks

94054b6

vcenterreceiver small refactors & single perf metric network call

c97d2ca

Makes a few small refactors for the vcenterreceiver scraper and metrics files. Fixes unit tests. Makes a single performance metrics network call for VMs rather than one per VM.

StefanKurek force-pushed the feat/faster-vcenter-vm-network-calls branch 2 times, most recently from 013fc38 to 9ba5070 Compare April 16, 2024 21:55

vcenterreceiver remove mockserver responses that are no longer in use

ad9bd99

StefanKurek force-pushed the feat/faster-vcenter-vm-network-calls branch from 9ba5070 to ad9bd99 Compare April 16, 2024 21:58

Merge branch 'main' into feat/faster-vcenter-vm-network-calls

a60f8d4

Caleb-Hurshman marked this pull request as ready for review April 17, 2024 12:57

Caleb-Hurshman requested review from a team and mx-psi April 17, 2024 12:57

github-actions bot assigned MovieStoreGuy Apr 17, 2024

djaglowski approved these changes Apr 17, 2024

View reviewed changes

djaglowski merged commit 11d2988 into open-telemetry:main Apr 17, 2024
158 checks passed

github-actions bot added this to the next release milestone Apr 17, 2024

StefanKurek deleted the feat/faster-vcenter-vm-network-calls branch April 18, 2024 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/vcenterreceiver] Optimize vCenter VM network calls #32201

[receiver/vcenterreceiver] Optimize vCenter VM network calls #32201

Caleb-Hurshman commented Apr 5, 2024

StefanKurek Apr 8, 2024

StefanKurek Apr 8, 2024

StefanKurek Apr 8, 2024

StefanKurek Apr 8, 2024

StefanKurek left a comment

StefanKurek commented Apr 8, 2024

StefanKurek commented Apr 10, 2024 •

edited

Loading

djaglowski Apr 15, 2024

StefanKurek Apr 15, 2024

djaglowski Apr 15, 2024

StefanKurek Apr 15, 2024 •

edited

Loading

djaglowski Apr 16, 2024

StefanKurek Apr 16, 2024

StefanKurek Apr 16, 2024

[receiver/vcenterreceiver] Optimize vCenter VM network calls #32201

[receiver/vcenterreceiver] Optimize vCenter VM network calls #32201

Conversation

Caleb-Hurshman commented Apr 5, 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

StefanKurek left a comment

Choose a reason for hiding this comment

StefanKurek commented Apr 8, 2024

StefanKurek commented Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKurek Apr 15, 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

StefanKurek commented Apr 10, 2024 •

edited

Loading

StefanKurek Apr 15, 2024 •

edited

Loading