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: add kube_node_status_addresses metric #2252

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

stonith
Copy link
Contributor

@stonith stonith commented Nov 28, 2023

What this PR does / why we need it:
Adds kube_node_status_addresses metric to account for mutiple addresses per type.

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Adds a new metric per node

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 28, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 28, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @stonith!

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/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 28, 2023
@stonith stonith changed the title add external_ip label to kube_node_info metric feat: add external_ip label to kube_node_info metric Nov 28, 2023
@stonith stonith marked this pull request as ready for review November 28, 2023 23:46
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2023
@dgrisonnet
Copy link
Member

/assign @rexagod
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 30, 2023
internal/store/node.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 5, 2023
@stonith
Copy link
Contributor Author

stonith commented Dec 5, 2023

Seems to work as expected in a dualstack GKE cluster:

kube_node_info{node="gke-dualstack-test-default-pool-b7e9ab8b-q7g4",kernel_version="5.15.109+",os_image="Container-Optimized OS from Google",container_runtime_version="containerd://1.7.0",kubelet_version="v1.27.3-gke.100",kubeproxy_version="v1.27.3-gke.100",provider_id="gce://test/us-west1-a/gke-dualstack-test-default-pool-b7e9ab8b-q7g4",pod_cidr="10.84.0.0/24",system_uuid="c880482c-39d8-8523-70de-0658b88a5fe6",internal_ip="10.0.0.3",internal_ip="2600:1900:4040:203:0:0:0:0",external_ip="35.203.180.214"} 1

@stonith stonith force-pushed the add-externalip-node-info branch 2 times, most recently from 2524d4d to 8497b03 Compare December 6, 2023 01:54
@dgrisonnet
Copy link
Member

I am not a fan of adding labels to the _info metric. Most of them are legacy and should be in their own metric. Ideally we should address that in the next major version since they are all marked as stable.

It would be better to have a dedicated kube_node_status_addresses metric instead with type and ip labels. @stonith could you introduce this new metric instead of adding the new label?

@mrueg interested about your opinion on this

@dgrisonnet
Copy link
Member

Ah well I just saw #2252 (comment), I guess our opinion align then 😆

@mrueg
Copy link
Member

mrueg commented Dec 6, 2023

Yep, the kube_node_status_addresses would be my preference. Also I don't think prometheus allows to have the same label multiple times.

Ideally it would look like this and we'll keep internal_ip still in there with a TODO to kill it in ksm-3.x

kube_node_status_address{type="internal", address="1.2.3.4"} 1
kube_node_status_address{type="internal", address="1::2"} 1
kube_node_status_address{type="external", address="2.3.4.5"} 1
kube_node_status_address{type="external", address="3::4"} 1
kube_node_status_address{type="hostname", address="example.com"} 1

@stonith stonith changed the title feat: add external_ip label to kube_node_info metric feat: add kube_node_status_address metric Dec 11, 2023
@stonith
Copy link
Contributor Author

stonith commented Dec 11, 2023

Updated PR to add kube_node_status_address metric

@mrueg
Copy link
Member

mrueg commented Dec 12, 2023

This looks good to me, two small requests:

  • Can you add a comment to the _info metric e.g.// TODO: remove internal_ip in v3, replaced by kube_node_status_address ?
  • Can you update the metrics documentation?

@stonith stonith changed the title feat: add kube_node_status_address metric feat: add kube_node_status_addresses metric Dec 14, 2023
@stonith
Copy link
Contributor Author

stonith commented Dec 14, 2023

This looks good to me, two small requests:

  • Can you add a comment to the _info metric e.g.// TODO: remove internal_ip in v3, replaced by kube_node_status_address ?
  • Can you update the metrics documentation?
  • Updated metric name to kube_node_status_addresses to be more accurate
  • added new metric to the docs/node-metrics.md
  • added comment to _info metric (I think that's where you meant?)

@stonith
Copy link
Contributor Author

stonith commented Dec 14, 2023

added the missed node label

@mrueg
Copy link
Member

mrueg commented Dec 14, 2023

/lgtm

thanks!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrueg, stonith

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 Dec 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit 0bc4b19 into kubernetes:main Dec 14, 2023
12 checks passed
@stonith stonith deleted the add-externalip-node-info branch December 14, 2023 18:52
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/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants