feat(inputs.vsphere): Add VM memory configuration

[6monlambert]

Required for all PRs

• Updated associated README.md.
• Wrote appropriate unit tests.
• Pull request title or commits are in conventional commit format

resolves #11588

I added the **summary.config.memorySizeMB" in the init function of finder.go for Virtual Machines, then updated the structure of the objects and the tag population in the endpoint.go file.

I tested the feature compiling the code and replacing the telegraf binary on one VM. The poller using the modified version of Telegraf allows me to retrieve the memorySizeMB tag, whereas my other pollers with the latest version of Telegraf do not. No malfunction was observed after adding this feature.

[srebhan]

Thanks @6monlambert for the contribution. I have one simplification of the code and the question if the memory-size should really be a tag rather than a field... It's somewhat counter intuitive... Would you really query for memory-size???

[6monlambert]

Hello,

Thank you for your suggestion. I modified the code to simplify it in my last commit.

I was not able to find how to use this metric as a field, as my guess is that all the fields value are gathered using the list provided in the configuration. If this is indeed the case, the configured memory on the virtual machine is not retrievable. But maybe I missed something here.

From this, I decided to use a tag because I could not see any other solution. I personnaly need to use the actual memory size of the virtual machines in order to compute other values (quantity of RAM configured on VMs on a specific host/resource pool/cluster, recommendations on VM configuration).

I am open to any other suggestion, especially to understand if it is indeed possible to get this metric as a field.

Simon

[reimda]

It seems like this should be a field to me too. When a tag value changes it carries a meaning of a new time series. If someone changes the actual memory size of a vm, it doesn't mean the vm is a completely new vm, it's just an attribute of the same old vm that changed. A field models this better.

[6monlambert]

I perfectly understand the logic behind it. The reasoning is however the same when a VM migrates from one ESX to another one. It creates a new timeseries though the VM is not an entirely new one.

I can work on making this metric a field, though I need to know if it is possible. The metric is not available in the virtual machine metrics which is, to me, the list of fields that we can gather with telegraf.

Any help would be appreciated on this. Thanks !

Simon

[srebhan]

@6monlambert why not just add the field in line 1247?

[6monlambert]

Hello,

Said like this it looks very easy but after reviewing the code that looks way more complex to do.

In the case that interests us (i.e. Virtual Machines), Line 1170 loops through all the VMs, and then for each VM we loop through the values that interest us (defined in configuration).

We put all the the metric inside a bucket, and then the buckets are emitted in line 1247.

Which means, to me, that I have to manually create a bucket corresponding to the (let's say) vm_mem_size field, that I should be able to retrieve with the objectRef (as I do now with objectRef.memorySizeMB)

I tried a quick implementation but there are a lot of missing fields, I'm not even able to get the value with the reference to the object and I copied the code from line 1194 to line 1238 with a manual definition of the metric name, but the code did not compile.

I'm guessing there is an easier way to do it but I don't see it at the moment.
If you have an idea, that could be useful. I'll try to get back on it soon.

Simon

[srebhan]

Honestly speaking, I don't quite understand why the code is so complex currently and does not have a populateTags and a populateFields call that directly create the corresponding maps instead of having the bucket intermediate step.
However, I see what you mean, you probably need to extend the buckets which in turn forces you to touch a lot of code. Is my understanding correct?

If so, I can live with the tag "solution" for now, but this plugin needs an overhaul IMO. @reimda what do you think?

[telegraf-tiger]

Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Page. Thank you!

[DStrand1]

Reopening + rebasing this PR since we decided we want this change without a more extensive plugin refactor

[srebhan]

Looks good. The only concern is the field name. Are all fields camel-cased or snake? We should stay in the convention to not make this alien.

[DStrand1]

Looks good. The only concern is the field name. Are all fields camel-cased or snake? We should stay in the convention to not make this alien.

Camel-cased it looks like, for example disk.maxTotalLatency.latest

[srebhan]

Ok then naming is fine with me. Maybe we should clarify what it is in the README and add the memory-reservation as you suggested. What do you think?

[DStrand1]

Ok then naming is fine with me. Maybe we should clarify what it is in the README and add the memory-reservation as you suggested. What do you think?

I agree, I'll update the PR

[telegraf-tiger]

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

👍 This pull request doesn't change the Telegraf binary size

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

[srebhan]

Looks awesome! Thanks @DStrand1!