feat: add etcd_debugging_lease_total gauge metric #18067
Conversation
|
Hi @jimmy-bro. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
|
@jimmy-bro: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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-sigs/prow repository. |
|
/ok-to-test |
jimmy-bro
force-pushed
the
metrics
branch
2 times, most recently
from
10a9754
to
d2bf743
Compare
server/lease/metrics.go
Outdated
| leaseTotal = prometheus.NewGauge(prometheus.GaugeOpts{ | ||
| Namespace: "etcd_debugging", | ||
| Subsystem: "lease", | ||
| Name: "total", |
There was a problem hiding this comment.
| Name: "total", | |
| Name: "active", |
Looking at the naming scheme above we have "granted_total", "revoked_total", "renewed_total", so if in the Help you called them "active leases" then let's put it in the name of the metric.
In prometheus conventions https://prometheus.io/docs/practices/naming/, suffix total is reserved for accumulating metrics like counters, for gauges we should skip it.
There was a problem hiding this comment.
Thank you for the reminder. I never knew about this before, now I've learned something new。
jimmy-bro
force-pushed
the
metrics
branch
4 times, most recently
from
81ae9e4
to
10174f3
Compare
|
@jimmy-bro could you please rebase and trigger the test again? |
Signed-off-by: Jimmy- <[email protected]>
|
Thanks for your mentiong @chaochn47 , I've already rebased the PR on git |
There was a problem hiding this comment.
Thanks for raising this @jimmy-bro - Overall it's looking good but can you please add some tests so we would be able to merge it?
Refer to #17983 for examples of unit and integration metrics tests.
The feature was discussed in #17874 .