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

Enable vSphere CPU and host metrics by default #34022

Conversation

BominRahmani
Copy link
Contributor

Description:

The following PR enables the following metrics by default

vcenter.vm.cpu.readiness
vcenter.host.network.packet.drop.rate
vcenter.host.cpu.capacity
vcenter.host.cpu.reserve.capacity

Link to tracking Issue:

Testing:
Updated golden files
Documentation:
Updated documentation using make generate

@BominRahmani BominRahmani marked this pull request as ready for review July 10, 2024 17:16
@BominRahmani BominRahmani requested a review from a team July 10, 2024 17:16
@crobert-1
Copy link
Member

Enabling metrics by default that were previously disabled by default is considered a breaking change. Please add a changelog entry accordingly.

More context if it's helpful.

@crobert-1 crobert-1 changed the title [chore] Enable vSphere CPU and host metrics by default Enable vSphere CPU and host metrics by default Jul 10, 2024
@BominRahmani
Copy link
Contributor Author

@crobert-1 Thanks for letting me know! Will update this shortly.

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.

Aside from the CHANGELOG entry mentioned, looks good.

@BominRahmani BominRahmani force-pushed the chore/enable-vcenter-cpu-host-metrics-default branch from a62b1e2 to 3e1e612 Compare July 11, 2024 00:54
@BominRahmani BominRahmani force-pushed the chore/enable-vcenter-cpu-host-metrics-default branch from b4fd183 to 6ead7b3 Compare July 11, 2024 00:59
@BominRahmani
Copy link
Contributor Author

While updating this, i noticed that the integration test was missing an update. Im not sure why it didn't error before, but it was complaining on the CI now, so I went ahead and updated that. I also noticed there was a missing metric from the mock file that caused it to not generate on the golden file, which I also updated.

I am more than happy to split this apart into its own PR if we think its necessary, but I figured since this PR already addresses updating those metrics that were in question to begin with, that I might be able to leave it in. Let me know what you think
@djaglowski

@djaglowski
Copy link
Member

Presumably the errors are due to enabling the metrics? Is any other reason possible?

@BominRahmani
Copy link
Contributor Author

@djaglowski The error was specifically related to enabling the vcenter.host.cpu.capacity metric, I figured this out by enabling each individual metric, until i found which was causing that CI error. The vcenter.host.cpu.capacity was missing a spot in the integration test, so it was finding less metrics than it was expecting.
The part I was a bit confused on is why this didn't cause an error on the integration test part of the CI when i originally introduced these metrics.

@djaglowski
Copy link
Member

The part I was a bit confused on is why this didn't cause an error on the integration test part of the CI when i originally introduced these metrics.

Wouldn't it be because the metric was not enabled in the integration test?

@BominRahmani
Copy link
Contributor Author

@djaglowski Yea, but neither was the other metrics i had added.

@djaglowski djaglowski merged commit debd148 into open-telemetry:main Jul 11, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 11, 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.

5 participants