KEP-4346: client-go: Add informer metrics #4349

chenk008 · 2023-11-29T09:02:38Z

One-line PR description: Add informer metrics about reflector/queue/eventHandler.

Issue link: client-go: Add metrics into informer #4346

Other comments:

linux-foundation-easycla · 2023-11-29T09:02:42Z

The committers listed above are authorized under a signed CLA.

✅ login: chenk008 (e481b2f, 4ce3b45, e7bdb50, df89d93, a82fe33, 073ee03)
✅ login: logicalhan / name: Han Kang (a82fe33, 073ee03)

k8s-ci-robot · 2023-11-29T09:02:47Z

Welcome @chenk008!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-11-29T09:02:48Z

Hi @chenk008. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

kikisdeliveryservice · 2023-12-01T20:27:31Z

/ok-to-test

add test describe fill section: Feature Enablement and Rollback mitigate memory usage add memory usage metrics Signed-off-by: chenk008 <kongchen28@gmail.com>

keps/sig-api-machinery/4346-informer-metrics/README.md

aojea · 2024-02-07T10:19:28Z

keps/sig-api-machinery/4346-informer-metrics/README.md

+
+- Introduce the informer metrics struct `informerMetrics` contains queue/eventHandler metrics
+- Introduce the informer metrics provider interface `informerMetricsProvider`, implement in `k8s.io/component-base/metrics`
+- Add an environment to enable informer metrics


why do we need an environment variable to enable them?

why do we need an environment variable to enable them?

I think this got updated to a featuregate. This type of metric has been attempted at least once (maybe twice?) and reverted for problems each time. Being able to disable our first attempt could help avoid a rush to patch all the impacted clients.

With the new client-go featuregates, we also have the option of enabling early in our control-plane side components via the composition mechanism.

Replace it with featuregate, PTAL

Replace it with featuregate, PTAL

forgot to push?

aojea · 2024-02-07T10:23:06Z

keps/sig-api-machinery/4346-informer-metrics/README.md

+
+- `<package>`: `<date>` - `<test coverage>`
+
+##### Integration tests


IIRC there was some tests for metrics and granularity of them @logicalhan @dgrisonnet can you help us here?

We do have some precedence of integration tests for metrics such as these ones for the apiserver: https://github.com/kubernetes/kubernetes/blob/87fa400d9ddf4fa48ab32a7d745a3b60dc785e5d/test/integration/metrics/metrics_test.go as well as some utilities to write some https://github.com/kubernetes/kubernetes/blob/87fa400d9ddf4fa48ab32a7d745a3b60dc785e5d/staging/src/k8s.io/component-base/metrics/testutil/testutil.go.

Add some tests for metrics subsystem/label/granularity, PTAL

Add some tests for metrics subsystem/label/granularity, PTAL

forgot to push?

keps/sig-api-machinery/4346-informer-metrics/README.md

deads2k · 2024-02-07T17:40:37Z

I'm happy with this for apimachinery and PRR. The comments from others should be addressed.

Co-authored-by: Han Kang <hankang@google.com>

deads2k · 2024-02-08T18:12:57Z

/tide merge-method-squash

logicalhan · 2024-02-08T18:22:23Z

keps/sig-api-machinery/4346-informer-metrics/README.md

+	listDuration        SummaryMetric
+	numberOfItemsInList SummaryMetric


SummaryMetrics are non-aggregatable. It'd be better to use histograms instead.

deads2k · 2024-02-08T20:38:19Z

/label tide-merge-method-squash

k8s-ci-robot · 2024-02-08T20:38:21Z

@deads2k: The label(s) /label tide-merge-method-squash cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, lead-opted-in, tracked/no, tracked/out-of-tree, tracked/yes. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label tide-merge-method-squash

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.

deads2k · 2024-02-08T20:38:46Z

/label tide/merge-method-squash

deads2k · 2024-02-08T20:42:04Z

It looks like the author agreed to all feedback, forgot to push, and won't make it back online before KEP freeze (timezone check in slack). The delta for the agreed (but forgot to push) changes is small. Rather than miss freeze, I'm going to approve this and accept the minor fix up tomorrow.

Should some comment suddenly become contentious (I doubt it), we'll revert.

approving for apimachinery and PRR.

/lgtm
/approve

k8s-ci-robot · 2024-02-08T20:42:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chenk008, deads2k

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:

~~keps/sig-api-machinery/OWNERS~~ [deads2k]

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 added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 29, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 29, 2023

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 29, 2023

k8s-ci-robot requested a review from fedebongio November 29, 2023 09:02

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Nov 29, 2023

k8s-ci-robot requested a review from jpbetz November 29, 2023 09:02

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 29, 2023

chenk008 changed the title ~~Add informer metrics~~ [WIP] Add informer metrics Nov 29, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2023

chenk008 changed the title ~~[WIP] Add informer metrics~~ [draft] Add informer metrics Nov 29, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2023

chenk008 marked this pull request as draft November 29, 2023 09:07

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2023

chenk008 force-pushed the add_informer_metrics branch from 5d2b9d1 to 1b36da4 Compare November 29, 2023 10:14

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 29, 2023

chenk008 changed the title ~~[draft] Add informer metrics~~ Add informer metrics Nov 29, 2023

chenk008 marked this pull request as ready for review November 29, 2023 10:17

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2023

k8s-ci-robot requested a review from deads2k November 29, 2023 10:17

Add informer metrics

e481b2f

chenk008 force-pushed the add_informer_metrics branch from 1b36da4 to e481b2f Compare November 29, 2023 10:22

kikisdeliveryservice changed the title ~~Add informer metrics~~ KEP-4346: Add informer metrics Nov 30, 2023

kikisdeliveryservice changed the title ~~KEP-4346: Add informer metrics~~ KEP-4346: client-go: Add informer metrics Nov 30, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 1, 2023

use feature gate

4ce3b45

add test describe fill section: Feature Enablement and Rollback mitigate memory usage add memory usage metrics Signed-off-by: chenk008 <kongchen28@gmail.com>

chenk008 force-pushed the add_informer_metrics branch from 32e4b1c to 4ce3b45 Compare February 7, 2024 01:55

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 7, 2024

chenk008 added 2 commits February 7, 2024 11:29

add description about prometheus client

e7bdb50

make update-toc

df89d93

aojea reviewed Feb 7, 2024

View reviewed changes

keps/sig-api-machinery/4346-informer-metrics/README.md Show resolved Hide resolved

aojea reviewed Feb 7, 2024

View reviewed changes

logicalhan reviewed Feb 7, 2024

View reviewed changes

chenk008 and others added 2 commits February 8, 2024 11:22

Update keps/sig-api-machinery/4346-informer-metrics/README.md

a82fe33

Co-authored-by: Han Kang <hankang@google.com>

Update keps/sig-api-machinery/4346-informer-metrics/README.md

073ee03

Co-authored-by: Han Kang <hankang@google.com>

chenk008 requested review from logicalhan, dgrisonnet and aojea February 8, 2024 03:52

logicalhan reviewed Feb 8, 2024

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 8, 2024

k8s-ci-robot assigned deads2k Feb 8, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2024

k8s-ci-robot merged commit 48f7a8f into kubernetes:master Feb 8, 2024
4 checks passed

k8s-ci-robot added this to the v1.30 milestone Feb 8, 2024

deads2k mentioned this pull request Feb 9, 2024

KEP-4346: add PRR metadata file for 4346 #4511

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-4346: client-go: Add informer metrics #4349

KEP-4346: client-go: Add informer metrics #4349

chenk008 commented Nov 29, 2023

linux-foundation-easycla bot commented Nov 29, 2023 •

edited

Loading

k8s-ci-robot commented Nov 29, 2023

k8s-ci-robot commented Nov 29, 2023

kikisdeliveryservice commented Dec 1, 2023

aojea Feb 7, 2024

deads2k Feb 7, 2024

deads2k Feb 7, 2024

chenk008 Feb 8, 2024

deads2k Feb 8, 2024

aojea Feb 7, 2024

dgrisonnet Feb 7, 2024

chenk008 Feb 8, 2024

deads2k Feb 8, 2024

deads2k commented Feb 7, 2024

deads2k commented Feb 8, 2024

logicalhan Feb 8, 2024

deads2k commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

deads2k commented Feb 8, 2024

deads2k commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024


		- `<package>`: `<date>` - `<test coverage>`

		##### Integration tests

KEP-4346: client-go: Add informer metrics #4349

KEP-4346: client-go: Add informer metrics #4349

Conversation

chenk008 commented Nov 29, 2023

linux-foundation-easycla bot commented Nov 29, 2023 • edited Loading

k8s-ci-robot commented Nov 29, 2023

k8s-ci-robot commented Nov 29, 2023

kikisdeliveryservice commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Feb 7, 2024

deads2k commented Feb 8, 2024

Choose a reason for hiding this comment

deads2k commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

deads2k commented Feb 8, 2024

deads2k commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

linux-foundation-easycla bot commented Nov 29, 2023 •

edited

Loading