handler: Expose nmstatectl stats as k8s metrics #1221

qinqon · 2023-12-11T09:54:34Z

Is this a BUG FIX or a FEATURE ?:
/kind enhancement

What this PR does / why we need it:
Now that nmstatectl is able to calculate some useful stats from network configuration [1], we can bubble them up and expose them as k8s metrics so k-nmstate users can digg on them using prometheus, graphana or the like.

This change add a new "Features" under nnce Status with the output of
nmstatectl st and also create a new deployment nmstate-metrics that
will gather the NNCEs features and reflecta that at a cluster wide
gaugue prometheus metric.

This is an example of nmstate feature stat

kubernetes_nmstate_features_applied{name="dhcpv4-custom-hostname"} 1

Depends on nmstate 2.2.20, looks like it's build but still not present at centos 9 stream

https://kojihub.stream.rdu2.redhat.com/koji/buildinfo?buildID=41534

[1] nmstate/nmstate#2420

TODO:

Compare old and new nncp stats to be able to decrease conunters.

Release note:

Expoxe statistics generated from `nmstatectl stats`

sradco · 2023-12-17T10:22:06Z

@machadovilaca @avlitman please see if you can help in reviewing this PR.

pkg/monitoring/metrics.go

test/e2e/handler/metrics.go

test/e2e/handler/metrics_test.go

test/e2e/handler/metrics.go

pkg/nmstatectl/nmstatectl.go

deploy/handler/operator.yaml

deploy/handler/role.yaml

test/e2e/handler/metrics_test.go

qinqon · 2023-12-21T12:53:53Z

@avlitman @machadovilaca @sradco can you take another look ?

machadovilaca · 2023-12-21T13:49:39Z

controllers/handler/nodenetworkconfigurationpolicy_controller.go

+		for _, t := range stats.Topology {
+			monitoring.ApplyTopologyTotal.WithLabelValues(t).Inc()
+		}
+		for _, f := range stats.Features {
+			monitoring.ApplyFeaturesTotal.WithLabelValues(f).Inc()
+		}


Per https://github.com/nmstate/kubernetes-nmstate/pull/1221/files#r1431131839, as I understand, I'm afraid this will have unbound cardinality as it will contain network state information like hostnames/interfaces/address/ports, is that correct?

Per https://github.com/nmstate/kubernetes-nmstate/pull/1221/files#r1431131839, as I understand, I'm afraid this will have unbound cardinality as it will contain network state information like hostnames/interfaces/address/ports, is that correct?

The cardinality is not unbound, is not like we are going to count "mycluster1" and "eth1", the topology is a combination of enumerations (that's a little unbound but not that much) and the features is a list of enums too.

Can this be a problem ?

Yes, labels need to have a limited values set. Except for specific labels like VM names, pod names, namespaces and other resources etc.
See note: https://prometheus.io/docs/practices/naming/#labels

so this is an example of the nmstate stdats

topology: - static_ip4,static_ip6 -> linux-bridge -> bond -> ethernet - static_ip4,static_ip6 -> vlan -> linux-bridge -> bond -> ethernet features: - sriov - mac-based-identifier

For "features" is clear that is limited on the enum variants, maybe the main problem would be "topology".

@sradco @machadovilaca do you know what kind of limits do we have for labels bound ?

I read the following at the docs

CAUTION: Remember that every unique combination of key-value label pairs represents a new time series, which can dramatically increase the amount of data stored. Do not use labels to store dimensions with high cardinality (many different label values), such as user IDs, email addresses, or other unbounded sets of values.

But I don't see specific limits, also topology is going to be smaller than something like generated ID or pod names.

pkg/monitoring/metrics.go

test/e2e/handler/metrics.go

test/e2e/handler/metrics_test.go

kubevirt-bot · 2023-12-21T13:56:13Z

@machadovilaca: changing LGTM is restricted to collaborators

In response to this:

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.

sradco · 2023-12-21T14:27:09Z

@qinqon I propose you to also add docs generator for the operator. I think this is really useful.

We have the automation, so that when the user adds a PR with a new metric, the test runs and checks if the metric is already documented. If not the user is asked to run make generate and this automatically updated the PR with the change to the metrics.md file with the new metric, description and type.

See an example to the metrics.md file here
https://github.com/kubevirt/hyperconverged-cluster-operator/blob/main/docs/metrics.md
and the docs generator is here
https://github.com/kubevirt/hyperconverged-cluster-operator/blob/main/tools/metricsdocs/metricsdocs.go
(Note: we plan to move it to /monitoring/tools/ )

qinqon · 2024-04-16T06:38:26Z

/hold cancel

Now is ready

qinqon · 2024-04-17T06:41:41Z

/retest

mkowalski · 2024-04-17T07:47:43Z

hack/prom_metric_linter.sh

+#
+set -e
+
+linter_image_tag="v0.0.3"


Any chance this could be consumed from the ENV and fallback to 0.0.3 in case nothing is defined? I have in mind make promlint-check so that 0.0.3 would be defined in the Makefile as promlinter_version and passed down to this script

Now that nmstatectl is able to calculate some useful stats from network configuration [1], we can bubble them up and expose them as k8s metrics so k-nmstate users can digg on them using prometheus, graphana or the like. This change add a new "Features" under nnce Status with the output of `nmstatectl st` and also create a new deployment `nmstate-metrics` that will gather the NNCEs features and reflecta that at a cluster wide gaugue prometheus metric. [1] nmstate/nmstate#2420 Signed-off-by: Enrique Llorente <ellorent@redhat.com>

mkowalski · 2024-04-17T08:58:47Z

/lgtm

qinqon · 2024-04-17T09:42:22Z

/approve

qinqon · 2024-04-17T09:42:48Z

/retest

kubevirt-bot · 2024-04-17T09:42:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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:

~~OWNERS~~ [qinqon]

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

qinqon · 2024-04-17T13:07:01Z

/retest

Now that nmstatectl is able to calculate some useful stats from network configuration [1], we can bubble them up and expose them as k8s metrics so k-nmstate users can digg on them using prometheus, graphana or the like. This change add a new "Features" under nnce Status with the output of `nmstatectl st` and also create a new deployment `nmstate-metrics` that will gather the NNCEs features and reflecta that at a cluster wide gaugue prometheus metric. [1] nmstate/nmstate#2420 Signed-off-by: Enrique Llorente <ellorent@redhat.com>

kubevirt-bot added kind/enhancement release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Dec 11, 2023

kubevirt-bot requested review from cybertron and phoracek December 11, 2023 09:54

qinqon force-pushed the expose-nmstatectl-stats branch 5 times, most recently from 6aede34 to 39570e3 Compare December 15, 2023 11:44

sradco reviewed Dec 17, 2023

View reviewed changes

pkg/monitoring/metrics.go Outdated Show resolved Hide resolved

sradco reviewed Dec 17, 2023

View reviewed changes

pkg/monitoring/metrics.go Outdated Show resolved Hide resolved

qinqon force-pushed the expose-nmstatectl-stats branch from 39570e3 to 568ea37 Compare December 18, 2023 10:40

machadovilaca reviewed Dec 18, 2023

View reviewed changes

qinqon force-pushed the expose-nmstatectl-stats branch 5 times, most recently from 13c356a to 00cc20b Compare December 21, 2023 12:51

qinqon requested review from sradco, machadovilaca and avlitman December 21, 2023 12:53

qinqon force-pushed the expose-nmstatectl-stats branch from 00cc20b to a301a86 Compare December 21, 2023 13:50

machadovilaca suggested changes Dec 21, 2023

View reviewed changes

kubevirt-bot added the dco-signoff: no Indicates the PR's author has not DCO signed all their commits. label Dec 21, 2023

qinqon force-pushed the expose-nmstatectl-stats branch 6 times, most recently from e813553 to 678209e Compare April 16, 2024 05:36

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2024

mkowalski reviewed Apr 17, 2024

View reviewed changes

qinqon force-pushed the expose-nmstatectl-stats branch from 678209e to 14cfaec Compare April 17, 2024 08:57

qinqon requested a review from mkowalski April 17, 2024 08:57

kubevirt-bot assigned mkowalski Apr 17, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2024

kubevirt-bot merged commit bf889c0 into nmstate:main Apr 17, 2024
8 checks passed

dfitzmau mentioned this pull request May 14, 2024

OSDOCS-10427: Created an NMState OP section for stat reporting openshift/openshift-docs#75781

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handler: Expose nmstatectl stats as k8s metrics #1221

handler: Expose nmstatectl stats as k8s metrics #1221

qinqon commented Dec 11, 2023 •

edited

sradco commented Dec 17, 2023 •

edited

qinqon commented Dec 21, 2023

machadovilaca Dec 21, 2023

qinqon Dec 21, 2023 •

edited

sradco Dec 22, 2023 •

edited

qinqon Jan 2, 2024 •

edited

kubevirt-bot commented Dec 21, 2023

sradco commented Dec 21, 2023

qinqon commented Apr 16, 2024

qinqon commented Apr 17, 2024

mkowalski Apr 17, 2024

qinqon Apr 17, 2024

mkowalski commented Apr 17, 2024

qinqon commented Apr 17, 2024

qinqon commented Apr 17, 2024

kubevirt-bot commented Apr 17, 2024

qinqon commented Apr 17, 2024

handler: Expose nmstatectl stats as k8s metrics #1221

handler: Expose nmstatectl stats as k8s metrics #1221

Conversation

qinqon commented Dec 11, 2023 • edited

sradco commented Dec 17, 2023 • edited

qinqon commented Dec 21, 2023

machadovilaca Dec 21, 2023

Choose a reason for hiding this comment

qinqon Dec 21, 2023 • edited

Choose a reason for hiding this comment

sradco Dec 22, 2023 • edited

Choose a reason for hiding this comment

qinqon Jan 2, 2024 • edited

Choose a reason for hiding this comment

kubevirt-bot commented Dec 21, 2023

sradco commented Dec 21, 2023

qinqon commented Apr 16, 2024

qinqon commented Apr 17, 2024

mkowalski Apr 17, 2024

Choose a reason for hiding this comment

qinqon Apr 17, 2024

Choose a reason for hiding this comment

mkowalski commented Apr 17, 2024

qinqon commented Apr 17, 2024

qinqon commented Apr 17, 2024

kubevirt-bot commented Apr 17, 2024

qinqon commented Apr 17, 2024

qinqon commented Dec 11, 2023 •

edited

sradco commented Dec 17, 2023 •

edited

qinqon Dec 21, 2023 •

edited

sradco Dec 22, 2023 •

edited

qinqon Jan 2, 2024 •

edited