feat: Use `dlclark/regexp2` over standard library's package #2616

rexagod · 2025-02-23T21:30:28Z

Please refer to the aforementioned issue for more details.

┌[rexagod@nebuchadnezzar] [/dev/ttys002] [2594] [1]
└[~/repositories/oss/kube-state-metrics]> go run . --kubeconfig=$KUBECONFIG --metric-allowlist='^kube_(?=namespace).*_created$'
I0310 18:27:57.420618   22613 wrapper.go:120] "Starting kube-state-metrics"
I0310 18:27:57.425000   22613 server.go:194] "Used default resources"
I0310 18:27:57.425050   22613 types.go:195] "Using all namespaces"
I0310 18:27:57.425159   22613 server.go:233] "Metric allow-denylisting" allowDenyStatus="Including the following lists that were on allowlist: ^kube_(?=namespace).*_created$"
I0310 18:27:57.429918   22613 utils.go:70] "Tested communication with server"
I0310 18:27:57.442175   22613 utils.go:75] "Run with Kubernetes cluster version" major="1" minor="31" gitVersion="v1.31.0" gitTreeState="clean" gitCommit="9edcffcde5595e8a5b1a35f88c421764e575afce" platform="linux/arm64"
I0310 18:27:57.442195   22613 utils.go:76] "Communication with server successful"
I0310 18:27:57.442674   22613 server.go:350] "Started kube-state-metrics self metrics server" telemetryAddress="[::]:8081"
I0310 18:27:57.442669   22613 server.go:361] "Started metrics server" metricsServerAddress="[::]:8080"
I0310 18:27:57.442711   22613 metrics_handler.go:110] "Autosharding disabled"
I0310 18:27:57.442958   22613 tls_config.go:347] "Listening on" address="[::]:8080"
I0310 18:27:57.443027   22613 tls_config.go:350] "TLS is disabled." http2=false address="[::]:8080"
I0310 18:27:57.442920   22613 tls_config.go:347] "Listening on" address="[::]:8081"
I0310 18:27:57.443087   22613 tls_config.go:350] "TLS is disabled." http2=false address="[::]:8081"
I0310 18:27:57.445301   22613 builder.go:283] "Active resources" activeStoreNames="certificatesigningrequests,configmaps,cronjobs,daemonsets,deployments,endpoints,horizontalpodautoscalers,ingresses,jobs,leases,limitranges,mutatingwebhookconfigurations,namespaces,networkpolicies,nodes,persistentvolumeclaims,persistentvolumes,poddisruptionbudgets,pods,replicasets,replicationcontrollers,resourcequotas,secrets,services,statefulsets,storageclasses,validatingwebhookconfigurations,volumeattachments"

┌[rexagod@nebuchadnezzar] [/dev/ttys003]
└[~]> curl -s localhost:8080/metrics | grep 'kube_.*_created'
# HELP kube_namespace_created [STABLE] Unix creation timestamp
# TYPE kube_namespace_created gauge
kube_namespace_created{namespace="default"} 1.741605321e+09
kube_namespace_created{namespace="kube-node-lease"} 1.741605321e+09
kube_namespace_created{namespace="kube-public"} 1.741605321e+09
kube_namespace_created{namespace="kube-system"} 1.741605321e+09
kube_namespace_created{namespace="local-path-storage"} 1.741605326e+09

k8s-ci-robot · 2025-02-23T21:30:36Z

This issue is currently awaiting triage.

If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

mrueg · 2025-03-10T11:45:36Z

pkg/allowdenylist/allowdenylist.go

+// Use ECMAScript as the default regexp spec to support lookarounds (#2594).
+var regexpDefaultSpec regexp.RegexOptions = regexp.ECMAScript
+
+func init() {


Can we not make use of an init() function here and instead wrap the regexp.Compile into a function that sets this?

I'd prefer to initialize upstream package variables at downstream package init()-time, is this an anti-pattern? This should also help avoid missing out on setting such values in any new functions in the future that may need to initialize the same.

I think in general init() is a bad pattern that we should avoid introducing because of side effects, import order etc.
Can we add this to the New() func instead?
https://github.com/kubernetes/kube-state-metrics/pull/2616/files#diff-b14b1102ef04d884e648b90166b5d35ee63edac2a544ef311770f5f289df48f4L36

Done, PTAL.

CatherineF-dev · 2025-03-10T14:13:26Z

pkg/allowdenylist/allowdenylist_test.go

@@ -124,22 +140,38 @@ func TestInclude(t *testing.T) {
 			t.Fatal("expected New() to not fail")
 		}

-		denylist.Exclude([]string{"kube_node_.*_cores", "kube_pod_.*_bytes"})
+		denylist.Exclude([]string{"kube_(?=node.*cores|pod.*bytes)"})


Is it compatible with "denylist.Exclude([]string{"kube_node_.cores", "kube_pod._bytes"})"?

Lookaround feature is more complicated. I will prefer using the legacy one unless performance is much better.

It is. The library being introduced here is a superset of go's regexp one, and will only incur expensive perf operations for exponential matches, that go's library doesn't support since it has to guarantee linear-time operations.

Also these operations are capped at a minute, so no leaks.

mrueg · 2025-03-18T09:06:58Z

Changes look good to me, thanks for the work. One small ask: Can you add some more documentation to the README on how to use this? Thanks!

mrueg · 2025-03-18T09:11:31Z

docs/developer/cli-arguments.md

@@ -58,7 +58,7 @@ Flags:
      --logtostderr                                log to standard error instead of files (default true)
      --metric-allowlist string                    Comma-separated list of metrics to be exposed. This list comprises of exact metric names and/or regex patterns. The allowlist and denylist are mutually exclusive.
      --metric-annotations-allowlist string        Comma-separated list of Kubernetes annotations keys that will be used in the resource' labels metric. By default the annotations metrics are not exposed. To include them, provide a list of resource names in their plural form and Kubernetes annotation keys you would like to allow for them (Example: '=namespaces=[kubernetes.io/team,...],pods=[kubernetes.io/team],...)'. A single '*' can be provided per resource instead to allow any annotations, but that has severe performance implications (Example: '=pods=[*]').
-      --metric-denylist string                     Comma-separated list of metrics not to be enabled. This list comprises of exact metric names and/or regex patterns. The allowlist and denylist are mutually exclusive.
+      --metric-denylist string                     Comma-separated list of metrics not to be enabled. This list comprises of exact metric names and/or *ECMAScript-based* regex patterns. The allowlist and denylist are mutually exclusive.
      --metric-labels-allowlist string             Comma-separated list of additional Kubernetes label keys that will be used in the resource' labels metric. By default the labels metrics are not exposed. To include them, provide a list of resource names in their plural form and Kubernetes label keys you would like to allow for them (Example: '=namespaces=[k8s-label-1,k8s-label-n,...],pods=[app],...)'. A single '*' can be provided per resource instead to allow any labels, but that has severe performance implications (Example: '=pods=[*]'). Additionally, an asterisk (*) can be provided as a key, which will resolve to all resources, i.e., assuming '--resources=deployments,pods', '=*=[*]' will resolve to '=deployments=[*],pods=[*]'.


Do you want to extend this to the labels and annotations allowlists in a later PR?

I'd like to keep this exclusive to the allowdeny lists for now, and expand once we start to see positive results in the near future.

pkg/allowdenylist/allowdenylist.go

Signed-off-by: Pranshu Srivastava <[email protected]>

mrueg · 2025-03-19T08:08:58Z

/lgtm

Thanks for your work on this!

k8s-ci-robot · 2025-03-19T08:09:08Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [mrueg,rexagod]

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 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 Feb 23, 2025

k8s-ci-robot requested review from logicalhan and mrueg February 23, 2025 21:30

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2025

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 23, 2025

rexagod mentioned this pull request Mar 4, 2025

MatchTimeout setting is not respected dlclark/regexp2#91

Closed

rexagod force-pushed the 2594 branch 2 times, most recently from 2c20c68 to 76d3788 Compare March 10, 2025 11:10

rexagod marked this pull request as ready for review March 10, 2025 11:16

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 Mar 10, 2025

k8s-ci-robot requested a review from CatherineF-dev March 10, 2025 11:17

rexagod force-pushed the 2594 branch from 76d3788 to 8a50360 Compare March 10, 2025 11:41

mrueg reviewed Mar 10, 2025

View reviewed changes

rexagod force-pushed the 2594 branch 2 times, most recently from 772822d to e8b298b Compare March 10, 2025 12:52

CatherineF-dev reviewed Mar 10, 2025

View reviewed changes

rexagod force-pushed the 2594 branch from e8b298b to 65d9bb8 Compare March 18, 2025 08:05

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2025

rexagod requested a review from mrueg March 18, 2025 08:19

mrueg reviewed Mar 18, 2025

View reviewed changes

rexagod force-pushed the 2594 branch 2 times, most recently from 7219efe to aab4eb5 Compare March 18, 2025 10:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2025

rexagod force-pushed the 2594 branch from aab4eb5 to 9b9bcd1 Compare March 18, 2025 10:25

rexagod commented Mar 18, 2025

View reviewed changes

pkg/allowdenylist/allowdenylist.go Outdated Show resolved Hide resolved

rexagod force-pushed the 2594 branch from 9b9bcd1 to dfd2dbd Compare March 18, 2025 10:44

rexagod force-pushed the 2594 branch from dfd2dbd to e652ad0 Compare March 18, 2025 10:47

feat: Use dlclark/regexp2 over standard library's package

8b631bb

Signed-off-by: Pranshu Srivastava <[email protected]>

rexagod force-pushed the 2594 branch from e652ad0 to 8b631bb Compare March 18, 2025 13:27

rexagod requested a review from mrueg March 18, 2025 13:34

k8s-ci-robot assigned mrueg Mar 19, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2025

k8s-ci-robot merged commit 5f11691 into kubernetes:main Mar 19, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use `dlclark/regexp2` over standard library's package #2616

feat: Use `dlclark/regexp2` over standard library's package #2616

rexagod commented Feb 23, 2025 •

edited

Loading

k8s-ci-robot commented Feb 23, 2025

mrueg Mar 10, 2025

rexagod Mar 10, 2025

mrueg Mar 10, 2025

rexagod Mar 18, 2025

CatherineF-dev Mar 10, 2025

rexagod Mar 18, 2025

rexagod Mar 18, 2025 •

edited

Loading

mrueg commented Mar 18, 2025

mrueg Mar 18, 2025

rexagod Mar 18, 2025

mrueg commented Mar 19, 2025

k8s-ci-robot commented Mar 19, 2025

feat: Use dlclark/regexp2 over standard library's package #2616

feat: Use dlclark/regexp2 over standard library's package #2616

Conversation

rexagod commented Feb 23, 2025 • edited Loading

k8s-ci-robot commented Feb 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rexagod Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

mrueg commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrueg commented Mar 19, 2025

k8s-ci-robot commented Mar 19, 2025

feat: Use `dlclark/regexp2` over standard library's package #2616

feat: Use `dlclark/regexp2` over standard library's package #2616

rexagod commented Feb 23, 2025 •

edited

Loading

rexagod Mar 18, 2025 •

edited

Loading