Skip to content

feat: make model metrics endpoints configurable #1000

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: make model metrics endpoints configurable #1000

wants to merge 1 commit into from

Conversation

nayihz
Copy link
Contributor

@nayihz nayihz commented Jun 17, 2025

fix: #16

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 17, 2025
@k8s-ci-robot k8s-ci-robot requested a review from Jeffwan June 17, 2025 10:10
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nayihz
Once this PR has been reviewed and has the lgtm label, please assign danehans for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 17, 2025
@netlify Netlify
Copy link

netlify bot commented Jun 17, 2025 •
edited
Loading

✅ Deploy Preview for gateway-api-inference-extension ready!

Name Link
🔨 Latest commit 7466a28
🔍 Latest deploy log https://app.netlify.com/projects/gateway-api-inference-extension/deploys/6852178d9b9a59000856d315
😎 Deploy Preview https://deploy-preview-1000--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@nirrozenbaum
Copy link
Contributor

nirrozenbaum commented Jun 17, 2025 •
edited
Loading

I have some doubts about adding additional fields to InferencePool.
there are alternative ways to achieve the same goal, like using command line args.
changes to CRDs should be discussed and get broad agreement using proposals.

cc @kfswain @ahg-g @robscott @danehans @elevran

/hold for others to comment.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2025
@nayihz nayihz force-pushed the feat_config_metric branch from ffad486 to f57478d Compare June 17, 2025 10:34
@nayihz
Copy link
Contributor Author

nayihz commented Jun 17, 2025

@nirrozenbaum , thanks for you advise. I think you are right.
Command line args or environment variable, which way do you prefer?

@nirrozenbaum
Copy link
Contributor

@nirrozenbaum , thanks for you advise. I think you are right. Command line args or environment variable, which way do you prefer?

@nayihz I would start with command-line args with default values (the existing ones).
we can always iterate if needed.

@nayihz nayihz force-pushed the feat_config_metric branch from f57478d to 9fa17f7 Compare June 17, 2025 12:40
@elevran
Copy link
Contributor

elevran commented Jun 17, 2025

@nayihz @nirrozenbaum this introduces a fixed endpoint for all model servers in the pool.
Would it make sense to use the Prometheus-formatted annotations as the source of truth when present and fallback to the configuration when missing?
For example:

apiVersion: v1
kind: Pod
metadata:
  name: my-app
  annotations:
    prometheus.io/scrape: "true"
    prometheus.io/path: "/metrics"
    prometheus.io/port: "8080"

@nirrozenbaum
Copy link
Contributor

nirrozenbaum commented Jun 17, 2025 •
edited
Loading

@nayihz @nirrozenbaum this introduces a fixed endpoint for all model servers in the pool. Would it make sense to use the Prometheus-formatted annotations as the source of truth when present and fallback to the configuration when missing? For example:

apiVersion: v1
kind: Pod
metadata:
  name: my-app
  annotations:
    prometheus.io/scrape: "true"
    prometheus.io/path: "/metrics"
    prometheus.io/port: "8080”

@elevran we already have a fixed endpoint, so this PR is not introducing it :). the intention was to make that endpoint configurable.
but your suggestion makes sense as an improvement.

danehans
danehans reviewed Jun 17, 2025
View reviewed changes
pkg/epp/backend/metrics/fake.go
@@ -73,6 +73,10 @@ func (f *FakePodMetricsClient) FetchMetrics(ctx context.Context, pod *backend.Po
return res.Clone(), nil
}

func (f *FakePodMetricsClient) GetMetricEndpoint(_ *backend.Pod, _ int32) string {
return "fake-metric-endpoint"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Pluralize func name and return value

danehans
danehans reviewed Jun 17, 2025
View reviewed changes
cmd/epp/runner/runner.go Outdated
@@ -110,6 +110,9 @@ var (
"vllm:lora_requests_info",
"Prometheus metric for the LoRA info metrics (must be in vLLM label format).")

metricPort = flag.Int("metricPort", 0, "Port to scrape metrics from pods")
Copy link
Contributor

@danehans danehans Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Pluralize CLI args

Additional verbosity should be used for the CLI arg names, e.g. podMetricsPort or modelMetricsPort.

@nirrozenbaum @kfswain any thoughts on the name for these CLI arga?

Copy link
Collaborator

Choose a reason for hiding this comment

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

podMetricsPort or modelMetricsPort seem reasonable enough. If we wanted to be very specific we could call it modelServerMetricsPort as we are scraping for very specific metrics. But thats honestly being very nitty.

Copy link
Contributor

Choose a reason for hiding this comment

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

modelServerMetricsPort sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed. PTAL.

@nirrozenbaum
Copy link
Contributor

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2025
@nayihz nayihz force-pushed the feat_config_metric branch from 9fa17f7 to 7466a28 Compare June 18, 2025 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danehans danehans danehans left review comments

@kfswain kfswain kfswain left review comments

@Jeffwan Jeffwan Awaiting requested review from Jeffwan

@nirrozenbaum nirrozenbaum Awaiting requested review from nirrozenbaum

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose baseline algorithm parameters as configurable
6 participants
@nayihz @k8s-ci-robot @nirrozenbaum @elevran @danehans @kfswain