Add flowcontrol queue length in bytes metric #2044
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
cncf-cla: yes
|
Hi @RyanRosario. Thanks for your PR. I'm waiting for a github.com 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. DetailsInstructions 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. |
k8s-ci-robot
added
the
size/M
RyanRosario
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/assign @LukeAVanDrie |
|
@ahg-g: GitHub didn't allow me to assign the following users: LukeAVanDrie. Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn 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. |
LukeAVanDrie
left a comment
LukeAVanDrie
left a comment
There was a problem hiding this comment.
Thanks, Ryan! I have left a few minor inline comments, but I have no blocking concerns.
This LGTM!
/assign @ahg-g
| metrics.AddFlowControlQueueBytes( | ||
| flowKey.ID, priority, | ||
| req.InferencePoolName(), | ||
| req.ModelName(), req.TargetModelName(), req.ByteSize()) | ||
| defer metrics.SubFlowControlQueueBytes( | ||
| flowKey.ID, priority, | ||
| req.InferencePoolName(), | ||
| req.ModelName(), req.TargetModelName(), req.ByteSize()) |
There was a problem hiding this comment.
nit: Even though req.ByteSize() is technically immutable right now, it is a defensive best practice to capture the size in a local variable before the defer.
This guarantees that the Add and Sub operations are always mathematically symmetric. If a future refactor makes changes how ByteSize() is calculated (making it mutable), we risk the Gauge drifting permanently (e.g., subtracting more than we added or vice versa).
| ) | ||
|
|
||
| // Basic Inc/Dec | ||
| AddFlowControlQueueBytes("user-a", "100", pool, model, target, 32.0) |
There was a problem hiding this comment.
nit: The helper function AddFlowControlQueueBytes accepts a uint64. While Go's untyped constants allow 32.0 to compile, it is cleaner to use integer literals (e.g., 32) to match the function signature.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LukeAVanDrie, RyanRosario The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Oh, @RyanRosario, since we are adding a new public metric that operators will use, this is a user-facing change. Please update the release note section in your PR description (on this and the other metrics PRs): E.g., |
5 participants
What type of PR is this?
/kind documentation
/kind feature
What this PR does / why we need it:
It adds new observability metrics for flow control.
Which issue(s) this PR fixes:
Related to #1708
Does this PR introduce a user-facing change?: