-
Notifications
You must be signed in to change notification settings - Fork 54
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
Refactor kserve metrics solution to work with prometheus annotations #200
Refactor kserve metrics solution to work with prometheus annotations #200
Conversation
a6795b0
to
147e9d3
Compare
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 think we need to add logic such that existing resources related to the old solution are deleted.
I'd also recommend changing the commit/PR title to reflect the generic solution rather than being vLLM specific. |
controllers/reconcilers/kserve_istio_servicemonitor_reconciler.go
Outdated
Show resolved
Hide resolved
3b0bed7
to
a79bd61
Compare
4aa1bc4
to
5da6f8f
Compare
Sure, updated. |
Code changes look ok to me (see minor comment about function name |
2124edf
to
e91aa6c
Compare
controllers/reconcilers/kserve_istio_servicemonitor_reconciler.go
Outdated
Show resolved
Hide resolved
controllers/reconcilers/kserve_istio_servicemonitor_reconciler.go
Outdated
Show resolved
Hide resolved
chore: Make sure that the vLLM metrics are getting correctly exposed when the prometheus annotations are set. Fixes [RHOAIENG-6264] - Ensure Metrics work in vLLM Signed-off-by: Spolti <fspolti@redhat.com>
Signed-off-by: Spolti <fspolti@redhat.com>
Signed-off-by: Spolti <fspolti@redhat.com>
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.
lgtm
Signed-off-by: Spolti <fspolti@redhat.com>
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 code looks good.
I haven't been able to try the changes. I'll use the upcoming ODH release to do any needed testings there. It is not idea, but it is for what I have time.
/lgtm
Great work @spolti & @VedantMahabaleshwarkar .
Key: "component", | ||
Operator: metav1.LabelSelectorOpIn, | ||
Values: []string{"predictor", "explainer", "transformer"}, |
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.
For later improvement, I inspected the pod and it also gets the serving.kserve.io/inferenceservice
label.
So, maybe we can change the selector similarly to what you did in the webhook fix.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: israel-hdez, spolti, VedantMahabaleshwarkar 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 |
chore: Make sure that the vLLM metrics are getting correctly exposed when the
prometheus annotations are set.
Fixes [RHOAIENG-6264] - Ensure Metrics work in vLLM
Signed-off-by: Spolti fspolti@redhat.com
Description
This change removes the hardcode monitoring Service, ServiceMonitor and PeerAuthentication resources in favor of a centralized configuration using the PodMonitor and Istio's ServiceMonitor resources to fit our needs and make them available in the aggregated metrics endpoint: http://:15020/stats/prometheus
How Has This Been Tested?
To test it follow the following steps (Can be tested with OVMS ServingRuntime as well):
When everything is deployed, check the metrics in the Prometheus console, it should show the
vllm
prefixed metrics:Merge criteria: