Commit 68ac69c
Add Prometheus custom metrics for scaling observability (#241)
* Add Prometheus custom metrics for scaling observability
Operators have no way to ask, from metrics alone, "did this autoscaler
scale, and why?" Reconcile logs answer the question for one event at a
time but cannot be aggregated, and the controller-runtime default
metrics describe reconcile loop health, not scaling outcomes.
Add an internal/observability package that registers custom collectors
on controller-runtime's metrics.Registry, so the same /metrics endpoint
that already serves the standard metrics now also exposes:
- State gauges: current/desired/min/max processing units (including the
schedule-expanded effective bounds), CPU utilization and target per
metric type, instance ready, active schedules count and additional PU
sum, last scale / last sync timestamps.
- Schedule counters: activations (cron firings) and deactivations
labeled by reason (expired | unregistered).
- Scale event counters and histogram: scale_events_total labeled by
direction and driver (cpu_high_priority | cpu_total | schedule);
scale_skipped_total labeled by reason; scale_pu_delta histogram of
the absolute PU change per event.
- Operational counters and histograms: instance_update_total /
instance_update_duration_seconds for Spanner UpdateInstance;
metrics_fetch_total / metrics_fetch_duration_seconds for Cloud
Monitoring GetInstanceMetrics.
Every business metric carries (namespace, name, project_id, instance_id)
directly rather than via an info-metric join, so the Datadog Agent's
OpenMetrics check and other flat-tag consumers can attribute series
without a PromQL join. Active series scale with the number of
SpannerAutoscaler resources (~80 series per resource); join-style
flexibility is preserved for future via an info metric if needed.
The driver label on scale_events_total is a best-effort attribution
computed at the call site: a desired value pinned to the effective min
floor that exceeds the spec-level min is attributed to "schedule"; in
dual CPU mode the per-metric candidates from calcDesiredPUFromCPU are
recomputed and the larger one wins.
A labelsCache on the reconciler tracks the last observed identity
labels per resource so DeleteSeries can be called on the NotFound
branch (when sa.Spec is gone) and on spec.targetInstance changes that
would otherwise leave orphan series on /metrics indefinitely.
The syncer constructor now takes projectID and instanceID directly so
the Cloud Monitoring fetch wrappers can emit labels without an extra
ctrlClient.Get per call.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Add HTTP exposition round-trip test for custom metrics
The existing unit tests prove each Record* helper updates its in-memory
vector, but do not exercise the path the controller binary actually
uses at runtime: promhttp.Handler serializing reg.Gather() into
Prometheus exposition format. A regression in label ordering, label
name typo, or registration omission would slip past the per-helper
tests.
Add a single TestMetricsHTTPExposition that:
- Stands up a fresh prometheus.NewRegistry, calls Register, drives one
sample through every Record* helper, and serves the registry through
httptest.NewServer wrapping promhttp.HandlerFor.
- Pulls /metrics over HTTP, parses the body with expfmt.TextParser, and
cross-checks the parsed metric families against reg.Gather() so the
expected metric set is derived automatically — adding a new
collector to allCollectors() and calling its Record* here is the
only edit needed.
- Verifies every spanner_autoscaler_* series carries the four identity
labels (namespace, name, project_id, instance_id) with the values the
test recorded. A future emit site that forgets to thread one of the
labels will fail this assertion without naming the offending metric.
Specific values, bucket boundaries, and exposition line wording are
intentionally not asserted; those change far more often than the
contract the test is meant to protect.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Defer schedule deactivation side effects until persistence
cleanupActiveSchedules is invoked inside retry.RetryOnConflict in
scheduler.Update; on a conflict-retry the closure re-runs and re-fires
every side effect, so the deactivation counter (and the log line that
landed via PR #240) could be incremented multiple times for a single
expiry. pruneActiveSchedules in the controller reconcile has an
analogous issue: it fires before Status.Update, so a Status.Update
failure that triggers a requeue causes the same orphan ActiveSchedule
entries to be re-observed and re-recorded until the write eventually
succeeds.
Refactor both helpers to be pure: return the entries kept and the
entries removed, with no logging or counter increments inside. The
callers now emit "scheduled scaling deactivated" / "removed currently
active schedule" logs and RecordScheduleDeactivation only after the
status update has been persisted, guaranteeing one side-effect per
actual transition.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Fix golangci-lint failures (gocyclo, nolintlint)
1. Reconcile cyclomatic complexity exceeded the project threshold (31 > 30)
after this PR added the labelsCache lookup in the NotFound branch and
the labels-change detection block.
Extract both into a single updateLabelsCache helper that takes the
current SpannerAutoscaler (or nil for the deletion path). The
inline NotFound block now reduces to one method call, and the
refresh path likewise. This brings the function back under the
threshold without dropping any behavior.
2. cmd/main.go carried two //nolint:staticcheck directives suppressing
a deprecation warning on mgr.GetEventRecorderFor, but staticcheck no
longer flags those call sites — leaving the directives in place
makes nolintlint fail with "unused". Remove them; when staticcheck
begins flagging GetEventRecorderFor again the warning will surface
naturally and we can address it on its own merits.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Address review: record state on every reconcile return path
The previous placement of observability.RecordState at the bottom of
Reconcile was skipped on the early-return branches for
InstanceState != Ready and CurrentProcessingUnits == 0. As nktks
pointed out (#241 r3285973010), a ready -> not-ready transition would
leave spanner_autoscaler_instance_ready stuck at 1, and the same
issue applies to every state gauge.
Move the call into a defer placed immediately after the labels-cache
refresh, where sa is guaranteed populated. The deferred call captures
&sa so it observes the final state at return time, regardless of which
early-return path is taken. RecordState is panic-free with any
non-nil *SpannerAutoscaler (all field accesses use value types or are
nil-guarded), so it is safe to invoke from a defer that runs on every
return.
Also restore the //nolint:staticcheck directives on the two
mgr.GetEventRecorderFor calls in cmd/main.go. They were removed when
local golangci-lint did not flag SA1019, but the CI version does
flag the deprecation, so the directives are still needed. The
migration to GetEventRecorder requires refactoring every Event call
site (which use the old record.EventRecorder type) and remains tracked
separately.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>1 parent d87446e commit 68ac69c
9 files changed
Lines changed: 1160 additions & 42 deletions
File tree
- cmd
- internal
- controller
- observability
- scheduler
- syncer
- test/integration
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
450 | 450 | | |
451 | 451 | | |
452 | 452 | | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
| 517 | + | |
| 518 | + | |
| 519 | + | |
| 520 | + | |
| 521 | + | |
| 522 | + | |
| 523 | + | |
| 524 | + | |
| 525 | + | |
| 526 | + | |
| 527 | + | |
| 528 | + | |
453 | 529 | | |
454 | 530 | | |
455 | 531 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| 33 | + | |
33 | 34 | | |
34 | 35 | | |
35 | 36 | | |
| |||
38 | 39 | | |
39 | 40 | | |
40 | 41 | | |
| 42 | + | |
41 | 43 | | |
42 | 44 | | |
43 | 45 | | |
| |||
101 | 103 | | |
102 | 104 | | |
103 | 105 | | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
104 | 111 | | |
105 | 112 | | |
106 | 113 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
16 | 19 | | |
17 | 20 | | |
18 | 21 | | |
| |||
57 | 60 | | |
58 | 61 | | |
59 | 62 | | |
| 63 | + | |
60 | 64 | | |
61 | 65 | | |
62 | 66 | | |
63 | 67 | | |
64 | 68 | | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | 69 | | |
69 | 70 | | |
70 | 71 | | |
| |||
0 commit comments