Remove deprecated inline telemetry field from CRDs#4819
Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4819 +/- ##
==========================================
- Coverage 69.10% 68.97% -0.13%
==========================================
Files 530 530
Lines 55379 55281 -98
==========================================
- Hits 38267 38128 -139
- Misses 14170 14212 +42
+ Partials 2942 2941 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: kubernetes-expert, code-reviewer, toolhive-expert, site-reliability-engineer
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Stale comments referencing removed types/functions | 10/10 | LOW | Fix |
| 2 | Architecture docs outdated | 8/10 | MEDIUM | Fix |
| 3 | thv export silently drops telemetry config |
8/10 | MEDIUM | Discuss |
| 4 | Example YAML files have stale deprecation comments | 8/10 | LOW | Fix |
| 5 | MCPRemoteProxy pre-existing env var gap | 7/10 | INFO | Note |
Overall
Clean, thorough deprecation removal. The structural changes are correct: field and type removals, deepcopy updates, CEL rule removals, controller fallback branch removals, CRD YAML regeneration, and test cleanup are all internally consistent across MCPServer, MCPRemoteProxy, and VirtualMCPServer. No compilation risks, nil pointer issues, or dead code paths found. Shared types (PrometheusConfig, OpenTelemetryTracingConfig, OpenTelemetryMetricsConfig) are correctly retained for MCPTelemetryConfig.
All five findings are non-blocking. The two MEDIUM findings (architecture doc drift and silent telemetry drop in thv export) are polish items that could be addressed in this PR or in a follow-up. The stale comment fixes are straightforward search-and-replace. The MCPRemoteProxy env var gap is pre-existing and out of scope for this PR.
File-level findings (not on diff lines)
[LOW] F1: Stale comments referencing removed types/functions (Consensus: 10/10)
Multiple comments still reference removed types, deleted functions, or stale phrases:
mcpserver_controller.go:1024,1634— "prefer TelemetryConfigRef over deprecated inline"mcpremoteproxy_runconfig.go:112— "prefer TelemetryConfigRef over deprecated inline Telemetry"mcptelemetryconfig_types.go:36— references deletedOpenTelemetryConfigtypeconverter_test.go:1484,1618— references deletedConvertTelemetryConfigconverter.go:126-127— references CEL mutual exclusion that was removed
Raised by: k8s-reviewer, code-reviewer, toolhive-reviewer, sre-reviewer
[INFO] F5: Pre-existing gap — MCPRemoteProxy missing GenerateOpenTelemetryEnvVarsFromRef (Consensus: 7/10)
Unlike MCPServer and VirtualMCPServer, MCPRemoteProxy never calls GenerateOpenTelemetryEnvVarsFromRef to inject OTEL_RESOURCE_ATTRIBUTES into pods. The removed GenerateOpenTelemetryEnvVars was the only env var injection path. This is pre-existing — not a regression from this PR — but worth noting for a follow-up.
Raised by: sre-reviewer
Documentation
docs/arch/09-operator-architecture.mdlines 161, 163, 662-664 still reference the now-removed inline telemetry field and CEL mutual-exclusivity rules. Per CLAUDE.md: "When making changes that affect architecture, update relevant docs in docs/arch/."examples/operator/mcp-servers/mcpserver_fetch_otel.yaml:25andexamples/operator/virtual-mcps/vmcp_with_telemetry_ref.yaml:79describe the field as "deprecated" rather than "removed."
Generated with Claude Code
The inline `telemetry` field on MCPServerSpec and MCPRemoteProxySpec, along with the `config.telemetry`/`telemetryConfigRef` mutual-exclusivity CEL rule on VirtualMCPServer, was deprecated in favor of TelemetryConfigRef which references a shared MCPTelemetryConfig resource. This removes all backward-compatibility shim code, CEL rules, deprecated utility functions, and the TelemetryConfig/OpenTelemetryConfig CRD types ahead of the v1beta1 API promotion. Existing CRs using the deprecated inline telemetry field will need to migrate to TelemetryConfigRef before upgrading. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add stderr warning in thv export when telemetry config is detected but cannot be exported inline, matching the existing secrets warning - Fix stale comment claiming CEL enforces config.telemetry / telemetryConfigRef mutual exclusivity (CEL rule was removed) - Fix normalizeTelemetry doc comment to mention the config.telemetry fallback path used by standalone CLI deployments - Fix stale call-site comment referencing deprecated inline Telemetry Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7c4b641 to
2c124e5
Compare
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Large PR justification has been provided. Thank you!
The operator-side converter no longer falls back to Config.Telemetry for VirtualMCPServer. Operator-managed vMCPs must use TelemetryConfigRef to configure telemetry. The Config.Telemetry field remains valid for standalone CLI deployments which read it directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Removed deprecated TelemetryConfig, OpenTelemetryConfig, and related inline telemetry type documentation from the generated CRD API reference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The operator no longer reads Config.Telemetry for VirtualMCPServer. Convert the e2e test to create an MCPTelemetryConfig resource and reference it via TelemetryConfigRef, matching the required migration path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The CEL rule rejecting both config.telemetry and telemetryConfigRef was removed in this PR. Remove the integration test that validated it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
e940bbd to
381b03b
Compare
Summary
The inline
telemetryfield on MCPServerSpec and MCPRemoteProxySpec was deprecated in favor oftelemetryConfigRef, which references a shared MCPTelemetryConfig resource. Similarly, theconfig.telemetry/telemetryConfigRefmutual-exclusivity CEL rule on VirtualMCPServer existed only to bridge the migration period. This removes all backward-compatibility shim code, CEL rules, deprecated utility functions, and the TelemetryConfig/OpenTelemetryConfig CRD types ahead of the v1beta1 API promotion.Additionally, the operator-side converter no longer falls back to
config.telemetryfor VirtualMCPServer (D4). Operator-managed vMCPs must usetelemetryConfigRef.Changes
api/v1alpha1/mcpserver_types.goTelemetryfield, CEL rule,TelemetryConfig/OpenTelemetryConfigtypes; keep shared sub-typesapi/v1alpha1/mcpremoteproxy_types.goTelemetryfield, CEL ruleapi/v1alpha1/virtualmcpserver_types.goconfig.telemetry/telemetryConfigRefcontrollers/mcpserver_runconfig.goelsebranch calling deprecatedAddTelemetryConfigOptionscontrollers/mcpserver_controller.gocontrollers/mcpremoteproxy_runconfig.goaddTelemetryOptionsto only handleTelemetryConfigRefcontrollers/mcpremoteproxy_deployment.gopkg/runconfig/telemetry.goAddTelemetryConfigOptionsfunctionpkg/spectoconfig/telemetry.goConvertTelemetryConfigfunctionpkg/controllerutil/tokenexchange.goGenerateOpenTelemetryEnvVarsfunctionpkg/vmcpconfig/converter.gonormalizeTelemetry; operator now returns nil whenTelemetryConfigRefis not setpkg/export/k8s.gocmd/thv/app/export.go*_test.go,test-integration/deploy/charts/operator-crds/Type of change
Test plan
task operator-test)task lint-fix)go build ./...)Does this introduce a user-facing change?
Yes — this is a breaking change for existing CRs. See migration guide below.
Migration guide
MCPServer and MCPRemoteProxy
Before (inline telemetry — no longer supported):
After (shared MCPTelemetryConfig resource):
VirtualMCPServer
config.telemetryis no longer read by the operator. Usespec.telemetryConfigRefinstead (same MCPTelemetryConfig pattern as above).thv exportthv export --format k8snow prints a stderr warning when telemetry config is detected but cannot be exported inline. Users must create an MCPTelemetryConfig resource manually.Large PR Justification
Special notes for reviewers
PrometheusConfig,OpenTelemetryTracingConfig, andOpenTelemetryMetricsConfigare still used byMCPTelemetryConfigand were intentionally retained.Config.Telemetryin shared struct: TheTelemetryfield inpkg/vmcp/config/config.gois still valid for standalone CLI deployments — only the operator-side fallback was removed.Closes #4828
Generated with Claude Code