From 3232c8efc7f841aa9c0913b035fc5b126fdeeba5 Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Tue, 21 Apr 2026 22:12:17 +0900 Subject: [PATCH 1/4] obs(redis-lua): per-cmd fast-path outcome counter Instrument redis.call() fast-path decisions inside Lua scripts so operators can see how often a given command actually takes the fast path vs falls back. Triggered by a production profile showing cmdZRangeByScoreSlow consuming ~14x the CPU of zsetRangeByScoreFast on a BullMQ-style workload, despite the fast path being eligible on paper. - Add elastickv_lua_cmd_fastpath_total{cmd,outcome} counter, with outcome = hit | skip_loaded | skip_cached_type | fallback. - Wire LuaFastPathObserver through Registry, RedisServer option, and luaScriptContext.observeFastPathOutcome. - Instrument cmdZRangeByScore as the first caller; other fast-path commands (ZSCORE, HGET, HEXISTS, SISMEMBER) will follow once this metric validates the instrumentation shape. --- adapter/redis.go | 40 ++++++++++++++++++++++-------------- adapter/redis_lua_context.go | 27 ++++++++++++++++++++++++ main.go | 1 + monitoring/hotpath.go | 37 +++++++++++++++++++++++++++++++++ monitoring/hotpath_test.go | 30 +++++++++++++++++++++++++++ monitoring/registry.go | 10 +++++++++ 6 files changed, 130 insertions(+), 15 deletions(-) diff --git a/adapter/redis.go b/adapter/redis.go index 917f6d23..fc7f8f8e 100644 --- a/adapter/redis.go +++ b/adapter/redis.go @@ -250,21 +250,22 @@ var argsLen = map[string]int{ } type RedisServer struct { - listen net.Listener - store store.MVCCStore - coordinator kv.Coordinator - readTracker *kv.ActiveTimestampTracker - redisTranscoder *redisTranscoder - pubsub *redisPubSub - scriptMu sync.RWMutex - scriptCache map[string]string - traceCommands bool - traceSeq atomic.Uint64 - redisAddr string - relay *RedisPubSubRelay - relayConnCache kv.GRPCConnCache - requestObserver monitoring.RedisRequestObserver - luaObserver monitoring.LuaScriptObserver + listen net.Listener + store store.MVCCStore + coordinator kv.Coordinator + readTracker *kv.ActiveTimestampTracker + redisTranscoder *redisTranscoder + pubsub *redisPubSub + scriptMu sync.RWMutex + scriptCache map[string]string + traceCommands bool + traceSeq atomic.Uint64 + redisAddr string + relay *RedisPubSubRelay + relayConnCache kv.GRPCConnCache + requestObserver monitoring.RedisRequestObserver + luaObserver monitoring.LuaScriptObserver + luaFastPathObserver monitoring.LuaFastPathObserver // baseCtx is the parent context for per-request handlers. // NewRedisServer creates a cancelable context here; Stop() cancels // it so in-flight handlers abort promptly instead of running @@ -320,6 +321,15 @@ func WithLuaObserver(observer monitoring.LuaScriptObserver) RedisServerOption { } } +// WithLuaFastPathObserver enables per-redis.call() fast-path outcome +// metrics inside Lua scripts. Used to diagnose fast-path hit ratios +// for commands like ZRANGEBYSCORE / ZSCORE / HGET. +func WithLuaFastPathObserver(observer monitoring.LuaFastPathObserver) RedisServerOption { + return func(r *RedisServer) { + r.luaFastPathObserver = observer + } +} + // redisMetricsConn wraps a redcon.Conn to detect whether WriteError was called. type redisMetricsConn struct { redcon.Conn diff --git a/adapter/redis_lua_context.go b/adapter/redis_lua_context.go index b9436136..d5f6f8d3 100644 --- a/adapter/redis_lua_context.go +++ b/adapter/redis_lua_context.go @@ -10,6 +10,7 @@ import ( "time" "github.com/bootjp/elastickv/kv" + "github.com/bootjp/elastickv/monitoring" "github.com/bootjp/elastickv/store" "github.com/cockroachdb/errors" ) @@ -2425,9 +2426,11 @@ func (c *luaScriptContext) cmdZRangeByScore(args []string, reverse bool) (luaRep // type-change on this key. Mirrors the cmdZScore / cmdHGet guards // so in-script ZADD / ZREM / DEL / SET behave exactly as before. if luaZSetAlreadyLoaded(c, key) { + c.observeFastPathOutcome(luaCmdZRangeByScore, monitoring.LuaFastPathOutcomeSkipAlreadyLoad) return c.cmdZRangeByScoreSlow(key, options, reverse) } if _, cached := c.cachedType(key); cached { + c.observeFastPathOutcome(luaCmdZRangeByScore, monitoring.LuaFastPathOutcomeSkipCachedType) return c.cmdZRangeByScoreSlow(key, options, reverse) } entries, hit, fastErr := c.zrangeByScoreFastPath(key, options, reverse) @@ -2435,14 +2438,38 @@ func (c *luaScriptContext) cmdZRangeByScore(args []string, reverse bool) (luaRep return luaReply{}, fastErr } if !hit { + c.observeFastPathOutcome(luaCmdZRangeByScore, monitoring.LuaFastPathOutcomeFallback) return c.cmdZRangeByScoreSlow(key, options, reverse) } + c.observeFastPathOutcome(luaCmdZRangeByScore, monitoring.LuaFastPathOutcomeHit) if len(entries) == 0 { return luaArrayReply(), nil } return zsetRangeReply(entries, options.withScores), nil } +// luaCmdZRangeByScore is the metric label for ZRANGEBYSCORE / +// ZREVRANGEBYSCORE fast-path outcomes. Both directions share one +// label because the fast-path implementation is identical except for +// scan direction. +const luaCmdZRangeByScore = "zrangebyscore" + +// observeFastPathOutcome records one fast-path decision via the +// server's observer. No-op when no observer is wired (tests, or a +// scriptCtx constructed without a RedisServer). +// +// cmd is accepted as a parameter (rather than hard-coded) so the +// next wave of fast-path commands (ZSCORE, HGET, HEXISTS, SISMEMBER) +// can plug in without widening the helper signature. +// +//nolint:unparam // cmd will gain additional values in follow-up commits. +func (c *luaScriptContext) observeFastPathOutcome(cmd, outcome string) { + if c.server == nil { + return + } + c.server.luaFastPathObserver.ObserveLuaFastPath(cmd, outcome) +} + // zrangeByScoreFastPath translates the caller's min/max bounds into a // bounded score-index scan through zsetRangeByScoreFast and returns // the decoded entries. hit=false means the server-side helper wants diff --git a/main.go b/main.go index a1aed4a8..2e5b43f6 100644 --- a/main.go +++ b/main.go @@ -553,6 +553,7 @@ func startRedisServer(ctx context.Context, lc *net.ListenConfig, eg *errgroup.Gr adapter.WithRedisActiveTimestampTracker(readTracker), adapter.WithRedisRequestObserver(metricsRegistry.RedisObserver()), adapter.WithLuaObserver(metricsRegistry.LuaObserver()), + adapter.WithLuaFastPathObserver(metricsRegistry.LuaFastPathObserver()), adapter.WithRedisCompactor(deltaCompactor), ) eg.Go(func() error { diff --git a/monitoring/hotpath.go b/monitoring/hotpath.go index 479b1303..30e7cd25 100644 --- a/monitoring/hotpath.go +++ b/monitoring/hotpath.go @@ -34,8 +34,19 @@ type HotPathMetrics struct { dispatchDroppedTotal *prometheus.CounterVec dispatchErrorsTotal *prometheus.CounterVec stepQueueFullTotal *prometheus.CounterVec + luaFastPathTotal *prometheus.CounterVec } +// LuaFastPathOutcome labels tag each Lua-side read fast-path decision +// so operators can see how often a given command (ZRANGEBYSCORE, +// ZSCORE, HGET, etc.) actually takes the fast path vs falls back. +const ( + LuaFastPathOutcomeHit = "hit" + LuaFastPathOutcomeSkipAlreadyLoad = "skip_loaded" + LuaFastPathOutcomeSkipCachedType = "skip_cached_type" + LuaFastPathOutcomeFallback = "fallback" +) + func newHotPathMetrics(registerer prometheus.Registerer) *HotPathMetrics { m := &HotPathMetrics{ leaseReadsTotal: prometheus.NewCounterVec( @@ -66,6 +77,13 @@ func newHotPathMetrics(registerer prometheus.Registerer) *HotPathMetrics { }, []string{"group"}, ), + luaFastPathTotal: prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "elastickv_lua_cmd_fastpath_total", + Help: "Per-redis.call() fast-path outcome inside Lua scripts. cmd identifies the command (zrangebyscore, zscore, ...); outcome is hit, skip_loaded, skip_cached_type, or fallback.", + }, + []string{"cmd", "outcome"}, + ), } registerer.MustRegister( @@ -73,10 +91,29 @@ func newHotPathMetrics(registerer prometheus.Registerer) *HotPathMetrics { m.dispatchDroppedTotal, m.dispatchErrorsTotal, m.stepQueueFullTotal, + m.luaFastPathTotal, ) return m } +// LuaFastPathObserver records a fast-path outcome for a single +// redis.call() inside a Lua script. The zero value is safe and +// silently drops samples so tests can pass LuaFastPathObserver{} as a +// stub. +type LuaFastPathObserver struct { + metrics *HotPathMetrics +} + +// ObserveLuaFastPath records (cmd, outcome). cmd should be the +// lowercase command name (e.g. "zrangebyscore"); outcome should be +// one of LuaFastPathOutcome*. +func (o LuaFastPathObserver) ObserveLuaFastPath(cmd, outcome string) { + if o.metrics == nil { + return + } + o.metrics.luaFastPathTotal.WithLabelValues(cmd, outcome).Inc() +} + // LeaseReadObserver implements kv.LeaseReadObserver by incrementing the // elastickv_lease_read_total counter vector. Callers grab an instance // via Registry.LeaseReadObserver(); the zero value is safe and silently diff --git a/monitoring/hotpath_test.go b/monitoring/hotpath_test.go index e59c3454..46a8dca4 100644 --- a/monitoring/hotpath_test.go +++ b/monitoring/hotpath_test.go @@ -30,6 +30,36 @@ elastickv_lease_read_total{node_address="10.0.0.1:50051",node_id="n1",outcome="m require.NoError(t, err) } +func TestLuaFastPathObserverCountsByCmdAndOutcome(t *testing.T) { + registry := NewRegistry("n1", "10.0.0.1:50051") + observer := registry.LuaFastPathObserver() + + observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeHit) + observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeHit) + observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeSkipAlreadyLoad) + observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeFallback) + + err := testutil.GatherAndCompare( + registry.Gatherer(), + strings.NewReader(` +# HELP elastickv_lua_cmd_fastpath_total Per-redis.call() fast-path outcome inside Lua scripts. cmd identifies the command (zrangebyscore, zscore, ...); outcome is hit, skip_loaded, skip_cached_type, or fallback. +# TYPE elastickv_lua_cmd_fastpath_total counter +elastickv_lua_cmd_fastpath_total{cmd="zrangebyscore",node_address="10.0.0.1:50051",node_id="n1",outcome="fallback"} 1 +elastickv_lua_cmd_fastpath_total{cmd="zrangebyscore",node_address="10.0.0.1:50051",node_id="n1",outcome="hit"} 2 +elastickv_lua_cmd_fastpath_total{cmd="zrangebyscore",node_address="10.0.0.1:50051",node_id="n1",outcome="skip_loaded"} 1 +`), + "elastickv_lua_cmd_fastpath_total", + ) + require.NoError(t, err) +} + +func TestLuaFastPathObserverZeroValueIsNoop(t *testing.T) { + var observer LuaFastPathObserver + require.NotPanics(t, func() { + observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeHit) + }) +} + func TestLeaseReadObserverZeroValueIsNoop(t *testing.T) { // LeaseReadObserver{} is documented as safe; the Coordinator // falls back to this when monitoring is disabled. Calling diff --git a/monitoring/registry.go b/monitoring/registry.go index bc58e20d..2145f979 100644 --- a/monitoring/registry.go +++ b/monitoring/registry.go @@ -112,6 +112,16 @@ func (r *Registry) LeaseReadObserver() LeaseReadObserver { return LeaseReadObserver{metrics: r.hotPath} } +// LuaFastPathObserver returns an observer for Lua-side redis.call() +// fast-path outcomes (hit / skip / fallback per command). Zero-value +// safe for tests and tools that do not wire a registry. +func (r *Registry) LuaFastPathObserver() LuaFastPathObserver { + if r == nil { + return LuaFastPathObserver{} + } + return LuaFastPathObserver{metrics: r.hotPath} +} + // DispatchCollector returns a collector that polls the etcd raft // engine's dispatch counters and exports them to Prometheus. Start it // with the node's raft sources after engine Open() completes. From 1cd79de4ee043dcf0a2f6706432164ebc33a097d Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Tue, 21 Apr 2026 22:24:54 +0900 Subject: [PATCH 2/4] perf(obs): pre-resolve lua fast-path counters off hot path Address gemini review on PR #571: CounterVec.WithLabelValues takes a mutex-guarded map lookup, which runs inside every redis.call() on the ZRANGEBYSCORE fast path. Resolve the four outcome counters once at server construction (WithLuaFastPathObserver) and call the pre-resolved handles from the Lua context so the hot path becomes a single atomic Inc per call. - LuaFastPathObserver.ForCommand(cmd) returns a LuaFastPathCmd bundle with pre-resolved Counter handles for hit / skip_loaded / skip_cached_type / fallback. - RedisServer stores the ZRANGEBYSCORE bundle and calls ObserveHit/etc. directly, no map lookups on the hot path. - Zero-value LuaFastPathCmd still safe: each ObserveXxx checks the handle and no-ops when nil (tests without a wired registry). --- adapter/redis.go | 15 ++++++++ adapter/redis_lua_context.go | 37 ++++++------------- monitoring/hotpath.go | 69 ++++++++++++++++++++++++++++++------ monitoring/hotpath_test.go | 17 +++++---- 4 files changed, 95 insertions(+), 43 deletions(-) diff --git a/adapter/redis.go b/adapter/redis.go index fc7f8f8e..ef709c08 100644 --- a/adapter/redis.go +++ b/adapter/redis.go @@ -266,6 +266,11 @@ type RedisServer struct { requestObserver monitoring.RedisRequestObserver luaObserver monitoring.LuaScriptObserver luaFastPathObserver monitoring.LuaFastPathObserver + // luaFastPathZRange is the pre-resolved counter bundle for the + // ZRANGEBYSCORE / ZREVRANGEBYSCORE Lua fast path. Resolved once in + // WithLuaFastPathObserver so the hot path does not pay for + // CounterVec.WithLabelValues on every redis.call(). + luaFastPathZRange monitoring.LuaFastPathCmd // baseCtx is the parent context for per-request handlers. // NewRedisServer creates a cancelable context here; Stop() cancels // it so in-flight handlers abort promptly instead of running @@ -324,12 +329,22 @@ func WithLuaObserver(observer monitoring.LuaScriptObserver) RedisServerOption { // WithLuaFastPathObserver enables per-redis.call() fast-path outcome // metrics inside Lua scripts. Used to diagnose fast-path hit ratios // for commands like ZRANGEBYSCORE / ZSCORE / HGET. +// +// Resolves per-command counter handles up front so the hot path +// avoids CounterVec.WithLabelValues on every redis.call(). func WithLuaFastPathObserver(observer monitoring.LuaFastPathObserver) RedisServerOption { return func(r *RedisServer) { r.luaFastPathObserver = observer + r.luaFastPathZRange = observer.ForCommand(luaFastPathCmdZRangeByScore) } } +// luaFastPathCmdZRangeByScore is the shared label for ZRANGEBYSCORE +// and ZREVRANGEBYSCORE fast-path outcomes. Both directions take the +// same branch through zsetRangeByScoreFast so sharing one label +// keeps the counter cardinality bounded. +const luaFastPathCmdZRangeByScore = "zrangebyscore" + // redisMetricsConn wraps a redcon.Conn to detect whether WriteError was called. type redisMetricsConn struct { redcon.Conn diff --git a/adapter/redis_lua_context.go b/adapter/redis_lua_context.go index d5f6f8d3..09db5898 100644 --- a/adapter/redis_lua_context.go +++ b/adapter/redis_lua_context.go @@ -10,7 +10,6 @@ import ( "time" "github.com/bootjp/elastickv/kv" - "github.com/bootjp/elastickv/monitoring" "github.com/bootjp/elastickv/store" "github.com/cockroachdb/errors" ) @@ -2425,12 +2424,18 @@ func (c *luaScriptContext) cmdZRangeByScore(args []string, reverse bool) (luaRep // Fast path eligibility: no script-local mutation / deletion / // type-change on this key. Mirrors the cmdZScore / cmdHGet guards // so in-script ZADD / ZREM / DEL / SET behave exactly as before. + // + // zrangeMetrics is a zero-value LuaFastPathCmd when no observer is + // wired (tests); Observe* methods on it are no-ops. When wired, + // each Observe* is a single atomic increment on a pre-resolved + // Counter (see WithLuaFastPathObserver). + zrangeMetrics := c.server.luaFastPathZRange if luaZSetAlreadyLoaded(c, key) { - c.observeFastPathOutcome(luaCmdZRangeByScore, monitoring.LuaFastPathOutcomeSkipAlreadyLoad) + zrangeMetrics.ObserveSkipAlreadyLoaded() return c.cmdZRangeByScoreSlow(key, options, reverse) } if _, cached := c.cachedType(key); cached { - c.observeFastPathOutcome(luaCmdZRangeByScore, monitoring.LuaFastPathOutcomeSkipCachedType) + zrangeMetrics.ObserveSkipCachedType() return c.cmdZRangeByScoreSlow(key, options, reverse) } entries, hit, fastErr := c.zrangeByScoreFastPath(key, options, reverse) @@ -2438,38 +2443,16 @@ func (c *luaScriptContext) cmdZRangeByScore(args []string, reverse bool) (luaRep return luaReply{}, fastErr } if !hit { - c.observeFastPathOutcome(luaCmdZRangeByScore, monitoring.LuaFastPathOutcomeFallback) + zrangeMetrics.ObserveFallback() return c.cmdZRangeByScoreSlow(key, options, reverse) } - c.observeFastPathOutcome(luaCmdZRangeByScore, monitoring.LuaFastPathOutcomeHit) + zrangeMetrics.ObserveHit() if len(entries) == 0 { return luaArrayReply(), nil } return zsetRangeReply(entries, options.withScores), nil } -// luaCmdZRangeByScore is the metric label for ZRANGEBYSCORE / -// ZREVRANGEBYSCORE fast-path outcomes. Both directions share one -// label because the fast-path implementation is identical except for -// scan direction. -const luaCmdZRangeByScore = "zrangebyscore" - -// observeFastPathOutcome records one fast-path decision via the -// server's observer. No-op when no observer is wired (tests, or a -// scriptCtx constructed without a RedisServer). -// -// cmd is accepted as a parameter (rather than hard-coded) so the -// next wave of fast-path commands (ZSCORE, HGET, HEXISTS, SISMEMBER) -// can plug in without widening the helper signature. -// -//nolint:unparam // cmd will gain additional values in follow-up commits. -func (c *luaScriptContext) observeFastPathOutcome(cmd, outcome string) { - if c.server == nil { - return - } - c.server.luaFastPathObserver.ObserveLuaFastPath(cmd, outcome) -} - // zrangeByScoreFastPath translates the caller's min/max bounds into a // bounded score-index scan through zsetRangeByScoreFast and returns // the decoded entries. hit=false means the server-side helper wants diff --git a/monitoring/hotpath.go b/monitoring/hotpath.go index 30e7cd25..495425be 100644 --- a/monitoring/hotpath.go +++ b/monitoring/hotpath.go @@ -96,22 +96,71 @@ func newHotPathMetrics(registerer prometheus.Registerer) *HotPathMetrics { return m } -// LuaFastPathObserver records a fast-path outcome for a single -// redis.call() inside a Lua script. The zero value is safe and -// silently drops samples so tests can pass LuaFastPathObserver{} as a -// stub. +// LuaFastPathObserver records fast-path outcomes for redis.call() +// inside Lua scripts. The zero value is safe and silently drops +// samples so tests can pass LuaFastPathObserver{} as a stub. +// +// Hot-path shape: each ObserveLuaFastPath-on-handle call is a single +// atomic increment on a pre-resolved prometheus.Counter. Callers +// resolve one LuaFastPathCmd per command at server construction to +// avoid CounterVec.WithLabelValues (mutex-guarded map lookup) on the +// hot path. type LuaFastPathObserver struct { metrics *HotPathMetrics } -// ObserveLuaFastPath records (cmd, outcome). cmd should be the -// lowercase command name (e.g. "zrangebyscore"); outcome should be -// one of LuaFastPathOutcome*. -func (o LuaFastPathObserver) ObserveLuaFastPath(cmd, outcome string) { +// LuaFastPathCmd is a pre-resolved bundle of fast-path outcome +// counters for a single Lua command. Construct once via +// LuaFastPathObserver.ForCommand(cmd) at server startup; call the +// Observe* methods per redis.call(). Safe to copy. +type LuaFastPathCmd struct { + hit prometheus.Counter + skipAlreadyLoad prometheus.Counter + skipCachedType prometheus.Counter + fallback prometheus.Counter +} + +// ForCommand pre-resolves the counter handles for cmd. Returns a +// zero-value LuaFastPathCmd when the observer is empty (tests), +// which silently drops all Observe* calls. +func (o LuaFastPathObserver) ForCommand(cmd string) LuaFastPathCmd { if o.metrics == nil { - return + return LuaFastPathCmd{} + } + vec := o.metrics.luaFastPathTotal + return LuaFastPathCmd{ + hit: vec.WithLabelValues(cmd, LuaFastPathOutcomeHit), + skipAlreadyLoad: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipAlreadyLoad), + skipCachedType: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipCachedType), + fallback: vec.WithLabelValues(cmd, LuaFastPathOutcomeFallback), + } +} + +// ObserveHit / ObserveSkipAlreadyLoaded / ObserveSkipCachedType / +// ObserveFallback record one outcome. Each is a single atomic +// increment when the counter is wired; a no-op on the zero value. +func (c LuaFastPathCmd) ObserveHit() { + if c.hit != nil { + c.hit.Inc() + } +} + +func (c LuaFastPathCmd) ObserveSkipAlreadyLoaded() { + if c.skipAlreadyLoad != nil { + c.skipAlreadyLoad.Inc() + } +} + +func (c LuaFastPathCmd) ObserveSkipCachedType() { + if c.skipCachedType != nil { + c.skipCachedType.Inc() + } +} + +func (c LuaFastPathCmd) ObserveFallback() { + if c.fallback != nil { + c.fallback.Inc() } - o.metrics.luaFastPathTotal.WithLabelValues(cmd, outcome).Inc() } // LeaseReadObserver implements kv.LeaseReadObserver by incrementing the diff --git a/monitoring/hotpath_test.go b/monitoring/hotpath_test.go index 46a8dca4..adb4514c 100644 --- a/monitoring/hotpath_test.go +++ b/monitoring/hotpath_test.go @@ -32,12 +32,12 @@ elastickv_lease_read_total{node_address="10.0.0.1:50051",node_id="n1",outcome="m func TestLuaFastPathObserverCountsByCmdAndOutcome(t *testing.T) { registry := NewRegistry("n1", "10.0.0.1:50051") - observer := registry.LuaFastPathObserver() + cmd := registry.LuaFastPathObserver().ForCommand("zrangebyscore") - observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeHit) - observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeHit) - observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeSkipAlreadyLoad) - observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeFallback) + cmd.ObserveHit() + cmd.ObserveHit() + cmd.ObserveSkipAlreadyLoaded() + cmd.ObserveFallback() err := testutil.GatherAndCompare( registry.Gatherer(), @@ -46,6 +46,7 @@ func TestLuaFastPathObserverCountsByCmdAndOutcome(t *testing.T) { # TYPE elastickv_lua_cmd_fastpath_total counter elastickv_lua_cmd_fastpath_total{cmd="zrangebyscore",node_address="10.0.0.1:50051",node_id="n1",outcome="fallback"} 1 elastickv_lua_cmd_fastpath_total{cmd="zrangebyscore",node_address="10.0.0.1:50051",node_id="n1",outcome="hit"} 2 +elastickv_lua_cmd_fastpath_total{cmd="zrangebyscore",node_address="10.0.0.1:50051",node_id="n1",outcome="skip_cached_type"} 0 elastickv_lua_cmd_fastpath_total{cmd="zrangebyscore",node_address="10.0.0.1:50051",node_id="n1",outcome="skip_loaded"} 1 `), "elastickv_lua_cmd_fastpath_total", @@ -55,8 +56,12 @@ elastickv_lua_cmd_fastpath_total{cmd="zrangebyscore",node_address="10.0.0.1:5005 func TestLuaFastPathObserverZeroValueIsNoop(t *testing.T) { var observer LuaFastPathObserver + cmd := observer.ForCommand("zrangebyscore") require.NotPanics(t, func() { - observer.ObserveLuaFastPath("zrangebyscore", LuaFastPathOutcomeHit) + cmd.ObserveHit() + cmd.ObserveSkipAlreadyLoaded() + cmd.ObserveSkipCachedType() + cmd.ObserveFallback() }) } From e75442eca8f23af87c12f3fa5ca98c66f5abb538 Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Tue, 21 Apr 2026 22:31:23 +0900 Subject: [PATCH 3/4] docs/naming: align skip_loaded across label, field, and method Address gemini review on PR #571: - Fix the doc comment on LuaFastPathObserver that referenced a non-existent "ObserveLuaFastPath-on-handle" method; describe the actual Observe* call shape and note the counters are non-blocking atomic Inc (client_golang's default Counter uses sync/atomic). - Rename the skipAlreadyLoad field, LuaFastPathOutcomeSkipAlreadyLoad constant, and ObserveSkipAlreadyLoaded method to skipLoaded / LuaFastPathOutcomeSkipLoaded / ObserveSkipLoaded so every layer agrees with the metric label value "skip_loaded". --- adapter/redis_lua_context.go | 2 +- monitoring/hotpath.go | 26 ++++++++++++++------------ monitoring/hotpath_test.go | 4 ++-- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/adapter/redis_lua_context.go b/adapter/redis_lua_context.go index 09db5898..8c423e54 100644 --- a/adapter/redis_lua_context.go +++ b/adapter/redis_lua_context.go @@ -2431,7 +2431,7 @@ func (c *luaScriptContext) cmdZRangeByScore(args []string, reverse bool) (luaRep // Counter (see WithLuaFastPathObserver). zrangeMetrics := c.server.luaFastPathZRange if luaZSetAlreadyLoaded(c, key) { - zrangeMetrics.ObserveSkipAlreadyLoaded() + zrangeMetrics.ObserveSkipLoaded() return c.cmdZRangeByScoreSlow(key, options, reverse) } if _, cached := c.cachedType(key); cached { diff --git a/monitoring/hotpath.go b/monitoring/hotpath.go index 495425be..c4cfdfee 100644 --- a/monitoring/hotpath.go +++ b/monitoring/hotpath.go @@ -42,7 +42,7 @@ type HotPathMetrics struct { // ZSCORE, HGET, etc.) actually takes the fast path vs falls back. const ( LuaFastPathOutcomeHit = "hit" - LuaFastPathOutcomeSkipAlreadyLoad = "skip_loaded" + LuaFastPathOutcomeSkipLoaded = "skip_loaded" LuaFastPathOutcomeSkipCachedType = "skip_cached_type" LuaFastPathOutcomeFallback = "fallback" ) @@ -100,11 +100,13 @@ func newHotPathMetrics(registerer prometheus.Registerer) *HotPathMetrics { // inside Lua scripts. The zero value is safe and silently drops // samples so tests can pass LuaFastPathObserver{} as a stub. // -// Hot-path shape: each ObserveLuaFastPath-on-handle call is a single -// atomic increment on a pre-resolved prometheus.Counter. Callers -// resolve one LuaFastPathCmd per command at server construction to -// avoid CounterVec.WithLabelValues (mutex-guarded map lookup) on the -// hot path. +// Hot-path shape: each Observe* call on a LuaFastPathCmd handle is a +// single non-blocking atomic increment on a pre-resolved +// prometheus.Counter (client_golang's default Counter uses +// sync/atomic internally). Callers resolve one LuaFastPathCmd per +// command at server construction to avoid +// CounterVec.WithLabelValues (mutex-guarded map lookup) on the hot +// path. type LuaFastPathObserver struct { metrics *HotPathMetrics } @@ -115,7 +117,7 @@ type LuaFastPathObserver struct { // Observe* methods per redis.call(). Safe to copy. type LuaFastPathCmd struct { hit prometheus.Counter - skipAlreadyLoad prometheus.Counter + skipLoaded prometheus.Counter skipCachedType prometheus.Counter fallback prometheus.Counter } @@ -130,13 +132,13 @@ func (o LuaFastPathObserver) ForCommand(cmd string) LuaFastPathCmd { vec := o.metrics.luaFastPathTotal return LuaFastPathCmd{ hit: vec.WithLabelValues(cmd, LuaFastPathOutcomeHit), - skipAlreadyLoad: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipAlreadyLoad), + skipLoaded: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipLoaded), skipCachedType: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipCachedType), fallback: vec.WithLabelValues(cmd, LuaFastPathOutcomeFallback), } } -// ObserveHit / ObserveSkipAlreadyLoaded / ObserveSkipCachedType / +// ObserveHit / ObserveSkipLoaded / ObserveSkipCachedType / // ObserveFallback record one outcome. Each is a single atomic // increment when the counter is wired; a no-op on the zero value. func (c LuaFastPathCmd) ObserveHit() { @@ -145,9 +147,9 @@ func (c LuaFastPathCmd) ObserveHit() { } } -func (c LuaFastPathCmd) ObserveSkipAlreadyLoaded() { - if c.skipAlreadyLoad != nil { - c.skipAlreadyLoad.Inc() +func (c LuaFastPathCmd) ObserveSkipLoaded() { + if c.skipLoaded != nil { + c.skipLoaded.Inc() } } diff --git a/monitoring/hotpath_test.go b/monitoring/hotpath_test.go index adb4514c..9b9d2969 100644 --- a/monitoring/hotpath_test.go +++ b/monitoring/hotpath_test.go @@ -36,7 +36,7 @@ func TestLuaFastPathObserverCountsByCmdAndOutcome(t *testing.T) { cmd.ObserveHit() cmd.ObserveHit() - cmd.ObserveSkipAlreadyLoaded() + cmd.ObserveSkipLoaded() cmd.ObserveFallback() err := testutil.GatherAndCompare( @@ -59,7 +59,7 @@ func TestLuaFastPathObserverZeroValueIsNoop(t *testing.T) { cmd := observer.ForCommand("zrangebyscore") require.NotPanics(t, func() { cmd.ObserveHit() - cmd.ObserveSkipAlreadyLoaded() + cmd.ObserveSkipLoaded() cmd.ObserveSkipCachedType() cmd.ObserveFallback() }) From 138cb736f0cb66c35f33f4bafe5798f216cf8ac6 Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Tue, 21 Apr 2026 22:59:43 +0900 Subject: [PATCH 4/4] style: realign gofmt columns after skip_loaded rename --- monitoring/hotpath.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/monitoring/hotpath.go b/monitoring/hotpath.go index c4cfdfee..61ec8fe6 100644 --- a/monitoring/hotpath.go +++ b/monitoring/hotpath.go @@ -41,10 +41,10 @@ type HotPathMetrics struct { // so operators can see how often a given command (ZRANGEBYSCORE, // ZSCORE, HGET, etc.) actually takes the fast path vs falls back. const ( - LuaFastPathOutcomeHit = "hit" - LuaFastPathOutcomeSkipLoaded = "skip_loaded" - LuaFastPathOutcomeSkipCachedType = "skip_cached_type" - LuaFastPathOutcomeFallback = "fallback" + LuaFastPathOutcomeHit = "hit" + LuaFastPathOutcomeSkipLoaded = "skip_loaded" + LuaFastPathOutcomeSkipCachedType = "skip_cached_type" + LuaFastPathOutcomeFallback = "fallback" ) func newHotPathMetrics(registerer prometheus.Registerer) *HotPathMetrics { @@ -116,10 +116,10 @@ type LuaFastPathObserver struct { // LuaFastPathObserver.ForCommand(cmd) at server startup; call the // Observe* methods per redis.call(). Safe to copy. type LuaFastPathCmd struct { - hit prometheus.Counter - skipLoaded prometheus.Counter - skipCachedType prometheus.Counter - fallback prometheus.Counter + hit prometheus.Counter + skipLoaded prometheus.Counter + skipCachedType prometheus.Counter + fallback prometheus.Counter } // ForCommand pre-resolves the counter handles for cmd. Returns a @@ -131,10 +131,10 @@ func (o LuaFastPathObserver) ForCommand(cmd string) LuaFastPathCmd { } vec := o.metrics.luaFastPathTotal return LuaFastPathCmd{ - hit: vec.WithLabelValues(cmd, LuaFastPathOutcomeHit), - skipLoaded: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipLoaded), - skipCachedType: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipCachedType), - fallback: vec.WithLabelValues(cmd, LuaFastPathOutcomeFallback), + hit: vec.WithLabelValues(cmd, LuaFastPathOutcomeHit), + skipLoaded: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipLoaded), + skipCachedType: vec.WithLabelValues(cmd, LuaFastPathOutcomeSkipCachedType), + fallback: vec.WithLabelValues(cmd, LuaFastPathOutcomeFallback), } }