feat(observability): measure gateway provision duration - #243
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a clean, well-scoped observability change: the new gateway.provision.duration histogram, its one-observation semantics, the spec, and the tests are consistent and correct as written against the current code. I have no blocking or critical findings on the PR itself; the notes below are hardening suggestions plus cross-PR coordination the maintainers should sequence.
Amber Analysis
The histogram is recorded on both the direct (Handle) and delayed (reconcileGatewayHealth) paths, timestamps come from the API server's created_at/updated_at (avoiding control-plane clock skew), and invalid/reversed/missing timestamps are safely ignored so telemetry cannot alter reconciliation behavior. The single-observation guarantee is real today, but it is split across two mechanisms with asymmetric guards, which is the main thing to watch.
Minor
-
Initial-reconcile record path has no explicit "first Running" guard. In
reconciler.go(~L1533-L1545) the metric is recorded wheneverupdateGatewayHealth(...,"Running",...)succeeds, with no equivalent of the health path'sisGatewayProvisionCompletioncheck. Its correctness is entirely load-bearing on the top-level phase gate (~L1360) returning early forRunning/Provisioning/Degraded. That holds now, but the coupling is implicit and fragile — if the gate ever admits an already-Runninggateway back into this block, the histogram will record a second, inflatedcreated_at→updated_atobservation. Consider recording only when the prior phase was notRunning(symmetry with the health path), which also self-documents the invariant. Confidence: High. -
New phase string literals reintroduce magic values.
metrics.go(isGatewayProvisionCompletion, L48) and the"Running"/"Healthy"literals added inreconciler.go/health.goduplicate phase strings that already appear across the control plane. A shared constant would prevent drift. Confidence: Medium.
Cross-PR coordination
Two open pull requests touch the assumptions this change depends on and need a maintainer decision or a merge ordering:
-
#151 (gate gateway re-provisioning on desired-state convergence) — This PR's single-observation guarantee for the direct path depends on the current phase gate in
GatewayReconciler.Handleblocking any already-Runninggateway from re-entering the full provision block. #151 re-keys that gate on convergence (observed_generation == generation) so that a desired-spec change to aRunninggateway falls through and re-applies "regardless of phase." Under #151, a spec change would drive aRunninggateway back toRunningthrough the unguarded record path here and emit a second, inflatedgateway.provision.durationsample, violating the spec's "first successful transition to Running" rule. Decide the ordering: if #151 lands, this PR's direct-path record must gain an explicit first-Running guard. -
#239 (standardize gateway health and readiness vocabulary) — #239 establishes a single-source-of-truth
gatewayhealthphase vocabulary and explicitly removes the magic phase literals from the samereconciler.go/health.gofunctions this PR edits, while this PR adds new literals (isGatewayProvisionCompletion,"Running"/"Healthy"). Whichever merges second must adopt the shared constants; the two should be reconciled so the standardization effort isn't immediately re-diluted.
Findings Summary (ordered by severity, highest first):
- [Minor] Direct-path metric record relies implicitly on the phase gate with no explicit first-Running guard - Robustness (reconciler.go L1533-L1545)
- [Minor] New phase string literals duplicate existing magic values instead of a shared constant - Consistency (metrics.go L48)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
Proper context propagation (no context.TODO()) |
Pass |
| Telemetry cannot alter reconciliation behavior | Pass |
| Test Diff Scrutiny (no flipped pre-existing assertions) | Pass |
| Spec/docs updated with the change | Pass |
| Conventional commit message | Pass |
| r.updateGatewayHealth(ctx, event.ResourceID, "Running", "Healthy") | ||
| // The phase gate prevents a second full provision after Running. | ||
| if runningGateway := r.updateGatewayHealth(ctx, event.ResourceID, "Running", "Healthy"); runningGateway != nil { | ||
| observeGatewayProvisionDuration(ctx, runningGateway) |
There was a problem hiding this comment.
The direct-path record fires whenever the update to Running succeeds, with no equivalent of the health path's isGatewayProvisionCompletion first-Running check. Its single-observation correctness is load-bearing on the top-level phase gate (~L1360) returning early for already-Running/Provisioning/Degraded gateways. That is correct today, but the coupling is implicit: if that gate is ever changed to admit an already-Running gateway back into this block, this would emit a second, inflated created_at->updated_at sample. Consider recording only when the prior phase was not Running, mirroring the health path and self-documenting the invariant.
There was a problem hiding this comment.
Addressed in b97a99d. The handler now uses the stored phase before it enters the phase gate or writes Provisioning. A retry also carries its phase before the retry adapter clears it. Work that started in Running or Degraded suppresses the observation. Thus, a future convergence gate cannot emit a second sample after a restart. A concurrent claim also coordinates the direct and health paths.
| // gateway can stay in Provisioning after the first reconcile while its route | ||
| // becomes ready. A Running gateway that fails moves through Degraded instead. | ||
| func isGatewayProvisionCompletion(currentPhase, desiredPhase string) bool { | ||
| return currentPhase == "Provisioning" && desiredPhase == "Running" |
There was a problem hiding this comment.
These phase strings ("Provisioning", "Running"), along with the "Running"/"Healthy" literals added in reconciler.go/health.go, duplicate phase values that already appear as magic literals across the control plane. A shared phase-vocabulary constant would prevent drift between the completion check and the values actually written by updateGatewayHealth.
There was a problem hiding this comment.
Addressed in b97a99d. Shared Gateway phase and healthy-status constants now live in gateway_vocabulary.go. The reconciler, health loop, and metric predicates use these constants. When PR 239 and this branch are combined, its cross-component gatewayhealth package can replace these local constants.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This change adds a well-scoped gateway.provision.duration OTLP histogram with careful coordination between the event-driven and health reconcile paths, sound timestamp validation, and thorough table-driven tests. The control-plane code is correct and merge-ready on its own; the one item that needs human attention is a cross-PR design decision about how the gateway phase vocabulary is standardized (see Cross-PR coordination).
Amber Assessment
Confidence: High (90%) on the correctness review; Medium on the cross-PR interaction pending a maintainer decision.
Strengths worth calling out:
- The one-observation-per-Gateway rule is enforced with a single process-wide
sync.Mapclaim, and both promotion paths funnel through it, so the event-driven and health paths cannot double-count the firstRunningtransition. gatewayProvisionDurationcorrectly rejects missing/invalid/reversed timestamps (CheckValid, negative duration), so telemetry can never alter reconciliation behavior.PhaseBeforeRetryis threaded through the watcher so a forced-recovery retry (which clears the phase to bypass the gate) still lets the reconciler distinguish aDegraded/Runningrecovery from a genuine first provision. Theproto.Clonecopy avoids mutating the shared latest entry.- The metric carries no Gateway identifier attribute, and the test explicitly asserts
point.Attributes.Len() == 0plus the exact bucket bounds and unit. Spec, README, and RECONCILE.md are updated consistently.
Minor observations:
observedGatewayProvisionsentries are only removed onEventDeleted. For every Gateway that reaches (or is seeded in)Running/Degraded, a claim entry persists for the life of the Gateway. This is bounded by the live Gateway count and acceptable, but if a delete event is ever missed the entry lingers. A periodic reconcile against the known Gateway set (or a TTL) would make cleanup self-healing. Not blocking.RecordGatewayProvisionDurationguardsduration < 0even thoughgatewayProvisionDurationalready filters negatives upstream. Harmless defensive redundancy.
No panic(), error paths log and return without swallowing failures, no secrets in logs, and the histogram is registered with an explicit error return. SecurityContext / RBAC / OpenAPI surfaces are untouched.
Cross-PR coordination
Another open pull request, #239, standardizes the exact same Gateway phase/status vocabulary that this PR touches, but with a different design and ownership boundary. This PR introduces a control-plane-local gateway_vocabulary.go (gatewayPhase*, gatewayStatusHealthy) and rewrites the magic-string phase literals in internal/reconciler/health.go, internal/reconciler/reconciler.go, and internal/watcher/watcher.go to use it. #239 replaces those same literals in those same files, but sources the constants from a new shared cross-component package (components/api-server/pkg/gatewayhealth) imported by both the API server and the control plane, and additionally adds API-server-side phase validation. These are two competing sources of truth for the same vocabulary.
Maintainers should decide which vocabulary source the control plane adopts, and merge in a defined order: if #239 lands first, this PR should drop gateway_vocabulary.go and consume gatewayhealth constants (the shared single source of truth); if this PR lands first, #239's owner should reconcile the control-plane edits against these local constants. Merging both as-is yields duplicated constant definitions and conflicting edits to the same phase literals. This is a design/ownership decision, not a mechanical merge conflict.
Findings Summary (ordered by severity, highest first)
- [Minor]
observedGatewayProvisionsclaim entries are removed only on delete events; a missed delete leaks one entry. Consider a periodic reconcile against the known Gateway set or a TTL - Observability / Resource lifecycle (metrics.go L16, L41-42) - [Minor] Redundant
duration < 0guard inRecordGatewayProvisionDuration(already filtered upstream) - Style (otel/metrics.go)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped / not swallowed on error paths | Pass |
| No secrets in logs or responses | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Status updated on error paths | Pass |
Context propagated (no context.TODO()) |
Pass |
| Test Diff Scrutiny (existing assertions) | Pass (only additive tests; existing seed_test.go change adds a new test) |
| Spec / docs consistent with code | Pass |
| Conventional commit messages | Pass |
1 participant
Summary
gateway.provision.durationOTLP histogram.created_atvalue to the successfulRunningupdate time.ProvisioningtoRunningcompletion.DegradedtoRunningrecovery events and Gateway identifiers.Validation
go test ./...incomponents/control-planemake lint-control-planemake check