stacklok / stacklok/toolhive

Metrics migration steps aren't performable/verifiable under the operator

Open
#6,434 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-triage
Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Summary

Step 1 of the migration in docs/observability.md says to point a scraper at the
diagnostics port and confirm metrics arrive. Under the operator today that step isn't
actually performable: mcpserver_controller.go:1467 declares only the proxy
containerPort, and :1561 only the proxy ServicePort. The diagnostics listener is
genuinely up on 9464 (or whatever prometheusPort resolves to) — it's just
undiscoverable. A PodMonitor has no named port to key on, and prometheusPort is
intentionally absent from MCPTelemetryConfigSpec (see the drift-table justification
in telemetry_drift_test.go), so there's no field to even declare it explicitly.

Separately, step 2 ("set metricsOnTransportPort: false and confirm nothing else was
still scraping it") isn't verifiable either. The startup WARN reports configuration,
not usage — it fires because the flag is on, not because a scraper actually hit the
endpoint. And the transport-port /metrics mux registration outranks the /
catch-all, so those requests bypass the telemetry middleware entirely; there's no
counter anywhere that would tell you whether traffic there is real.

Proposed fixes

  1. Discoverability: add a named containerPort (e.g. metrics) on the workload
    pod when EnablePrometheusMetricsPath is set, so a PodMonitor can target it by
    name. Leave the Service alone — deliberately, per the existing docs guidance that
    the diagnostics port must not be routed through anything internet-facing.
  2. Verifiability: wrap the transport-port /metrics handler (mounted at
    pkg/runner/runner.go around the mountPrometheusHandlerOnTransportPort call)
    with a request counter, so step 2 has something concrete to check — "requests to
    the transport-port copy since startup: N" — before an operator turns it off.

Neither blocks #6371; raised there as informational.

Raised by @jhrozek on #6371:
https://github.com/stacklok/toolhive/pull/6371#pullrequestreview-5027693972

Part of #6271. Related: #6384.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the migration steps in docs/observability.md, then inspect mcpserver_controller.go around lines 1467 and 1561 and telemetry_drift_test.go for the existing port constraints. Review pkg/runner/runner.go around mountPrometheusHandlerOnTransportPort for the transport-port metrics path. Done means the diagnostics port is discoverable by a named workload port and transport-port requests expose a count that makes the migration checks verifiable.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes, prometheus
Domain
devops, observability-sre
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.