Refactor: Define PodMetricsClient interface and hide implementation details of vllm metrics processing #26
Conversation
…etails of vllm metrics processing
k8s-ci-robot
added
cncf-cla: yes
| @@ -130,3 +130,37 @@ func (p *Provider) refreshPodsOnce() error { | |||
| p.podMetrics.Range(mergeFn) | |||
| return nil | |||
| } | |||
|
|
|||
| func (p *Provider) refreshMetricsOnce() error { | |||
There was a problem hiding this comment.
This is moved from metrics.go, no new code
There was a problem hiding this comment.
The name of this file is slightly confusing. It reads as if it's intended to be a factory class, but then we have metrics related funcs, while there is a metrics.go file.
What is this file intended to do?
There was a problem hiding this comment.
yeah probably the name is too broad. It intends to "provide info of the backend", thus "provider", and metrics is part of the "info".
| } | ||
|
|
||
| // FetchMetrics fetches metrics from a given pod. | ||
| func (p *PodMetricsClientImpl) FetchMetrics(pod backend.Pod, existing *backend.PodMetrics) (*backend.PodMetrics, error) { |
There was a problem hiding this comment.
This is moved from pod_client.go, no new code
| @@ -75,7 +73,7 @@ func main() { | |||
|
|
|||
| s := grpc.NewServer() | |||
|
|
|||
| pp := backend.NewProvider(&backend.PodMetricsClientImpl{}, &backend.FakePodLister{Pods: pods}) | |||
| pp := backend.NewProvider(&vllm.PodMetricsClientImpl{}, &backend.FakePodLister{Pods: pods}) | |||
There was a problem hiding this comment.
why did we move it to a vllm package? shouldn't this be agnostic the model server?
There was a problem hiding this comment.
Model servers don't always share the same metric names, so I expect we will need some "adapter code" for each model server.
There was a problem hiding this comment.
We will likely share some helper functions across model servers, but until we integrate with the next model server, I put metrics scraping code in vllm.
|
/lgtm just so we move forward |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, liu-cong 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 |
k8s-ci-robot
added
the
approved
No description provided.