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

added new metrics for vcenter receiver VM performance related met… #37489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samiura
Copy link
Contributor

@samiura samiura commented Jan 25, 2025

Description

This PR adds the following VM performance metrics for vcenter.

  1. vcenter.vm.cpu.time
  2. vcenter.vm.network.multicast.packet.rate
  3. vcenter.vm.network.broadcast.packet.rate

More information on these metrics can be found here. and also succinctly described here

#37488

Testing

The metrics were scraped from a test vCenter environment, and golden test files were updated accordingly to reflect the addition of the metric.

Documentation

Documentation was updated according to the metadata.yaml
evidence1
Screenshot 2025-01-29 at 10 48 33 AM
Screenshot 2025-01-29 at 10 53 54 AM
Screenshot 2025-01-29 at 10 54 11 AM

@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch 2 times, most recently from 816b219 to 8994147 Compare January 25, 2025 01:22
@samiura samiura marked this pull request as ready for review January 25, 2025 01:38
@samiura samiura requested a review from a team as a code owner January 25, 2025 01:38
@samiura samiura requested a review from fatsheep9146 January 25, 2025 01:38
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch from 8994147 to 9ddb446 Compare January 27, 2025 17:29
receiver/vcenterreceiver/metrics.go Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metrics.go Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metrics.go Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metrics.go Outdated Show resolved Hide resolved
receiver/vcenterreceiver/scraper_test.go Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metrics.go Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch 4 times, most recently from 35288a1 to 71df9a7 Compare January 28, 2025 00:28
@samiura samiura requested a review from StefanKurek January 28, 2025 00:30
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch from 71df9a7 to 5864227 Compare January 28, 2025 16:54
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch 3 times, most recently from 8945f4f to 83c94de Compare January 28, 2025 17:30
Copy link
Contributor

@StefanKurek StefanKurek left a comment

Choose a reason for hiding this comment

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

I think it mostly looks pretty good now, but I still have a couple of questions for possible improvements.

receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch 5 times, most recently from cc899eb to 3fe930c Compare January 28, 2025 20:21
@samiura samiura requested a review from StefanKurek January 28, 2025 20:22
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch from 3fe930c to 6c76884 Compare January 28, 2025 20:33
@StefanKurek
Copy link
Contributor

@samiura something doesn't look quite right with the CPU Times FYI. I would have expected the values to all be < 100. So there's probably still some work to figure out what's going on with the data there.

Also after digging a bit, the cpu.wait.summation included idle state's wait time too if you look at the description here in this link(Total CPU time spent in wait state.The wait total includes time spent the CPU Idle, CPU Swap Wait, and CPU I/O Wait states.)

Well even then it should be less than 100 by itself if we were calculating correctly (and the simulator is giving valid numbers....which it may not be).

Just thinking out loud, are we sure the summation are taken over 20s time period for api-version 7.0 and 8.0? Just thinking out loud.

So, according to Broadcom (Vmware's parent) the default vcenter setting for intervals is 20seconds. I wonder whether that is the case for VCSim.

I have pushed a new commit and instead of assuming 20 second interval rate for gathering performance metrics, I have getting that interval period from VSphere API as we have done for VSANMetrics gathering in the code.

@samiura The interval for performance metrics is hardcoded in the receiver to be 20s FYI. Even so, your suggestion is good practice. I'm curious if it changes the values you're seeing from the simulator?

@samiura
Copy link
Contributor Author

samiura commented Jan 29, 2025

@samiura something doesn't look quite right with the CPU Times FYI. I would have expected the values to all be < 100. So there's probably still some work to figure out what's going on with the data there.

Also after digging a bit, the cpu.wait.summation included idle state's wait time too if you look at the description here in this link(Total CPU time spent in wait state.The wait total includes time spent the CPU Idle, CPU Swap Wait, and CPU I/O Wait states.)

Well even then it should be less than 100 by itself if we were calculating correctly (and the simulator is giving valid numbers....which it may not be).

Just thinking out loud, are we sure the summation are taken over 20s time period for api-version 7.0 and 8.0? Just thinking out loud.

So, according to Broadcom (Vmware's parent) the default vcenter setting for intervals is 20seconds. I wonder whether that is the case for VCSim.
I have pushed a new commit and instead of assuming 20 second interval rate for gathering performance metrics, I have getting that interval period from VSphere API as we have done for VSANMetrics gathering in the code.

@samiura The interval for performance metrics is hardcoded in the receiver to be 20s FYI. Even so, your suggestion is good practice. I'm curious if it changes the values you're seeing from the simulator?

I have not seen any values over 100 as it should logically be the case. It seems to be sane. Not sure why but I think it looks solid now (screenshots updated). Also, from I am running just the default valuedvcsim binary without any parameters to make things simplistic.

@StefanKurek
Copy link
Contributor

@samiura Sounds good! Chances are the simulator was ignoring our request for a 20s interval and was using something else instead. So now that that interval is dynamically part of the calculation, we're good. This shouldn't come up in a real environment, but the code is safer either way. Thanks for all the work!

@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch 3 times, most recently from 784e4fe to f526137 Compare January 30, 2025 16:27
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch 8 times, most recently from 866b0c4 to 493a703 Compare January 31, 2025 22:38
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM

receiver/vcenterreceiver/metrics.go Show resolved Hide resolved
@pjanotti
Copy link
Contributor

pjanotti commented Feb 1, 2025

Adding ready to merge label since it was already approved by a code owner.

@pjanotti pjanotti added the ready to merge Code review completed; ready to merge by maintainers label Feb 1, 2025
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch 9 times, most recently from b53b09a to 299d0c1 Compare February 3, 2025 22:21
@samiura samiura force-pushed the add-more-vecenter-receiver-virtual-machine-preformace-metrics branch from 299d0c1 to 60c5303 Compare February 3, 2025 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/vcenter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants