feat: Support list expansions #2068

rexagod · 2023-05-15T23:33:27Z

What this PR does / why we need it: Allow CRS configuration to take in path values for lists, denoted by "*". This means it's possible to specify [..., <list>, "*", foo, ...] in the configuration to dynamically generate multiple metrics that reflect different states of foo in various list elements.

How does this change affect the cardinality of KSM: No change.

k8s-ci-robot · 2023-05-15T23:33:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [rexagod]

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

🎉

docs/customresourcestate-metrics.md

CatherineF-dev · 2023-06-14T12:39:35Z

A related #1978

Current Custom State Resource API is a little complicated. Adding more features might bring more maintaining work and make it harder to migrate into cel.

I think cel can support this.

dgrisonnet · 2023-06-14T13:38:35Z

I agree with Catherine, we shouldn't add more complexity to the existing config but rather focus on the new model.

rexagod · 2023-06-14T14:23:38Z

So, are we sunsetting the older model?

rexagod · 2023-06-14T14:30:17Z

Also, there was a thread regarding separating the CRS featureset into its own repository (the "non-turing" solution here being CEL-like languages, that do not offer an exhaustive way of defining the configuration, unlike go-based templating solutions).

CatherineF-dev · 2023-06-14T14:39:24Z

Reuse the wheel (cel, a query model) can reduce our maintaining work on how to extract fields.
Then we can spend efforts on other things, instead of putting many efforts on implementing a query language.

rexagod · 2023-06-14T14:48:16Z

Will this mean that KSM's config capabilities will be restricted to what the underlying solution (#1978) has to offer?

CatherineF-dev · 2023-06-14T14:52:03Z

cel should be a complete language, which means it can extract every field.

cel is more powerful than current APIs and is capable of extracting more things.

rexagod · 2023-06-14T15:11:54Z

@dgrisonnet I'm assuming going forward with the newer model will mean we'll deprecate the older one. So, with that in mind, are PRs such as this and others like: #2067, #2052 obsolete and should be closed?

dgrisonnet · 2023-06-14T15:41:09Z

I need to catch up on the discussions, a lot happened when I was away, but my general feeling is that we are starting to make the configuration way too complex. My fear is that this is going to become too confusing for the users and I am also worries about the current state of the parser. Without a proper lexer/parser combination this is slowly becoming unmaintainable.

IMO with the state of things, we should rather focus on building new foundations for the config rather than making the existing one more complex since it will just make the migration process harder. wdyt?

rexagod · 2023-06-14T16:59:43Z

I couldn't agree more.

The current CRS featureset, in addition to being complex and hardly maintainable for developers, doesn't have a clear, semantical, or easy-to-understand UX at the moment. Off the top of my mind, I can think of a few examples that go on to show a couple of such instances. I personally would not at all expect the user to be familiar with the following intricacies.

Relative paths can only be defined once, and the fact that technically there's two labelsFromPath possible but the other one does not support it.

       each:
         type: Info
         info:
           path: [spec, selector, labels]
           labelsFromPath:
             "*": [matchLabels]
       labelsFromPath:
         name: [metadata, name]

The outer lablesFromPath operates on the subset that satified the inner labelsFromPath. In the configuration snippet below, the outer labelsFromPath will only generate name labels for all objects that satisfied the inner labelsFromPath, i.e., a non-nil foo label.

  	  each:
         type: Info
         info:
           path: [metadata]
           labelsFromPath:
             "foo_label": [labels, foo]
       labelsFromPath:
         name: [metadata, name]

The difference in each of the configurations below, which looks more like a side-effect of another change.

 	  each:
         type: Info
         info:
           path: [users]
           labelsFromPath:
             user: []

outputs: kube_customresource_metric_name{..., user="user1"} 1 and kube_customresource_metric_name{..., user="user2"} 1 while,

 	  each:
         type: Info
         info:
           labelsFromPath:
             user: [users]

outputs: kube_customresource_metric_name{..., user="[user1 user2]"} 1.

TBH only reason I raised the aforementioned PRs was because I wasn't sure about the direction in which we were going to move forward over the last month regarding this, and assumed that doing so would entail discussions spanning a good while, so things would basically be done the same way as they were till now.

But, I'm enthusiastic and happy to see that there's a consensus on improving the current approach, and the discussions are yeilding actionable results. It'd be nice to see the proposed solution replacing as many configuration constructs as possible, in order to achieve a more accessible and deterministic UX for the user, thanks to @CatherineF-dev. 🎉

pkg/customresourcestate/registry_factory.go

rexagod · 2023-08-29T20:49:47Z

(pulled in #2052 to combine all patches for the dynamic valueFrom)

rexagod · 2023-08-29T20:54:31Z

docs/customresourcestate-metrics.md

 labelsFromPath:
  # this can be combined with the wildcard prefixes feature introduced in #2052
  "available_*": [available]
+  "lorem_*": [pending]
+# this can be combined with dynamic valueFrom expressions introduced in #2068
+valueFrom: [pending_resourceB]
 # outputs:
-...{...,available_resourceA="10",available_resourceB="20",...} ...
+...{...,available_resourceA="10",available_resourceB="20",lorem_resourceA="0",lorem_resourceB="6"...} 6


valueFrom now allows users to utilise the generated wildcard labels as metric values.

rexagod · 2023-08-30T08:18:53Z

/hold
Let me improve on this a bit (we should be able to replace the x_*: [y], valueFrom: [y_*] with x_*: [y], valueFrom: [x_*]).

rexagod · 2023-08-30T10:59:54Z

/hold cancel
Up for reviews.

k8s-triage-robot · 2024-01-22T12:29:54Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-02-21T12:50:33Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

rexagod · 2024-02-21T18:02:31Z

/remove-lifecycle rotten

rexagod · 2024-03-03T16:28:31Z

FYI This is up for reviews (or merging, if this looks good).

dgrisonnet

Some new thoughts, I will try to have an actual look at the code later this week

docs/customresourcestate-metrics.md

eugenepaniot · 2024-03-20T10:23:43Z

We're interested in this feature and hope to see it in the next release soon!

Allow CRS configuration to take in path values for lists, denoted by "*". This means it's possible to specify `[..., <list>, "*", foo, ...]` in the configuration to dynamically generate multiple metrics that reflect different states of `foo` in various list elements.

allow specifying dynamic valueFrom based on wildcard labelFromPaths Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

dgrisonnet · 2024-04-02T14:36:04Z

pkg/customresourcestate/registry_factory_test.go

+			path: mustCompilePath(t, "spec", "order", "*", "value"),
+			want: []valuePath{
+				mustCompilePath(t, "spec", "order", "0", "value"),
+				mustCompilePath(t, "spec", "order", "1", "value"),
+			},


what would happen if value was an optional field and wasn't present in one of the order?

dgrisonnet · 2024-04-02T15:00:07Z

pkg/customresourcestate/registry_factory.go

+				// search space to resolve the dynamic valueFrom from the wildcard labels
+				dynamicValueFromScope := c.compiledCommon.labelFromPath
+				lastUnderscoreIndex := strings.LastIndex(extractedValueFrom, "_")
+				if lastUnderscoreIndex != -1 {
+					// For:
+					// labelsFromPath:
+					//  "foo_*": ["spec", "fooObj"]
+					unresolvedKey := extractedValueFrom[:lastUnderscoreIndex] + "_*"
+					dynamicPaths, ok := dynamicValueFromScope[unresolvedKey]
+					if ok {
+						var resolvedKeyArr []string
+						for _, dynamicPath := range dynamicPaths {
+							resolvedKeyArr = append(resolvedKeyArr, dynamicPath.part)
+						}
+						// resolvedKey will map to unresolved key "foo_*"'s corresponding valid path string, i.e., "spec_fooObj_*".
+						resolvedKey := strings.Join(resolvedKeyArr, "_")
+						extractedValueFrom = resolvedKey + extractedValueFrom[lastUnderscoreIndex:]
+					}
+				}


Why do we do that?

As far as I can tell this is made to propagate the wildcard to the value no? Like:

"foo_*": ["spec", "fooObj"]

yields the following path:

spec_fooObj_*

no?

dgrisonnet · 2024-04-02T15:01:21Z

pkg/customresourcestate/registry_factory.go

+	handleNil := func(object interface{}, part string) interface{} {
+		switch tobj := object.(type) {
+		case map[string]interface{}:
+			return tobj[part]
+		case []interface{}:
+			if part == "*" {
+				return tobj
+			}
+			idx, err := strconv.Atoi(part)
+			if err != nil {
+				return nil
+			}
+			if idx < 0 || idx >= len(tobj) {
+				return nil
+			}
+			return tobj[idx]
+		default:
+			return nil
+		}
+	}


could you make that its own function so that the code is easier to read?

dgrisonnet · 2024-04-02T15:02:55Z

pkg/customresourcestate/registry_factory.go

@@ -674,14 +780,85 @@ func famGen(f compiledFamily) generator.FamilyGenerator {
 	}
 }

+func findWildcard(path valuePath, i *int) bool {


I am not a fan of modifying the index and returning whether we find the wildcard or not. A more idiomatic approach would be similar to the search functions in the strings package where if found you return the index, otherwise -1

dgrisonnet · 2024-04-02T15:04:41Z

pkg/customresourcestate/registry_factory.go

+		pathCopyStart := make(valuePath, i)
+		copy(pathCopyStart, path[:i])
+		pathCopyWildcard := make(valuePath, len(path)-i-1)
+		copy(pathCopyWildcard, path[i+1:])
+		for k := 0; k < l; k++ {
+			pathCopyStart = append(pathCopyStart, pathOp{part: strconv.Itoa(k)})
+			pathCopyStart = append(pathCopyStart, pathCopyWildcard...)
+			expandedPaths = append(expandedPaths, pathCopyStart)
+		}


since we know the size of the path from the get go, it would be less expensive in terms of allocations to just allocate one slice and then fill it correctly

k8s-ci-robot · 2024-04-04T23:57:31Z

PR needs rebase.

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.

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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 15, 2023

k8s-ci-robot requested review from CatherineF-dev and fpetkovski May 15, 2023 23:33

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 15, 2023

rexagod force-pushed the support-list-expansions branch 2 times, most recently from 5c5949d to 5586928 Compare May 16, 2023 00:00

rexagod marked this pull request as ready for review May 16, 2023 00:00

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 May 16, 2023

k8s-ci-robot requested a review from dgrisonnet May 16, 2023 00:01

rexagod force-pushed the support-list-expansions branch 2 times, most recently from 954a59a to 0eecd57 Compare May 16, 2023 00:15

rexagod referenced this pull request in rexagod/kube-state-metrics Jun 11, 2023

Allow defining valueFrom paths on dynamic paths

c2362ce

🎉

mrueg reviewed Jun 14, 2023

View reviewed changes

docs/customresourcestate-metrics.md Show resolved Hide resolved

rexagod closed this Jun 14, 2023

This was referenced Jun 15, 2023

feat: allow field KV general matching #2067

Merged

feat: Allow prefixing wildcard labelsFromPath #2052

Merged

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

mrueg changed the title ~~Support list expansions~~ feat: Support list expansions Aug 29, 2023

mrueg reviewed Aug 29, 2023

View reviewed changes

pkg/customresourcestate/registry_factory.go Outdated Show resolved Hide resolved

rexagod force-pushed the support-list-expansions branch from c7cdba3 to 96f8b37 Compare August 29, 2023 20:48

rexagod commented Aug 29, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2023

rexagod force-pushed the support-list-expansions branch from 96f8b37 to c86bc94 Compare August 30, 2023 10:59

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2023

rexagod force-pushed the support-list-expansions branch from c86bc94 to 5159901 Compare August 30, 2023 11:04

rexagod requested review from dgrisonnet and mrueg October 17, 2023 14:16

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 21, 2024

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 21, 2024

dgrisonnet reviewed Mar 4, 2024

View reviewed changes

docs/customresourcestate-metrics.md Show resolved Hide resolved

rexagod added 2 commits March 26, 2024 20:42

add dynamic valueFrom

56b4bb7

allow specifying dynamic valueFrom based on wildcard labelFromPaths Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

rexagod force-pushed the support-list-expansions branch from 5159901 to 56b4bb7 Compare March 26, 2024 15:16

dgrisonnet reviewed Apr 2, 2024

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support list expansions #2068

feat: Support list expansions #2068

rexagod commented May 15, 2023

k8s-ci-robot commented May 15, 2023

CatherineF-dev commented Jun 14, 2023 •

edited

dgrisonnet commented Jun 14, 2023

rexagod commented Jun 14, 2023

rexagod commented Jun 14, 2023

CatherineF-dev commented Jun 14, 2023 •

edited

rexagod commented Jun 14, 2023 •

edited

CatherineF-dev commented Jun 14, 2023 •

edited

rexagod commented Jun 14, 2023 •

edited

dgrisonnet commented Jun 14, 2023

rexagod commented Jun 14, 2023

rexagod commented Aug 29, 2023

rexagod Aug 29, 2023

rexagod commented Aug 30, 2023 •

edited

rexagod commented Aug 30, 2023

k8s-triage-robot commented Jan 22, 2024

k8s-triage-robot commented Feb 21, 2024

rexagod commented Feb 21, 2024

rexagod commented Mar 3, 2024

dgrisonnet left a comment

eugenepaniot commented Mar 20, 2024

dgrisonnet Apr 2, 2024

dgrisonnet Apr 2, 2024

dgrisonnet Apr 2, 2024

dgrisonnet Apr 2, 2024

dgrisonnet Apr 2, 2024

k8s-ci-robot commented Apr 4, 2024

feat: Support list expansions #2068

Are you sure you want to change the base?

feat: Support list expansions #2068

Conversation

rexagod commented May 15, 2023

k8s-ci-robot commented May 15, 2023

CatherineF-dev commented Jun 14, 2023 • edited

dgrisonnet commented Jun 14, 2023

rexagod commented Jun 14, 2023

rexagod commented Jun 14, 2023

CatherineF-dev commented Jun 14, 2023 • edited

rexagod commented Jun 14, 2023 • edited

CatherineF-dev commented Jun 14, 2023 • edited

rexagod commented Jun 14, 2023 • edited

dgrisonnet commented Jun 14, 2023

rexagod commented Jun 14, 2023

rexagod commented Aug 29, 2023

rexagod Aug 29, 2023

Choose a reason for hiding this comment

rexagod commented Aug 30, 2023 • edited

rexagod commented Aug 30, 2023

k8s-triage-robot commented Jan 22, 2024

k8s-triage-robot commented Feb 21, 2024

rexagod commented Feb 21, 2024

rexagod commented Mar 3, 2024

dgrisonnet left a comment

Choose a reason for hiding this comment

eugenepaniot commented Mar 20, 2024

dgrisonnet Apr 2, 2024

Choose a reason for hiding this comment

dgrisonnet Apr 2, 2024

Choose a reason for hiding this comment

dgrisonnet Apr 2, 2024

Choose a reason for hiding this comment

dgrisonnet Apr 2, 2024

Choose a reason for hiding this comment

dgrisonnet Apr 2, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 4, 2024

CatherineF-dev commented Jun 14, 2023 •

edited

CatherineF-dev commented Jun 14, 2023 •

edited

rexagod commented Jun 14, 2023 •

edited

CatherineF-dev commented Jun 14, 2023 •

edited

rexagod commented Jun 14, 2023 •

edited

rexagod commented Aug 30, 2023 •

edited