Skip to content

Commit 2f6012d

Browse files
committed
obs(redis): lua pool metrics — r1 review fixes
Round-1 fixes on commit f7354b4: 1. coderabbit CRITICAL @ JSON:1749 — row ID 24 collision The new "Lua VM Pool" row reused id:24, which the existing "Hot Path (legacy PR #560)" row also uses for one of its nested panels. Grafana requires unique IDs across the entire dashboard; collisions cause import failures / ambiguous UI refs. Changed the row's id to 33 (max existing was 32). Codex P2 raised the same finding. 2. coderabbit MAJOR @ JSON:1817 — missing node_id template filter All four new Lua VM Pool queries hardcoded {job="elastickv"} and ignored the $node_id template variable that every other panel in this dashboard threads through. Operators picking a specific node in the dropdown got cluster-wide data on this row only. Added node_id=~"$node_id" to all 8 query strings (idle, max_idle, hits, misses, drops_rate, drops_range, and the saturation ratio's numerator + denominator). 3. gemini medium @ lua_pool.go:86 — AlreadyRegisteredError absorption RegisterLuaPool now wraps the loop with errors.As(err, *prometheus.AlreadyRegisteredError) and skips. Test harnesses that share a registry across sub-tests (and any hypothetical hot-reload path in main.go) can now retry without tripping a spurious slog.Warn. Other registration errors (invalid metric name, label conflict) still propagate. Test: go test -race -count=1 -run TestRegisterLuaPool ./monitoring -- green. golangci-lint run --config=.golangci.yaml ./monitoring ./adapter . -- 0 issues. python3 -c "import json; json.load(open( 'monitoring/grafana/dashboards/elastickv-redis-summary.json'))" -- JSON valid. New test TestRegisterLuaPool_IdempotentOnDoubleRegister locks in the idempotent-double-register contract per CLAUDE.md's test-with-fix convention.
1 parent 05da85d commit 2f6012d

2 files changed

Lines changed: 28 additions & 0 deletions

File tree

monitoring/lua_pool.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,15 @@ func RegisterLuaPool(registerer prometheus.Registerer, src LuaPoolSource) error
8181
}
8282
for _, c := range collectors {
8383
if err := registerer.Register(c); err != nil {
84+
// Idempotent on AlreadyRegistered so callers that retry
85+
// (test harnesses with shared registries, hypothetical
86+
// hot-reload paths) don't trip a spurious slog.Warn in
87+
// main.go. Any other error — invalid metric name,
88+
// inconsistent labels, registry conflict — surfaces.
89+
var already prometheus.AlreadyRegisteredError
90+
if errors.As(err, &already) {
91+
continue
92+
}
8493
return errors.Wrap(err, "register lua pool collector")
8594
}
8695
}

monitoring/lua_pool_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,25 @@ func TestRegisterLuaPool_NilSafe(t *testing.T) {
100100
require.NoError(t, RegisterLuaPool(nil, nil))
101101
}
102102

103+
// TestRegisterLuaPool_IdempotentOnDoubleRegister locks in the
104+
// AlreadyRegisteredError absorption added per gemini r1 review:
105+
// calling RegisterLuaPool a second time against the same registerer
106+
// must succeed silently rather than returning a noisy error that
107+
// main.go would surface as a slog.Warn. Test harnesses that share a
108+
// registry across sub-tests rely on this.
109+
func TestRegisterLuaPool_IdempotentOnDoubleRegister(t *testing.T) {
110+
t.Parallel()
111+
reg := prometheus.NewRegistry()
112+
src := &fakeLuaPool{maxIdle: 64}
113+
require.NoError(t, RegisterLuaPool(reg, src))
114+
// Second call must be a no-op success: each of the five
115+
// CounterFunc / GaugeFunc collectors AlreadyRegisteredError-
116+
// returns on Register(), and the loop should absorb that.
117+
require.NoError(t, RegisterLuaPool(reg, src),
118+
"double registration must be idempotent so noisy retry paths "+
119+
"don't crash main.go via the slog.Warn return")
120+
}
121+
103122
// srcCollectorByName is a tiny helper that fetches a registered
104123
// collector by metric name so testutil.ToFloat64 can read its
105124
// scrape value. It hides the gather + scan boilerplate from each

0 commit comments

Comments
 (0)