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
[WIP] Add sampling histogram #94672
[WIP] Add sampling histogram #94672
Conversation
This covers SamplingHistogram but not SamplingHistogramVec (because the vec support of prometheus is not public).
/sig api-machinery |
10f02a3
to
c338899
Compare
cad34f3
to
64943ed
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MikeSpreitzer The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Before this change, the staticcheck test in the pull-kubernetes-verify job complained about some possible nil pointer dereferences, but those would never happen because `t.Errorf` aborts the test. Added control flow (that will never be reached, if you understand `t.Errorf`) to make it obvious that the nil dereferences will never happen.
@MikeSpreitzer: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
/test pull-kubernetes-bazel-test |
This is waiting on prometheus/client_golang#803 to merge. |
kubernetes/utils#190 is going to be important |
prometheus/client_golang#803 is merged now. Sorry for the delay. I'll cut a release v1.8.0 of prometheus/client_golang today so you don't need to lock to a particular commit in your If you need help with what I said about implementing a more efficient sampling histogram, let me know. For example, I could create a PR on top of this, once it is merged (or even before, if you prefer that). |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
As this has gone stale now, let me repeat my offer above to help implementing my suggestion. Let me know if that's something you would appreciate. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
/remove-lifecycle rotten |
OK, I'll try to propose something in the next few days. |
@MikeSpreitzer I created a PR against your fork to demonstrate my proposal. |
@MikeSpreitzer: PR needs rebase. 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. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. 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/test-infra repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
In https://github.com/kubernetes/apiserver/blob/master/pkg/util/flowcontrol/metrics/sample_and_watermark.go today there is the abstraction of sampling a variable to populate a histogram. It has an awkward bit of code: a loop to call the
Observe
method of a Prometheus Histogram a variable number of times. This led to the suggestion prometheus/client_golang#796 . The discussion there suggested I create a new Prometheus abstraction, possibly out of (prometheus) tree. This PR is my start on such a creation, indeed out of the prometheus tree.There are three bits of tension here:
sampling-histogram_test.go
uses fake clocks. That's the only reasonable way to get sufficient control in a test that runs quickly. But the Prometheus tree does not include such clocks, so moving this code into the Prometheus tree would be problematic.sample_and_watermark.go
traffics in passive clocks (and with good reason: to support event-based testing of API Priority and Fairness code), but k8s.io/utils/clock does not define passive clocks.Which issue(s) this PR fixes:
Fixes prometheus/client_golang#796
Special notes for your reviewer:
For the moment I put a personal copyright on the code that could go in either Kubernetes or Prometheus, depending on how the relevant tensions are relaxed.
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: