perf: configurable stats interval + opt-in pprof for CPU diagnosis - #36
Merged
Merged
Conversation
Static review found no busy loop or accidental fan-out; the only load that scales with the deployment is the monitor's stats sweep over all running containers (also driving the Docker daemon). A live capture on a 7-container host showed ~0 CPU at idle, so high CPU is environment-specific — give operators the means to measure and to dial the sweep down. - config: DC_METRICS_INTERVAL (default 15s) makes the stats sampling interval configurable; Monitor.SetStatsInterval applies it (was a hard-coded const). - config: DC_PPROF (off by default) exposes net/http/pprof under /debug/pprof, restricted to loopback (loopbackOnly middleware → 404 off-box) so goroutine/ heap detail never leaks over the network. - docs: deployment.md env table + a "diagnosing high CPU" runbook; CHANGELOG. Tests: config mapping + default; SetStatsInterval (override + ignore ≤0); loopbackOnly gate (loopback pass / remote 404); mountPProf serves the endpoints. All under -short.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds operational controls and diagnostics to help investigate and mitigate high CPU usage on container-dense hosts by (1) making the monitor’s stats sweep interval configurable and (2) optionally exposing Go pprof endpoints gated to loopback access.
Changes:
- Add
DC_METRICS_INTERVAL/-metrics-interval(default15s) and plumb it into the monitor viaMonitor.SetStatsInterval. - Add opt-in
DC_PPROF=1/-pprofto mount/debug/pprof/*endpoints with a loopback-only gate, plus tests. - Update deployment docs and changelog with the new tuning/diagnostics knobs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/monitor/monitor.go | Introduces statsInterval field + SetStatsInterval, and uses it in the stats ticker. |
| internal/monitor/stats_interval_test.go | Adds coverage for default/override/ignored (≤0) stats interval behavior. |
| internal/config/config.go | Adds MetricsInterval and PProf to config; maps env/flags. |
| internal/config/config_test.go | Adds tests for DC_METRICS_INTERVAL/DC_PPROF mapping and defaults. |
| cmd/dockercmd/main.go | Applies cfg.MetricsInterval to the monitor before starting it. |
| internal/api/server.go | Conditionally mounts pprof routes when enabled. |
| internal/api/pprof.go | Adds pprof route wiring and loopbackOnly middleware. |
| internal/api/pprof_test.go | Tests loopback gating and basic pprof endpoint availability when mounted. |
| docs/deployment.md | Documents new env vars and adds a “Diagnosing high CPU” runbook snippet. |
| CHANGELOG.md | Records the new configurable interval and pprof option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+31
to
+41
| // loopbackOnly rejects any request that doesn't originate from the loopback | ||
| // interface with a 404 (not 403, so the path's existence isn't advertised). | ||
| func loopbackOnly(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if !isLoopback(r) { | ||
| http.NotFound(w, r) | ||
| return | ||
| } | ||
| next.ServeHTTP(w, r) | ||
| }) | ||
| } |
Comment on lines
+16
to
+29
| func (s *Server) mountPProf(r chi.Router) { | ||
| r.Route("/debug/pprof", func(r chi.Router) { | ||
| r.Use(loopbackOnly) | ||
| // The index and the named profiles (heap, goroutine, allocs, …) are all | ||
| // served by pprof.Index, which routes on the trailing path segment. | ||
| r.HandleFunc("/", pprof.Index) | ||
| r.HandleFunc("/{profile}", pprof.Index) | ||
| // These four have dedicated handlers (streaming / query-param driven). | ||
| r.HandleFunc("/cmdline", pprof.Cmdline) | ||
| r.HandleFunc("/profile", pprof.Profile) | ||
| r.HandleFunc("/symbol", pprof.Symbol) | ||
| r.HandleFunc("/trace", pprof.Trace) | ||
| }) | ||
| } |
Comment on lines
+290
to
+295
| // Profiling endpoints (DC_PPROF=1). Off by default; when on, restricted to | ||
| // loopback so goroutine/heap detail never leaks over the network even if the | ||
| // server binds a public interface — reach it via an SSH tunnel. | ||
| if s.cfg.PProf { | ||
| s.mountPProf(r) | ||
| } |
Comment on lines
134
to
137
| c.RedisDB = envInt("DC_REDIS_DB", 0) | ||
| c.MetricsRetention = *retention | ||
| c.MetricsInterval = *interval | ||
| c.SessionTTL = *ttl |
Comment on lines
+21
to
+24
| key := make([]byte, 32) | ||
| _, _ = rand.Read(key) | ||
| c, _ := crypto.New(key) | ||
| st.SetCipher(c) |
… on main Security review of the previous commit found the pprof gate was bypassable: loopbackOnly relied on isLoopback(r), but the main router runs behind chi's middleware.RealIP, which rewrites r.RemoteAddr from X-Forwarded-For/X-Real-IP (chi itself deprecates RealIP as spoofable). So `X-Forwarded-For: 127.0.0.1` would have defeated the gate on a directly-exposed server. Fix: don't gate by client IP at all. Serve pprof from a dedicated 127.0.0.1:6060 listener started in main, physically unreachable off-box regardless of bind interface or forwarded headers. Removed mountPProf and the loopbackOnly middleware from the main router; PProfHandler() now just builds the mux and documents that the loopback listener is the boundary. Verified live: :6060 serves pprof and a real `go tool pprof` capture; the main port returns only the SPA for /debug/pprof/ (no profile data), with or without a spoofed X-Forwarded-For. (Note for a separate change: the localhostNo2fa exemption uses the same isLoopback + RealIP combination and is spoofable the same way — out of scope here.)
- config: clamp a non-positive DC_METRICS_INTERVAL to the 15s default at load, so the resolved Config value is never a misleading ≤0 the monitor would just ignore. Adds a clamp test. - test: handle rand.Read / crypto.New errors in the stats-interval test instead of discarding them, so a setup failure surfaces immediately. (The pprof X-Forwarded-For spoofing Copilot flagged was already fixed earlier in this PR — pprof now runs on a dedicated 127.0.0.1:6060 listener, not IP-gated on the main router.)
# Conflicts: # CHANGELOG.md # docs/deployment.md # internal/config/config.go # internal/config/config_test.go
# Conflicts: # CHANGELOG.md # internal/monitor/monitor.go
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.
Performance diagnostics + a lever for container-dense hosts
Follow-up to a report that the app sits at ~60% CPU constantly. Targeting 1.4, kept as its own PR.
What I found (static + live)
A full pass over every long-lived goroutine, ticker and loop turned up no busy-loop and no accidental fan-out:
select { …; default: }inStreamStatsis not a spin —dec.Decode()blocks on the next frame (~1/s);The only cost that scales with the deployment is the monitor's stats sweep over all running containers every 15s, which also makes the Docker daemon compute cgroup stats. A live
go tool pprofcapture on this machine (7 running containers, no browser) showed ~0 CPU samples at idle — so the 60% is environment-specific (more containers, an open dashboard/log view, chatty containers, or dockerd itself). The right fix is to let operators measure and tune, not to guess-patch.Changes
DC_METRICS_INTERVAL(default15s) — makes the stats sampling interval configurable (Monitor.SetStatsInterval); raise it (30s/60s) on a container-dense host. Was a hard-coded const.DC_PPROF=1(off by default) — exposes Go'snet/http/pprofunder/debug/pprof, restricted to loopback (aloopbackOnlymiddleware returns 404 off-box, so goroutine/heap detail never leaks even if the server binds0.0.0.0). Capture a profile through an SSH tunnel:deployment.mdenv table + a "diagnosing high CPU" runbook; CHANGELOG.Tests
DC_METRICS_INTERVAL/DC_PPROFmapping + the 15s/off defaults.SetStatsIntervalapplies a positive value, ignores ≤0.loopbackOnlypasses loopback (127.0.0.1,::1) and 404s remote IPs;mountPProfserves/debug/pprof/{,heap,goroutine}./debug/pprof/+ a realgo tool pprofcapture work end-to-end.go test -short ./...,go vet,gofmtall green. No UI changes.For the colleague
With
DC_PPROF=1, capture a 30s profile during the 60% spike and sharego tool pprof -top— that names the exact function/goroutine. Knowing the running-container count also helps.🤖 Generated with Claude Code