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

Add DeprecatedVersion to struct FamilyGenerator and func NewFamilyGenerator #1160

Merged
merged 1 commit into from
Aug 7, 2020

Conversation

YuikoTakada
Copy link
Contributor

@YuikoTakada YuikoTakada commented Jun 25, 2020

What this PR does / why we need it:

Currently, in kube-state-metrics, deprecated metrics is managed just in document. And also there are many docs about metrics(node-metrics.md, pod-metrics.md, ...)
This PR adds a new field "DeprecatedVersion" to struct FamilyGenerator and new func "NewFamilyGenerator" which generate struct FamilyGenerator. In NewFamilyGenerator, if the metrics is deprecated, "(Deprecated since x.x)" will be added at the begining of help string. It makes easier to manage deprecated metrics.

Also see:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/metrics/opts.go#L198

type SummaryOpts struct {
	Namespace         string
	Subsystem         string
	Name              string
	Help              string
	ConstLabels       map[string]string
	Objectives        map[float64]float64
	MaxAge            time.Duration
	AgeBuckets        uint32
	BufCap            uint32
	DeprecatedVersion string <---
	deprecateOnce     sync.Once
	annotateOnce      sync.Once
	StabilityLevel    StabilityLevel
}

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 25, 2020
@brancz
Copy link
Member

brancz commented Jun 25, 2020

I like this a lot! Similar to the metrics stability framework, could you add that the description is prefixed with the deprecated version?

@YuikoTakada
Copy link
Contributor Author

@brancz Thank you for your comments. Sorry I don't understand what you say. You mean DeprecatedVersion will be set like "DeprecatedVersion: 1.19"? It would be written as just "1.19". I'm sorry if I misunderstand what you say.

@brancz
Copy link
Member

brancz commented Jun 30, 2020

In Kubernetes, when a metric is deprecated, it will say in the help text for example:

kube_pod_info (Deprecated since 1.19)

I would like that we also do this in kube-state-metrics.

@YuikoTakada
Copy link
Contributor Author

@brancz Thanks, I understand. In kubernetes, in case of kube-scheduler, the string "Deprecated since x.xx.x" in #HELP is written manually like https://github.com/kubernetes/kubernetes/pull/92160/files (This is my PR, approved but not merged yet😅)

@brancz
Copy link
Member

brancz commented Jul 1, 2020

In kubernetes/kubernetes a metric is annotated with it's deprecated version (if it's deprecated), and since which version. https://github.com/kubernetes/component-base/blob/d18546f5347cab6a8690e9b863d9ab977f771079/metrics/desc.go#L196

@YuikoTakada
Copy link
Contributor Author

@brancz oh, I didn't know it. Thank you for the advice. I will try to fix this PR.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 8, 2020
@YuikoTakada
Copy link
Contributor Author

I've added new func NewFamilyGenerator also. If it looks good, I will fix all callers of FamilyGenerator to use NewFamilyGenerator. Thanks.

@brancz
Copy link
Member

brancz commented Jul 8, 2020

The strategy looks good, but there are some failures that need fixing.

@lilic
Copy link
Member

lilic commented Jul 9, 2020

Seems like the failures is:

ERRO Running error: golint: analysis skipped: errors in package: [/home/travis/gopath/src/k8s.io/kube-state-metrics/pkg/metric_generator/generator.go:47:35: undeclared name: obj /home/travis/gopath/src/k8s.io/kube-state-metrics/pkg/metric_generator/generator.go:47:22: cannot use generateFunc(obj) (value of type *metric.Family) as func(obj interface{}) *metric.Family value in struct literal] 

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 10, 2020
@YuikoTakada YuikoTakada force-pushed the add_deprecatedversion branch 2 times, most recently from fbafca9 to f495176 Compare July 10, 2020 01:24
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 10, 2020
@YuikoTakada YuikoTakada changed the title Add DeprecatedVersion to struct FamilyGenerator Add DeprecatedVersion to struct FamilyGenerator and func NewFamilyGenerator Jul 10, 2020
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2020
@YuikoTakada YuikoTakada force-pushed the add_deprecatedversion branch 2 times, most recently from 1125649 to bbe7a25 Compare August 6, 2020 09:20
@brancz
Copy link
Member

brancz commented Aug 7, 2020

/lgtm

Leaving approval up to @tariq1890 or @lilic

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, YuikoTakada

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2020
@k8s-ci-robot k8s-ci-robot merged commit c293f9c into kubernetes:master Aug 7, 2020
@brancz
Copy link
Member

brancz commented Aug 7, 2020

sorry I didn't realize that it would also automatically approve the PR. @tariq1890 @lilic please feel free to comment anyways! :)

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants