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

remove deprecated metric and promote the replacement to STABLE #110310

Merged
merged 1 commit into from May 31, 2022

Conversation

logicalhan
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR removes the deprecated apiserver_longrunning_gauge metric and promotes its replacement apiserver_longrunning_requests.

Does this PR introduce a user-facing change?

apiserver_longrunning_gauge is removed from the codebase. Please use apiserver_longrunning_requests instead. 

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 31, 2022
@logicalhan
Copy link
Member Author

/sig api-machinery instrumentation

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 31, 2022
@logicalhan
Copy link
Member Author

/hold

(I'm an approver so I want to ensure this gets vetted properly in reviews).

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 31, 2022
@logicalhan
Copy link
Member Author

/assign @dgrisonnet

@k8s-ci-robot k8s-ci-robot added the area/stable-metrics Issues or PRs involving stable metrics label May 31, 2022
@k8s-triage-robot
Copy link

This PR may require stable metrics review.

Stable metrics are guaranteed to not change. Please review the documentation for the requirements and lifecycle of stable metrics and ensure that your metrics meet these guidelines.

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.

Since the deprecated metric and the alpha one replacing it have the same labels, I think it is fine to graduate the new metric to stable at the same time as removing the old one since users have been using this API for a while now without needing to extend it.

For what it's worth, I would have been against doing the two changes during the same release if the labels were different. That's because unless the metric is completely removed, you can't make sure that users transitioned to the new API and have experienced it.

/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 May 31, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, logicalhan

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

@dgrisonnet
Copy link
Member

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2022
@logicalhan
Copy link
Member Author

Since the deprecated metric and the alpha one replacing it have the same labels, I think it is fine to graduate the new metric to stable at the same time as removing the old one since users have been using this API for a while now without needing to extend it.

For what it's worth, I would have been against doing the two changes during the same release if the labels were different. That's because unless the metric is completely removed, you can't make sure that users transitioned to the new API and have experienced it.

It was actually already deprecated in a previous release.

@dgrisonnet
Copy link
Member

Yeah, I know, but even though the metric was deprecated, we were still exposing it. Maybe I am too cautious with things becoming Stable, but since once they graduate we are not able to change the API anymore, I'd rather give more time to users to experiment with metrics and find potential gaps rather than have to deal with two metrics serving similar purposes.

@logicalhan
Copy link
Member Author

Yeah, I know, but even though the metric was deprecated, we were still exposing it.

This is actually not correct. After deprecation, metrics become auto-hidden, thereby triggering people to manually unhide it if they want to continue using the metric.

@k8s-ci-robot k8s-ci-robot merged commit 5219122 into kubernetes:master May 31, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone May 31, 2022
@leilajal
Copy link
Contributor

leilajal commented Jun 2, 2022

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 2, 2022
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. area/apiserver area/stable-metrics Issues or PRs involving stable metrics cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants