observability: delete SLO rules that query a metric nothing emits; make the canary gate fail closed - #1117
Open
mdheller wants to merge 2 commits into
Open
observability: delete SLO rules that query a metric nothing emits; make the canary gate fail closed#1117mdheller wants to merge 2 commits into
mdheller wants to merge 2 commits into
Conversation
`http_server_request_duration_seconds` has zero emitters and always has. The
two recording rules over it, the HighErrorRatio alert, the Argo Rollouts canary
gate and a KEDA scale-to-zero trigger all consumed it. Every one of them
reported healthy while reading an empty vector.
Verified, not assumed:
count(http_server_request_duration_seconds_count) -> 0 series
HighErrorRatio -> health=ok state=inactive
GET /metrics on compute-gateway, gateway, dashboard-bff, hellgraph-service,
health-twin, eval-fabric-api -> HTTP 404 (api: refused)
and no OpenTelemetry SDK, prometheus_client, promhttp or client_golang appears
in any requirements.txt or any of the four go.mod files.
DECIDED: delete, not enable.
`metrics.enabled` in charts/socioprophet-service gates only the ServiceMonitor.
It is scrape-side and never reaches application code, so turning it on aims
Prometheus at that 404 and fires TargetDown — a new false signal, not a real
one. A genuine emitter means HTTP middleware in both Python and Go, an OTel SDK
added to ~47 services, and every image rebuilt; Actions is at its spend cap, so
none of that can be built or verified now. Shipping "enabled" unverified
rebuilds the defect being removed.
Coverage lost: none that ever existed. These rules produced no signal from the
day they were written.
What replaces them, all over series confirmed present on the live server:
HellGraphDown re-expressed over kube-state-metrics. It was also dead,
for a second reason: `up{job=~".*hellgraph.*"}` has no
members because the ServiceMonitor meant to create that
job selected `app: hellgraph`, a label no Service
carries. It produced no target at all, not even a
failing one.
WorkloadUnavailable deployment at 0 available replicas while spec > 0.
WorkloadCrashLooping restart increase over 15m.
AlertInputSeriesMissing the meta-guard. absent() over each input every other
rule depends on. This is the primitive that was missing
estate-wide: a threshold alert whose data stops arriving
goes quiet and reads as healthy. Verified both ways —
absent() returns nothing over a live metric and 1 over
the dead one.
Also removed or unwired, same defect:
* the `hellgraph` ServiceMonitor — selector matches no Service, and the
endpoint it names 404s. Inert twice over.
* keda-scaledobject-vllm.yaml -> .example.yaml. Its trigger read the same dead
metric; an empty result sits below activationThreshold, so it could only ever
hold the GPU Deployment at zero replicas. A cost control whose only reachable
output is "off" is indistinguishable from one that works. Query corrected to
vllm:num_requests_running, preconditions written down, left unwired because
KEDA is not installed and nothing scrapes mesh-vllm.
…From Caught by server-side apply against the real CRD the moment the controller existed: .spec.args[2].valueFrom.podTemplateHashValue is not in the AnalysisTemplate schema. It is only valid on a Rollout's analysis args. The template declares the arg bare; the Rollout sources it. Example Rollout updated to pass it, since without it the AnalysisRun cannot start. Worth noting the class: this file sat in the repo for weeks describing itself as the live canary gate, and no validation anywhere caught the invalid field — because the CRD was never installed, so nothing could ever have applied it.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Updates the observability and progressive-delivery controls to avoid “healthy over empty vector” failures by removing SLO rules that queried non-emitted metrics, adding absent-series guards, and making the canary analysis fail closed when metrics are missing.
Changes:
- Replace request-metric-based SLO recording/alerting with kube-state-metrics-based workload health alerts + an
absent()meta-guard. - Rewrite the Argo Rollouts AnalysisTemplate to require result existence (
len(result) == 1) so canary gating fails closed on missing series. - Relocate progressive delivery Argo Applications into the watched
deploy/argocd/tree; strand autoscaling/KEDA resources as examples only.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| infra/k8s/rollouts/base/rollout-hellgraph-service.example.yaml | Passes canary pod-template-hash into analysis args to scope Prometheus queries to canary pods. |
| infra/k8s/rollouts/base/analysistemplate-slo.yaml | Replaces dead request-SLO gates with kube-state-metrics-based gates and fail-closed success conditions. |
| infra/k8s/observability/base/servicemonitor.yaml | Removes inert HellGraph ServiceMonitor and documents why it cannot work yet. |
| infra/k8s/observability/base/prometheusrule-slos.yaml | Deletes dead request-SLO rules; adds workload health alerts plus an absent() input-series guard. |
| infra/k8s/autoscaling/base/kustomization.yaml | Unwires autoscaling resources (now explicitly empty) and documents why. |
| infra/k8s/autoscaling/base/keda-scaledobject-vllm.yaml | Removes the previously-wired ScaledObject that depended on a non-existent metric. |
| infra/k8s/autoscaling/base/keda-scaledobject-vllm.example.yaml | Adds an example ScaledObject with corrected vLLM metrics and documented preconditions. |
| infra/argocd/progressive-delivery.yaml | Removes stranded rollouts apps from unwatched tree; leaves mesh app with warning. |
| deploy/argocd/progressive-delivery.yaml | Adds watched-tree Argo Applications for argo-rollouts and rollouts base resources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+84
to
+100
| - name: canary-not-restarting | ||
| # Catches a canary that comes up, serves, then crashes — readiness alone can | ||
| # flap too slowly to catch that inside one analysis window. | ||
| initialDelay: 30s | ||
| interval: 20s | ||
| count: 5 | ||
| # Fail if p99 latency exceeds 1.5s. | ||
| failureCondition: result[0] >= 1.5 | ||
| failureLimit: 1 | ||
| consecutiveErrorLimit: 2 | ||
| successCondition: len(result) == 1 && result[0] <= 2 | ||
| provider: | ||
| prometheus: | ||
| address: http://kube-prometheus-stack-prometheus.observability.svc.cluster.local:9090 | ||
| query: | | ||
| job:request_latency_p99:5m{job="{{args.service}}"} | ||
| sum(kube_pod_container_status_restarts_total{ | ||
| namespace="{{args.namespace}}", | ||
| pod=~"{{args.service}}-{{args.canary-hash}}-.*" | ||
| }) |
Comment on lines
+77
to
+85
| - alert: WorkloadUnavailable | ||
| expr: | | ||
| sum by (job) (rate(http_server_request_duration_seconds_count{http_response_status_code=~"5.."}[5m])) | ||
| / | ||
| clamp_min(sum by (job) (rate(http_server_request_duration_seconds_count[5m])), 1) | ||
| # p99 latency per service — the second canary gate metric. | ||
| - record: job:request_latency_p99:5m | ||
| expr: | | ||
| histogram_quantile(0.99, sum by (job, le) (rate(http_server_request_duration_seconds_bucket[5m]))) | ||
| - alert: HighErrorRatio | ||
| expr: job:request_error_ratio:rate5m > 0.05 | ||
| kube_deployment_status_replicas_available{namespace="socioprophet"} == 0 | ||
| and on (namespace, deployment) | ||
| kube_deployment_spec_replicas{namespace="socioprophet"} > 0 | ||
| for: 10m | ||
| labels: { severity: critical } | ||
| annotations: | ||
| summary: "{{ $labels.deployment }} has 0/{{ $labels.spec_replicas }} available replicas for 10m" |
Comment on lines
+86
to
+88
| - alert: WorkloadCrashLooping | ||
| expr: increase(kube_pod_container_status_restarts_total{namespace=~"socioprophet|observability|serving"}[15m]) > 3 | ||
| for: 5m |
Comment on lines
+108
to
+112
| - alert: AlertInputSeriesMissing | ||
| expr: | | ||
| absent(up) | ||
| or absent(kube_deployment_status_replicas_available{namespace="socioprophet"}) | ||
| or absent(kube_pod_container_status_restarts_total{namespace="socioprophet"}) |
Comment on lines
+48
to
+49
| # The Rollout's name — canary pods are named <rollout>-<hash>-<suffix>. | ||
| - name: service |
Member
Author
|
Merge-gate disposition (Copilot 3-channel + adversarial pass) Canary fails closed on a missing metric (verified): Acknowledged, non-blocking: Verdict: MERGE-READY. Ordering: land before #1116 (the other lane owns #1116). |
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.
What
Three controls in the observability/delivery plane reported healthy and could not fire, all because they read
http_server_request_duration_seconds— a metric with zero emitters estate-wide. This deletes them, re-expresses each over metrics that actually have series, and adds theabsent()meta-guard that was missing everywhere. Every expression here was run against the liveobservabilityPrometheus before commit, and the canary gate was proved to reject a bad rollout on the live cluster.Companion to #1105 (storage lane). Same defect class — a control that is
health=okover an empty vector — in a different plane.The finding, verified not assumed
No OpenTelemetry SDK,
prometheus_client,promhttporclient_golangin anyrequirements.txtor any of the fourgo.modfiles. The metric was never emitted, sojob:request_error_ratio:rate5mandjob:request_latency_p99:5mrecorded empty forever, andHighErrorRatiosatinactive— indistinguishable on every dashboard from "services healthy". The recording rule'sclamp_min(denominator, 1)made it worse: no-data resolved to a numerically healthy0.0rather than a gap.Decision: delete, not enable
Picked deliberately, not for expedience.
metrics.enabledincharts/socioprophet-servicegates only the ServiceMonitor — it is scrape-side and never reaches application code. Turning it on aims Prometheus at that confirmed 404 and firesTargetDown: a new false signal, not a real one. A genuine emitter means HTTP middleware in both Python and Go, an OTel SDK added to ~47 services, and every image rebuilt — none of which can be built or verified while Actions is at its spend cap. Shippingenabled: trueunverified would rebuild the exact defect being removed.Coverage lost: none that ever existed. These rules produced no signal from the day they were written. This PR cannot give you a 5xx ratio — that requires the emitter work above, tracked separately — but it replaces "cannot fire" with alerts that do.
What replaces them — all over series confirmed present live
HellGraphDownkube_deployment_status_replicas_available{deployment="hellgraph-service"}==0up{job=~".*hellgraph.*"}had no members because thehellgraphServiceMonitor selectedapp: hellgraph, a label no Service carries — it produced no target at all, not even a failing oneWorkloadUnavailablearcticdb-gateway) the old plane was blind toWorkloadCrashLoopingAlertInputSeriesMissingabsent(up) or absent(kube_deployment_status_replicas_available{…}) or absent(kube_pod_container_status_restarts_total{…})absent()was used nowhere in the estateAlertInputSeriesMissingis the primitive that was missing: a threshold alert whose data stops arriving goes quiet and reads as healthy. Verified both directions live —absent()returns nothing over a live metric and1over a dead one.Also unwired, same defect:
hellgraphServiceMonitor removed — selector matches no Service, endpoint 404s. Inert twice over.keda-scaledobject-vllm.yaml→.example.yaml. Its trigger read the same dead metric; an empty result sits belowactivationThreshold, so it could only ever hold the GPU Deployment at 0 replicas — a cost control whose only reachable output is "off". Query corrected tovllm:num_requests_running, three blocking preconditions written down, left unwired because KEDA is not installed and nothing scrapesmesh-vllm.The canary gate — proved both ways on the live cluster
The Argo Rollouts
AnalysisTemplategated on the same dead recording rules, and had only afailureCondition— so in Rollouts semantics a measurement that isn't a failure scores Successful, i.e. an empty result promoted. Every canary passed, three ways over (no data; wrong polarity;clamp_min).Rewritten so every metric requires the series to exist before it can pass:
&&short-circuits, so an empty result never reaches the threshold term — it fails the condition and fails the analysis. Missing metric aborts the rollout instead of promoting it. Queries readkube-state-metrics(canary-scoped by pod-template-hash), which has series today.Argo Rollouts was not installed at all when I started (
kubectl get rollouts→the server doesn't have a resource type), so this gate had never been evaluable — it was YAML nobody read while READMEs called it live. I installed the controller via the relocated Application (below), then:Test B — healthy canary → PROMOTED. AnalysisRun
Successful, real measurementscanary-available=[1]×5,canary-not-restarting=[0]×5. Rollout advanced to 100%, stable revision became7885b6f6f4.Test C — deliberately-bad canary (
exit 1) → REJECTED and ROLLED BACK. The load-bearing proof:Not just a breached threshold in a query: the AnalysisRun Failed, the Rollout aborted, and traffic reverted to the previous healthy ReplicaSet.
Fail-closed on a missing metric (item 3, distinct from Test C's present-but-zero). An isolated AnalysisRun with the same guard shape querying a nonexistent metric:
The old template scored this Successful; the new one fails closed.
All probe objects (rollout, analysis runs, broken deployment, proof PrometheusRule) and the temporarily-installed Rollouts controller/CRDs/namespace were deleted afterwards — the cluster is back to matching
main(observabilityappSyncedonmain, livesocioprophet-slosunchanged,postgresPV/PVC untouched at 50Gi).Also here: progressive-delivery relocated into the watched tree
argo-rollouts+progressive-delivery-baseApplications were authored ininfra/argocd/, which the root app-of-apps (infra/tofu/.../argocd.tf,gitops_path=deploy/argocd, recurse) does not watch — the reason there was no controller. Moved todeploy/argocd/progressive-delivery.yamlwithsocioprophet.io/tier: reference(required bytools/preflight_deploy_contract.pycheck 5). Cilium deliberately stays ininfra/argocd/— GKE Autopilot already runs Cilium as its dataplane; relocating it would install a second CNI.Also fixed, caught by server-side apply against the real CRD the moment the controller existed:
canary-hashis a Rollout-supplied arg (valueFrom.podTemplateHashValue), invalid on an AnalysisTemplate arg — wrong this whole time, uncatchable because the CRD was never installed.Not verified / not addressed (deliberate)
preflight_deploy_contract.pyexits 0 (and 1 with the tier annotation removed — teeth confirmed); all baseskustomize build; all YAML parses. Everything else is unverified by CI.len(result)floor). Out of scope.