Restore transport-port metrics behind a migration switch - #6370
Merged
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>
amirejaz
requested review from
ChrisJBurns,
JAORMX,
aponcedeleonch,
blkt,
jerm-dro,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
August 19, 2026 03:22
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6370 +/- ##
==========================================
+ Coverage 73.04% 77.73% +4.68%
==========================================
Files 744 749 +5
Lines 78611 72139 -6472
==========================================
- Hits 57424 56075 -1349
+ Misses 17169 16059 -1110
+ Partials 4018 5 -4013 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Aug 19, 2026
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
github-actions
Bot
added
size/S
amirejaz
added a commit
that referenced
this pull request
Aug 19, 2026
#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>
jhrozek
previously approved these changes
Aug 19, 2026
jhrozek
left a comment
jhrozek
left a comment
Contributor
There was a problem hiding this comment.
suggestion: The migration section says /metrics is currently served on both ports, but this table says the transport-port route returns 404. Could we make the row conditional—e.g.
served during the migration window when metricsOnTransportPort is unset/true; 404 when it is false or after cutover? This is fine to address in the next stack PR, but the final stack
should not leave contradictory exposure guidance.
suggestion: The config-resolution tests cover the tri-state, and the stacked PRs cover CLI/vMCP behavior, but the standard runner has no test proving the resolved value reaches
transportConfig.PrometheusHandler . Could a follow-up add unset/true/false coverage around pkg/runner/runner.go:387-388 ? That would catch a regression where the migration switch
resolves correctly but is not mounted by standard workloads.
jhrozek's review on #6296 flagged two things. The "what lives on the transport port" table said /metrics always 404s there, contradicting the migration-window section a few paragraphs above that says it is served on both ports for now. Make the row conditional on the window rather than stating one state as if it were permanent. Extract the transport-port mount decision out of Run() into mountPrometheusHandlerOnTransportPort, and add unset/true/false coverage for it. The tri-state itself was already tested in pkg/telemetry, and the stacked PRs test CLI parsing and vMCP's mux, but nothing proved the resolved value actually reached transportConfig.PrometheusHandler for a standard workload -- a regression there would leave the switch resolving correctly while silently never mounting the transport-port copy. Part of #6271
github-actions
Bot
added
size/M
Contributor
Author
|
Both addressed — table is now conditional on the migration window, and |
jhrozek
approved these changes
Aug 20, 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>
1 task
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
2 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
#6296 moved
/metricsfrom the transport port to a dedicated diagnostics port. Thatbreaks any deployment scraping the old location. It has not shipped yet —
v0.44.0was cut before it merged — so there is still room to give it a noticewindow rather than land it cold.
This restores the old location alongside the new one, behind a switch that defaults
to on:
metricsOnTransportPortcontrols the transport-port copy and defaults to on, sono existing scrape configuration breaks.
The migration an operator can now follow: point the scraper at the diagnostics port,
confirm metrics arrive, then set
metricsOnTransportPort: falseto prove nothing elsewas still scraping the old one. Closing the window later is a one-line change to
DefaultMetricsOnTransportPort.A side benefit of dual-serving: the diagnostics listener gets real production exercise
during the window, so bugs in it surface before everyone is moved onto it rather than
after.
Part of #6271
Type of change
Test plan
task test)task test-e2e)task lint-fix)Pre-existing failures on
mainin untouched files:TestValidateOCIRegistryHost,TestParseGitReference_*, and the lint findings incmd/thv/app/upgrade.goandpkg/vmcp/config/crd_cli_roundtrip_test.go.Special notes for reviewers
The pointer is the load-bearing part, and it is worth a look.
MetricsOnTransportPortis a*boolwith no+kubebuilder:defaultmarker. Thatis deliberate, because a plain bool would make the eventual cutover a no-op:
RunConfig.TelemetryConfigis serialised (pkg/runner/config.go:173,WriteJSONat:349), so a resolved value gets written into every workload'sstored config.
+kubebuilder:defaultmarker is materialised into CRD objects by the API serverat admission.
Either way, a workload created during the window would carry the window's value
permanently, and changing the default later would move nobody — while looking like it
had.
TestMetricsOnTransportPortNotPersistedWhenUnsetasserts the unset value isomitted from the serialised form and still resolves to the current default after a
round trip, so this cannot regress silently.
Deployments that set the field explicitly are deliberately not moved by the flip.
Leaving it unset is what opts you into the new default.
Follow-ups, in order: vMCP needs the same switch (#6368 currently moves it
unconditionally); then the CLI flag and
MCPTelemetryConfigfield so the opt-in isreachable without editing stored config; then the flip; then removal of the field.
Drift table:
metricsOnTransportPortis classified as runtime-only with ajustification saying it is transitional and should be removed rather than promoted
into the CRD.
Generated with Claude Code