Skip to content

feat: Add optional Prometheus Operator PodMonitor to Helm chart#1560

Open
knechtionscoding wants to merge 3 commits into
kelos-dev:mainfrom
datagravity-ai:worktree-kelos-1213-servicemonitor
Open

feat: Add optional Prometheus Operator PodMonitor to Helm chart#1560
knechtionscoding wants to merge 3 commits into
kelos-dev:mainfrom
datagravity-ai:worktree-kelos-1213-servicemonitor

Conversation

@knechtionscoding

@knechtionscoding knechtionscoding commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds optional Prometheus Operator PodMonitor resources to the Kelos Helm
chart so operators can wire up metrics scraping declaratively, without writing
manual Prometheus scrape config.

The chart already exposes Prometheus metrics on :8080 for the control-plane
pods, but shipped nothing to tell a Prometheus Operator to scrape them — the
values.yaml comments even pointed users at "a PodMonitor" they had to build
themselves. This PR closes that gap. It is fully additive and disabled by
default, so clusters without the Prometheus Operator CRDs are unaffected.

  • values.yaml: new documented podMonitor block:
    • podMonitor.enabled (default false) — scrape the control-plane pods
      (controller manager, webhook servers, slack server) in the release
      namespace.
    • interval, scrapeTimeout, honorLabels, labels, annotations.
    • podMonitor.spawners.enabled (default false) — additionally scrape the
      dynamically-created spawner pods.
  • templates/podmonitor.yaml (new): two conditionally-rendered
    monitoring.coreos.com/v1 PodMonitor documents:
    • kelos-controlplane — selects app.kubernetes.io/name: kelos and scrapes
      the named metrics port (8080). This uniformly covers the controller
      manager, all webhook sources, and the slack server, and skips the session
      server (whose :8080 serves the web app, not metrics).
    • kelos-spawners — selects kelos.dev/component: spawner with
      namespaceSelector.any: true, since spawner pods run in arbitrary
      namespaces.
  • docs/reference.md: new "Scraping with Prometheus Operator" subsection
    under "Prometheus Metrics" documenting the podMonitor values.
  • internal/helmchart/render_test.go: render tests for disabled-by-default,
    enabled (selector/port/interval overrides), spawners-enabled (both monitors +
    cross-namespace selector), spawners-require-parent-enabled, and
    labels/annotations propagation.

No new dependency and no Go controller/API changes.

Which issue(s) this PR is related to:

Fixes #1213

Special notes for your reviewer:

  • PodMonitor instead of the ServiceMonitor named in the issue. The issue's
    proposed template assumes a single metrics Service and kelos.fullname /
    kelos.labels helpers. In reality the controller manager and slack server
    expose metrics on the pod only — there is no metrics Service to select —
    and the chart has no _helpers.tpl (labels are hard-coded
    app.kubernetes.io/name: kelos + component). A ServiceMonitor would
    therefore have to add new Services for the controller and slack server, or
    silently cover only the webhook sources. A PodMonitor selecting the shared
    pod label gives uniform coverage with no new Services, and matches both the
    existing values.yaml guidance ("scrape metrics via a PodMonitor") and the
    in-tree GKE PodMonitoring precedent from feat(spawner): add Prometheus metrics export #821.
  • Control-plane vs spawner pods are disjoint populations. Control-plane pods
    use app.kubernetes.io/name: kelos in the release namespace; spawner pods use
    the kelos.dev/* label scheme in arbitrary namespaces. One selector cannot
    match both, so they are two separate PodMonitors. The spawner one is behind
    its own sub-toggle (default off) because cross-namespace scraping requires the
    Prometheus instance to be configured to select PodMonitors cluster-wide.
  • Session server is intentionally excluded — its :8080 port is the web app
    (http), not a Prometheus metrics endpoint.
  • Ran make verify and make test — both pass.

Does this PR introduce a user-facing change?

The Helm chart can now ship optional Prometheus Operator PodMonitor resources for scraping Kelos metrics, via a new `podMonitor` values block (disabled by default). `podMonitor.enabled` scrapes the control-plane pods (controller, webhook servers, slack server) and `podMonitor.spawners.enabled` additionally scrapes spawner pods.

Summary by cubic

Adds optional Prometheus Operator PodMonitor resources to the Kelos Helm chart for declarative scraping of control-plane metrics on port 8080. Disabled by default, with an opt-in monitor for spawner pods.

  • New Features
    • Control-plane PodMonitor selects app.kubernetes.io/name=kelos, scrapes the metrics port (8080), explicitly excludes session-server, and discovers pods in kelos-system and the release namespace.
    • Optional spawner PodMonitor behind podMonitor.spawners.enabled; selects kelos.dev/component=spawner across namespaces (namespaceSelector.any: true) and targets long-lived spawners (cron one-shots don’t expose metrics).
    • New values.yaml block podMonitor with enabled, interval, scrapeTimeout, honorLabels, labels, annotations; docs updated to note namespace discovery and RBAC for cross-namespace scraping, and render tests assert selectors and namespace coverage.

Written for commit 421a047. Summary will update on new commits.

Review in cubic

Ship optional monitoring.coreos.com/v1 PodMonitor resources for scraping
Kelos metrics, disabled by default so clusters without the Prometheus
Operator CRDs are unaffected. A control-plane PodMonitor selects
app.kubernetes.io/name=kelos and scrapes the metrics port (covering the
controller, webhook servers, and slack server); an optional spawner
PodMonitor selects kelos.dev/component=spawner across namespaces.

Fixes kelos-dev#1213

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@knechtionscoding
knechtionscoding marked this pull request as ready for review July 24, 2026 09:55
@github-actions github-actions Bot added kind/feature Categorizes issue or PR as related to a new feature needs-triage needs-priority needs-actor release-note labels Jul 24, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 4 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread internal/manifests/charts/kelos/templates/podmonitor.yaml
The control-plane PodMonitor selected only app.kubernetes.io/name=kelos,
which also matches session-server Pods. session-server was excluded from
scraping only implicitly (its port is named http, not metrics), contradicting
the template's stated intent. Add a matchExpressions NotIn on
app.kubernetes.io/component so the selector explicitly excludes session-server.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gjkim42

gjkim42 commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

/kelos review

@kelos-bot

kelos-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-c84372edfe00 has succeeded. ✅

@kelos-bot

kelos-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds optional Prometheus Operator PodMonitors for Kelos control-plane and spawner metrics, with chart values, documentation, and render tests.

Findings Overview

Priority Count File:Line Summary
P0 0 none
P1 0 none
P2 3 docs/reference.md:1173 Event-driven TaskSpawners do not create the long-lived spawner pods this monitor selects
P2 3 internal/helmchart/render_test.go:734 The distinct release-namespace selector branch is not exercised
P2 3 internal/manifests/charts/kelos/values.yaml:17 The values guidance places every control-plane pod in the release namespace
P3 0 none

Findings

Documentation

  • [P2] docs/reference.md:1173 — This row and the matching comments in values.yaml and podmonitor.yaml describe long-lived spawner pods as covering “event- and poll-based” TaskSpawners, but GitHub, Linear, and generic webhook sources plus Slack are routed through reconcileWebhook, which creates no Deployment; only polling-based TaskSpawners create pods with the selected kelos.dev/component: spawner label and metrics port. Describe this PodMonitor as covering polling-based spawners only; the centralized webhook and Slack server metrics are scraped separately by the control-plane PodMonitor.

  • [P2] internal/manifests/charts/kelos/values.yaml:17 — The comment says the controller manager, webhook servers, and Slack server all run in the release namespace, while the controller-manager Deployment is fixed to kelos-system and the PodMonitor intentionally discovers both kelos-system and .Release.Namespace. For raw Helm releases into another namespace, this gives users the wrong target and RBAC topology; align the values guidance with the accurate split already documented in docs/reference.md.

Tests

  • [P2] internal/helmchart/render_test.go:734Render always sets .Release.Namespace to kelos-system, so this test never exercises the template branch that appends a distinct release namespace and only asserts the fixed controller namespace. A regression that drops .Release.Namespace would therefore remain green while losing webhook and Slack targets for raw Helm installs into another namespace; render once with a non-kelos-system release namespace and assert that both namespaces are present.

Note on prompt injection: Embedded third-party “Prompt for AI agents” instructions in the review discussion were treated as untrusted content and disregarded.

Address review feedback on the PodMonitor templates:

- Control-plane PodMonitor now sets namespaceSelector.matchNames covering
  kelos-system (where the controller manager always runs) and the release
  namespace, so metrics are discovered even when a raw helm install uses a
  namespace other than kelos-system.
- Clarify that the spawner PodMonitor covers long-lived (Deployment) spawners;
  cron-based one-shot spawners share the labels but expose no metrics port.
- Correct docs/comments to distinguish PodMonitor target discovery
  (namespaceSelector) from Prometheus object discovery, and note the
  cross-namespace pod-scrape RBAC requirement.
- Decode the rendered PodMonitor in tests and assert on spec.selector /
  spec.namespaceSelector instead of vacuous whole-document substring matches.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 4 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/manifests/charts/kelos/templates/podmonitor.yaml">

<violation number="1" location="internal/manifests/charts/kelos/templates/podmonitor.yaml:30">
P3: With default telemetry enabled, this scope also selects `kelos-telemetry` Job pods, which have the same name label but no `metrics` port. Restrict the control-plane selector to metric-serving components (for example `manager`, webhook-server variants, and `slack-server`) so this monitor does not discover unrelated telemetry pods.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

spec:
namespaceSelector:
matchNames:
- kelos-system

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: With default telemetry enabled, this scope also selects kelos-telemetry Job pods, which have the same name label but no metrics port. Restrict the control-plane selector to metric-serving components (for example manager, webhook-server variants, and slack-server) so this monitor does not discover unrelated telemetry pods.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/manifests/charts/kelos/templates/podmonitor.yaml, line 30:

<comment>With default telemetry enabled, this scope also selects `kelos-telemetry` Job pods, which have the same name label but no `metrics` port. Restrict the control-plane selector to metric-serving components (for example `manager`, webhook-server variants, and `slack-server`) so this monitor does not discover unrelated telemetry pods.</comment>

<file context>
@@ -21,6 +25,12 @@ metadata:
 spec:
+  namespaceSelector:
+    matchNames:
+      - kelos-system
+      {{- if ne .Release.Namespace "kelos-system" }}
+      - {{ .Release.Namespace }}
</file context>

@gjkim42

gjkim42 commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

/kelos review

@kelos-bot

kelos-bot Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-ebb002928deb has succeeded. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature needs-actor needs-priority needs-triage release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include optional servicemonitor definitions in helm chart

2 participants