[Metricbeat][vSphere] Support for configurable IntervalId for performance API

[kush-elastic]

• Enhancement

The vSphere Performance API offers support for Intervals and Levels, allowing users to collect various types of counters and metrics from vSphere environments. Depending on the configured Interval, the API provides detailed or aggregated performance data from vSphere.

For more information, refer to the following resources:

• Performance Data Collection
• Data Collection Intervals
• Data Collection Levels

We can add support for configuration that help users collect the specific performance data they need.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

• Closes https://github.com/elastic/obs-integration-team/issues/36

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kush-elastic? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

[devamanv]

we should make sure that results has at least one value like we have done for samples. Or it could be a potential panic.

[kush-elastic]

There is another check before it if there are zero samples for results then it will return from there itself and if there is any issue while converting it to series we will return with error.

[devamanv]

Got your point. However, looking at the definition of ToMetricSeries, I still see two potential issues with it

• If the type assertion s, ok := series[i].(*types.PerfEntityMetric) fails, the function panics. This could lead to a crash unless you want this behavior. Should we return an error instead of panicking to handle unexpected input more gracefully?
• The line v := s.Value[j].(*types.PerfMetricIntSeries) assumes that all metrics in s.Value are of type *types.PerfMetricIntSeries. If that's not guaranteed (or there are other possible types), this will panic.

Both cases could potentially result in a crash. Unless we intend to abort the execution or expect this to happen, we should be handling it.

[shmsr]

I agree with Aman. The only case it is returning an error is when CounterInfoByKey fails. Even if it passes; it doesn't guarantee that the returned slice from ToMetricSeries will have at least 1 element.

So, we should ideally handle it here just to be on the safe side.

[kush-elastic]

You are right about panic.
We should probably handle it.
Either we handle panic or we need to manually convert to the series.
I can create new method as well because we are already doing CounterInfoByName. I can just reuse it.
And for other stuff we can return error instead of Panic.
What do you think?

[kush-elastic]

Adding recover to make sure code does not fail.

[devamanv]

@kush-elastic I'm thinking if instead of just logging, would setting the returned error value to this would be more useful? From the official Go blog about recover:

The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values

One such example would be this. Here it explicitly sets the error value for the caller to handle it.
Either way, use of recover looks like a good way to handle. I will let you decide on how to treat this error.

[kush-elastic]

got it.

[shmsr]

I checked this pretty quick; will request you to verify as well as I am busy in some other work:

Query: https://github.com/vmware/govmomi/blob/c1151f859ba649771c4ab60992953019904355fe/performance/manager.go#L268

Query calls QueryPerf and that makes sure that the types are right: https://github.com/vmware/govmomi/blob/c1151f859ba649771c4ab60992953019904355fe/simulator/performance_manager.go#L219

Except for length, I don't think we need to check for types.

[shmsr]

It can only iff https://github.com/vmware/govmomi/blob/c1151f859ba649771c4ab60992953019904355fe/simulator/performance_manager.go#L249 is done. But you are not setting the format in here:

spec := types.PerfQuerySpec{
		Entity:     hst.Reference(),
		MetricId:   metricIds,
		MaxSample:  1,
		IntervalId: refreshRate,
	}

So safe.

[ishleenk17]

shall we also mention that this will not impact the metrics collection other than perf metrics.

[kush-elastic]

Nice idea. let me do that.

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 36-vsphere-support-for-configurable-intervalid-for-performance-api upstream/36-vsphere-support-for-configurable-intervalid-for-performance-api
git merge upstream/main
git push upstream 36-vsphere-support-for-configurable-intervalid-for-performance-api

[ishleenk17]

Once you incorporate the documentation change, GTG