Serve vMCP metrics on a separate diagnostics listener - #6368
Merged
Conversation
amirejaz
requested review from
ChrisJBurns,
JAORMX,
blkt,
jerm-dro,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
August 19, 2026 02:00
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6368 +/- ##
==========================================
+ Coverage 77.82% 77.87% +0.05%
==========================================
Files 760 761 +1
Lines 72997 73182 +185
==========================================
+ Hits 56808 56991 +183
- Misses 16184 16186 +2
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Aug 19, 2026
amirejaz
force-pushed
the
vmcp-diagnostics-listener
branch
from
August 19, 2026 03:37
6d45011 to
c8ff688
Compare
amirejaz
changed the base branch from
main
to
metrics-transport-port-deprecation
August 19, 2026 03:37
amirejaz
force-pushed
the
vmcp-diagnostics-listener
branch
from
August 19, 2026 14:19
c8ff688 to
07b71c6
Compare
Virtual MCP registered /metrics on the mux that serves MCP traffic, under a comment noting it was unauthenticated. That is the vMCP half of finding E in #6271; #6296 moved the proxy half. Bind it to the diagnostics listener, so access can be governed by port: NetworkPolicy matches on pods, ports, and protocols and cannot filter on HTTP path, so a shared port makes "allow MCP, deny scraping" unexpressible. This does not authenticate the endpoint; the diagnostics listener carries no middleware. Honour the same migration switch the proxy path uses, so this is not a breaking change on its own: while metricsOnTransportPort is on, /metrics stays reachable on the MCP port too, and a deprecation warning names it. That matters more here than for the proxy. Nothing shipped enables the metrics path for MCPServer, but three vMCP artifacts do -- examples/vmcp-config.yaml and two operator docs -- so more deployments plausibly have it on. Add Server.DiagnosticsAddress so the resolved port can be discovered programmatically. The listener falls back to an available port when the configured one is taken, so tests and callers cannot construct the address; the vMCP telemetry tests now scrape through it. Part of #6271 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
amirejaz
force-pushed
the
vmcp-diagnostics-listener
branch
from
August 20, 2026 12:36
07b71c6 to
acafafb
Compare
jhrozek
reviewed
Aug 25, 2026
jhrozek
left a comment
Contributor
There was a problem hiding this comment.
Read through this one carefully — the port split and the reasoning in pkg/diagnostics both look right to me. Three things I'd want changed before it goes in, all small. A few smaller nits (the local 127.0.0.1 constant, the unguarded diagnosticsServer field vs the exported DiagnosticsAddress, and freePort's find-then-release race in the new test) I'm happy to leave as follow-ups.
Three findings from Jakub's review. Use diagnostics.NotServedHereHandler once metricsOnTransportPort is false, matching the three proxies. vMCP was the one place still returning a bare 404, which is indistinguishable from a typo and leaves an operator with a still-pointed-at-the-MCP-port scraper no way to learn where metrics went. Move startDiagnostics before the MCP listener is created, matching the runner's ordering. It previously ran after go s.httpServer.Serve(listener) and before close(s.ready), so a diagnostics failure returned an error from Start with the MCP listener already accepting connections in a background goroutine and Ready() never closed -- anything blocked on it would hang forever. Document that a failure here is fatal to Start, on purpose, matching the runner's choice for the same tradeoff: metrics are opt-in, so failing loudly beats silently shipping without the observability #6271 exists to provide. The reorder introduces its own narrower version of the problem it fixes: if diagnostics starts successfully but the subsequent MCP net.Listen fails, the diagnostics listener is now orphaned with nothing to stop it. Close that by stopping diagnostics on that failure path. Add a regression test forcing startDiagnostics to fail (an unresolvable host) and asserting Start returns before the MCP listener is created and before Ready() closes, plus a body assertion on the existing opted-out-metrics test so a regression to a bare 404 is caught here, not only in pkg/diagnostics. Clarify which telemetry config path reaches prometheusPort and metricsOnTransportPort. Both live on telemetry.Config, which only the inline (deprecated) spec.config.telemetry path embeds directly; the shared MCPTelemetryConfig used by the preferred telemetryConfigRef does not carry them. The doc section describing these knobs sat under both examples without saying so. Part of #6271
reyortiz3
approved these changes
Aug 25, 2026
2 tasks
amirejaz
added a commit
that referenced
this pull request
Aug 26, 2026
Fix the pointer aliasing in NormalizeMCPTelemetryConfig. It assigned spec.Prometheus.MetricsOnTransportPort straight into config, and NormalizeTelemetryConfig's "Create a copy to avoid modifying the input" is only a shallow copy of the struct -- so a future write through the returned config's pointer would reach back into the CRD spec object. Nil still means unset, but a non-nil value is now cloned. Added a table case per state (unset/true/false) plus a dedicated regression test asserting the result does not alias the input and that mutating it leaves the input untouched. Point the CLI flag's default at telemetry.DefaultMetricsOnTransportPort instead of a hardcoded true, so `thv run --help` stops lying the moment the cutover flips the constant. Trim four doc comments back to what the reader actually needs: - Two exported field docs (telemetry.Config and the CRD's PrometheusConfig) drop the "why a pointer with no kubebuilder default" paragraph. Those comments become the OpenAPI description under kubectl explain and in swagger.json; a cluster admin configuring a scrape target doesn't need kubebuilder marker semantics, and the reasoning already lives at its canonical home on TestMetricsOnTransportPortNotPersistedWhenUnset. - WithMetricsOnTransportPort's doc drops the "why a separate option instead of a constructor parameter" paragraph, keeping only the actual calling contract (apply after telemetry config). - Two comments citing a review discussion by name (mountPrometheusHandler- OnTransportPort's extraction rationale, and a test doc naming jhrozek's #6370 comment) are reworded to describe the code on its own terms. Drop the duplicate ptr[T] helper in metrics_transport_port_test.go; the package already has boolPtr in run_flags_test.go. Two findings don't map to a line in this diff and are tracked separately rather than expanding this PR's scope: vMCP's own switch already exists on the unmerged #6368, not duplicated here (#6433 tracks landing order); and the operator doesn't yet expose a named port or a way to verify the transport-port copy is unused before disabling it (#6434). Part of #6271 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
amirejaz
added a commit
that referenced
this pull request
Aug 26, 2026
* Restore transport-port metrics behind a migration switch #6296 moved /metrics to a diagnostics port. That is a breaking change for any deployment scraping the old location, and it has not shipped yet, so there is still room to give it a notice window rather than land it cold. Serve /metrics on both ports for now. The diagnostics listener always runs; the transport-port copy is controlled by MetricsOnTransportPort and defaults to on, so no existing scrape configuration breaks. Operators can move a scraper to the new port, verify it, and set the field to false to prove nothing else depended on the old one. Closing the window is a one-line change to DefaultMetricsOnTransportPort. The field is a pointer with no kubebuilder default, which is what makes the eventual flip work. RunConfig.TelemetryConfig is serialised and CRD defaults are materialised at admission, so a plain bool would be written into every workload created during the window and would survive the cutover unchanged -- the flip would silently move nobody. Nil means unset and is resolved at startup instead, with tests covering both the resolution and the round trip. Deployments that set the field explicitly are deliberately not moved by the flip. Part of #6271 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Point the deprecation notice at a tracking issue The notice said the transport-port copy would be removed but never said when, and a deprecation without a deadline is easy to ignore. Users also had nowhere to look for the plan. Reference #6384 from the startup warning, the field documentation, and the migration steps. A tracking issue rather than a version or date: the release that closes the window is not known yet and may move, and the issue can be revised where these strings cannot -- the field description ships to users through the CRD schema and kubectl explain. Also point DefaultMetricsOnTransportPort at it, since flipping that constant is not the whole job and the issue carries the cleanup list. Part of #6271 * Expose the metrics migration switch to CLI and operator #6370 added metricsOnTransportPort but left it reachable only by editing stored configuration, so nobody could actually use the migration window it exists for. Add --otel-metrics-on-transport-port and prometheus.metricsOnTransportPort on MCPTelemetryConfig, so an operator can move a scraper to the diagnostics port and then turn the old location off to prove nothing else depended on it. Both preserve the tri-state. The CLI flag is bound as a plain bool and read through resolveMetricsOnTransportPort, which uses Flags().Changed so an absent flag stays unset rather than resolving to false; the CRD field is an optional pointer with no default marker so nothing is materialised at admission. Either would otherwise pin the window's value into every workload created during it, and the cutover would move nobody. The runtime field is now mapped on both sides, so it moves from the drift table's runtime-only ignores into the mappings. Carried by a dedicated builder option rather than a further parameter on the two telemetry constructors: the field is transitional, and removing it later should touch one function instead of two long signatures. Part of #6271 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Point the migration switch surfaces at the tracking issue The CLI flag help and the MCPTelemetryConfig field description said the default would change but not when, and gave nowhere to look for the plan. Both are user-facing -- the flag through thv run --help and the field through kubectl explain -- so they need the reference as much as the startup warning does. Reference #6384 from both. See its sibling commit on the base branch for why a tracking issue rather than a version or date. Part of #6271 * Drop the now-stale runtime-only drift entry for metricsOnTransportPort Merging main duplicated the field across both drift tables: main still lists it in telemetryIgnoredOnRuntimeOnly with a justification saying it is deliberately absent from the CRD, while this branch already promoted it into telemetryFieldMappings by adding the CRD field. Remove the stale ignore entry; the mapping is the current truth. * Address review: real aliasing bug, doc trims, one duplicate helper Fix the pointer aliasing in NormalizeMCPTelemetryConfig. It assigned spec.Prometheus.MetricsOnTransportPort straight into config, and NormalizeTelemetryConfig's "Create a copy to avoid modifying the input" is only a shallow copy of the struct -- so a future write through the returned config's pointer would reach back into the CRD spec object. Nil still means unset, but a non-nil value is now cloned. Added a table case per state (unset/true/false) plus a dedicated regression test asserting the result does not alias the input and that mutating it leaves the input untouched. Point the CLI flag's default at telemetry.DefaultMetricsOnTransportPort instead of a hardcoded true, so `thv run --help` stops lying the moment the cutover flips the constant. Trim four doc comments back to what the reader actually needs: - Two exported field docs (telemetry.Config and the CRD's PrometheusConfig) drop the "why a pointer with no kubebuilder default" paragraph. Those comments become the OpenAPI description under kubectl explain and in swagger.json; a cluster admin configuring a scrape target doesn't need kubebuilder marker semantics, and the reasoning already lives at its canonical home on TestMetricsOnTransportPortNotPersistedWhenUnset. - WithMetricsOnTransportPort's doc drops the "why a separate option instead of a constructor parameter" paragraph, keeping only the actual calling contract (apply after telemetry config). - Two comments citing a review discussion by name (mountPrometheusHandler- OnTransportPort's extraction rationale, and a test doc naming jhrozek's #6370 comment) are reworded to describe the code on its own terms. Drop the duplicate ptr[T] helper in metrics_transport_port_test.go; the package already has boolPtr in run_flags_test.go. Two findings don't map to a line in this diff and are tracked separately rather than expanding this PR's scope: vMCP's own switch already exists on the unmerged #6368, not duplicated here (#6433 tracks landing order); and the operator doesn't yet expose a named port or a way to verify the transport-port copy is unused before disabling it (#6434). Part of #6271 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Virtual MCP registers
/metricson the mux that serves MCP traffic — under a commentthat acknowledged it was unauthenticated. That's the vMCP half of Finding E in #6271;
#6296 moved the proxy half. Until both move, the endpoint still shares the port
deployments route publicly.
/metricson the dedicated diagnostics listener (pkg/diagnostics), so accesscan be governed by port.
NetworkPolicymatches on pods, ports, and protocols andcannot filter on HTTP path, so while
/metricsshares the MCP port there is no wayto express "allow MCP traffic, deny metrics scraping".
change on its own. While
metricsOnTransportPortis on (the default),/metricsstays reachable on the MCP port too and a deprecation warning names it. Once it is
off, the application mux 404s rather than letting
/metricsfall through to the/MCP handler.
Server.DiagnosticsAddress()so the resolved port is discoverable.What this does not do: it does not authenticate, rate limit, or audit
/metrics.The diagnostics listener carries no middleware, and the host is inherited from the MCP
listener (
0.0.0.0under the operator), so the endpoint stays reachable from otherpods. Restricting who can reach that port is what protects it — see the
NetworkPolicyexample in
docs/observability.md.Part of #6271
Type of change
Test plan
task test)task test-e2e)task lint-fix)New coverage in
pkg/vmcp/server/diagnostics_test.go: no listener without telemetry,metrics bound to a port distinct from the MCP port, configured port honoured, host
defaulting, idempotent stop.
TestServeHandlerRegistersMetricsWhenTelemetryEnabledisrenamed to
TestServeHandlerDoesNotServeMetricsand now asserts the 404, since its oldassertion described the behaviour this PR removes.
Three telemetry tests scraped
/metricsoff the MCP address and now go throughDiagnosticsAddress()— necessary rather than cosmetic, since the listener falls backto an available port when the configured one is taken, so the address cannot be
constructed.
Remaining local failures (
TestValidateOCIRegistryHost,TestParseGitReference_*) andlint findings (
cmd/thv/app/upgrade.go,pkg/vmcp/config/crd_cli_roundtrip_test.go)are pre-existing on
mainin files this PR does not touch.Does this introduce a user-facing change?
Not on merge.
/metricsbecomes available on the diagnostics port(
prometheusPort, default9464) while remaining on the MCP port, so existingscrape configurations keep working until the window closes.
When it does close, this is the half that matters more.
The blast radius is larger here than for the proxy half. Nothing shipped enables the
metrics path for MCPServer, but three vMCP artifacts do —
examples/vmcp-config.yaml,docs/operator/virtualmcpserver-api.md, anddocs/operator/virtualmcpserver-observability.md— so more deployments plausibly haveit on. The example and the observability guide are updated to point at the diagnostics
port.
This should ship in the same release as #6296 and #6370, so operators get one
migration event rather than the same feature moving twice. None of it has shipped —
v0.44.0was cut before #6296 merged.Special notes for reviewers
DiagnosticsAddress()is worth a look beyond vMCP: the resolved diagnostics portcurrently only appears in a startup log, which is why the E2E metrics helpers in #6296
hardcode
9464(Copilot flagged this, and those threads are still open). This accessoris the vMCP-side answer; the equivalent for the proxy path — surfacing it in workload
status — is still outstanding.
TestServeHandlerMetricsOnTransportPortpins both ends of the migration — default andexplicitly-on serve on the MCP port, opted-out 404s — so neither the window nor the
cutover can regress unnoticed.
Generated with Claude Code