feat(metrics): config-apply readiness gauge — the half of #216 that never shipped - #306
Merged
Merged
Conversation
thc1006
force-pushed
the
feat/config-apply-ready-metric
branch
from
July 31, 2026 01:00
7135876 to
9c04353
Compare
…ever shipped Issue #216 scoped readiness gauges for BOTH runtime ephemeris push and config apply, and said so explicitly: "Apply the same readiness pattern to ConfigApplyErrorsTotal (ntn_operators_config_apply_ready) so runtime push and config apply are consistent, rather than leaving an asymmetry (push has readiness, apply does not)" — under a heading that reads "Do this holistically, not as a one-metric bolt-on". PR #218 shipped ephemeris_push_ready and gp_fetch_ready and closed the issue; config_apply_ready was never written. The gap is worse than a missing symmetry. ConfigApplyErrorsTotal is incremented ONLY by an ApplyCellConfig failure, so three other ways the apply can be broken increment it ZERO times and return a nil error — invisible to NTNConfigApplyErrors (a rate alert on that counter) and to controller_runtime_reconcile_errors_total alike: InternalError provider registry not configured requeues 1m UnsupportedProvider spec.provider.type unregistered NO requeue StatusCheckFailed post-apply verification could not confirm requeues 1m UnsupportedProvider is the permanent, non-requeuing case that motivated the gauge for the push path in the first place — and here the counter does not even fire the single sample the push path got. - ntn_operators_config_apply_ready{namespace,config}: 1 only on a verified apply, 0 on all four failure paths, released on CR deletion. - Keyed namespace+config like ephemeris_push_ready, deliberately WITHOUT the counter's provider label, so a provider-type edit cannot strand a stale series at 0 under the old value. This is a deliberate deviation from the proposal. - Alert NTNConfigApplyNotReady (== 0 for 15m) + a runbook section that triages by ConfigApplied reason, since the three causes have different fixes. Scope kept to one logical change: no refactor of the surrounding label maps, which would collide with the three PRs currently open on this file. Mutation-verified per set-point: dropping the three Set(0) calls, the Set(1), and the DeletePartialMatch each redden a distinct assertion. The tests read gauges without instantiating them (testutil.ToFloat64 goes through With(), which creates a missing series at 0 and would make "want 0" pass even with the code removed).
thc1006
force-pushed
the
feat/config-apply-ready-metric
branch
from
July 31, 2026 01:26
9c04353 to
ba531e2
Compare
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.
Completes #216. Every claim below was checked against the code before being accepted.
The tracking gap is real
#216's body scopes both gauges, in as many words:
…under a heading that reads "Do this holistically, not as a one-metric bolt-on."
PR #218 — titled "readiness gauges for ephemeris push & GP fetch (#216)" — shipped
ephemeris_push_readyandgp_fetch_readyand closed the issue.config_apply_readywas never written. The closure was premature; I've left a correction on #216.The gap is worse than a missing symmetry
ConfigApplyErrorsTotalis incremented in exactly one place — theApplyCellConfigfailure branch. Three other ways the apply can be broken increment it zero times and return a nil error, so neitherNTNConfigApplyErrors(a rate alert on that counter) norcontroller_runtime_reconcile_errors_totalcan see them:ConfigAppliedreasonInternalErrorUnsupportedProviderspec.provider.typeunregisteredStatusCheckFailedApplyFailed/OwnershipConflictApplyCellConfigfailedUnsupportedProvideris the permanent, non-requeuing case that motivated the gauge for the push path in the first place — and here the counter does not even emit the single sample the push path got. Config apply was therefore strictly worse off than ephemeris push was before #216.The review's own counter-consideration is fair and I checked it:
ProviderRef.Typecarries+kubebuilder:validation:Enum=ocudu, so a newly created CR cannot reachUnsupportedProvider. That constrains one row. It does not touchInternalErrororStatusCheckFailed, and it does not apply to objects stored before the enum or if the enum widens.Change
ntn_operators_config_apply_ready{namespace,config}— 1 only on a verified apply, 0 on all four failure paths,DeletePartialMatchon CR deletion.NTNConfigApplyNotReady(== 0for 15m), companion to the existing rate alert.ConfigAppliedreason, because the three causes have genuinely different fixes (operator wiring / spec edit / ConfigMap access) — including thejsonpathto read the reason.Deliberate deviation from the proposal
The proposal was
{namespace, config, provider}, matching the counter's labels. I dropped theproviderlabel. Both existing readiness gauges are 2-label (ephemeris_push_ready{namespace,config},gp_fetch_ready{namespace,ephemeris}), and a label that can change identity needs stale-series management a 2-label gauge simply does not have: aspec.provider.typeedit would otherwise strand a series at 0 under the old value and alert forever. That also makes the proposal's "clean up stale series when the provider path is disabled" requirement unnecessary rather than implemented-and-fragile. Joining to the counter on{namespace,config}still works.SmallCL / Boy Scout notes
One logical change, ~200 lines including tests and docs. I deliberately did not factor out the repeated
prometheus.Labels{"namespace": …, "config": …}map even though it now appears nine times in this file: #297, #301 and #304 are all open againstntncellconfig_controller.go, and one of them touches the adjacentEphemerisPushReadymetric block. Tidying there would buy a cosmetic win and pay for it in merge conflicts. Left as-is, matching the surrounding idiom.Verification
go build,go vet,golangci-lint v2.12.2: 0 issues;gofmtclean.go test ./internal/controller/... ./pkg/...green (envtest 1.36.2).helm templaterenders the new rule.testutil.ToFloat64(g.With(labels))creates a missing series at 0, so a naive "want 0" assertion passes even with the production code deleted — the first version of these tests had exactly that hole.gaugeValue/counterValuecollect and match labels, returning afoundbool, which is what makes the assertions load-bearing.Set(0)calls →no config_apply_ready series for InternalError: an absent series can never fire == 0Set(1)→config_apply_ready = 0 (found=true), want 1 once the apply verifiesDeletePartialMatch→the finalizer left 1 config_apply_ready series behind