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

Deprecate apiserver_longrunning_gauge metric in favor of apiserver_longrunning_requests #14856

Merged
merged 6 commits into from
Jul 3, 2023

Conversation

arapulido
Copy link
Contributor

What does this PR do?

kube_apiserver metric longrunning_gauge has been deprecated since 1.23 in favor of longrunning_requests.

See PR: kubernetes/kubernetes#103799

More over, the deprecated metric was fully removed in 1.25 https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.25.md#other-cleanup-or-flake-2

This is also a metric that we use in the OOTB dashboard, so this dashboard has an empty widget for all customers running >= 1.25

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #14856 (ad7731c) into master (63cfc86) will not change coverage.
The diff coverage is n/a.

❗ Current head ad7731c differs from pull request most recent head 191452d. Consider uploading reports for the commit 191452d to get more accurate results

Flag Coverage Δ
kube_apiserver_metrics 97.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@github-actions
Copy link

github-actions bot commented Jun 23, 2023

Test Results

  4 files    4 suites   37s ⏱️
11 tests 11 ✔️ 0 💤 0
24 runs  22 ✔️ 2 💤 0

Results for commit 191452d.

♻️ This comment has been updated with latest results.

@arapulido arapulido marked this pull request as ready for review June 23, 2023 10:37
@arapulido arapulido requested review from a team as code owners June 23, 2023 10:37
@@ -62,6 +63,8 @@
# https://kubernetes.io/docs/reference/using-api/deprecation-policy/#rest-resources-aka-api-objects
'apiserver_requested_deprecated_apis': 'requested_deprecated_apis',
# For Kubernetes >= 1.23
# https://github.com/kubernetes/kubernetes/pull/103799
'apiserver_longrunning_requests': 'longrunning_requests',
Copy link
Contributor

Choose a reason for hiding this comment

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

usually we try to remap the new metrics name to the previous name, to ease the life of users (they don't need to update their dashboards or monitors)

If the new metric return exactly the same value (unit) and is expose with the same type (here gauge), we can remap the metric to the previous name.

if the unit change, we can use a transformer function to comeback to the previous unit. (when it makes sens)

Suggested change
'apiserver_longrunning_requests': 'longrunning_requests',
'apiserver_longrunning_requests': 'longrunning_gauge',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@clamoriniere what happens in the case for Kubernetes versions (like 1.23) that had both metrics (the deprecated one, and the new one)

Copy link
Contributor

Choose a reason for hiding this comment

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

So the last applied will override the other. which is fine since the value should be equal

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@arapulido arapulido force-pushed the ara.pulido/deprecate_longrunning_gauge branch from ad7731c to 191452d Compare June 28, 2023 16:06
@arapulido arapulido requested a review from clamoriniere June 28, 2023 16:23
@arapulido arapulido merged commit 5087dd2 into master Jul 3, 2023
@arapulido arapulido deleted the ara.pulido/deprecate_longrunning_gauge branch July 3, 2023 08:13
edengorevoy pushed a commit that referenced this pull request Jul 3, 2023
…ngrunning_requests (#14856)

* Deprecate apiserver_longrunning_gauge in favor of apiserver_longrunning_requests

* Add longrunning_requests metric to the metadata

* Override the longrunning_gauge metric for backwards compatibility

* Fix metadata validation

* fix tests

* Fix metadata
@therc
Copy link
Contributor

therc commented Jul 10, 2023

Is this only going to be fixed in Agent version 7.47.0?

Thank you

@clamoriniere
Copy link
Contributor

hi @therc

Yes in the next agent release so 7.47.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants