[HYPERSHELL-79] feat(control-plane): add OpenTelemetry tracing and metrics

[JuanmaBM]

Summary

• Add OpenTelemetry instrumentation to the control plane, mirroring the API server's pattern (HYPERSHELL-26 / PR [HYPERSHELL-26] feat(api-server): OpenTelemetry HTTP, gRPC, and database tracing #158)
• OTel SDK bootstrap with opt-in via OTEL_EXPORTER_OTLP_ENDPOINT, parent-based sampler, graceful shutdown flush
• Reconcile spans per Handle() call named by kind and operation (reconcile Gateway, delete Gateway, etc.)
• gRPC client interceptors via otelgrpc for all outbound API server calls (phase updates, health checks)
• Watch stream lifecycle spans with reconnect counter metrics
• Kubernetes API client transport instrumented via otelhttp for all client-go calls
• Metrics: reconcile.duration histogram, reconcile.errors counter, watch.reconnects counter
• KIND_JAEGER=true patches the controller deployment with the OTLP endpoint (alongside API server and BFF)
• Desired-state spec: specs/platform/control-plane-observability.spec.md

Files Changed

Area	Files
OTel package	internal/otel/otel.go, metrics.go, reconcile.go, grpc.go, k8s.go
Main wiring	cmd/hypershell-controller/main.go
Reconcilers	reconciler.go, health.go, namespace.go, sandboxcount.go, role_binding_reconciler.go
Watcher	watcher.go (watch stream lifecycle spans + reconnect metrics)
Kind scripts	scripts/kind/up.sh (controller OTel env var patching)
Spec	specs/platform/control-plane-observability.spec.md, specs/index.spec.md

Out of Scope

• Correlating reconcile traces to the originating browser/API request trace (separate story -- requires span links + trace-context persistence)

Test plan

• go build ./... and go vet ./... pass
• go test ./... passes (all existing tests unaffected)
• KIND_JAEGER=true make kind-up deploys Jaeger and patches the controller with OTEL env vars
• Reconcile spans appear in Jaeger under hypershell-controller service
• Watch stream lifecycle spans appear on connect/disconnect
• gRPC client spans appear as children of reconcile spans
• Kubernetes API client spans appear as children of reconcile spans
• Without OTEL_EXPORTER_OTLP_ENDPOINT, no OTel overhead (SDK not initialized)

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d3f97c8e-ec5a-4115-ac2e-b599ba666a06

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jsell-rh]

This adds the right OpenTelemetry building blocks and matches the established API/BFF transport split, W3C propagation, service naming, parent-based sampling, and Kind wiring. It is not safe to merge yet: graceful watch EOF can panic the controller, and the current trace boundaries, privacy/cardinality handling, disabled-state behavior, error reporting, validation, and coverage diverge from the platform contracts.

Critical

1. components/control-plane/internal/watcher/watcher.go:639 dereferences a nil error after graceful EOF. Branch on err == nil, mark that lifecycle span OK, and cover EOF/error/cancellation in tests. Confidence: High (100%).

Major

1. components/control-plane/internal/otel/k8s.go:22 puts concrete Kubernetes paths—including namespace, object, and Secret names—into span names. Canonicalize them to bounded path templates, matching the API/BFF route-template pattern. Confidence: High (99%).
2. components/control-plane/internal/otel/reconcile.go:28-29 exports raw production errors as status text and exception.message. Emit bounded sanitized error classes so Secret references, usernames, and upstream bodies cannot leave the controller through telemetry. Confidence: High (98%).
3. components/control-plane/internal/otel/reconcile.go:20 inherits the long-lived watch span for inline handlers, making all reconciles on one connection share one trace and one sampling decision. Start each asynchronous reconcile as a new root and keep gRPC/Kubernetes work beneath it. Confidence: High (96%).
4. components/control-plane/internal/reconciler/namespace.go:110, sandboxcount.go:265, and health.go:169 do not propagate all tick failures to the span/metric result. Return or collect partial errors so failed work is not reported as OK. Confidence: High (100%).
5. components/control-plane/cmd/hypershell-controller/main.go:55 and the instrumentation helpers key off environment presence rather than successful SDK initialization; manual spans also run when tracing is absent. Return and use an explicit enabled runtime, as the API bootstrap and BFF disabled port do. Confidence: High (97%).
6. The new 286-line internal/otel package and watch lifecycle behavior have no focused tests. Add enabled/disabled/failure, redaction/cardinality, trace-boundary, metric-result, and EOF/error/cancel coverage. Confidence: High (100%).

Minor

1. components/control-plane/internal/otel/otel.go:118 accepts non-finite and out-of-range sample ratios. Validate [0,1], consistent with the BFF configuration contract. Confidence: High (99%).
2. Commit 44ae4ca and the PR title begin with [HYPERSHELL-79], so the squash subject does not match type(scope): description. Rename it to something like feat(control-plane): add OpenTelemetry tracing and metrics (HYPERSHELL-79). Confidence: High (100%).

Verification

All GitHub repository-policy, control-plane lint, E2E Kind, and E2E gate checks are green. I also ran go test ./..., go vet ./..., bash -n scripts/kind/up.sh, and git diff --check; all passed, but none exercises the semantic failure paths above.

Overall assessment: REQUEST_CHANGES

— Amber

Findings Summary (ordered by severity, highest first):

1. [Critical] Graceful watch EOF dereferences nil and panics the controller - Runtime Safety / CP-OBS-04 (L639)
2. [Major] Kubernetes span names expose raw identifiers and Secret references - Security / Cardinality / CP-OBS-05/06 (L22)
3. [Major] Raw reconcile errors are exported without sanitization - Security / Telemetry Privacy / CP-OBS-06 (L28-L29)
4. [Major] Inline reconciles inherit one long-lived watch trace and sampling decision - Trace Architecture / Cross-Component Consistency (L20)
5. [Major] Periodic reconciler failures are reported as successful spans and metrics - Observability Correctness / CP-OBS-02/07 (L110, L265, L169)
6. [Major] Instrumentation is not gated on successful SDK initialization - Disabled-State Behavior / CP-OBS-01 (L55)
7. [Major] New OTel and watch behavior lacks focused regression coverage - Testing (L36)
8. [Minor] Sampling ratio accepts invalid and non-finite configuration - Input Validation / Cross-Component Consistency (L118)
9. [Minor] Squash subject is not a conventional commit - Commit Discipline (44ae4ca)

Convention Checklist (omit conventions not applicable to the diff):

Convention	Result
No panic() / production panic paths	Fail
Errors wrapped with fmt.Errorf context	Pass
No secrets or Secret references in telemetry	Fail
Telemetry configuration validated	Fail
Proper Go context propagation (no context.TODO())	Pass
Reconcile outcome reflected in status and metrics	Fail
Bounded telemetry names and attributes	Fail
Telemetry disabled when unconfigured or initialization fails	Fail
New behavior covered by focused tests	Fail
Conventional commit messages	Fail
go test ./... and go vet ./...	Pass

[jsell-rh]

[Critical] Handle graceful EOF before dereferencing err. Every watch callback intentionally returns nil on io.EOF (for Gateway this comes through streamErr), so this line calls err.Error() on nil and panics the controller instead of marking the lifecycle span OK and reconnecting as CP-OBS-04 requires.

Fix: Branch on err == nil: set codes.Ok without RecordError, and use the error path only for a non-nil disconnect. Add a watchLoop regression test covering graceful EOF, a non-nil disconnect, and parent cancellation.

Confidence: High (100%).

[jsell-rh]

[Major] Template Kubernetes paths before using them as span names. r.URL.Path contains concrete namespace and object names; requests such as /api/v1/namespaces/<ns>/secrets/<name> also export a Secret reference. That violates CP-OBS-05/06 and differs from the API/BFF implementation, which deliberately collapses identifiers before naming spans.

Fix: Introduce a Kubernetes-path canonicalizer that replaces namespace and named-resource segments with bounded placeholders, then name the span from the method plus that template. Add table tests for core and grouped APIs, including Secret paths, list/watch paths, and query strings.

Confidence: High (99%).

[jsell-rh]

[Major] Do not export raw reconcile error strings. The status description and RecordError both export err.Error() (the latter as exception.message). Reconcile errors already contain namespace and Secret references, and RoleBinding/Keycloak paths can contain usernames or upstream response bodies; CP-OBS-06 explicitly forbids sensitive data and Secret references in telemetry. The BFF avoids this by emitting bounded outcome/error classes rather than raw messages.

Fix: Map failures to a bounded, sanitized error type and use a generic status description; do not call RecordError with an unsanitized production error. Add seeded-secret and identifier redaction tests.

Confidence: High (98%).

[jsell-rh]

[Major] Start each asynchronous reconcile as a new trace root. watchLoop passes its streamCtx to inline handlers, so this default parent behavior makes every Fleet, RoleBinding, and other inline reconcile on one connection a child of the long-lived watch <kind> span. The parent-based sampler therefore samples all or none of those reconciles per connection, and one trace grows for the entire stream lifetime; that is inconsistent with the bounded workflow/request traces used by the browser, BFF, and API.

Fix: Start reconcile spans with trace.WithNewRoot() (future request correlation can use span links, as the spec notes), while continuing to pass the returned context to gRPC/Kubernetes calls so those remain children of the reconcile. Add a test asserting the watch span and consecutive reconciles have distinct trace IDs.

Confidence: High (96%).

[jsell-rh]

[Major] This periodic span can never report a failure because tickErr is never assigned. Failures from liveNamespaces, the namespace LIST, and every per-namespace reconcile are logged and then exported as OK, so reconcile.errors also stays unchanged. The same dead error accumulator appears in sandboxcount.go:265; the health tick records only the initial list failure and misses per-gateway failures.

Fix: Return or collect errors from each tick and its item helpers, assign/join them into the deferred result, and retain best-effort processing where appropriate. Add span/metric tests for top-level and partial item failures.

Confidence: High (100%).

[jsell-rh]

[Major] Gate instrumentation on successful SDK initialization, not merely endpoint presence. If Init reports an error, main says it is continuing without telemetry, but GRPCDialOptions and InstrumentK8sConfig still install instrumentation because the endpoint remains set. Conversely, when the endpoint is absent, every manual reconcile/watch path still constructs no-op spans, attributes, timers, and closures. This diverges from API startup and the BFF disabledTracing port, and violates CP-OBS-01 zero-instrumentation/zero-overhead behavior.

Fix: Have Init return an explicit enabled runtime/handle only after setup succeeds and use that state for gRPC, Kubernetes, watch, and reconcile instrumentation. Add absent-endpoint and failed-initialization tests.

Confidence: High (97%).

[jsell-rh]

[Major] Add focused tests for the new telemetry behavior before merging. This PR adds the 286-line internal/otel package and changes the watch lifecycle without adding a test file; the existing suite passes while missing the graceful-EOF panic, false-success spans, disabled-state overhead, unsafe path naming, and trace-boundary behavior above. The web/BFF OTel work has adapter and wiring tests for these same contract edges.

Fix: At minimum cover enabled/disabled/failed init, sampler validation, sanitized errors, bounded Kubernetes names, reconcile root/child relationships, metric success/error recording, and watchLoop EOF/error/cancel behavior.

Confidence: High (100%).

[jsell-rh]

[Minor] Validate that the sampling ratio is finite and within [0,1]. strconv.ParseFloat accepts NaN, infinities, negative values, and values above one; the Go sampler then silently clamps or behaves unexpectedly. CP-OBS-01 defines a 0.0-to-1.0 contract, and the BFF rejects out-of-range configuration at startup.

Fix: Reject math.IsNaN, math.IsInf, parsed < 0, and parsed > 1, then use a documented fallback or return a configuration error. Add boundary and non-finite tests.

Confidence: High (99%).

[jsell-rh]

Since the previous Amber review at 9e891228, 16cd8c1f fixes the graceful-EOF panic, gives reconciles independent trace roots, validates the sampler, sanitizes reconcile status, and adds initial unit coverage. The delta is still not safe to merge: Kubernetes telemetry continues to export concrete identifiers, the new enabled state does not gate manual spans, partial sandbox self-heal failures still report success, and the critical watch lifecycle branch remains untested.

Major

1. components/control-plane/internal/otel/k8s.go:20-47 changes the span name but still emits raw url.full, and its resource allowlist misses CRDs used by the controller. Replace the default raw-URL attributes with a generic safe Kubernetes path template and test the complete exported span for seeded identifiers. Confidence: High (100%).
2. components/control-plane/internal/otel/otel.go:34-60 gates only gRPC and Kubernetes wrappers; reconcile and watch spans still run when telemetry is absent or initialization fails. Gate every instrumentation path on successful initialization and test disabled/failed startup. Confidence: High (100%).
3. components/control-plane/internal/reconciler/sandboxcount.go:272-278 records a failed namespace list but still discards per-namespace cache/RPC failures, reporting partial ticks as successful. Return and aggregate item errors while preserving best-effort processing. Confidence: High (100%).
4. components/control-plane/internal/watcher/watcher.go:639-644 fixes the nil-EOF panic without a regression test for EOF, disconnect, or cancellation. Add direct lifecycle tests before merging. Confidence: High (100%).

Verification

The active repository-policy, control-plane lint, image-build, and E2E checks are green. In a detached worktree at 16cd8c1f, I also ran go test ./..., go vet ./..., go test -race ./internal/otel ./internal/reconciler ./internal/watcher, and git diff --check; all passed.

Overall assessment: REQUEST_CHANGES

— Amber

Findings Summary (ordered by severity, highest first):

1. [Major] Kubernetes spans still export raw and unbounded resource identifiers - Security / Cardinality / CP-OBS-05/06 (L20-L47)
2. [Major] Enabled state does not disable manual reconcile and watch instrumentation - Disabled-State Behavior / CP-OBS-01 (L34-L60)
3. [Major] Sandbox self-heal item failures still report a successful tick - Observability Correctness / CP-OBS-02/07 (L272-L278)
4. [Major] Graceful watch EOF fix lacks lifecycle regression coverage - Testing / CP-OBS-04 (L639-L644)

Convention Checklist (omit conventions not applicable to the diff):

Convention	Result
No panic() / production panic paths	Pass
No secrets or Secret references in telemetry	Fail
Telemetry configuration validated	Pass
Proper Go context propagation	Pass
Reconcile outcome reflected in status and metrics	Fail
Bounded telemetry names and attributes	Fail
Telemetry disabled when unconfigured or initialization fails	Fail
New behavior covered by focused tests	Fail
Post-review commit uses conventional format	Pass

[jsell-rh]

[Major] The canonicalized name does not keep identifiers out of the span. WithSpanNameFormatter changes only the span name; otelhttp v0.70.0 independently adds url.full from r.URL.String(), so a request such as /api/v1/namespaces/openshell-abc/secrets/db-password still exports both the namespace and Secret reference as an attribute (including query selectors). The resource allowlist also misses paths this controller actually calls—httproutes, routes, clusters, certificates, issuers, grpcroutes, and backendtlspolicies—so those concrete names remain in the span name too.

Fix: Parse core and grouped Kubernetes API path structure generically, and use instrumentation that records only that safe template rather than otelhttp's raw url.full. Add an in-memory-exporter test that searches the whole emitted span (name, attributes, and events) for seeded namespace, Secret, custom-resource, and query values.

Confidence: High (100%).

[jsell-rh]

[Major] The new enabled state still does not gate manual instrumentation. Its only reads are in GRPCDialOptions and InstrumentK8sConfig; Enabled() has no callers. StartReconcileSpan still allocates a span, timer, and closure and records metrics on every reconcile, while watchLoop still creates lifecycle spans when the endpoint is absent or Init fails, so the CP-OBS-01 zero-instrumentation/zero-overhead contract and the previous review finding remain unmet. A failure after the trace provider is installed can also leave those calls targeting a shut-down global provider.

Fix: Gate the reconcile and watch helpers on successful initialization as well (return the original context plus a no-op closure when disabled), and roll global state back on partial setup failure. Cover absent-endpoint and forced-init-failure cases with recording-provider/spy tests.

Confidence: High (100%).

[jsell-rh]

[Major] This records only the top-level namespace-list failure. Once that list succeeds, healNamespace logs and discards both cache-read and per-namespace set RPC errors, so tickErr remains nil and the tick still exports an OK span without incrementing reconcile.errors. This leaves the partial-failure part of the previous observability finding unresolved.

Fix: Return an error from healNamespace, collect/join item errors while continuing the rest of the pass, and finish the tick with that aggregate. Add tests for both lister and set failures that assert the span/metric outcome.

Confidence: High (100%).

[jsell-rh]

[Major] The critical EOF fix still has no regression test. No test invokes watchLoop; the existing watcher tests exercise seed helpers only, so graceful EOF, a non-nil disconnect, and parent cancellation do not verify span status or reconnection behavior. This exact lifecycle branch previously dereferenced nil and panicked the controller.

Fix: Add deterministic watchLoop tests for EOF, error, and cancellation (make the backoff injectable or cancel after the first attempt), and assert both no panic and the emitted lifecycle-span result.

Confidence: High (100%).