Expose the metrics migration switch to CLI and operator - #6371
Conversation
#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>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6371 +/- ##
========================================
Coverage 77.82% 77.82%
========================================
Files 760 761 +1
Lines 72997 73190 +193
========================================
+ Hits 56808 56960 +152
- Misses 16184 16225 +41
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
#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>
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
061c443 to
35b6e1e
Compare
github-actions
Bot
added
size/S
…t-optin # Conflicts: # pkg/runner/runner.go
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.
github-actions
Bot
added
size/S
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Reviewed this with a few specialist passes. The core design is right and I want to say that clearly before the nits: the pointer tri-state with no kubebuilder default is correct, and I verified it actually holds end to end — no defaulting webhook on MCPTelemetryConfig, omitempty keeps nil out of both the applied object under SSA and the persisted RunConfig, so flipping DefaultMetricsOnTransportPort later really will move workloads that already exist. That was the hard part.
Comments below are mostly small. Two bigger things that don't map to a line in this diff:
vMCP won't cut over. pkg/vmcp/server/server.go:610 mounts /metrics on the main mux unconditionally, and ServeMetricsOnTransportPort() is consulted in exactly one place (pkg/runner/diagnostics.go:76). The inline telemetry.Config path already exposed this field on virtualmcpservers.yaml before this PR, so that part isn't yours — but vmcpconfig/converter.go:513 routes TelemetryConfigRef through NormalizeMCPTelemetryConfig, so this PR adds a second path where the field is settable and inert. When #6384 flips the default, MCPServer moves and vMCP silently doesn't. Worth a companion issue.
The migration isn't performable in-cluster yet. docs/observability.md:221 step 1 says point your scraper at the diagnostics port and confirm metrics arrive. Under the operator there's nothing to point at: mcpserver_controller.go:1467 declares only the proxy containerPort and :1561 only the proxy ServicePort, and prometheusPort is intentionally absent from MCPTelemetryConfigSpec. The pod is listening on 9464, it's just undiscoverable — a PodMonitor has no named port to key on. One named containerPort when EnablePrometheusMetricsPath is set would fix it (leave the Service alone, the docs are right that it shouldn't be on internet-facing Services).
Related: step 2 says set it false "and confirm nothing else was still scraping it", but there's no way to confirm that. The startup WARN reports configuration, not usage, and the transport-port /metrics outranks the / catch-all so those scrapes bypass the telemetry middleware entirely. Wrapping the handler at runner.go:388 with a counter is about ten lines and makes that step real.
Neither blocks this PR. Not requesting changes.
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>
github-actions
Bot
added
size/M
2 participants
Summary
#6370 added
metricsOnTransportPort, but left it reachable only by editing storedconfiguration — so nobody could actually use the migration window it exists for.
This exposes it:
--otel-metrics-on-transport-portprometheus.metricsOnTransportPortonMCPTelemetryConfigThat makes the intended migration possible: point a scraper at the diagnostics port,
confirm metrics arrive, then turn the transport-port copy off to prove nothing else
depended on the old location — all before the default flips.
Part of #6271
Type of change
Test plan
task test)task test-e2e)task lint-fix)TestResolveMetricsOnTransportPortcovers all three CLI states — absent, explicitlytrue, explicitly false — driving a real cobra command rather than asserting on the
resolver in isolation, since the behaviour depends on
Flags().Changed.Pre-existing failures on
mainin untouched files:TestValidateOCIRegistryHost,TestParseGitReference_*, and lint findings incmd/thv/app/upgrade.goandpkg/vmcp/config/crd_cli_roundtrip_test.go.Does this introduce a user-facing change?
Yes, additive: a new CLI flag and a new optional CRD field. Neither changes behaviour
unless set — the default is unchanged.
Special notes for reviewers
Preserving the tri-state is the whole point, and both surfaces could have broken it.
bool(cobra cannot bind*bool) and readthrough
resolveMetricsOnTransportPort, which consultsFlags().Changedso anabsent flag stays unset instead of resolving to
false.+kubebuilder:default, so theAPI server materialises nothing at admission.
Either shortcut would pin the window's value into every workload created during it,
and the eventual cutover would move nobody while appearing to work.
Drift table:
metricsOnTransportPortnow exists on both sides, so it moves out oftelemetryIgnoredOnRuntimeOnlyand intotelemetryFieldMappings.Builder option instead of another parameter.
WithMetricsOnTransportPortis adedicated option rather than an 11th positional argument on
WithTelemetryConfigFromFlags/MaybeMakeConfig. The field is transitional, anddeleting it later should touch one function rather than two long signatures. It must
be applied after the telemetry config, which the call site does and the doc comment
states.
Remaining before the window can start: nothing on the opt-in side. After it: flip
DefaultMetricsOnTransportPort, then remove the field, the flag, and the CRD entry.Generated with Claude Code