feat: Allow prefixing wildcard labelsFromPath

[rexagod]

What this PR does / why we need it: Allow prefixing * paths, for instance, a "foo_" prefix like so, foo_*: [foo, bar, baz], will result in foo_baz being the key for all object resolutions under baz.

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

Which issue(s) this PR fixes: Self-driven.

[rexagod]

TODO: Add a doc entry.

[mrueg]

/triage accepted

[mrueg]

After looking a bit closer at what this does, do the semantics of this syntax look intentional to users?

If I see * I wouldn't think that this is adding a prefix but more a filter to only include keys with this specific prefix.
Can we find a better syntax?

[rexagod]

Closing in favor of the newer model.

[rexagod]

Reopening in favor of #2068 (comment).

[dgrisonnet]

/triage accepted
/assign

[rexagod]

Up for reviews.

[dgrisonnet]

Should we just support globbing in general for labels from path and use https://github.com/gobwas/glob instead of doing the parsing manually?

[rexagod]

For the CRS snippet below, the output will contain labels with the prefix foo_ for every key present in the fooObj and it's values marshalled into the respective label values. There isn't actually any Match happening, and the * is but a placeholder to denote where the label keys will be inserted at.

labelsFromPath:
  "foo_*": ["bar", "fooObj"]

An actual regex match over a sample set of fooObjA, fooObjB for foo_* will return empty (which is why I doubt I might be failing to see here how we'd benefit from this library, and where). This syntax, in this context, addresses the need to manipulate generated labels, as opposed to validating them against an expression.

[rexagod]

This basically builds on top of the originally present * labelFromPath key, which would marshal every key-value pair in an object as is, into label-values, whereas this allows prepending text to these "as-is" keys.

[rexagod]

I can modify this PR, or send another patch in the future to truly denote * as the resolved object keys and insert keys there, not just at the prefix.

[rexagod]

Do you mean supporting expressions such as foo_b?r.* for labelFromPaths using the library? As in, all fooObj keys that match the b?r.* pattern and include only those as labels? This may prove ambiguous for estimating where the prefix ends and the regexp begins (do we want to move prefixes to a different field)?

[dgrisonnet]

Unrelated to the actual PR, but why does *1 yield "qux": "quxx". Is this the way we tell the code to take the first annotation?

[dgrisonnet]

Disregard #2052 (review), I got super confused with the syntax.

The use of the * character is so confusing here. I would see a lot of people, myself included to expect some kind of globbing from this *, but that not the case at all, we are just precising a prefix or suffix, which shouldn't need a * in the syntax.

The current approach has 4 scenarios:

"*": [metadata, labels]
"prefix_*": [metadata, labels]
"*_suffix": [metadata, labels]
"prefix_*_suffix": [metadata, labels]

This relies on an implicit behavior from the parser that requires users to have an understand of the code. Whereas we could make it more expressive and have something like:

labelsFromPath:
  - path: [metadata, labels]
    prefix: "lorem"
    suffix: "ipsum"

IMO, the more explicit a syntax is, the easier it is for an external user to understand.

Prometheus-adapter is a prime example of a very bad syntax. Its configuration was made overly complex in an attempt to make it more generic and adaptable (I guess), but the result is that it confuses everyone and discourage users from using the project.
Here is a "simple" example from the repo:

rules:
- seriesQuery: '{__name__=~"^container_.*",container!="POD",namespace!="",pod!=""}'
  resources:
    overrides:
      namespace: {resource: "namespace"}
      pod: {resource: "pod"}
  name:
    matches: "^container_(.*)_seconds_total$"
  metricsQuery: "sum(rate(<<.Series>>{<<.LabelMatchers>>,container!="POD"}[2m])) by (<<.GroupBy>>)"

We need to avoid reaching that point at all cost.

[rexagod]

I see. My only concern here is when moving from,

"*": [metadata, labels]
"prefix_*": [metadata, labels]
"*_suffix": [metadata, labels]
"prefix_*_suffix": [metadata, labels]

to,

labelsFromPath:
  - path: [metadata, labels]
    prefix: "lorem"
    suffix: "ipsum"

will require multiple paths, since the lorem_* approach allows for specifying distinct paths that can differ from each other, such as,

"*": [metadata, labels, foo]
"prefix_*": [metadata, labels, bar]
"*_suffix": [metadata, labels, baz]
"prefix_*_suffix": [metadata, labels, que]

as compared to,

each:
  type: Gauge
  gauge:
    path: [status, conditions]
    labelsFromPath:
      - path: [metadata, labels, foo]
      	prefix: ""
      	suffix: ""
      - path: [metadata, labels, bar]
      	prefix: "prefix"
      	suffix: ""
      <"*_suffix" and "prefix_*_suffix" as well>
      type: ["type"]
    valueFrom: ["status"]

which may give rise to ambiguous conclusions since there's a path definition outside labelsFromPath as well, to allow relative definitions for labels such as type (above).

[dgrisonnet]

which may give rise to ambiguous conclusions since there's a path definition outside labelsFromPath as well, to allow relative definitions for labels such as type (above).

I see what you mean here, although that was already the case implicitly, it will now be explicit and could be very confusing. Maybe we could call it from or fromPath?

[k8s-triage-robot]

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

[rexagod]

/remove-lifecycle stale

[rexagod]

@dgrisonnet Please correct me if I'm wrong but I believe transforming labelsFromPath to a list will be a breaking change. For instance, the following (currently the way to specify label key-values with no wildcards) will need to be adjust to conform to the newer YAML list-based format.

labelsFromPath:
  status: [conditions, "0", status]

could either be,

labelsFromPath:
  - path : [conditions, "0", status]
    prefix: "status"
    suffix: ""

or,

labelsFromPath:
  - path : [conditions, "0", status]
    prefix: ""
    suffix: "status"

both being breaking changes to the existing configuration interpretation logic IIUC.

[dgrisonnet]

Yes it would be a breaking changes. That would also be the case for this comment on your other PR which would apply also here: #2068 (comment)

We will need to break the config entirely if we want to build solid foundation with CEL, so hopefully we will change it in the future. In the meantime we can continue with the current UX, but we will need to draw a line at one point. I would be fine with drawing it once your two PRs are merged and then stop accepting feature requests until we've migrated to CEL.

[dgrisonnet]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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