Skip to content
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

prefix GVK labels in CustomResourceMonitoring #1942

Conversation

bavarianbidi
Copy link
Contributor

Signed-off-by: Mario Constanti mario@constanti.de

What this PR does / why we need it:

This PR will prefix the auto-generated GVK labels for CustomResources with cr_ to make it a bit more clear that these labels got generated.
It also make it more easier to reflect e.g. a version of a CR in a metric by just label it version instead of some specific app_version (or similar).

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)

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 #

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 3, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 3, 2023
@k8s-ci-robot
Copy link
Contributor

@bavarianbidi: 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/test-infra repository.

@CatherineF-dev
Copy link
Contributor

I think "group" is fine and it aligns with k8s documentations, cc @rexagod to have another review.

@dgrisonnet
Copy link
Member

I am a bit on the fence here as I like the current naming because it aligns with the labels we have in the kube-apiserver HTTP metrics. But at the same time I can see how one could want to use a version label for other purposes. Maybe we could prefix them with crd similarly to how we prefix the metrics name kube_crd?

Do you have an opinion on that @logicalhan?

@bavarianbidi
Copy link
Contributor Author

bavarianbidi commented Jan 4, 2023

Don't get me wrong, because it's really nice to get the group, version and kind data as label values in the metric without additional work.
But the main purpose from a user point of view is to reflect information from CRs.
As many third-party-components have a version field which i'm more interested in than the version of the CRD itself (as i already have to define the GVK for a CRD in the configuration) i'm very happy if we find a solution for that.

As an alternative we could also think about an additional configuration value/flag where a user can opt-in/opt-out the auto-generated GVK labels.

Beside the origin issue with the version field, i've already stumbled 2 times this morning about the version field in a CR where i have to decided how to continue.

from cluster-api-azure, where an AzureMachinePool can reflect the version of an image in the status field:

k explain azuremachinepool.status.image.marketplace.version
KIND:     AzureMachinePool
VERSION:  infrastructure.cluster.x-k8s.io/v1beta1

FIELD:    version <string>

DESCRIPTION:
     Version specifies the version of an image sku. The allowed formats are
     Major.Minor.Build or 'latest'. Major, Minor, and Build are decimal numbers.
     Specify 'latest' to use the latest version of an image available at deploy
     time. Even if you use 'latest', the VM image will not automatically update
     after deploy time even if a new version becomes available.

from flux, where you can define a HelmRelease and point to a specific version:

$ k explain helmreleases.spec.chart.spec.version
KIND:     HelmRelease
VERSION:  helm.toolkit.fluxcd.io/v2beta1

FIELD:    version <string>

DESCRIPTION:
     Version semver expression, ignored for charts from v1beta2.GitRepository
     and v1beta2.Bucket sources. Defaults to latest when omitted.

@dgrisonnet
Copy link
Member

As many third-party-components have a version field which i'm more interested in than the version of the CRD itself (as i already have to define the GVK for a CRD in the configuration) i'm very happy if we find a solution for that.

Yeah this scenario totally makes sense.

As an alternative we could also think about an additional configuration value/flag where a user can opt-in/opt-out the auto-generated GVK labels.

I wouldn't be in favor of that since I don't think that there are any advantages to removing the GVK labels since they provide key information for barely any overhead.

@@ -49,7 +49,7 @@ spec:
- --resources=certificatesigningrequests,configmaps,cronjobs,daemonsets,deployments,endpoints,foos,horizontalpodautoscalers,ingresses,jobs,limitranges,mutatingwebhookconfigurations,namespaces,networkpolicies,nodes,persistentvolumeclaims,persistentvolumes,poddisruptionbudgets,pods,replicasets,replicationcontrollers,resourcequotas,secrets,services,statefulsets,storageclasses,validatingwebhookconfigurations,volumeattachments,verticalpodautoscalers
```

NOTE: The `group`, `version`, and `kind` common labels are reserved, and will be overwritten by the values from the `groupVersionKind` field.
NOTE: The `cr_group`, `cr_version`, and `cr_kind` common labels are reserved, and will be overwritten by the values from the `groupVersionKind` field.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we had some consistency between this prefix and the one in the metric name. Currently the metric name is using the crd prefix which might not be well suited. Might be better to go with kube_cr or kube_custom_resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean we should use the future prefix in the metric name and in the label itself?
I like kube_custom_resource more than kube_cr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'd like to avoid having crd somewhere and cr elsewhere, it's better be consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, would be nice to have one thing. I believe right now we mix Custom Resource, Custom Resource Definition and Custom Resource State. kube_customresource would align with kube_horizontalpodautoscaler (no additional underscores).

Should we rename everything custom resource state into simply custom resource and abbreviate it with "cr" where necessary?

Copy link
Contributor Author

@bavarianbidi bavarianbidi Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've pushed a new commit (ed85920) - this should reflect the way we want to go further

rebased and squashed

Copy link
Contributor Author

@bavarianbidi bavarianbidi Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Based on the previous commit i started to improve the unit-tests because they didn't cover the default generated group,version,kind labels atm. will continue on monday morning.

@CatherineF-dev
Copy link
Contributor

Another way might be kube-state-metrics to support renaming labels for CRs.
So that kube-state-metrics collects HelmRelease.version and renames it into helm_version.

$ k explain helmreleases.spec.chart.spec.version
KIND:     HelmRelease
VERSION:  helm.toolkit.fluxcd.io/v2beta1

FIELD:    version <string>

DESCRIPTION:
     Version semver expression, ignored for charts from v1beta2.GitRepository
     and v1beta2.Bucket sources. Defaults to latest when omitted.

This will prefix the auto-generated GVK labels for CustomResources with
customresource_ to make it a bit more clear that these labels got generated.

Signed-off-by: Mario Constanti <mario@constanti.de>
@bavarianbidi bavarianbidi force-pushed the prefix_gvk_labels_in_customresourcemonitoring branch from ed85920 to 7130bd0 Compare January 9, 2023 14:07
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 9, 2023
@bavarianbidi bavarianbidi requested review from mrueg and dgrisonnet and removed request for fpetkovski, CatherineF-dev and mrueg January 9, 2023 14:08
Help: "metrics for testing",
Each: &compiledInfo{},
Labels: map[string]string{
"customresource_group": "apps",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

customresource_group to cr_group?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comming from this comment

I agree, would be nice to have one thing. I believe right now we mix Custom Resource, Custom Resource Definition and Custom Resource State. kube_customresource would align with kube_horizontalpodautoscaler (no additional underscores).

Should we rename everything custom resource state into simply custom resource and abbreviate it with "cr" where necessary?

i've started with customresource (because i like it a bit more) but as i'm not very opinionated i'm also fine to switch to cr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @mrueg which label name do you prefer? cr_group or customresource_group

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal preference is the second one, two letter acronyms can become ambiguous

@bavarianbidi bavarianbidi requested review from mrueg and dgrisonnet and removed request for dgrisonnet and mrueg January 9, 2023 15:11
@bavarianbidi bavarianbidi requested review from mrueg and removed request for dgrisonnet January 10, 2023 12:20
Signed-off-by: Mario Constanti <mario@constanti.de>
@bavarianbidi bavarianbidi force-pushed the prefix_gvk_labels_in_customresourcemonitoring branch from cfb26ce to b410166 Compare January 10, 2023 12:31
@mrueg
Copy link
Member

mrueg commented Jan 10, 2023

/approve

Thanks for your contribution!

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2023
@bavarianbidi bavarianbidi requested review from dgrisonnet and CatherineF-dev and removed request for dgrisonnet and CatherineF-dev January 10, 2023 14:01
@rexagod
Copy link
Member

rexagod commented Jan 10, 2023

/approve

@bavarianbidi
Copy link
Contributor Author

I've got 2 approvals... Who could give an LGTM to get this merged?

@mrueg
Copy link
Member

mrueg commented Jan 11, 2023

I'll make sure to merge it by the end of this week, want to provide other maintainers the opportunity to give further feedback

Copy link
Member

@dgrisonnet dgrisonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for bearing with us @bavarianbidi :)

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bavarianbidi, 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:

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 k8s-ci-robot merged commit b7a7070 into kubernetes:main Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants