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

Add 'aggregate' label to nova metrics #74

Closed
prolane opened this issue Dec 19, 2019 · 4 comments
Closed

Add 'aggregate' label to nova metrics #74

prolane opened this issue Dec 19, 2019 · 4 comments

Comments

@prolane
Copy link
Contributor

prolane commented Dec 19, 2019

Hi!
I'd like to open up a former discussion about adding the 'aggregate' label to some nova metrics. @alistarle initially created a PR for this over here: #35. Because of the discussions about how to best implement this, the PR ended in adding an AZ label instead (still very useful btw, thank you for adding).

I would like to start using the aggregate label though, therefore I'd like to discuss this again. I've been thinking about what the best approach would be and here is my proposal:

Proposal

How about we make the aggregate label optional by setting a flag when starting the exporter? If you explicitly set this aggregate flag, the way the nova metrics are generated changes a bit. Instead of looping over all hypervisors, we would be first looping over all aggregates and then for each aggregate loop over all hypervisors in that aggregate.

I do understand like @alistarle commented in #35, you would lose the possibility to query sum(vcpus_available) (as one hypervisor is potentially part of more than one aggregate). But this is why I think enabling the aggregate information optionally with a flag makes sense, as you then explicitly accept the changed behavior. I don't think people who work with aggregates want to see the sum of vcpus_available over all hypervisors. What they do want to see is the sum of vcpus_available grouped by aggregate. Which will be possible with the aggregate label implemented as suggested.

Let me know what you guys think. I'll be happy to create a PR for it.

@prolane prolane changed the title Add 'aggregate' label to applicable nova metrics Add 'aggregate' label to nova metrics Dec 19, 2019
@niedbalski
Copy link
Member

@prolane

In my opinion a single label named 'aggregates', which can be a space/comma separated list of aggregates on which a specific hypervisor belongs to should suffice to solve this issue as you can filter by a specific label and sum the vcpus_available. Any drawbacks on using this approach?

@prolane
Copy link
Contributor Author

prolane commented Dec 20, 2019

Thats certainly a possibility @niedbalski. I guess if we make the space/comma separated list in alphabetical order, we make sure we don't end up with lots of different combinations. For example, you don't get one hypervisor which has the 'aggregates' label value set to "az-west,ssd-hosts,shared-cluster" and one other set to "ssd-hosts,az-west,shared-cluster".

Just one drawback of this approach is, if you have hypervisors being part of more than one aggregate, you can not group by one aggregate. For example, with the example aggregates label values above, I won't be able to group by just the ssd-hosts. Not without making hard coded dashboards in Grafana for example.

Ideally, you would want to do something like this:
sum(vcpus_available) by(aggregates)

And make rules (for alerting), like this:
sum(vcpus_available) by(aggregates) < 20

This will still work for the collection of hypervisors which all have the same combination of aggregates (e.g. "az-west,ssd-hosts,shared-cluster"), but if I then would want to alert on when vcpus are running low on just the ssd-hosts, I would have to make it hardcoded like:
sum(vcpus_available{aggregates=~".*ssd-hosts.*"}) by(aggregates) < 20

Perhaps this is fine, as I guess usually teams don't often add new aggregates?

@prolane
Copy link
Contributor Author

prolane commented Dec 23, 2019

This PR does the job: #75

Happy X-mas holidays! Cheers!

@niedbalski
Copy link
Member

Merged #75 , Thank you very much for your amazing contributions @prolane !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants