Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(inputs.vsphere): Add VM memory configuration #11591

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

6monlambert
Copy link
Contributor

Required for all PRs

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.

@telegraf-tiger telegraf-tiger bot added area/vsphere feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Aug 2, 2022
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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???

plugins/inputs/vsphere/endpoint.go Outdated Show resolved Hide resolved
@srebhan srebhan self-assigned this Aug 2, 2022
@6monlambert
Copy link
Contributor Author

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
Copy link
Contributor

reimda commented Aug 2, 2022

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
Copy link
Contributor Author

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
Copy link
Member

srebhan commented Aug 5, 2022

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

@6monlambert
Copy link
Contributor Author

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
Copy link
Member

srebhan commented Sep 27, 2022

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?

@powersj powersj added the waiting for response waiting for response from contributor label Oct 20, 2022
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Nov 4, 2022

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!

@telegraf-tiger telegraf-tiger bot closed this Nov 4, 2022
@DStrand1 DStrand1 reopened this Aug 13, 2024
@DStrand1 DStrand1 removed the waiting for response waiting for response from contributor label Aug 13, 2024
@DStrand1 DStrand1 assigned DStrand1 and unassigned srebhan Aug 13, 2024
@DStrand1
Copy link
Member

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

@telegraf-tiger
Copy link
Contributor

@DStrand1 DStrand1 assigned powersj and srebhan and unassigned DStrand1 Aug 13, 2024
@DStrand1 DStrand1 added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 13, 2024
@@ -1338,6 +1341,9 @@ func (e *Endpoint) populateTags(objectRef *objectRef, resourceType string, resou
if resourceType == "vm" && objectRef.rpname != "" {
t["rpname"] = objectRef.rpname
}
if resourceType == "vm" && objectRef.memorySizeMB != 0 {
t["memorySizeMB"] = strconv.Itoa(int(objectRef.memorySizeMB))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the original concern was that memory size, a numeric value, should not be a tag. Happy to see this as a field, but setting as a tag would cause a lot of issues with cardinality.

@srebhan srebhan changed the title feat(inputs.vsphere): add VM memory configuration as a tag feat(inputs.vsphere): Add VM memory configuration Aug 16, 2024
@powersj powersj assigned DStrand1 and unassigned powersj and srebhan Aug 16, 2024
@powersj powersj removed the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vsphere feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inputs.vsphere - Add VMs memory configuration as a tag
5 participants