-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Untwist apf metrics #110162
Untwist apf metrics #110162
Conversation
1d5b72f
to
5e84826
Compare
/retest |
…mples Members are not used in (waiting,executing) pairs, so stopped using the wrapper that adds such pairing.
5e84826
to
cd33c7c
Compare
The force-push to cd33c7c is the rebase onto master. Now GitHub shows just the changes of this PR. |
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/sample_and_watermark.go
Show resolved
Hide resolved
RequestsWaiting: spg.urVec.NewForLabelValuesSafe(0, initialWaitingDenominator, append([]string{LabelValueWaiting}, labelValues...)), | ||
RequestsExecuting: spg.urVec.NewForLabelValuesSafe(0, initialExecutingDenominator, append([]string{LabelValueExecuting}, labelValues...)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the phase values are now passed down inside of labelValues
with your recent changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR stops using the pair layer for the read-vs-write vector, and does not change the use of the pairing layer for priority-oriented vector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand your point, we are still using this to initialize the watermarks: https://github.com/kubernetes/kubernetes/pull/110162/files#diff-d835764bb6390d3466e60b871d322044538e32acc03b67675d2aaba1d37e134dR95.
In the list of the label values passed down to this function we are already providing LabelValueExecuting
when initializing the watermark, so we will end up with label values equal to []{"executing", "executing", "mutating"}
, which means that the requestKind
label will be set to executing
instead of "mutating".
/hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgrisonnet : note that this PR changes the line 95 that you cited from calling ReadWriteConcurrencyPairVec
, omitting the phase label name in its arguments and letting that func add the phase label, to calling ReadWriteConcurrencyGaugeVec
instead, which does not add the phase label to those given on line 95.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah indeed, I got confused, thank you for clarifying
/unhold
/triage accepted |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MikeSpreitzer, wojtek-t 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 |
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR removes the unhelpful use of the (waiting,executing) pair structure for the read_vs_write_request_count_samples and ..._watermark histograms. This histograms are not used in such a paired association.
This is part of the larger campaign to replace same-and-watermark histograms with timing ratio histograms. The actual replacement will come in a later PR, of which #110104 is the latest version.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This PR is built on #110101. That means that currently this PR also includes the commits of #110101. Once that one merges, I will rebase this onto
master
.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig api-machinery
/cc @dgrisonnet
/cc @wojtek-t
@tkashem