Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Add custom-metrics-prometheus chart #7159

Closed
wants to merge 1 commit into from
Closed

Add custom-metrics-prometheus chart #7159

wants to merge 1 commit into from

Conversation

steven-sheehy
Copy link
Collaborator

@steven-sheehy steven-sheehy commented Aug 13, 2018

What this PR does / why we need it: Adds a Custom Metrics API adapter for Prometheus. Custom metrics are necessary in Kubernetes to scale Horizontal Pod Autoscalers based upon your own metric pulled from an external metrics provider like Prometheus. This chart is similar to and complements the metrics-server chart that provides resource only metrics.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): N/A

Special notes for your reviewer: This is my first chart I've submitted so I'm not sure who to put in maintainers in Chart.yaml or who to assign for review.

This chart expects a list of rules to be defined in a configmap so that it knows what Prometheus metrics to import and serve as custom metrics. It comes with a set of default metrics that are enabled by default via rules.default that were recommended in the source repository manifests. I only included the container_* metrics as the others caused too many metrics to be queried and did not work in all situations depending upon how Prometheus label mapping was configured. You can add custom rules in addition to the default via rules.custom.

Also, concerning the name I went with custom-metrics-prometheus since the source repository mentions multiple names like "k8s-prometheus-adapter", "Custom Metrics Adapter for Prometheus" and "custom-metrics-apiserver". I felt the first and last were too generic and the middle one was too long. custom-metrics-prometheus clearly defines that it is specifically for prometheus and leaves open the possibility to add additional custom metrics providers in the future like custom-metrics-heapster or custom-metrics-sysdig.

@DirectXMan12 If you could please provide any feedback it would be appreciated.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 13, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: steven-sheehy
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: foxish

If they are not already assigned, you can assign the PR to them by writing /assign @foxish in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

Hi @steven-sheehy. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 13, 2018
@steven-sheehy
Copy link
Collaborator Author

/assign @foxish

@gajus
Copy link

gajus commented Aug 14, 2018

Have you checked if this is compatible with https://github.com/helm/charts/tree/master/stable/prometheus?

@plumdog
Copy link
Contributor

plumdog commented Aug 14, 2018

@steven-sheehy #6082 was merged very recently, and looks like it does broadly the same thing.

It's not immediately clear to me how similar the charts are, but I notice that yours has the Values.rules.default and Values.rules.custom options, which are lacking from the chart already in stable. In my case at least, these options would be necessary before the chart would be useful.

So I think perhaps you could retarget this PR to update the chart in stable, which might be a pain, but hopefully that will result in a best-of-both-worlds chart.

@steven-sheehy
Copy link
Collaborator Author

@gajus Yes, I'm using it against stable/prometheus chart and it is working well.

@plumdog You're right, that is a chart for the same image. I didn't see it before since it was just merged 18 hrs ago. I can close this and submit another pull request to enhance that one with any missing functionality that mine has.

@DirectXMan12
Copy link

NB: for future reference: custom-metrics-apiserver is the name of the generic library used by the Prometheus adapter for writing a custom metrics adapter, not the name of this specific adapter.

@steven-sheehy steven-sheehy deleted the custom-metrics-prometheus branch August 15, 2018 21:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants