Add control-process runtime memory metrics + pprof endpoint#892
Add control-process runtime memory metrics + pprof endpoint#892evanphx wants to merge 2 commits into
Conversation
The coordinator process is the one thing miren ships VictoriaMetrics for but never scrapes: every memory_usage_bytes series is per app-sandbox, there is no series for the control process itself, and it exposes no /metrics or pprof endpoint. That is exactly the blind spot the recurring ~4h memory balloon lives in (the miren.service cgroup hits ~50G while all sandboxes sum to ~2.5G, so the growth is the coordinator's own RSS), so today it is invisible to the bundled VM and unattributable. Add a RuntimeMemory collector in the same push-only style as the existing sandbox collectors: a Monitor goroutine samples runtime.ReadMemStats every 10s and pushes go_mem_* plus process_resident_memory_bytes (entity="miren/control") through the same VictoriaMetricsWriter. The gap between process RSS and go_mem_heap_inuse_bytes distinguishes a Go-heap balloon (a heap profile can name the site) from off-heap growth (mmap'd bbolt/etcd or the buildkit content store, which a heap profile cannot see). Also register net/http/pprof on a localhost-only :6060 listener so a heap profile can be pulled live during a balloon without crashing the process. Purely additive; no schema/entity changes, so schema.IndexHash is unchanged and swapping this binary triggers no reindex. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 31542fd0955973f2f429a50e472b4bb91566fce4)
Follow-up to jcasimir's control-process metrics + pprof commit, applying the PR #876 review feedback: - Switch the RuntimeMemory collector from runtime.ReadMemStats (which stops the world to take a consistent snapshot) to runtime/metrics.Read, which reads live counters with no STW pause (evanphx). The emitted go_mem_* series names are preserved so existing dashboards/PromQL keep working; series that don't map to a single sample (HeapInuse, HeapSys, HeapIdle, Sys) are derived via the equivalences documented in the runtime/metrics package, and any absent sample is skipped rather than emitted as a bogus zero. - Serve pprof on a private http.ServeMux with the handlers registered explicitly instead of blank-importing net/http/pprof into http.DefaultServeMux, so nothing else in the process can inadvertently expose handlers on this listener (evanphx). Wrap it in an http.Server that binds via net.Listen up front (a bind failure is now a startup Error, not a silent Warn) and shuts down cleanly on context cancellation instead of leaking until process exit (miren-code-agent). Document the hard-coded-port / one-instance-per-host assumption. - Add unit coverage for RuntimeMemory.collect() (metric names, entity label) and the nil-Writer no-op path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant startPprofServer
participant RuntimeMemory
participant runtimeMetrics as runtime/metrics
participant VictoriaMetricsWriter
Server->>RuntimeMemory: NewRuntimeMemory(log, writer).Monitor(sub)
loop every tick
RuntimeMemory->>runtimeMetrics: Read(samples)
RuntimeMemory->>RuntimeMemory: derive series, compute RSS
RuntimeMemory->>VictoriaMetricsWriter: WritePoints(ctx, points)
end
Server->>startPprofServer: startPprofServer(sub, ctx.Log)
startPprofServer->>startPprofServer: serve /debug/pprof/ on 127.0.0.1:6060
sub-->>startPprofServer: ctx.Done()
startPprofServer->>startPprofServer: Shutdown()
Related issues: None specified. Related PRs: None specified. Suggested labels: metrics, observability, cli Suggested reviewers: None specified. Poem: Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
metrics/runtime_memory.go (1)
88-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMetric mapping verified correct.
Cross-checked the
emit(...)formulas against Prometheus'sclient_golanggo_collector.go (which documents the same MemStats→runtime/metrics equivalences) and an external explainer of theSys - HeapReleasedformula — all ofheap_alloc,heap_inuse,heap_sys,heap_idle,heap_released, andgo_mem_sys_bytesmatch the documented equivalences exactly.One minor note:
labels(line 107) is a single map instance reused by reference across everyMetricPointin the batch (lines 123, 146, 154-159). This is safe today since nothing mutates per-point labels downstream, but it's a latent trap if a future change (e.g. per-series labels) mutates one point'sLabelsand inadvertently affects all others in the batch.♻️ Optional: clone labels per point
- ts := time.Now() - labels := map[string]string{"entity": r.Entity} + ts := time.Now() + newLabels := func() map[string]string { + return map[string]string{"entity": r.Entity} + }And use
newLabels()at eachMetricPointconstruction site instead of the sharedlabelsvariable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metrics/runtime_memory.go` around lines 88 - 163, The RuntimeMemory.collect batch reuses one labels map for every MetricPoint, so any future mutation would affect all emitted series. Update collect (and the shared labels variable it builds) to create a fresh label map per point, or add a helper like newLabels() and use it at each MetricPoint construction site so each metric owns its own labels independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@metrics/runtime_memory.go`:
- Around line 88-163: The RuntimeMemory.collect batch reuses one labels map for
every MetricPoint, so any future mutation would affect all emitted series.
Update collect (and the shared labels variable it builds) to create a fresh
label map per point, or add a helper like newLabels() and use it at each
MetricPoint construction site so each metric owns its own labels independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be93f1d9-a581-4a0b-8dec-9ab3683a1e91
📒 Files selected for processing (5)
cli/commands/server.gometrics/runtime_memory.gometrics/runtime_memory_linux.gometrics/runtime_memory_other.gometrics/runtime_memory_test.go
Supersedes #876. Brings in @jcasimir's control-process instrumentation (his commit is preserved with full attribution) and applies the review feedback from that PR.
Problem
The bundled VictoriaMetrics only receives per-app-sandbox metrics (
memory_usage_bytes{entity="app/..."}); there is no series for the control process itself, and it exposes no/metricsor pprof endpoint. So when the coordinator's own Go heap grows (e.g. a memory balloon), it is invisible to the very metrics stack miren ships, and unattributable without rebuilding.Change
A
RuntimeMemorycollector in the same push-only style as the existing sandbox collectors: a goroutine samples runtime memory every 10s and pushesgo_mem_*,go_goroutines, andprocess_resident_memory_bytes(entity="miren/control") through the existingVictoriaMetricsWriter. The gap between process RSS andgo_mem_heap_inuse_bytesdistinguishes a Go-heap balloon (a heap profile can name the site) from off-heap growth (mmap'd bbolt/etcd, the buildkit content store).Also registers
net/http/pprofon a localhost-only:6060listener so a heap profile can be pulled live during an incident without crashing the process.Purely additive; no schema/entity changes.
Review feedback addressed (from #876)
runtime/metrics.Read(live counters, no stop-the-world) instead ofruntime.ReadMemStats. Emitted series names are preserved; theruntime.MemStats-derived series (HeapInuse/HeapSys/HeapIdle/Sys) are computed from the equivalences documented in theruntime/metricspackage, and any absent sample is skipped rather than emitted as a zero.http.ServeMuxrather than blank-imported intohttp.DefaultServeMux, so nothing else in the process can inadvertently be exposed on this listener.http.Serverthat binds vianet.Listenup front (a bind failure is a startupError, not a silentWarn) and shuts down cleanly on context cancellation instead of leaking. The hard-coded-port / one-instance-per-host assumption is documented.RuntimeMemory.collect()(metric names, entity label) and the nil-Writerno-op path.Verification
go test ./metrics/(incl. new collect + nil-writer tests) passes;go vetandgolangci-lintclean.go_mem_heap_sys_bytes= inuse + released + free,go_mem_sys_bytes= total − released)./debug/pprof/,/debug/pprof/heap, and/debug/pprof/cmdlineall serve on the private mux, and the server becomes unreachable after context cancellation (graceful shutdown honoured).