fix(operator): apply deprecated VirtualMCPServer inline telemetry - #6277
Open
RaviTharuma wants to merge 2 commits into
Open
fix(operator): apply deprecated VirtualMCPServer inline telemetry#6277RaviTharuma wants to merge 2 commits into
RaviTharuma wants to merge 2 commits into
Conversation
Operator converter previously ignored spec.config.telemetry entirely after stacklok#4819, while docs still claimed the inline field worked. Setting only enablePrometheusMetricsPath then produced a silent no-op: the rendered vmcp config had no telemetry block, /metrics was never registered, and Prometheus scrapes received MCP streamable-HTTP 406 JSON-RPC. Restore a deprecated fallback that applies inline config.telemetry when TelemetryConfigRef is unset, logs a migration warning, and keeps TelemetryConfigRef preferred when both are set. Update unit tests and operator observability docs. Signed-off-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
RaviTharuma
requested review from
ChrisJBurns,
JAORMX,
blkt,
jerm-dro,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
August 10, 2026 21:37
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Collaborator
|
Awaiting reply to #6276 (comment) |
Contributor
There was a problem hiding this comment.
Pull request overview
Restores operator support for the deprecated VirtualMCPServer.spec.config.telemetry by applying it as a fallback when spec.telemetryConfigRef is unset, so /metrics can be registered again for Prometheus scrapes while still preferring the shared MCPTelemetryConfig path.
Changes:
- Reintroduce inline telemetry normalization fallback (with a migration log message) when
telemetryConfigRefis absent. - Add/adjust unit tests to cover inline-telemetry application and ref-precedence behavior.
- Update observability docs to remove the incorrect “CEL mutual exclusion” claim and document the
/metrics→ MCP 406 failure mode when telemetry is not configured.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/operator/virtualmcpserver-observability.md | Updates vMCP telemetry docs to describe inline fallback + 406 scrape symptom when telemetry is unset. |
| docs/observability.md | Aligns general observability docs with vMCP telemetry fallback behavior and failure mode. |
| cmd/thv-operator/pkg/vmcpconfig/converter.go | Restores deprecated inline telemetry fallback and emits a migration log message. |
| cmd/thv-operator/pkg/vmcpconfig/converter_test.go | Updates/adds tests for inline telemetry application and TelemetryConfigRef precedence. |
| cmd/thv-operator/pkg/spectoconfig/telemetry.go | Clarifies function usage in comments now that inline fallback is restored. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+511
to
+513
| "name", vmcp.Name, | ||
| "namespace", vmcp.Namespace, | ||
| ) |
Comment on lines
+507
to
+510
| ctxLogger.Info( | ||
| "VirtualMCPServer.spec.config.telemetry is deprecated for operator deployments; "+ | ||
| "migrate to spec.telemetryConfigRef referencing an MCPTelemetryConfig. "+ | ||
| "Inline telemetry is still applied for compatibility.", |
… Warning event - Remove per-reconcile Info log from converter.normalizeTelemetry - Add emitInlineTelemetryDeprecatedEvent controller method following emitPrimaryUpstreamProviderDeprecatedEvent pattern - Emit Warning event once per spec change when inline telemetry is used - Event guidance: migrate spec.config.telemetry to spec.telemetryConfigRef Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.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
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
Fixes #6276: operator-managed VirtualMCPServer silently ignored
spec.config.telemetryafter #4819, soenablePrometheusMetricsPath: truenever registered/metrics. Prometheusscrapes then hit the MCP streamable-HTTP handler and got HTTP 406 JSON-RPC
(
Client must accept text/event-stream) instead of Prometheus text.This also unblocked collecting Go runtime series for #5860.
Changes
config.telemetryfallback whentelemetryConfigRefis unset (TelemetryConfigRef still wins when both set)MCPTelemetryConfigwhen telemetry is not configured
Preferred long-term path (unchanged)
with
spec.telemetryConfigRefon VirtualMCPServer.Test plan
go test ./cmd/thv-operator/pkg/vmcpconfig/ -run 'TestConverter_InlineTelemetry|TestConverter_TelemetryConfigRefWins|TestConverter_TelemetryNil'enablePrometheusMetricsPath: trueshould render
telemetryinto the vmcp ConfigMap and serve HTTP 200 onGET /metricswithAccept: text/plainSecurity
No secrets, private hostnames, or customer data in this PR or linked issues.