Skip to content

refactor: refactor monitoring session #1906

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

Open
capri-xiyue wants to merge 5 commits into
base: main
Choose a base branch
from
+1 −5
Open

refactor: refactor monitoring session #1906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
315bb66
refactor monitoring session
capri-xiyue Nov 26, 2025
67a1f40
changed format
capri-xiyue Nov 26, 2025
ce3f246
changed it back to gke only monitoring
capri-xiyue Dec 11, 2025
1532533
fixed pipeline
capri-xiyue Dec 11, 2025
13d0ddd
updated monitoring
capri-xiyue Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/charts/inferencepool/templates/gke.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ spec:
logging:
enabled: true # log all requests by default
---
{{- if or .Values.inferenceExtension.monitoring.gke.enabled (and .Values.inferenceExtension.monitoring.prometheus.enabled .Values.inferenceExtension.monitoring.prometheus.auth.enabled) }}
{{- if and .Values.inferenceExtension.monitoring.prometheus.enabled .Values.inferenceExtension.monitoring.prometheus.auth.enabled }}
Copy link
Contributor

@JeffLuoo JeffLuoo Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, the prometheus enabled is by default set to false in the values.yaml: https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/v1.2.0-rc.1/config/charts/inferencepool/values.yaml#L55-L56

This implies that the monitoring stack on GKE won't have all required objects like roles binding by default if inferenceExtension.monitoring.prometheus.enabled is set to false with this change.

Shall we use inferenceExtension.monitoring.prometheus.enabled to control the enablement of monitoring stacks regardless of the provider? If so, we need to update the mentioning of inferenceExtension.monitoring.gke.enable=true, e.g. https://github.com/search?q=repo%3Allm-d%2Fllm-d%20inferenceExtension.monitoring.gke.enable%3Dtrue&type=code

wdyt? @ahg-g

Copy link
Contributor

@ahg-g ahg-g Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, the prometheus enabled is by default set to false in the values.yaml: https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/v1.2.0-rc.1/config/charts/inferencepool/values.yaml#L55-L56

This implies that the monitoring stack on GKE won't have all required objects like roles binding by default if inferenceExtension.monitoring.prometheus.enabled is set to false with this change.

Can you elaborate here? the existing gke.enable flag that we are removing in this PR is not controlling that either, right?

Shall we use inferenceExtension.monitoring.prometheus.enabled to control the enablement of monitoring stacks regardless of the provider? If so, we need to update the mentioning of inferenceExtension.monitoring.gke.enable=true, e.g. https://github.com/search?q=repo%3Allm-d%2Fllm-d%20inferenceExtension.monitoring.gke.enable%3Dtrue&type=code

wdyt? @ahg-g

I agree as long as all what this flag enables is indeed prometheus-related.

Copy link
Contributor

@JeffLuoo JeffLuoo Dec 12, 2025

Choose a reason for hiding this comment

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

Yes. EPP uses Prometheus as the metric source behind the metrics endpoint.

The PR LGTM then, we just need to make sure we update all references later to include inferenceExtension.monitoring.prometheus.enabled in addition to setting the provider to GKE.

Copy link
Contributor

@JeffLuoo JeffLuoo Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

One more place to update -

gateway-api-inference-extension/site-src/guides/latency-based-predictor.md

Line 62 in db2b7ce

--set inferenceExtension.monitoring.gke.enabled=true \

With this change, we switch to use inferenceExtension.monitoring.prometheus.enabled so mentioning of inferenceExtension.monitoring.gke.enabled needs to be updated as well. This is the only mentioning I found in EPP repo: https://github.com/search?q=repo%3Akubernetes-sigs%2Fgateway-api-inference-extension+monitoring.gke&type=code

{{- $metricsReadSA := printf "%s-metrics-reader-sa" .Release.Name -}}
{{- $metricsReadSecretName := printf "%s-metrics-reader-secret" .Release.Name -}}
{{- $metricsReadRoleName := printf "%s-%s-metrics-reader" .Release.Namespace .Release.Name -}}
Expand Down
4 changes: 0 additions & 4 deletions config/charts/inferencepool/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,6 @@ inferenceExtension:
secretName: inference-gateway-sa-metrics-reader-secret
# additional labels for the ServiceMonitor
extraLabels: {}

# DEPRECATED: The 'gke' configuration will be removed in the next release.
gke:
enabled: false
tracing:
enabled: false
otelExporterEndpoint: "http://localhost:4317"
Expand Down