From 75642d90cfd5e2a9fe113dae315d819cdc829a54 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 30 Jun 2026 12:38:55 +0900 Subject: [PATCH] feat(metrics): add named felis_* Prometheus collectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce internal/metrics exposing the four metric families spec §23 mandates at minimum: felis_servers_total (gauge by desired state), felis_start_duration_seconds (histogram with Minecraft cold-start buckets), felis_image_build_failures_total and felis_reaper_worlds_deleted_total (counters). Collectors are package-level vars so any subsystem records without an import cycle; Register wires them into a prometheus.Registerer and is idempotent. Wire registration into the operator against controller-runtime's global Registry, so /metrics on the manager's existing metrics endpoint carries the felis_* families. Instrument the reaper to increment felis_reaper_worlds_deleted_total in lockstep with Summary.WorldsReaped, at the one point a world's PVC has actually been deleted. --- cmd/felis/operator.go | 12 ++++ internal/metrics/metrics.go | 89 ++++++++++++++++++++++++++++++ internal/metrics/metrics_test.go | 94 ++++++++++++++++++++++++++++++++ internal/reaper/reaper.go | 5 ++ internal/reaper/reaper_test.go | 31 +++++++++++ 5 files changed, 231 insertions(+) create mode 100644 internal/metrics/metrics.go create mode 100644 internal/metrics/metrics_test.go diff --git a/cmd/felis/operator.go b/cmd/felis/operator.go index 893f978..2ac0e92 100644 --- a/cmd/felis/operator.go +++ b/cmd/felis/operator.go @@ -6,12 +6,14 @@ import ( "io" "felis.lolicon.best/internal/apis/felis/v1alpha1" + felismetrics "felis.lolicon.best/internal/metrics" "felis.lolicon.best/internal/operator" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" + ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" ) @@ -57,6 +59,16 @@ func cmdOperator(args []string, _, stderr io.Writer) int { } fmt.Fprintf(stderr, "felis operator: watching namespace %q\n", *namespace) + // Publish the named felis_* metrics (spec §23) on the endpoint the manager + // already serves (metricsAddr). controller-runtime's metrics server exposes + // its global Registry, so registering into it is all that is needed for + // /metrics to carry felis_servers_total and friends. Register is idempotent, + // so an in-process restart never double-registers fatally. + if err := felismetrics.Register(ctrlmetrics.Registry); err != nil { + fmt.Fprintf(stderr, "felis operator: register metrics: %v\n", err) + return 1 + } + r := &operator.Reconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go new file mode 100644 index 0000000..805eb11 --- /dev/null +++ b/internal/metrics/metrics.go @@ -0,0 +1,89 @@ +// Package metrics defines and registers the named felis_* Prometheus +// collectors mandated by spec §23. +// +// The collectors are package-level vars so any subsystem (operator, reaper, +// image builder, prober) can record into them without importing back into a +// metrics owner and risking an import cycle. Wiring them into a registry is a +// single Register call that takes a prometheus.Registerer — the same +// inject-the-interface, fake-at-the-edge pattern used elsewhere in Felis +// (ownerStore, PVCResolver): production passes controller-runtime's global +// Registry (served by the operator's :metrics endpoint), tests pass a fresh +// prometheus.NewRegistry() so assertions never collide with global state. +package metrics + +import ( + "errors" + + "github.com/prometheus/client_golang/prometheus" +) + +// namespace prefixes every collector, so the exposed names are exactly +// felis_ — matching the metric names spec §23 mandates. +const namespace = "felis" + +var ( + // ServersTotal is the current number of MinecraftServers the operator knows + // about, partitioned by desiredState. It is a gauge, not a monotonic counter: + // the reconcile loop Set()s it to the live fleet size, so it can fall as + // servers are deleted. (The _total suffix follows the name spec §23 fixed.) + ServersTotal = prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Namespace: namespace, + Name: "servers_total", + Help: "Current number of Minecraft servers known to the operator, by desired state.", + }, []string{"state"}) + + // StartDurationSeconds observes the wall-clock time from desiredState=Running + // to a server reporting ready. Buckets are tuned for Minecraft cold starts + // (seconds to a few minutes), not the default sub-second web-latency buckets. + StartDurationSeconds = prometheus.NewHistogram(prometheus.HistogramOpts{ + Namespace: namespace, + Name: "start_duration_seconds", + Help: "Time from desiredState=Running to a server reporting ready, in seconds.", + Buckets: []float64{1, 2, 5, 10, 20, 30, 45, 60, 90, 120, 180, 300, 600}, + }) + + // ImageBuildFailuresTotal counts modpack/image build failures (spec §8 lane). + ImageBuildFailuresTotal = prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: namespace, + Name: "image_build_failures_total", + Help: "Total number of image build failures.", + }) + + // ReaperWorldsDeletedTotal counts world volumes deleted by the reaper. + ReaperWorldsDeletedTotal = prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: namespace, + Name: "reaper_worlds_deleted_total", + Help: "Total number of world volumes deleted by the reaper.", + }) +) + +// Collectors returns every felis_* collector in a stable order. Production and +// tests register the same slice, so the test asserting the full set is exposed +// also pins the production surface. +func Collectors() []prometheus.Collector { + return []prometheus.Collector{ + ServersTotal, + StartDurationSeconds, + ImageBuildFailuresTotal, + ReaperWorldsDeletedTotal, + } +} + +// Register wires every felis_* collector into r. +// +// It is idempotent: re-registering an already-registered collector (a manager +// restart in-process, a test that calls Register twice) is tolerated rather +// than fatal, so a benign double-call never takes the operator down. Any other +// registration error is returned to the caller to surface at startup. +func Register(r prometheus.Registerer) error { + for _, c := range Collectors() { + if err := r.Register(c); err != nil { + var already prometheus.AlreadyRegisteredError + if errors.As(err, &already) { + continue + } + return err + } + } + return nil +} diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go new file mode 100644 index 0000000..5e27b2a --- /dev/null +++ b/internal/metrics/metrics_test.go @@ -0,0 +1,94 @@ +package metrics + +import ( + "strings" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil" +) + +// wantNames is the exact set of metric names spec §23 mandates "at minimum". +// Pinning them here makes a rename a deliberate, test-visible act rather than a +// silent dashboard/alert break. +var wantNames = []string{ + "felis_servers_total", + "felis_start_duration_seconds", + "felis_image_build_failures_total", + "felis_reaper_worlds_deleted_total", +} + +func TestRegisterExposesNamedFelisMetrics(t *testing.T) { + reg := prometheus.NewRegistry() + if err := Register(reg); err != nil { + t.Fatalf("Register: %v", err) + } + + // Record into each collector through its typed API so a Gather emits the + // family — this proves the exported vars are the ones actually registered, + // not shadow copies. + ServersTotal.WithLabelValues("Running").Set(3) + StartDurationSeconds.Observe(12.5) + ImageBuildFailuresTotal.Inc() + ReaperWorldsDeletedTotal.Add(2) + + mfs, err := reg.Gather() + if err != nil { + t.Fatalf("Gather: %v", err) + } + got := map[string]bool{} + for _, mf := range mfs { + got[mf.GetName()] = true + } + for _, name := range wantNames { + if !got[name] { + t.Errorf("mandated metric %q not exposed by the registry", name) + } + } + // Nothing may leak out from under the felis_ namespace — a stray name would + // mean a collector was declared without the namespace prefix. + for name := range got { + if !strings.HasPrefix(name, "felis_") { + t.Errorf("metric %q is not under the felis_ namespace", name) + } + } +} + +func TestRegisterIsIdempotent(t *testing.T) { + reg := prometheus.NewRegistry() + if err := Register(reg); err != nil { + t.Fatalf("first Register: %v", err) + } + // A second Register into the same registry must not error: setup that runs + // twice (manager restart, test re-entry) should be harmless, not fatal. + if err := Register(reg); err != nil { + t.Fatalf("second Register should be idempotent, got: %v", err) + } +} + +func TestCountersRecordExpectedValues(t *testing.T) { + // These collectors are package-level singletons. This test asserts on a + // gauge child labelled "Stopped" (no other test touches it) and a >=1 bound + // on the build-failure counter, so values stay stable regardless of test + // ordering or accumulation across the binary. + reg := prometheus.NewRegistry() + if err := Register(reg); err != nil { + t.Fatalf("Register: %v", err) + } + + ImageBuildFailuresTotal.Inc() + if got := testutil.ToFloat64(ImageBuildFailuresTotal); got < 1 { + t.Errorf("image build failures = %v, want >= 1", got) + } + + g := ServersTotal.WithLabelValues("Stopped") + g.Set(7) + if got := testutil.ToFloat64(g); got != 7 { + t.Errorf("servers_total{state=Stopped} = %v, want 7", got) + } + + StartDurationSeconds.Observe(30) + if n := testutil.CollectAndCount(StartDurationSeconds); n != 1 { + t.Errorf("start_duration_seconds collected %d metrics, want 1 histogram", n) + } +} diff --git a/internal/reaper/reaper.go b/internal/reaper/reaper.go index 665c28b..c90e975 100644 --- a/internal/reaper/reaper.go +++ b/internal/reaper/reaper.go @@ -33,6 +33,7 @@ import ( "time" "felis.lolicon.best/internal/backup" + "felis.lolicon.best/internal/metrics" ) // Day is a calendar day; the retention windows in §18 are expressed in days. @@ -382,6 +383,10 @@ func (r *Reaper) reap(ctx context.Context, now time.Time, c Candidate, crd Serve } sum.WorldsReaped++ + // felis_reaper_worlds_deleted_total (spec §23) advances in lockstep with the + // per-run Summary tally — incremented here, at the one point a world's PVC has + // actually been deleted, not at evaluation time. + metrics.ReaperWorldsDeletedTotal.Inc() r.log().Info("reaper: world reaped", "server", c.Name, "former_owner", c.OwnerID, "backup_ref", ref) return nil } diff --git a/internal/reaper/reaper_test.go b/internal/reaper/reaper_test.go index 026bf2a..6824d90 100644 --- a/internal/reaper/reaper_test.go +++ b/internal/reaper/reaper_test.go @@ -12,6 +12,9 @@ import ( "time" "felis.lolicon.best/internal/backup" + "felis.lolicon.best/internal/metrics" + + "github.com/prometheus/client_golang/prometheus/testutil" ) // testNow is the frozen clock for every hermetic case. Idle is expressed as an @@ -307,6 +310,34 @@ func TestReapIdleWorldFullSequence(t *testing.T) { } } +// §23 instrumentation: felis_reaper_worlds_deleted_total advances by exactly one +// per world whose PVC is actually deleted — in lockstep with Summary.WorldsReaped, +// and never for a skipped/preserved world. Asserted as a delta because the counter +// is a process-global singleton other tests in this package also advance. +func TestReapIncrementsDeletedWorldsMetric(t *testing.T) { + before := testutil.ToFloat64(metrics.ReaperWorldsDeletedTotal) + + r, _, cl, _ := newReaper(DefaultConfig(), + Candidate{Name: "metric-a", OwnerID: "user-9", LastActiveAt: idleBy(20 * Day)}, + Candidate{Name: "metric-b", OwnerID: "user-9", LastActiveAt: idleBy(20 * Day)}, + // Fresh server: under the deadline, must NOT be reaped or counted. + Candidate{Name: "metric-fresh", OwnerID: "user-9", LastActiveAt: idleBy(2 * Day)}, + ) + + sum := mustRun(t, r) + if sum.WorldsReaped != 2 { + t.Fatalf("WorldsReaped = %d, want 2", sum.WorldsReaped) + } + if cl.deletePVCCalls != 2 { + t.Fatalf("deletePVC calls = %d, want 2", cl.deletePVCCalls) + } + + delta := testutil.ToFloat64(metrics.ReaperWorldsDeletedTotal) - before + if delta != float64(sum.WorldsReaped) { + t.Fatalf("felis_reaper_worlds_deleted_total advanced by %v, want %d (one per reaped world)", delta, sum.WorldsReaped) + } +} + // CENTERPIECE — red line ④: when the archive fails, the PVC is never deleted, // ownership is untouched, no backup row is written, and the same server is // retried (state unchanged) on the next run.