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 build info metric #1332

Merged
merged 1 commit into from
Jan 6, 2021
Merged

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Dec 18, 2020

Signed-off-by: Ben Ye yb532204897@gmail.com

What this PR does / why we need it:

This pr adds the kube_state_metrics_build_info metric as required by #1132

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1132

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 18, 2020
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 18, 2020
@yeya24
Copy link
Contributor Author

yeya24 commented Dec 18, 2020

PTAL @lilic ^^

main.go Outdated
@@ -80,6 +81,7 @@ func main() {
storeBuilder := store.NewBuilder()

ksmMetricsRegistry := prometheus.NewRegistry()
registerBuildInfo(ksmMetricsRegistry)
Copy link
Contributor

Choose a reason for hiding this comment

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

@yeya24 Any reason why you didn't consider the existing collector? https://github.com/prometheus/common/blob/master/version/info.go

Copy link
Member

Choose a reason for hiding this comment

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

Nice Kemal, agreed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kakkoyun I was thinking about using that directly. But that works well with promu because promu injects all the info via build flags to that pkg.

Right now the injected flags are added https://github.com/kubernetes/kube-state-metrics/blob/master/Makefile#L55 here, I still need to add more flags to make the VersionCollector usable. I feel it is not necessary.

Copy link
Contributor

@kakkoyun kakkoyun Dec 18, 2020

Choose a reason for hiding this comment

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

True. I have done exactly what you said in here https://github.com/kakkoyun/observable-remote-write/blob/56b5c6dedc8eb30ece404cd4885c3aabb779d2b8/Makefile#L14

Hard to judge just which way is simpler. I guess it's a matter of preference. My preference would be obviously reusing the package from the prometheus/common.

@lilic What do you think? Which code looks simpler to you?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, reusing Prometheus/common would be my preference here as well. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! I will use that package. Thanks @kakkoyun for the example!

@yeya24
Copy link
Contributor Author

yeya24 commented Jan 5, 2021

Updated. Please take a look. @lilic @kakkoyun 🤗

Some screenshots:

Screenshot from 2021-01-05 01-29-05

Screenshot from 2021-01-05 01-30-01

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 5, 2021
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 5, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 5, 2021
@yeya24
Copy link
Contributor Author

yeya24 commented Jan 5, 2021

Umm, not sure how to fix CI. I tried make validate-manifests locally and I didn't get any error output.

Copy link
Contributor

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM.

Concerning CI failure, I'd suggest making sure your tooling version is the same as the CI.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! One suggestion only, but a nit.

The failure seems odd, but do you mind installing all the tools we use (some might need bumping so feel free to bump if you have the time) and try again.

I can also have a look tomorrow if you don't manage, thanks!!

@@ -150,6 +150,12 @@ http_request_duration_seconds_sum{handler="metrics",method="get"} 0.021113919999
http_request_duration_seconds_count{handler="metrics",method="get"} 30
```

kube-state-metrics exposes the build info metric:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Would be good to add a few lines what those are useful for. Similiar to have we have them for the list and watch errors above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

```

`kube_state_metrics_build_info` is used to expose version and other build information. For more usage about the info pattern,
please check the blog post [here](https://www.robustperception.io/exposing-the-software-version-to-prometheus).
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure we are allowed to link to external companies blog posts? Otherwise, agreed this blog is very useful! @brancz wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is a general rule here, if this was self-promotion I would be more critical, but here it's really just a helpful resource.

Copy link
Member

Choose a reason for hiding this comment

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

Yes sounds good, was on the edge. lgtm apart from the need to squash commits.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Do you mind just squashing your commits as the bot will merge as is, thanks! Otherwise lgtm, thanks again!

Signed-off-by: Ben Ye <yb532204897@gmail.com>
@yeya24
Copy link
Contributor Author

yeya24 commented Jan 6, 2021

Updated. @lilic

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for the contribution and addressing all the comments so quickly! :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 6, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kakkoyun, lilic, yeya24

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2021
@k8s-ci-robot k8s-ci-robot merged commit 2465de0 into kubernetes:master Jan 6, 2021
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: no Indicates the PR's author has not signed the CNCF CLA. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for kube_state_metrics_build_info
5 participants