Skip to content

feature: add slo metrics to flowcontrol#944

Open
loicmarchal wants to merge 5 commits into
llm-d:mainfrom
loicmarchal:feature/add-slo-metrics-to-flowcontrol
Open

feature: add slo metrics to flowcontrol#944
loicmarchal wants to merge 5 commits into
llm-d:mainfrom
loicmarchal:feature/add-slo-metrics-to-flowcontrol

Conversation

@loicmarchal

@loicmarchal loicmarchal commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds the following metrics:

  • request duration per SLO TTFT class
  • adds SLO TTFT class to queue size and queue bytes metrics
  • counter for incoming requests per SLO class.

The TTFT SLO is passed with the request in the headers (x-slo-ttft-ms).

These metrics will allow tracking the percentage of requests with a duration below or above their associated SLO class as well as the number of incoming and queued requests per SLO class.

The SLO classes might need to have smaller intervals (currently the interval is 200 ms), but it would increase cardinality of the Prometheus histogram

Which issue(s) this PR fixes:
Fixes #945

Does this PR introduce a user-facing change?:

NONE

@github-actions

Copy link
Copy Markdown
Contributor

🚨 Unsigned commits detected! Please sign your commits.

For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation.

@github-actions github-actions Bot requested review from ahg-g and vMaroon April 30, 2026 19:39
@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch 4 times, most recently from 490ffc7 to 227bfeb Compare May 4, 2026 21:22
@loicmarchal

Copy link
Copy Markdown
Contributor Author

@kfswain I reopened the PR in this repo. We still have the ongoing discussion about defining the SLO classes for the metrics in the InferenceObjective (kubernetes-sigs/gateway-api-inference-extension#2685 (comment)).

One simple approach could be to simply add an SLO class label on the InferenceObjective, but it ties the InferenceObjective to the SLO ms target, and it relies on the user setting the right headers (x-gateway-inference-objective and x-slo-ttft-ms)

apiVersion: inference.networking.x-k8s.io/v1alpha2
kind: InferenceObjective
metadata:
  name: realtime
spec:
  poolRef:
    name: my-pool
  priority: 10
  sloClass: fast

We could add SLO class bucket min and max values, and check if the value in x-slo-ttft-ms is within the bucket defined in the InferenceObjective

apiVersion: inference.networking.x-k8s.io/v1alpha2
kind: InferenceObjective
metadata:
  name: realtime
spec:
  poolRef:
    name: my-pool
  priority: 10
  sloClass:
    label: fast
    minMs: 0
    maxMs: 100

But what to do if it's not within the bucket? Send the metrics with the wrong label anyway? Fix the label by finding the right label from another InferenceObjective with the right min/max? Consider the request has the wrong InferenceObjective given the SLO deadline defined in x-slo-ttft-ms? We also need to validate the InferenceObjectives, as SLO class buckets should not overlap?

If we enable the SLO deadline request ordering, requests will be effectively prioritized based on the values defined in the x-slo-ttft-ms header. So it is in fact an InferenceObjective?

What's your take on this? Thank you!

Another question: the InferenceObjective type is not yet migrated from GIE. Should we migrate it in this repo?

@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch 2 times, most recently from 606d9f8 to c637df6 Compare May 8, 2026 21:49
@github-actions github-actions Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 8, 2026
@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch from c637df6 to d3abda4 Compare May 12, 2026 03:24
@github-actions github-actions Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2026
@loicmarchal loicmarchal marked this pull request as ready for review May 12, 2026 03:27
@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch from d3abda4 to 379b882 Compare May 12, 2026 17:11
@loicmarchal

Copy link
Copy Markdown
Contributor Author

I have moved the SLO metrics range definition to the InferenceObjective. Here is a summary of the design choices.

The SLO class seems semantically tied to the objective's contract: a high-priority realtime objective implies tight TTFT deadlines, and a low-priority batch objective implies loose ones. A request shouldn't realistically declare a realtime objective with a 500ms deadline.

InferenceObjective name = SLO class label

The InferenceObjective name (e.g., realtime, batch) is used directly as the SLO class label in metrics. No separate sloClass.label field is needed.

Per-metric ranges

The spec uses a nested structure with separate TTFT and TPOT ranges, since both x-slo-ttft-ms and x-slo-tpot-ms headers already exist in the system:

spec:
  slo:
    ttft:
      minMs: 0
      maxMs: 100
    tpot:
      maxMs: 20

This avoids a breaking API change when TPOT support is needed later.

Ranges are optional and can be open-ended

  • slo is optional — objectives without it still work for priority-based scheduling (SLO class defaults to "none" in metrics).
  • minMs and maxMs are independently optional: omitting minMs implies no lower bound (0), omitting maxMs implies no upper bound.
  • Semantics are [minMs, maxMs) (inclusive lower, exclusive upper).

Overlapping ranges are allowed

The SLO class is determined by the objective name (set via the x-gateway-inference-objective header), not by matching the TTFT value against ranges. So overlapping ranges between objectives don't cause ambiguity. The range is a validation hint, not a classification key. This avoids the complexity of admission-time overlap checking.

Warning logs for mismatches

When a request's x-slo-ttft-ms (or x-slo-tpot-ms) value falls outside the range defined on its target InferenceObjective, a warning is logged. The request is still processed normally — the mismatch indicates a client misconfiguration, not a rejection condition.

@kfswain does it align with what you had in mind?

@loicmarchal

Copy link
Copy Markdown
Contributor Author

/cc @kfswain @nirrozenbaum

@github-actions github-actions Bot requested review from kfswain and nirrozenbaum May 12, 2026 17:26
@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch 4 times, most recently from fbc36c6 to a50e511 Compare May 19, 2026 20:01
@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch from a50e511 to 2fac756 Compare June 15, 2026 16:54
@loicmarchal loicmarchal requested review from a team, LukeAVanDrie and shmuelk as code owners June 15, 2026 16:54
@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch from 2fac756 to aa33d6e Compare June 15, 2026 16:55
Comment on lines +183 to +190
func extractHeader(req flowcontrol.FlowControlRequest, name string) string {
infReq := req.InferenceRequest()
if infReq == nil || infReq.Headers == nil {
return ""
}
return request.GetHeader(infReq.Headers, name)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this function is needed.

  1. req.InferenceRequest() should never be nil
  2. Even if req.InferenceRequest().Headers is nil, request.Headers will not crash. If one does val, ok := m[k], where m is map will a nil value, ok will be set to false, correctly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, nil for req.InferenceRequest().Headers is covered by the val, ok := headers[key] access to the map.
req.InferenceRequest() should not be nil, I would still check for nil to be safe, but that does not require an extra function. I'll fix it and remove the function

Comment on lines +345 to +351
func extractHeader(req flowcontrol.FlowControlRequest, name string) string {
infReq := req.InferenceRequest()
if infReq == nil || infReq.Headers == nil {
return ""
}
return fwkrequest.GetHeader(infReq.Headers, name)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment about the same function in the other package.

@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch 4 times, most recently from e97d8d0 to 1217b9c Compare June 22, 2026 21:25
@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch 2 times, most recently from 32ccde3 to 2d2dea9 Compare July 10, 2026 03:26
Signed-off-by: Loic Marchal <lmarchal@redhat.com>
Signed-off-by: Loic Marchal <lmarchal@redhat.com>
Signed-off-by: Loic Marchal <lmarchal@redhat.com>
Signed-off-by: Loic Marchal <lmarchal@redhat.com>
Signed-off-by: Loic Marchal <lmarchal@redhat.com>
@loicmarchal loicmarchal force-pushed the feature/add-slo-metrics-to-flowcontrol branch from 2d2dea9 to dd1d849 Compare July 10, 2026 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

[Flow Control] Add SLO values to flow control metrics

3 participants