-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Simplify custom resource state metrics API using CEL #2059
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CatherineF-dev The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This issue is currently awaiting triage. If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the The 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. |
0e9c7c1
to
059ed38
Compare
version: "v1" | ||
mode: for_loop # or merged | ||
metrics: | ||
- name: "ready_count" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is the metrics type defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume we'll still need fields with hardcoded values, eg., type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting here that we still have non-path-based constructs, like so:
type
(used to check OpenMetrics conformance, and ifvalue
field is applicable)path
(relative paths when used withlabelsFrom*
, these can be CEL expressions with the following subsections)- non-relative
- relative
- multi-relative (relative to different paths) (KSM does not support this right now)
commonLabels
list
Something we might want to persue under the configuration revamp is to keep such constructs to a minimum (and try to replace most of these using CEL, wherever possible), as we may find ourselves implementing custom logic and introducing complexity as such constructs grow over time.
type-a: 1 | ||
type-b: 3 | ||
conditions: | ||
- name: a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have a configuration example using CEL that loops over this list and generates the following metrics?
ready_count{phase="Pending", condition="a"} 45
ready_count{phase="Pending", condition="b"} 66
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to add on to what Manuel asked, would it be possible to reproduce this example using CEL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
## Goal | ||
|
||
- Simplify 8 operations into one operation to reduce maintaining work. | ||
- A complete API, so that can support more cases. For example, querying number of CRs under one CRD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure what a complete API
means here? AFAICT the proposal targets redefining a subset of configuration constructs, specifically ones that work with path resolutions. Is that not the case?
|
||
| Existing operation | CEL | | ||
| :--- | :--- | | ||
| path: [status, sub] \n labelFromKey: type | x.status.sub.map(y, {"name": y}) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document what's x
here? Is this a representation for all CRs that come under the specified GVK?
type-a: 1 | ||
type-b: 3 | ||
conditions: | ||
- name: a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to add on to what Manuel asked, would it be possible to reproduce this example using CEL?
version: "v1" | ||
mode: for_loop # or merged | ||
metrics: | ||
- name: "ready_count" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting here that we still have non-path-based constructs, like so:
type
(used to check OpenMetrics conformance, and ifvalue
field is applicable)path
(relative paths when used withlabelsFrom*
, these can be CEL expressions with the following subsections)- non-relative
- relative
- multi-relative (relative to different paths) (KSM does not support this right now)
commonLabels
list
Something we might want to persue under the configuration revamp is to keep such constructs to a minimum (and try to replace most of these using CEL, wherever possible), as we may find ourselves implementing custom logic and introducing complexity as such constructs grow over time.
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
While this is easily one of the features we'd like to see implemented, tested, and polished enough throughout |
|
||
|
||
``` | ||
kind: CustomResourceStateMetricsV2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind: CustomResourceStateMetricsV2 | |
kind: CustomResourceStateMetrics | |
apiVersion: v2 |
I'd propose to use the common versioning keys :-) We could even provide conversion from the old to the new version if we want
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
What this PR does / why we need it: Simplify custom resource state metrics API
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality) No
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1978