Skip to content

feat(helm): add metrics endpoint and optional ServiceMonitor support#1802

Open
mesutoezdil wants to merge 3 commits intokagent-dev:mainfrom
mesutoezdil:feat/helm-metrics-servicemonitor
Open

feat(helm): add metrics endpoint and optional ServiceMonitor support#1802
mesutoezdil wants to merge 3 commits intokagent-dev:mainfrom
mesutoezdil:feat/helm-metrics-servicemonitor

Conversation

@mesutoezdil
Copy link
Copy Markdown
Contributor

@mesutoezdil mesutoezdil commented May 5, 2026

Revives #1435 which was closed by stalebot. The controller already supports METRICS_BIND_ADDRESS and METRICS_SECURE via flags, this just wires them up properly in the chart.

When controller.metrics.enabled is true the chart sets the env vars, exposes the metrics port on the Deployment and Service, and optionally creates a ServiceMonitor for Prometheus Operator.

Usage:

controller:
  metrics:
    enabled: true
    port: 9093
    secure: false
    serviceMonitor:
      enabled: true
      labels:
        release: prometheus

Closes #1369

Copilot AI review requested due to automatic review settings May 5, 2026 19:29
@github-actions github-actions Bot added the enhancement New feature or request label May 5, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds Helm-chart support for exposing the controller’s existing metrics endpoint and optionally creating a Prometheus Operator ServiceMonitor, so observability can be enabled from chart values instead of manual env/service customization.

Changes:

  • Adds controller.metrics values for enabling metrics, choosing the port, configuring secure mode, and toggling ServiceMonitor creation.
  • Wires metrics settings into the controller ConfigMap, Deployment, and Service so the endpoint is exposed when enabled.
  • Adds Helm unit tests covering Service, Deployment, and ServiceMonitor rendering for the new metrics options.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
helm/kagent/values.yaml Adds chart values for controller metrics and ServiceMonitor settings.
helm/kagent/tests/controller-servicemonitor_test.yaml Adds render tests for the new ServiceMonitor template.
helm/kagent/tests/controller-service_test.yaml Verifies the controller Service adds a metrics port when enabled.
helm/kagent/tests/controller-deployment_test.yaml Verifies the controller Deployment/ConfigMap pick up metrics ports and env vars.
helm/kagent/templates/controller-servicemonitor.yaml Introduces the optional Prometheus Operator ServiceMonitor.
helm/kagent/templates/controller-service.yaml Exposes the metrics port on the controller Service.
helm/kagent/templates/controller-deployment.yaml Exposes the metrics port on the controller container.
helm/kagent/templates/controller-configmap.yaml Sets metrics-related env vars consumed by the controller.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -0,0 +1,20 @@
{{- if and .Values.controller.metrics.enabled .Values.controller.metrics.serviceMonitor.enabled }}
Comment on lines +17 to +19
- port: metrics
interval: {{ .Values.controller.metrics.serviceMonitor.interval }}
scrapeTimeout: {{ .Values.controller.metrics.serviceMonitor.scrapeTimeout }}
Comment thread helm/kagent/values.yaml
metrics:
enabled: false
port: 9093
secure: false
Comment on lines +15 to +19
{{- if .Values.controller.metrics.enabled }}
- port: {{ .Values.controller.metrics.port }}
targetPort: {{ .Values.controller.metrics.port }}
protocol: TCP
name: metrics
@github-actions github-actions Bot added enhancement New feature or request and removed enhancement New feature or request labels May 5, 2026
Adds controller.metrics values to enable the Prometheus metrics endpoint
and optionally deploy a ServiceMonitor for Prometheus Operator.

When controller.metrics.enabled is true the chart sets METRICS_BIND_ADDRESS
and METRICS_SECURE env vars, exposes the metrics container port, and adds
a named port to the controller Service. Setting controller.metrics.serviceMonitor.enabled
to true additionally creates a ServiceMonitor resource.

Closes kagent-dev#1369

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
- Gate ServiceMonitor on Capabilities.APIVersions to avoid install failures
  on clusters without Prometheus Operator CRDs
- Add scheme/tlsConfig to ServiceMonitor endpoint when secure=true
- Document secure default and NodePort exposure in values.yaml

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
@mesutoezdil mesutoezdil force-pushed the feat/helm-metrics-servicemonitor branch from 47384db to 2008de2 Compare May 5, 2026 19:49
@danielorbach
Copy link
Copy Markdown

danielorbach commented May 6, 2026

Hey — I've put up an alternative shape for #1369 in #1803. The main substantive difference is RBAC: the controller binary defaults --metrics-secure=true, which engages controller-runtime's WithAuthenticationAndAuthorization filter on /metrics. That filter calls TokenReview and SubjectAccessReview against the API server for every scrape, so the controller's ServiceAccount needs create rights on authentication.k8s.io/tokenreviews and authorization.k8s.io/subjectaccessreviews. As things stand here, enabling metrics with secure-serving on (the default) returns 5xx on every scrape because those rights aren't granted, and there's no unbound metrics-reader ClusterRole for cluster operators to bind to their scraper's ServiceAccount.

The kubebuilder/operator-sdk default — and what the kmcp sub-chart already shipped in this same umbrella does — is two ClusterRoles plus a single auth ClusterRoleBinding. That's the shape #1803 takes. It also uses a separate dedicated metrics Service rather than reusing the existing one, again matching kmcp. ServiceMonitor is deferred there.

Both shapes should solve #1369; happy to converge in either direction if maintainers have a preference.

@mesutoezdil
Copy link
Copy Markdown
Contributor Author

mesutoezdil commented May 6, 2026

@danielorbach Added metrics-auth-role (ClusterRole + binding) for the controller SA's TokenReview/SubjectAccessReview rights, and an unbound metrics-reader ClusterRole operators can bind their scraper SA to. Both gated on controller.metrics.enabled. If the dedicated metrics Service shape from #1803 is preferred, we can add that too @EItanya.

Adds metrics-auth-role ClusterRole and ClusterRoleBinding so the
controller ServiceAccount can call TokenReview and SubjectAccessReview
when --metrics-secure=true is in use. Adds an unbound metrics-reader
ClusterRole that operators can bind to their scraper ServiceAccount.

Both resources are gated on controller.metrics.enabled.

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
@mesutoezdil mesutoezdil force-pushed the feat/helm-metrics-servicemonitor branch from 5a3305c to eace68e Compare May 6, 2026 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Helm Chart: Add support for enabling metrics endpoint and optional ServiceMonitor

3 participants