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 Metrics to VPA Recommender #1631

Closed
wyb1 opened this issue Jan 30, 2019 · 8 comments
Closed

Add Metrics to VPA Recommender #1631

wyb1 opened this issue Jan 30, 2019 · 8 comments
Labels
area/vertical-pod-autoscaler sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation.

Comments

@wyb1
Copy link
Contributor

wyb1 commented Jan 30, 2019

Hello,
we would like to use the vertical pod autoscaler and it would help if we could monitor the recommendations that the VPA recommender gives with prometheus. Would it be possible to expose these metrics in the VPA recommender?

I'm hoping that the metric could look something like this:

vpa_recommender_recommendation{container="<container>",job="vpa",pod="<pod>",recommendation_type="lower_bound",resource_name="cpu",vpa_name="<vpa>"}

I'd be willing to open a PR for this if it is accepted.

@bskiba
Copy link
Member

bskiba commented Jan 30, 2019

I assume container and vpa_name is a label? In that case I am afraid that this is against the label best practices in Kubernetes - we want to avoid metrics with huge cardinality and this means that all possible label values should ideally be known beforehand and not too many: https://github.com/kubernetes/community/blob/b3349d5b1354df814b67bbdee6890477f3c250cb/contributors/devel/instrumentation.md#dimensionality--cardinality

@wyb1
Copy link
Contributor Author

wyb1 commented Jan 30, 2019

Would there be another way to label the metric? In essence there are only 3 labels that I need:

  • recommendation_type="lower_bound|target|uncappedTarget|upper_bound"
  • resource_name ="cpu|memory"
  • Something to identify which VPA made this recommendation.

For the last point, the easiest way is probably to use the VPA name and namespace, but this goes against the best practices.

@bskiba
Copy link
Member

bskiba commented Jan 30, 2019

I think it would be best to consult sig-instrumentation on this, can you try their slack channel? https://kubernetes.slack.com/messages/sig-instrumentation

@bskiba bskiba added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Jan 30, 2019
@wyb1
Copy link
Contributor Author

wyb1 commented Feb 8, 2019

Any update here? I asked in the slack channel and haven't gotten a response yet.
However, I believe that the metrics are reasonable to add. I assume that in most cases VPAs are stable and do not get created/deleted very often unlike pods. Each VPA would have one set of the proposed metrics. So the amount of metrics go up and down based on the number of VPAs in the cluster. With the aforementioned metrics it's 8 metrics per VPA. It shouldn't be a problem exposing them either because as far as I can tell the information for this is being held in memory anyway.

@bskiba
Copy link
Member

bskiba commented Feb 8, 2019

In general, I am still against putting this into core VPA. The biggest problem is that the set of label values is not known up-front and can be arbitrarily big, this is exactly what we want to avoid in core metrics. VPAs can be created for all the deployments in a cluster, in big clusters this can get very problematic.

It shouldn't however be too hard to implement your own controller scraping this information and exporting the metrics (you can take the code reading VPA objects from recommender). You can also try contributing this to the https://github.com/kubernetes/kube-state-metrics project - this add-on exports exactly these sort of metrics (per-object) and it's understood that if you run it, you run the risk of getting high cardinality in big clusters.

@bskiba
Copy link
Member

bskiba commented Feb 8, 2019

I'll close this one, unless I get clear signal from sig-instrumentation that this would be somehow ok, I'm against.
/close

@k8s-ci-robot
Copy link
Contributor

@bskiba: Closing this issue.

In response to this:

I'll close this one, unless I get clear signal from sig-instrumentation that this would be somehow ok, I'm against.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@milesbxf
Copy link

FYI I've just raised a PR to add this to kube-state-metrics: kubernetes/kube-state-metrics#791

yaroslava-serdiuk pushed a commit to yaroslava-serdiuk/autoscaler that referenced this issue Feb 22, 2024
* [api/v1alpha1] Make MultiKueueCluster an API object.

* [multikueue] Make MultiKueueCluster an API object.

* Fix after rebase

* Review Remarks

* Review Remarks

* Review Remarks

* [API/multikueue] Drop KubeConfig.Name

* Review remarks

* Review remarks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertical-pod-autoscaler sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation.
Projects
None yet
Development

No branches or pull requests

4 participants