Add flowcontrol queue length in bytes metric#2044
Add flowcontrol queue length in bytes metric#2044k8s-ci-robot merged 4 commits intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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. |
|
/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
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
|
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., |
| prometheus.GaugeOpts{ | ||
| Subsystem: InferenceExtension, | ||
| Name: "flow_control_queue_bytes", | ||
| Help: metricsutil.HelpMsgWithStability("Current number of bytes associated with requests actively managed by the EPP flow control layer, from the start of the EnqueueAndWait call until a final outcome is reached.", compbasemetrics.ALPHA), |
There was a problem hiding this comment.
please remove the phrase EnqueueAndWait as it is an internal detail and describe the metric in abstract terms.
There was a problem hiding this comment.
Ryan is mirroring my description for the flow_control_queue_size (len) metric I already added. Ryan, do you mind updating it there as well?
There was a problem hiding this comment.
Will do. Thank you both for catching that.
There was a problem hiding this comment.
Ah, I looked through the last commit and saw that two were updated (queue_size and queue_duration). We still need queue_bytes.
There was a problem hiding this comment.
Sorry about that. This is addressed.
fa18396 to
055ad5d
Compare
|
All feedback addressed. Ready for final review. |
|
/approve |
|
/assign @LukeAVanDrie |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, LukeAVanDrie, RyanRosario The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…-api-inference-extension#2044) * Add flow_control_queue_bytes metric * Add flow_control_queue_bytes metric and documentation * Address reviewer feedback * Update comment to remove internal function name --------- Co-authored-by: Ryan Rosario <6713180+RyanRosario@users.noreply.github.com>
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?: