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

[receiver/vcenter] Adds additional resource pool metrics #33741

Conversation

BominRahmani
Copy link
Contributor

@BominRahmani BominRahmani commented Jun 25, 2024

Description:

The following PR adds these metrics

- vcenter.resource_pool.memory.swapped
- vcenter.resource_pool.memory.ballooned
- vcenter.resource_pool.memory.granted {memory_granted: shared / private}

It also adds the attribute memory_usage_type and splits up the vcenter.resource_pool.memory.usage metric into the following attributes: host, guest, overhead

Link to tracking Issue: #33607

Testing:
Golden files were generated to show the addition of the new metrics.

Documentation:
Documentation generated by make generate.

receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Show resolved Hide resolved
receiver/vcenterreceiver/metrics.go Outdated Show resolved Hide resolved
receiver/vcenterreceiver/testdata/metrics/expected.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/scraper_test.go Outdated Show resolved Hide resolved
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.

Just some minor changes. Otherwise looks good.

receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
@BominRahmani BominRahmani force-pushed the feat/add-vcenter-resource-pool-metrics branch from 8ec60a9 to 5bc7964 Compare July 2, 2024 18:03
@BominRahmani BominRahmani marked this pull request as ready for review July 2, 2024 18:09
@BominRahmani BominRahmani requested a review from a team July 2, 2024 18:09
@BominRahmani BominRahmani force-pushed the feat/add-vcenter-resource-pool-metrics branch 2 times, most recently from e97a851 to 242f54a Compare July 2, 2024 18:24
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Is there a reason these are separate metrics, instead of a single metric with an attribute to differentiate the data points?

In other words, are these mutually exclusive sections of memory that can be added together to represent a useful total?

Can we use well defined attributes such as system.memory.state?

@BominRahmani
Copy link
Contributor Author

Is there a reason these are separate metrics, instead of a single metric with an attribute to differentiate the data points?

In other words, are these mutually exclusive sections of memory that can be added together to represent a useful total?

Can we use well defined attributes such as system.memory.state?

The metrics are not mutually exclusive sections of memory. Summing the different metrics together wouldn't give you any meaningful total.

@BominRahmani BominRahmani requested a review from djaglowski July 8, 2024 13:13
@djaglowski
Copy link
Member

I don't understand the relationship between these metrics but I am skeptical that we need four metrics with the same model to describe memory. Can you link documentation for them?

@BominRahmani
Copy link
Contributor Author

I don't understand the relationship between these metrics but I am skeptical that we need four metrics with the same model to describe memory. Can you link documentation for them?

Yeah, heres a link for those metrics

@djaglowski
Copy link
Member

Based on the descriptions is unclear to me why private, shared, and swapped wouldn't be the same metric. Thoughts?

@BominRahmani
Copy link
Contributor Author

Based on the descriptions is unclear to me why private, shared, and swapped wouldn't be the same metric. Thoughts?

I'd be alright combining these three metrics and keeping the ballooned separate.

@StefanKurek
Copy link
Contributor

@djaglowski I believe all the memory metrics in question are coming from here.

Also, we do have precedent for the ballooned and swapped memory metrics already exist for VMs.

Private and shared memory could possibly be combined into a single metric with an attribute differentiating them. I'm not sure what a good metric name would be though.

@BominRahmani BominRahmani requested a review from StefanKurek July 8, 2024 18:03
@BominRahmani BominRahmani force-pushed the feat/add-vcenter-resource-pool-metrics branch from 0bbc87d to 14c2496 Compare July 8, 2024 19:50
receiver/vcenterreceiver/README.md Outdated Show resolved Hide resolved
.chloggen/add-vcenter-resource-pool-metrics.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metrics.go Outdated Show resolved Hide resolved
receiver/vcenterreceiver/metrics.go Outdated Show resolved Hide resolved
@BominRahmani BominRahmani force-pushed the feat/add-vcenter-resource-pool-metrics branch from 83837e3 to 27a69e1 Compare July 9, 2024 03:51
@BominRahmani BominRahmani force-pushed the feat/add-vcenter-resource-pool-metrics branch from 27a69e1 to 76d64dc Compare July 9, 2024 04:00
@BominRahmani BominRahmani force-pushed the feat/add-vcenter-resource-pool-metrics branch from ce64e5b to cd99c08 Compare July 9, 2024 14:36
@djaglowski djaglowski merged commit 9188fa4 into open-telemetry:main Jul 9, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants