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

feat: enable customresource metrics by configuration #1710

Merged
merged 14 commits into from
Jun 2, 2022

Conversation

iamnoah
Copy link
Contributor

@iamnoah iamnoah commented Mar 23, 2022

What this PR does / why we need it:

Building off of the new customresource.RegistryFactory (thanks @Garrybest !), I found myself making a list of metrics I wanted on my custom resources and thought it made more sense to drive collection of those metrics from a config file than having to write pretty similar code for each one.

I've put together some examples in the .md file, and the tests cover most of the ins and outs.

This is a pretty early rough draft that suits my needs, but I thought I would put it out there for feedback and see if it's useful.

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)

Users may add unlimited custom metrics, but the default cardinality remains unchanged.

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

Welcome @iamnoah!

It looks like this is your first PR to kubernetes/kube-state-metrics 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kube-state-metrics has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 23, 2022
@Garrybest
Copy link
Member

Thanks @iamnoah, it is a really good practice!

Copy link
Member

@mrueg mrueg left a comment

Choose a reason for hiding this comment

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

Overall looks really interesting. Thanks for the contribution!

main.go Outdated Show resolved Hide resolved
docs/customresourcestate-metrics.md Outdated Show resolved Hide resolved
@iamnoah
Copy link
Contributor Author

iamnoah commented Apr 4, 2022

Is that linter error related to my changes in some way?

@fpetkovski
Copy link
Contributor

I think it's an issue in golangci-lint: golangci/golangci-lint#2374. Seems to be happening with go 1.18. I can take a look today to see if we can get it quickly resolved.

@fpetkovski
Copy link
Contributor

This PR should fix the CI issue: #1713

@fpetkovski
Copy link
Contributor

The CI should be fixed with #1713. Can you rebase your branch against main?

@iamnoah iamnoah force-pushed the master branch 2 times, most recently from 576fa12 to dc6f869 Compare April 5, 2022 17:42
docs/customresourcestate-metrics.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2022
@iamnoah iamnoah force-pushed the master branch 2 times, most recently from dadea8e to be1ab05 Compare May 6, 2022 04:01
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2022
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

I think this would be a great addition to KSM 👍

pkg/customresourcestate/registry_factory.go Show resolved Hide resolved
pkg/customresourcestate/registry_factory.go Show resolved Hide resolved
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Other than one last nit, I'm okay with merging this as it is. I think we should just announce it as an experimental feature in the changelog and keep an eye on incoming feedback.

@mrueg or @dgrisonnet any thoughts?

pkg/customresourcestate/registry_factory.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
@mrueg
Copy link
Member

mrueg commented May 30, 2022

@iamnoah we're planning for a next release in the upcoming days, are you interested in getting your contribution into that one? If you want to get it in please rebase on latest master and help with the linting/unit tests.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2022
The main use case is just surfacing fields from custom resources without having to write a lot of code for each one.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2022
docs/README.md Outdated Show resolved Hide resolved
docs/customresourcestate-metrics.md Outdated Show resolved Hide resolved
docs/customresourcestate-metrics.md Outdated Show resolved Hide resolved
pkg/options/options.go Outdated Show resolved Hide resolved
pkg/options/options.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2022
iamnoah and others added 5 commits June 1, 2022 10:11
Co-authored-by: Manuel Rüger <manuel@rueg.eu>
is there a tool for this?
Co-authored-by: Manuel Rüger <manuel@rueg.eu>
@fpetkovski
Copy link
Contributor

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2022
@mrueg
Copy link
Member

mrueg commented Jun 2, 2022

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fpetkovski, iamnoah, mrueg

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

@mrueg
Copy link
Member

mrueg commented Jun 2, 2022

Thanks for your contribution @iamnoah !

@k8s-ci-robot k8s-ci-robot merged commit 1e78285 into kubernetes:master Jun 2, 2022
@mrueg mrueg mentioned this pull request Jun 2, 2022
mhulscher added a commit to mhulscher/helm-charts that referenced this pull request Jun 17, 2022
… strings in arguments

The newly introduced `--custom-resource-state-config` argument
(kubernetes/kube-state-metrics#1710) accepts a multi-line string as value.
The deployment.yaml template currently loops over each .Values.extraArgs, which causes indentation
issues when passing a multi-line string. This PR fixes that by rendering .Values.extraArgs as YAML
with proper indentation.

Signed-off-by: Mitch Hulscher <mitch.hulscher@lib.io>
monotek pushed a commit to prometheus-community/helm-charts that referenced this pull request Jun 17, 2022
… strings in arguments (#2167)

The newly introduced `--custom-resource-state-config` argument
(kubernetes/kube-state-metrics#1710) accepts a multi-line string as value.
The deployment.yaml template currently loops over each .Values.extraArgs, which causes indentation
issues when passing a multi-line string. This PR fixes that by rendering .Values.extraArgs as YAML
with proper indentation.

Signed-off-by: Mitch Hulscher <mitch.hulscher@lib.io>
spr-mweber3 pushed a commit to spring-media/helm-charts that referenced this pull request Jun 21, 2022
… strings in arguments (prometheus-community#2167)

The newly introduced `--custom-resource-state-config` argument
(kubernetes/kube-state-metrics#1710) accepts a multi-line string as value.
The deployment.yaml template currently loops over each .Values.extraArgs, which causes indentation
issues when passing a multi-line string. This PR fixes that by rendering .Values.extraArgs as YAML
with proper indentation.

Signed-off-by: Mitch Hulscher <mitch.hulscher@lib.io>
galina-tochilkin pushed a commit to mtp-devops/3d-party-helm that referenced this pull request Aug 2, 2023
… strings in arguments (#2167)

The newly introduced `--custom-resource-state-config` argument
(kubernetes/kube-state-metrics#1710) accepts a multi-line string as value.
The deployment.yaml template currently loops over each .Values.extraArgs, which causes indentation
issues when passing a multi-line string. This PR fixes that by rendering .Values.extraArgs as YAML
with proper indentation.

Signed-off-by: Mitch Hulscher <mitch.hulscher@lib.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants