feat(inputs.vsphere): Allow adding custom properties as fields#19161
feat(inputs.vsphere): Allow adding custom properties as fields#19161tguenneguez wants to merge 19 commits into
Conversation
|
Thanks for your contribution @tguenneguez! Can you please check and fix the linter issues!? Furthermore, you can't check "I've used no AI" and "I've used AI" at the same time. :-) Please fix the checkboxes in the PR description. :-) |
|
Yes, I have seen it, but I do lot off think in the same time so, i change when I have time. |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
srebhan
left a comment
There was a problem hiding this comment.
Thanks for your contribution @tguenneguez! I do have some comments in the code. IMO, the most important issues are
- You are changing unrelated parts of the code resulting in a larger diff than necessary. Please avoid changing code you don't need to touch in this PR. If you want it changed, do it in a separate PR!
- Spelling and comments are French AFAICS. Please check spelling and convert everything to English please!
Looking forward to your update of the PR!
| return cvs | ||
| } | ||
|
|
||
| func (e *endpoint) loadCustomProperties(entity interface{}, propertieInclude []string) map[string]interface{} { |
There was a problem hiding this comment.
propertieInclude is a typo. Did you mean propertiesInclude? The same thing occurs in multiple places throughout the PR...
There was a problem hiding this comment.
Exact, that's because of in config, you use :
CustomAttributeInclude
But it's a list of attibutes.
In this way I think it will be great to be "propertieInclude" every where ?
| // resourceInfo is a utility class grouping a type and relevant parameters. | ||
| type resourceInfo struct { | ||
| resType string | ||
| custoFields []string |
There was a problem hiding this comment.
This abbreviation saves one character while humpering readability a lot. Please be generous and add the m here. :-)
There was a problem hiding this comment.
It's because of resourceFilter containe a resType
| fields = append(fields, af...) | ||
| } | ||
| fields = append(fields, resourceInfo.custoFields...) | ||
| uniqueFields := removeDuplicates(fields) |
There was a problem hiding this comment.
fields are defined by resourceInfo.custoFields which in turn stem from user-settings, don't they? If so, wouldn't it be better to deduplicate those user-settings in Init instead of doing it in every gather cycle?
There was a problem hiding this comment.
It will be, but they are agregate to addFields[resourceInfo.resType].
To be able to do this, it will be necessaire to agregate this info only one time...
There was a problem hiding this comment.
No change display ? Can you explain me ...
| VMMetricExclude []string `toml:"vm_metric_exclude"` | ||
| VMInclude []string `toml:"vm_include"` | ||
| VMExclude []string `toml:"vm_exclude"` | ||
| VMPropertieInclude []string `toml:"vm_propertie_include"` |
There was a problem hiding this comment.
Typo! Please fix spelling! Same for all other options you introduced.
There was a problem hiding this comment.
You speak about :
VMMetricExclude vs VmMetricExclude ?
| err = f.find(t.Context(), "Datacenter", "/DC0", &dc) | ||
| ri := resourceInfo{ | ||
| resType: "Datacenter", | ||
| custoFields: nil, |
There was a problem hiding this comment.
Don't explicitly set fields to values they have anyway by Golang's defaults. Same for all other instances.
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
…uenneguez/telegraf into add-custom-properties-vsphere
Summary
Add the capabilitie to collecte properties of elements.
For sample :
This change collect data for VM Power Off. There was no information about this subtility.
Checklist
Related issues
resolves #18858