From 608d4491d5582da635f8cb8a28a8aedc968304a2 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Wed, 23 Sep 2026 09:28:00 +0800 Subject: [PATCH 1/9] fix(keys): reclaim expired entries in batches remove_expired_keys() reclaims the expired shared dict entries with an unbounded flush_expired() call (#18). That call holds the dict lock until it returns, walking the whole LRU queue, so an hour's worth of backlog is reclaimed in a single uninterrupted hold and every worker repeats the walk. Issue the reclamation in batches of 10000 instead, pausing in between, so the other workers get the lock back: a batch is ~1ms of reclaim work on OpenResty 1.29.2.4, against ~70ms for an unbounded call over a 750k-entry backlog. A call that frees less than a batch has already walked the whole queue, so the loop stops there. Expose it as KeyIndex:flush_expired() / prometheus:flush_expired() and add the auto_flush_expired option, so callers can reclaim from a single process on their own schedule instead of from every worker. The default is unchanged. --- CHANGELOG.md | 7 +++++ README.md | 29 ++++++++++++++++++ prometheus.lua | 19 +++++++++++- prometheus_keys.lua | 75 ++++++++++++++++++++++++++++++++++++--------- prometheus_test.lua | 57 ++++++++++++++++++++++++++++++++++ 5 files changed, 172 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a467929..21f35ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ This file only calls out major changes. Please see [the list of Git commits]( https://github.com/api7/nginx-lua-prometheus/commits/main) for the full list of changes. +## Unreleased + +- Reclaim expired shared-dict entries in batches, so no single + `flush_expired()` call holds the dict lock for a whole backlog, and let + callers take the reclamation over with the `auto_flush_expired` option and + `prometheus:flush_expired()` (#23). + ## 1.0.0 Starting with this release, this fork (`nginx-lua-prometheus-api7`) uses diff --git a/README.md b/README.md index ff28be6..c57b407 100644 --- a/README.md +++ b/README.md @@ -112,6 +112,13 @@ section of nginx configuration. RAM, you might want to increase this to avoid cache getting flushed too often. Decreasing this makes sense if you have a very large number of metrics or need to minimize memory usage of this library. + * `remove_expired_keys_interval` (number): how often, in seconds, each worker + drops the metrics that have expired and reclaims the shared dict entries + they left behind. Defaults to 3600, which is also the maximum. + * `auto_flush_expired` (boolean): whether that reclamation also happens on + the interval above, in every worker. Defaults to true. Set it to false to + schedule [`prometheus:flush_expired()`](#prometheusflush_expired) yourself, + for instance from a single process. Returns a `prometheus` object that should be used to register metrics. @@ -236,6 +243,28 @@ location /metrics { Returns metric data as an array of strings. +### prometheus:flush_expired() + +**syntax:** prometheus:flush_expired() + +Reclaims the shared dict entries left behind by metrics that have expired, and +returns how many were reclaimed. + +Expired entries are only logically gone: every shared dict API reports them as +missing, but they keep holding their memory until something reclaims them. The +passive reclamation nginx performs on writes cannot do it, because it stops at +the first entry that has not expired, and a metric registered without an +`exptime` inevitably ends up sitting there. + +This is done for you on `remove_expired_keys_interval`, in every worker, unless +`auto_flush_expired` is set to false. Call this instead if you would rather +reclaim from a single process, or on your own schedule. + +The reclamation is issued in batches, because the underlying +`ngx.shared.DICT:flush_expired()` holds the dict lock until it returns: a +bounded number of entries is reclaimed per call, and the other workers get the +lock back in between. + ### counter:inc() **syntax:** counter:inc(*value*, *label_values*) diff --git a/prometheus.lua b/prometheus.lua index 8556b8d..7641ef9 100644 --- a/prometheus.lua +++ b/prometheus.lua @@ -746,16 +746,19 @@ function Prometheus.init(dict_name, options_or_prefix) and options_or_prefix.remove_expired_keys_interval < MAX_REMOVE_EXPIRED_KEYS_INTERVAL and options_or_prefix.remove_expired_keys_interval or MAX_REMOVE_EXPIRED_KEYS_INTERVAL + self.auto_flush_expired = options_or_prefix.auto_flush_expired ~= false else self.prefix = options_or_prefix or '' self.error_metric_name = DEFAULT_ERROR_METRIC_NAME self.sync_interval = DEFAULT_SYNC_INTERVAL self.lookup_max_size = DEFAULT_LOOKUP_MAX_SIZE self.remove_expired_keys_interval = MAX_REMOVE_EXPIRED_KEYS_INTERVAL + self.auto_flush_expired = true end self.registry = {} - self.key_index = key_index_lib.new(self.dict, KEY_INDEX_PREFIX, self.remove_expired_keys_interval) + self.key_index = key_index_lib.new(self.dict, KEY_INDEX_PREFIX, + self.remove_expired_keys_interval, self.auto_flush_expired) self.initialized = true @@ -930,6 +933,20 @@ end -- Returns: -- Array of strings with all metrics in a text format compatible with -- Prometheus. +-- Reclaims the expired entries of the metrics shared dict, in batches, and +-- returns how many were reclaimed. +-- +-- Only needed by callers that set auto_flush_expired to false and schedule the +-- reclamation themselves, in a single process instead of in every worker. +function Prometheus:flush_expired() + if not self.initialized then + ngx.log(ngx.ERR, "Prometheus module has not been initialized") + return 0 + end + + return self.key_index:flush_expired() +end + function Prometheus:metric_data() if not self.initialized then ngx.log(ngx.ERR, "Prometheus module has not been initialized") diff --git a/prometheus_keys.lua b/prometheus_keys.lua index faeaa66..ee56683 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -14,6 +14,20 @@ KeyIndex.__index = KeyIndex -- and the index converges even when far more slots need repairing. local MAX_KEY_COUNT_REPAIRS = 1000 +-- Entries a single flush_expired() call may reclaim. That call holds the shared +-- dict lock until it returns, so this is what bounds how long the other workers +-- can be kept waiting on the lock: measured at ~1ms per 10000 entries reclaimed +-- on OpenResty 1.29.2.4. +local FLUSH_EXPIRED_BATCH = 10000 + +-- Upper bound on the batches one call may run, so a large backlog is drained +-- over several ticks instead of in a single long stretch. What is left over is +-- picked up by the next tick. +local FLUSH_EXPIRED_MAX_BATCHES = 30 + +-- Pause between batches, so the lock is not taken back to back. +local FLUSH_EXPIRED_BATCH_DELAY = 1 + -- check and remove expired keys local function remove_expired_keys(_, self) @@ -21,9 +35,11 @@ local function remove_expired_keys(_, self) end -function KeyIndex.new(shared_dict, prefix, remove_expired_keys_interval) +function KeyIndex.new(shared_dict, prefix, remove_expired_keys_interval, + auto_flush_expired) local self = setmetatable({}, KeyIndex) self.dict = shared_dict + self.auto_flush_expired = auto_flush_expired ~= false self.key_prefix = prefix .. "key_" self.delete_count = prefix .. "delete_count" self.key_count = prefix .. "key_count" @@ -52,19 +68,50 @@ function KeyIndex:remove_expired_keys() end end - -- The loop above only drops worker-local references. The expired shared-dict - -- entries themselves (both the __ngx_prom__key_N index slots and the metric - -- value keys, which live in the same dict) are only *logically* dead: every - -- dict API treats them as missing, but their slab pages stay allocated. The - -- passive per-write expiry scan cannot reclaim them either, because it stops - -- at the first non-expired entry at the LRU tail, and a permanent entry (the - -- error metric, or any metric registered without an exptime) inevitably ends - -- up sitting there. Without this call the dict grows without bound under - -- label churn: index slots are never reused, so free_space steps down on - -- every new series and never recovers (apache/apisix#13658). Since expired - -- entries are indistinguishable from absent ones through every dict API, - -- reclaiming them here cannot change any observable behaviour. - self.dict:flush_expired() + -- The loop above only drops worker-local references, so the expired entries + -- still have to be reclaimed from the dict itself. Callers that schedule + -- flush_expired() themselves -- in a single process rather than in every + -- worker -- turn this off with auto_flush_expired. + if self.auto_flush_expired then + self:flush_expired() + end +end + + +-- Reclaims the expired entries of the shared dict, in batches. +-- +-- The expired entries (both the __ngx_prom__key_N index slots and the metric +-- value keys, which live in the same dict) are only *logically* dead: every +-- dict API treats them as missing, but their slab pages stay allocated. The +-- passive per-write expiry scan cannot reclaim them either, because it stops +-- at the first non-expired entry at the LRU tail, and a permanent entry (the +-- error metric, or any metric registered without an exptime) inevitably ends +-- up sitting there. Without this the dict grows without bound under label +-- churn: index slots are never reused, so free_space steps down on every new +-- series and never recovers (apache/apisix#13658). Since expired entries are +-- indistinguishable from absent ones through every dict API, reclaiming them +-- cannot change any observable behaviour. +-- +-- flush_expired() holds the dict lock for its whole scan of the LRU queue, so +-- it is called with a batch size: a bounded number of entries is reclaimed per +-- call, and the workers get the lock back in between. +-- +-- Returns the number of entries reclaimed. +function KeyIndex:flush_expired() + local total = 0 + for _ = 1, FLUSH_EXPIRED_MAX_BATCHES do + local freed = self.dict:flush_expired(FLUSH_EXPIRED_BATCH) + total = total + freed + -- freeing less than a full batch means this call has already walked the + -- whole queue, so there is nothing left to reclaim + if freed < FLUSH_EXPIRED_BATCH then + break + end + + ngx.sleep(FLUSH_EXPIRED_BATCH_DELAY) + end + + return total end -- Loads new keys that might have been added by other workers since last sync. diff --git a/prometheus_test.lua b/prometheus_test.lua index b485b0a..0bfe05d 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -920,6 +920,63 @@ function TestKeyIndex:testRemoveExpiredKeysReclaimsSharedDict() luaunit.assertNotNil(self.dict.dict["permanent"]) end +-- flush_expired() holds the dict lock for its whole scan of the LRU queue, so +-- the reclamation is issued in bounded batches. A backlog larger than one batch +-- must still be reclaimed in full, by looping. +function TestKeyIndex:testFlushExpiredRunsInBatches() + local expired = 20000 + for i = 1, expired do + self.dict:set("value_" .. i, 1, 1) + end + -- A permanent entry, like the error metric or any metric without an exptime. + self.dict:set("permanent", 1) + + sleep(2) + + -- record what each call asks the dict to reclaim: an unbounded call would + -- hold the dict lock for the whole backlog at once + local batches = {} + local flush_expired = SimpleDict.flush_expired + self.dict.flush_expired = function(dict, n) + batches[#batches + 1] = n + return flush_expired(dict, n) + end + + -- a batch reclaims 10000, so reaching all of them takes more than one call + luaunit.assertEquals(self.key_index:flush_expired(), expired) + luaunit.assertEquals(batches, {10000, 10000, 10000}) + + -- nothing left: the loop must stop after the call that comes back short + batches = {} + luaunit.assertEquals(self.key_index:flush_expired(), 0) + luaunit.assertEquals(batches, {10000}) + + self.dict.flush_expired = nil + luaunit.assertNotNil(self.dict.dict["permanent"]) +end + +-- Callers that schedule the reclamation themselves, in a single process rather +-- than in every worker, turn the automatic flush off. remove_expired_keys() +-- must then only drop the worker-local references. +function TestKeyIndex:testAutoFlushExpiredDisabled() + local key_index = require('prometheus_keys').new(self.dict, "_noflush_", 1, false) + luaunit.assertEquals(key_index:add("expkey", "eviction_err", 1), nil) + -- A metric value key: lives in the same dict, and KeyIndex never reads it. + self.dict:set("expkey", 1, 1) + + sleep(2) + key_index:remove_expired_keys() + + luaunit.assertNil(key_index.index["expkey"]) + -- both entries are still physically present, unlike with the default + luaunit.assertNotNil(self.dict.dict["_noflush_key_1"]) + luaunit.assertNotNil(self.dict.dict["expkey"]) + + luaunit.assertEquals(key_index:flush_expired(), 2) + luaunit.assertNil(self.dict.dict["_noflush_key_1"]) + luaunit.assertNil(self.dict.dict["expkey"]) +end + -- The index slots of a churning metric must not accumulate: each round expires -- the previous slot, and the reclaim must keep the dict bounded rather than -- letting every new series step free space down for good. From 1da18a90993e69b484de7572d3434ef3c90df6d5 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Sat, 10 Oct 2026 09:15:25 +0800 Subject: [PATCH 2/9] fix(keys): size the reclaim batch for total lock time, and guard the round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The batch was set as if it bounded one lock hold. It does not. flush_expired(n) walks the LRU queue from its tail and only stops early once n entries have been freed, so one call costs walk_cost * entries in the dict + free_cost * entries freed and only the second term is bounded by n (~7.5ns and ~90-148ns per entry on OpenResty 1.29.2.4). Every call pays the walk again, so a small batch barely shortens a single hold while multiplying the total time the lock is held and the number of requests waiting on it. Measured with 8 workers at 20k rps, one fixed backlog, flush issued from another worker: | dict / backlog | n | calls | one hold | total hold | p99 | p99.9 | |------------------|--------|-------|----------|------------|---------|---------| | 600k / 200k | none | — | — | — | 2.02ms | 2.30ms | | 600k / 200k | 10000 | 21 | 20.4ms | 92.6ms | 2.02ms | 69.6ms | | 600k / 200k | 50000 | 5 | 24.7ms | 45.3ms | 2.08ms | 22.1ms | | 600k / 200k | 0 | 1 | 35.6ms | 35.6ms | 2.04ms | 12.6ms | | 3M / 1.5M | none | — | — | — | 2.03ms | 2.33ms | | 3M / 1.5M | 10000 | 151 | 27.8ms | 1771ms | 1.53s | 1.75s | | 3M / 1.5M | 50000 | 31 | 32.7ms | 480ms | 240ms | 457ms | | 3M / 1.5M | 0 | 1 | 151.1ms | 151.1ms | 2.19ms | 128ms | Going from 10000 to 50000 adds 4-5ms to one hold and halves everything else, because one hold is dominated by the walk, which the batch cannot reduce. So the batch is now the largest value whose own freeing cost still keeps a hold short: a normal round -- the expiry rate times the reclaim interval -- drains in a single call, and the batching only comes into play for a backlog that is not normal, which is also why the cap on batches per round can come down to 10. The round is now guarded as well. It yields between batches, and ngx.timer.every does not wait for the previous run, so rounds could overlap and race for the dict lock; the second one now steps aside. The loop runs under pcall so a throwing ngx.sleep -- a caller reaching flush_expired() from a phase that does not allow yielding -- cannot leave the guard set and stop the reclamation for the life of the worker. remove_expired_keys_interval gains a minimum of 60s for the same reason: a round may occupy its timer for FLUSH_EXPIRED_MAX_BATCHES * BATCH_DELAY seconds and is meant to be a small fraction of the interval. A shorter interval is raised to the minimum with a warning rather than left to drop rounds silently. The three constants are exposed on the module so a caller scheduling its own reclamation can reason about the drain rate and about how long a round can take. Tests: testFlushExpiredRunsInBatches no longer hard-codes the batch size; testFlushExpiredDoesNotOverlap covers the guard (fails without it: the re-entrant round returns 10 instead of 0), testFlushExpiredReleasesTheGuard- AfterAnError covers the pcall (without it the error escapes the suite), and testInitClampsTheRemoveExpiredKeysInterval covers both interval bounds. --- prometheus.lua | 20 +++++++++-- prometheus_keys.lua | 80 ++++++++++++++++++++++++++++++++++------- prometheus_test.lua | 86 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 164 insertions(+), 22 deletions(-) diff --git a/prometheus.lua b/prometheus.lua index 7641ef9..8ef8f61 100644 --- a/prometheus.lua +++ b/prometheus.lua @@ -95,6 +95,14 @@ local DEFAULT_SYNC_INTERVAL = 1 -- Max value for check and remove expired keys interval (seconds). local MAX_REMOVE_EXPIRED_KEYS_INTERVAL = 3600 +-- Shortest reclaim interval accepted. A round reclaims in batches with a pause +-- in between, so it can occupy its timer for several seconds (see +-- FLUSH_EXPIRED_MAX_BATCHES and FLUSH_EXPIRED_BATCH_DELAY in +-- prometheus_keys.lua); a round is meant to be a small fraction of the +-- interval, not most of it. Overlapping rounds are refused rather than run, so +-- a shorter interval would only drop rounds silently. +local MIN_REMOVE_EXPIRED_KEYS_INTERVAL = 60 + -- Default max size of lookup table local DEFAULT_LOOKUP_MAX_SIZE = 1000 @@ -742,9 +750,15 @@ function Prometheus.init(dict_name, options_or_prefix) DEFAULT_SYNC_INTERVAL self.lookup_max_size = options_or_prefix.lookup_max_size or DEFAULT_LOOKUP_MAX_SIZE - self.remove_expired_keys_interval = options_or_prefix.remove_expired_keys_interval - and options_or_prefix.remove_expired_keys_interval < MAX_REMOVE_EXPIRED_KEYS_INTERVAL - and options_or_prefix.remove_expired_keys_interval + local interval = options_or_prefix.remove_expired_keys_interval + if interval and interval < MIN_REMOVE_EXPIRED_KEYS_INTERVAL then + ngx.log(ngx.WARN, "remove_expired_keys_interval of ", interval, + "s is shorter than the minimum of ", + MIN_REMOVE_EXPIRED_KEYS_INTERVAL, "s, using the minimum") + interval = MIN_REMOVE_EXPIRED_KEYS_INTERVAL + end + self.remove_expired_keys_interval = + interval and interval < MAX_REMOVE_EXPIRED_KEYS_INTERVAL and interval or MAX_REMOVE_EXPIRED_KEYS_INTERVAL self.auto_flush_expired = options_or_prefix.auto_flush_expired ~= false else diff --git a/prometheus_keys.lua b/prometheus_keys.lua index ee56683..9253790 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -14,21 +14,47 @@ KeyIndex.__index = KeyIndex -- and the index converges even when far more slots need repairing. local MAX_KEY_COUNT_REPAIRS = 1000 --- Entries a single flush_expired() call may reclaim. That call holds the shared --- dict lock until it returns, so this is what bounds how long the other workers --- can be kept waiting on the lock: measured at ~1ms per 10000 entries reclaimed --- on OpenResty 1.29.2.4. -local FLUSH_EXPIRED_BATCH = 10000 - --- Upper bound on the batches one call may run, so a large backlog is drained --- over several ticks instead of in a single long stretch. What is left over is --- picked up by the next tick. -local FLUSH_EXPIRED_MAX_BATCHES = 30 - --- Pause between batches, so the lock is not taken back to back. +-- Entries a single flush_expired() call may reclaim. +-- +-- flush_expired(n) walks the LRU queue from its tail and only stops early once +-- n entries have been freed, so one call costs +-- +-- walk_cost * entries in the dict + free_cost * entries freed +-- +-- and only the second term is bounded by n: every call pays the walk again +-- (measured on OpenResty 1.29.2.4: ~7.5ns and ~90-148ns per entry). A small +-- batch therefore barely shortens a single hold -- it multiplies the walk, and +-- with it the total time the lock is held and the number of requests that wait +-- on it. Reclaiming a 1.5M backlog out of 3M entries took 151 calls and 1771ms +-- of lock time at n=10000 and pushed the request p99 to 1.53s, against a single +-- 151ms hold and a p99 of 2.19ms unbounded. +-- +-- So n is the largest value whose own freeing cost still keeps one hold short +-- (~5ms here). A normal round -- the expiry rate times the reclaim interval -- +-- is drained in one call, and the batching only comes into play for a backlog +-- that is not. +local FLUSH_EXPIRED_BATCH = 50000 + +-- Upper bound on the batches one round may run, so a backlog that is not normal +-- -- inherited across an upgrade, or left by a dict that filled up with expired +-- entries -- is drained over several rounds instead of one long stretch. What is +-- left over is picked up by the next round. +local FLUSH_EXPIRED_MAX_BATCHES = 10 + +-- Pause between batches, so the lock is not taken back to back. It also bounds +-- how long one round can occupy its timer: FLUSH_EXPIRED_MAX_BATCHES times this. local FLUSH_EXPIRED_BATCH_DELAY = 1 +-- Exposed so a caller that schedules the reclamation itself can reason about it: +-- one round drains at most FLUSH_EXPIRED_BATCH * FLUSH_EXPIRED_MAX_BATCHES +-- entries and may occupy its timer for FLUSH_EXPIRED_MAX_BATCHES * +-- FLUSH_EXPIRED_BATCH_DELAY seconds. +KeyIndex.FLUSH_EXPIRED_BATCH = FLUSH_EXPIRED_BATCH +KeyIndex.FLUSH_EXPIRED_MAX_BATCHES = FLUSH_EXPIRED_MAX_BATCHES +KeyIndex.FLUSH_EXPIRED_BATCH_DELAY = FLUSH_EXPIRED_BATCH_DELAY + + -- check and remove expired keys local function remove_expired_keys(_, self) self:remove_expired_keys() @@ -97,7 +123,9 @@ end -- call, and the workers get the lock back in between. -- -- Returns the number of entries reclaimed. -function KeyIndex:flush_expired() +-- +-- The batch loop itself, so that the public entry point below can guard it. +local function flush_expired_batches(self) local total = 0 for _ = 1, FLUSH_EXPIRED_MAX_BATCHES do local freed = self.dict:flush_expired(FLUSH_EXPIRED_BATCH) @@ -114,6 +142,32 @@ function KeyIndex:flush_expired() return total end + +function KeyIndex:flush_expired() + -- A round yields between batches, so the timer can fire again before the + -- previous run finished (ngx.timer.every does not wait for it), and a caller + -- scheduling its own round can overlap with it too. Two rounds racing for the + -- dict lock double the stall and reclaim nothing extra, so the second one + -- steps aside. + if self.flushing then + return 0 + end + + self.flushing = true + -- pcall so that a throwing ngx.sleep -- a caller reaching this from a phase + -- that does not allow yielding -- cannot leave the guard set and stop the + -- reclamation for the life of the worker. + local ok, total = pcall(flush_expired_batches, self) + self.flushing = false + + if not ok then + ngx.log(ngx.ERR, "failed to reclaim expired entries: ", tostring(total)) + return 0 + end + + return total +end + -- Loads new keys that might have been added by other workers since last sync. function KeyIndex:sync() local delete_count = self.dict:get(self.delete_count) or 0 diff --git a/prometheus_test.lua b/prometheus_test.lua index 0bfe05d..d2e43d3 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -206,11 +206,30 @@ function TestPrometheus:testInitOptions() assert(p4.error_metric_name == "foobar") assert(p4.remove_expired_keys_interval ~= 0 and p4.remove_expired_keys_interval ~= "") - local p5 = require('prometheus').init("metrics", {remove_expired_keys_interval=3}) - assert(p5.remove_expired_keys_interval == 3) + local p5 = require('prometheus').init("metrics", {remove_expired_keys_interval=600}) + assert(p5.remove_expired_keys_interval == 600) luaunit.assertEquals(ngx.logs, nil) end + +-- A reclaim round reclaims in batches with a pause in between, so it can occupy +-- its timer for several seconds. An interval shorter than that is raised to the +-- minimum, with a warning, rather than left to drop rounds silently. +function TestPrometheus:testInitClampsTheRemoveExpiredKeysInterval() + self.dict = setmetatable({}, SimpleDict) + ngx.shared.metrics = self.dict + + local p1 = require('prometheus').init("metrics", {remove_expired_keys_interval=3}) + assert(p1.remove_expired_keys_interval == 60) + luaunit.assertEquals(#ngx.logs, 1) + luaunit.assertStrContains(ngx.logs[1], "remove_expired_keys_interval") + ngx.logs = nil + + -- the upper bound was already clamped, and silently + local p2 = require('prometheus').init("metrics", {remove_expired_keys_interval=7200}) + assert(p2.remove_expired_keys_interval == 3600) + luaunit.assertEquals(ngx.logs, nil) +end function TestPrometheus:testInitWorker() self.dict = setmetatable({}, SimpleDict) ngx.shared.metrics = self.dict @@ -773,6 +792,9 @@ function TestKeyIndex:setUp() end function TestKeyIndex.tearDown() ngx.logs = nil + -- the flush tests stub ngx.sleep; drop the stub here too, so a failing + -- assertion before their own restore cannot leak it into the next test + ngx.sleep = nil end function TestKeyIndex.testInit() luaunit.assertEquals(ngx.logs, nil) @@ -924,7 +946,8 @@ end -- the reclamation is issued in bounded batches. A backlog larger than one batch -- must still be reclaimed in full, by looping. function TestKeyIndex:testFlushExpiredRunsInBatches() - local expired = 20000 + local batch = self.key_index.FLUSH_EXPIRED_BATCH + local expired = batch + 10 for i = 1, expired do self.dict:set("value_" .. i, 1, 1) end @@ -942,19 +965,70 @@ function TestKeyIndex:testFlushExpiredRunsInBatches() return flush_expired(dict, n) end - -- a batch reclaims 10000, so reaching all of them takes more than one call + -- the backlog is one entry past a batch, so reaching all of it takes a + -- second call, which then comes back short luaunit.assertEquals(self.key_index:flush_expired(), expired) - luaunit.assertEquals(batches, {10000, 10000, 10000}) + luaunit.assertEquals(batches, {batch, batch}) -- nothing left: the loop must stop after the call that comes back short batches = {} luaunit.assertEquals(self.key_index:flush_expired(), 0) - luaunit.assertEquals(batches, {10000}) + luaunit.assertEquals(batches, {batch}) self.dict.flush_expired = nil luaunit.assertNotNil(self.dict.dict["permanent"]) end +-- One round yields between batches, so the timer can fire again before it +-- finishes -- ngx.timer.every does not wait for the previous run. The second +-- round must step aside instead of racing this one for the dict lock. +function TestKeyIndex:testFlushExpiredDoesNotOverlap() + local batch = self.key_index.FLUSH_EXPIRED_BATCH + for i = 1, batch + 10 do + self.dict:set("value_" .. i, 1, 1) + end + + sleep(2) + + local reentered + ngx.sleep = function() + if reentered == nil then + reentered = self.key_index:flush_expired() + end + end + local total = self.key_index:flush_expired() + ngx.sleep = nil + + luaunit.assertEquals(reentered, 0) + -- the round that was already running still reclaims everything + luaunit.assertEquals(total, batch + 10) + luaunit.assertEquals(ngx.logs, nil) +end + +-- A throwing ngx.sleep -- a caller reaching flush_expired() from a phase that +-- does not allow yielding -- must not leave the guard set, or the worker would +-- stop reclaiming for good. +function TestKeyIndex:testFlushExpiredReleasesTheGuardAfterAnError() + local batch = self.key_index.FLUSH_EXPIRED_BATCH + for i = 1, batch + 10 do + self.dict:set("value_" .. i, 1, 1) + end + + sleep(2) + + ngx.sleep = function() error("API disabled in the current context") end + luaunit.assertEquals(self.key_index:flush_expired(), 0) + ngx.sleep = nil + + luaunit.assertEquals(#ngx.logs, 1) + luaunit.assertStrContains(ngx.logs[1], "failed to reclaim expired entries") + ngx.logs = nil + + -- the guard is released, so what the failed round did not get to is still + -- reclaimable + luaunit.assertEquals(self.key_index:flush_expired(), 10) +end + -- Callers that schedule the reclamation themselves, in a single process rather -- than in every worker, turn the automatic flush off. remove_expired_keys() -- must then only drop the worker-local references. From d15f3cf8c35311014491c7940cc1ccc1952cb751 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Sat, 10 Oct 2026 09:16:37 +0800 Subject: [PATCH 3/9] docs: document the batch sizing, the round guard and the interval minimum --- CHANGELOG.md | 7 ++++--- README.md | 18 ++++++++++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21f35ea..35945be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,10 @@ of changes. ## Unreleased - Reclaim expired shared-dict entries in batches, so no single - `flush_expired()` call holds the dict lock for a whole backlog, and let - callers take the reclamation over with the `auto_flush_expired` option and - `prometheus:flush_expired()` (#23). + `flush_expired()` call holds the dict lock for a whole backlog, refuse + overlapping reclamation rounds, require `remove_expired_keys_interval` to be + at least 60 seconds, and let callers take the reclamation over with the + `auto_flush_expired` option and `prometheus:flush_expired()` (#23). ## 1.0.0 diff --git a/README.md b/README.md index c57b407..e9cad7d 100644 --- a/README.md +++ b/README.md @@ -114,7 +114,12 @@ section of nginx configuration. metrics or need to minimize memory usage of this library. * `remove_expired_keys_interval` (number): how often, in seconds, each worker drops the metrics that have expired and reclaims the shared dict entries - they left behind. Defaults to 3600, which is also the maximum. + they left behind. Defaults to 3600, which is also the maximum; values below + the minimum of 60 are raised to it, with a warning, because one round of + reclamation can occupy its timer for several seconds and is meant to be a + small fraction of the interval. Pick it from how much of the dict you are + willing to let expired entries hold: they accumulate at the expiry rate + times this interval. * `auto_flush_expired` (boolean): whether that reclamation also happens on the interval above, in every worker. Defaults to true. Set it to false to schedule [`prometheus:flush_expired()`](#prometheusflush_expired) yourself, @@ -263,7 +268,16 @@ reclaim from a single process, or on your own schedule. The reclamation is issued in batches, because the underlying `ngx.shared.DICT:flush_expired()` holds the dict lock until it returns: a bounded number of entries is reclaimed per call, and the other workers get the -lock back in between. +lock back in between. Note that the batch bounds only the freeing, not the +walk: that call scans the whole LRU queue and stops early only once the batch +has been freed, so every call pays for the scan again. The batch is therefore +sized so that a normal round -- the expiry rate times +`remove_expired_keys_interval` -- is drained in a single call, and a backlog +larger than one round is spread over several, at most 500000 entries and 10 +seconds per round. What is left over is picked up by the next round. + +Overlapping rounds are refused: a call that arrives while one is still running +reclaims nothing and returns 0. ### counter:inc() From 347099f881e60dca3459ac2e2bb2eb080b889612 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Sat, 10 Oct 2026 09:40:49 +0800 Subject: [PATCH 4/9] fix: default the reclaim interval to 60s, and drop its upper bound The interval has one observable consequence: how much backlog a round has to reclaim, which is the expiry rate times the interval times two (a series leaves a value key and an index slot behind). Both bounds on the interval come from that, and both were measured on a 128m dict -- the APISIX default -- which holds 521,184 entries of a typical shape. The hard bound is the dict itself. Once the backlog no longer fits beside the live entries, the writes that create new series force-evict live ones, and the dead entries win because they were written last. With 100,000 live entries: backlog written entries live entries left after reclaiming 120,000 220,000 100,000 350,000 450,000 100,000 450,000 521,184 71,184 -- 28,816 live metrics destroyed 700,000 521,184 0 -- all of them At 3600s that bound is reached by an expiry rate of ~44 series/s, which is ordinary for a gateway with an exptime configured; this is the shape of apache/apisix#13658. At 60s it takes ~2,675 series/s. The soft bound is request latency. Same dict, 8 workers, a steady 20k rps, one reclaim round in the middle of the run: backlog calls one hold p99 p99.9 max none - - 2.02ms 2.32ms 4.52ms 12,000 1 4.3ms 2.03ms 2.33ms 5.97ms 60,000 2 15.1ms 2.20ms 7.64ms 22.67ms 120,000 3 17.8ms 2.03ms 3.63ms 19.01ms 240,000 5 24.8ms 7.13ms 14.94ms 26.29ms 350,000 8 15.2ms 2.13ms 10.09ms 16.91ms A backlog of ~12,000 entries is indistinguishable from not reclaiming at all; past that the tail starts to move, and at 240,000 even p99 does. 60s keeps a round inside that zone for expiry rates up to ~100 series/s. Running often is what makes the choice asymmetric, because it is cheap: a round with nothing to reclaim costs only the LRU walk, measured at 1.8-9.5ms depending on how full the dict is, so 60s spends ~0.4s of dict lock time per hour per worker against ~0.06s at 3600s. Being too frequent costs fractions of a percent of a core; being too rare destroys live metrics. The upper bound is gone: how long an interval a deployment can afford follows from its dict size and expiry rate, which only the caller knows, and clamping 3600s silently was hiding that. The lower bound is now derived from the batch constants -- twice the longest a round can take -- instead of being a round number. Note for callers reclaiming from a single process: with auto_flush_expired left on, every worker runs its own round. Eight rounds issued at once on a 120,000 backlog took p99.99 to 33.25ms and max to 37.95ms, against 15.90ms and 19.01ms for one. --- CHANGELOG.md | 11 ++++++-- README.md | 22 +++++++++++---- prometheus.lua | 68 ++++++++++++++++++++++++++++++++++++--------- prometheus_test.lua | 27 ++++++++++++------ 4 files changed, 99 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35945be..dc9a60e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,18 @@ of changes. ## Unreleased +- **Changed the default `remove_expired_keys_interval` from 3600 to 60 + seconds**, and removed its upper bound. At 3600s an expiry rate of only ~44 + series/s fills a default-sized dict with dead entries, after which creating a + new series force-evicts live metrics (apache/apisix#13658); running the + reclamation every 60s costs ~0.4s of dict lock time per hour per worker + (#23). - Reclaim expired shared-dict entries in batches, so no single `flush_expired()` call holds the dict lock for a whole backlog, refuse overlapping reclamation rounds, require `remove_expired_keys_interval` to be - at least 60 seconds, and let callers take the reclamation over with the - `auto_flush_expired` option and `prometheus:flush_expired()` (#23). + at least twice the longest a round can take, and let callers take the + reclamation over with the `auto_flush_expired` option and + `prometheus:flush_expired()` (#23). ## 1.0.0 diff --git a/README.md b/README.md index e9cad7d..7ec622a 100644 --- a/README.md +++ b/README.md @@ -114,12 +114,22 @@ section of nginx configuration. metrics or need to minimize memory usage of this library. * `remove_expired_keys_interval` (number): how often, in seconds, each worker drops the metrics that have expired and reclaims the shared dict entries - they left behind. Defaults to 3600, which is also the maximum; values below - the minimum of 60 are raised to it, with a warning, because one round of - reclamation can occupy its timer for several seconds and is meant to be a - small fraction of the interval. Pick it from how much of the dict you are - willing to let expired entries hold: they accumulate at the expiry rate - times this interval. + they left behind. Defaults to 60. There is no upper bound; values below the + minimum of 20 are raised to it, with a warning, because one round of + reclamation can occupy its timer for up to 10 seconds and must not take + more than half of its own interval. + + The interval decides how much backlog one round has to reclaim -- the + expiry rate times the interval, times two, since a series leaves a value + key and an index slot behind. Two things follow from that. A backlog larger + than the dict's spare capacity makes the writes that create new series + force-evict live ones, so the interval has a hard ceiling of + `(dict entries - live entries) / (2 * expiry rate)`. And a backlog past + roughly ten thousand entries starts to show up in the request tail while + the lock is held, which puts a softer ceiling at `6000 / expiry rate`. The + default is sized for the second one at about 100 expiring series per + second; raise it if your churn is lower, and give the dict more room rather + than raising it if your churn is higher. * `auto_flush_expired` (boolean): whether that reclamation also happens on the interval above, in every worker. Defaults to true. Set it to false to schedule [`prometheus:flush_expired()`](#prometheusflush_expired) yourself, diff --git a/prometheus.lua b/prometheus.lua index 8ef8f61..00d8e6c 100644 --- a/prometheus.lua +++ b/prometheus.lua @@ -92,16 +92,59 @@ local DEFAULT_ERROR_METRIC_NAME = "nginx_metric_errors_total" -- Default value for per-worker counter sync interval (seconds). local DEFAULT_SYNC_INTERVAL = 1 --- Max value for check and remove expired keys interval (seconds). -local MAX_REMOVE_EXPIRED_KEYS_INTERVAL = 3600 - --- Shortest reclaim interval accepted. A round reclaims in batches with a pause --- in between, so it can occupy its timer for several seconds (see --- FLUSH_EXPIRED_MAX_BATCHES and FLUSH_EXPIRED_BATCH_DELAY in --- prometheus_keys.lua); a round is meant to be a small fraction of the --- interval, not most of it. Overlapping rounds are refused rather than run, so --- a shorter interval would only drop rounds silently. -local MIN_REMOVE_EXPIRED_KEYS_INTERVAL = 60 +-- Default interval, in seconds, between two rounds of dropping expired metrics +-- and reclaiming the dict entries they left behind. +-- +-- The interval has one observable consequence: how much backlog a round has to +-- reclaim, which is the expiry rate times the interval times two (a value key +-- and an index slot per series). Both bounds on it come from that. +-- +-- The hard bound is the dict: once the backlog no longer fits beside the live +-- entries, the writes that create new series start force-evicting live ones, +-- and the dead entries win because they were written last. Measured on a 128m +-- dict (521,184 entries of a typical shape) holding 100,000 live entries: +-- +-- backlog written entries live entries left after reclaiming +-- 120,000 220,000 100,000 +-- 350,000 450,000 100,000 +-- 450,000 521,184 71,184 -- 28,816 live metrics destroyed +-- 700,000 521,184 0 -- all of them +-- +-- At 3600s that bound is reached by an expiry rate of ~44 series/s, which is +-- ordinary for a gateway with an exptime configured; this is the shape of +-- apache/apisix#13658. At 60s it takes ~2,675 series/s. +-- +-- The soft bound is request latency. Same dict, 8 workers, a steady 20k rps, +-- one reclaim round in the middle: +-- +-- backlog calls one hold p99 p99.9 max +-- none - - 2.02ms 2.32ms 4.52ms +-- 12,000 1 4.3ms 2.03ms 2.33ms 5.97ms +-- 60,000 2 15.1ms 2.20ms 7.64ms 22.67ms +-- 240,000 5 24.8ms 7.13ms 14.94ms 26.29ms +-- +-- A backlog of ~12,000 entries is indistinguishable from not reclaiming at all; +-- past that the tail starts to move. 60s keeps a round inside that zone for +-- expiry rates up to ~100 series/s. +-- +-- Running often is cheap, which is what makes the choice asymmetric: a round +-- with nothing to reclaim costs only the LRU walk -- 1.8-9.5ms depending on how +-- full the dict is -- so 60s spends ~0.4s of lock time per hour per worker +-- against ~0.06s at 3600s. Being too frequent costs fractions of a percent of a +-- core; being too rare destroys live metrics. +local DEFAULT_REMOVE_EXPIRED_KEYS_INTERVAL = 60 + +-- Shortest interval accepted: a round may occupy its timer for +-- FLUSH_EXPIRED_MAX_BATCHES * FLUSH_EXPIRED_BATCH_DELAY seconds and must not +-- take more than half of its own interval. Overlapping rounds are refused +-- rather than run, so a shorter interval would only drop rounds silently. +local MIN_REMOVE_EXPIRED_KEYS_INTERVAL = + key_index_lib.FLUSH_EXPIRED_MAX_BATCHES * + key_index_lib.FLUSH_EXPIRED_BATCH_DELAY * 2 + +-- There is deliberately no upper bound: how long an interval a deployment can +-- afford follows from its dict size and expiry rate, which only the caller +-- knows. -- Default max size of lookup table local DEFAULT_LOOKUP_MAX_SIZE = 1000 @@ -758,15 +801,14 @@ function Prometheus.init(dict_name, options_or_prefix) interval = MIN_REMOVE_EXPIRED_KEYS_INTERVAL end self.remove_expired_keys_interval = - interval and interval < MAX_REMOVE_EXPIRED_KEYS_INTERVAL and interval - or MAX_REMOVE_EXPIRED_KEYS_INTERVAL + interval or DEFAULT_REMOVE_EXPIRED_KEYS_INTERVAL self.auto_flush_expired = options_or_prefix.auto_flush_expired ~= false else self.prefix = options_or_prefix or '' self.error_metric_name = DEFAULT_ERROR_METRIC_NAME self.sync_interval = DEFAULT_SYNC_INTERVAL self.lookup_max_size = DEFAULT_LOOKUP_MAX_SIZE - self.remove_expired_keys_interval = MAX_REMOVE_EXPIRED_KEYS_INTERVAL + self.remove_expired_keys_interval = DEFAULT_REMOVE_EXPIRED_KEYS_INTERVAL self.auto_flush_expired = true end diff --git a/prometheus_test.lua b/prometheus_test.lua index d2e43d3..e721160 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -212,22 +212,33 @@ function TestPrometheus:testInitOptions() luaunit.assertEquals(ngx.logs, nil) end --- A reclaim round reclaims in batches with a pause in between, so it can occupy --- its timer for several seconds. An interval shorter than that is raised to the --- minimum, with a warning, rather than left to drop rounds silently. -function TestPrometheus:testInitClampsTheRemoveExpiredKeysInterval() +-- A reclaim round reclaims in batches with a pause in between, so it must not +-- take more than half of its own interval: a shorter interval is raised to the +-- minimum, with a warning. There is deliberately no upper bound -- how long an +-- interval a deployment can afford follows from its dict size and expiry rate. +function TestPrometheus:testInitBoundsTheRemoveExpiredKeysInterval() self.dict = setmetatable({}, SimpleDict) ngx.shared.metrics = self.dict - local p1 = require('prometheus').init("metrics", {remove_expired_keys_interval=3}) - assert(p1.remove_expired_keys_interval == 60) + local keys = require('prometheus_keys') + local minimum = keys.FLUSH_EXPIRED_MAX_BATCHES * keys.FLUSH_EXPIRED_BATCH_DELAY * 2 + + local p1 = require('prometheus').init("metrics", + {remove_expired_keys_interval = minimum - 1}) + assert(p1.remove_expired_keys_interval == minimum) luaunit.assertEquals(#ngx.logs, 1) luaunit.assertStrContains(ngx.logs[1], "remove_expired_keys_interval") ngx.logs = nil - -- the upper bound was already clamped, and silently local p2 = require('prometheus').init("metrics", {remove_expired_keys_interval=7200}) - assert(p2.remove_expired_keys_interval == 3600) + assert(p2.remove_expired_keys_interval == 7200) + + -- the default applies to both forms of the second argument + local p3 = require('prometheus').init("metrics", {}) + assert(p3.remove_expired_keys_interval == 60) + local p4 = require('prometheus').init("metrics", "pref_") + assert(p4.remove_expired_keys_interval == 60) + luaunit.assertEquals(ngx.logs, nil) end function TestPrometheus:testInitWorker() From 3aea12a8783fb7bb4db54be2726da4490d946bcc Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Sat, 10 Oct 2026 10:39:56 +0800 Subject: [PATCH 5/9] fix(keys): bound the local scan too, and stagger the per-worker timers Sizing the reclaim batch was not enough: what a round actually costs is the local scan, not the reclamation. remove_expired_keys() asks the dict for the ttl of every slot the worker knows has one, which is a lock acquisition per slot, and every worker's timer starts in init_worker, so all of them do it at the same moment. Measured with 150,000 expiring series of which none actually expires during the run -- every round is therefore empty -- 8 workers, a steady 20k rps, 180s: what the round does slots p99 p99.9 max nothing (no round fired) 150,000 2.02ms 2.38ms 9.93ms scan only, no reclamation 150,000 267.01ms 448.00ms 526.34ms scan and reclamation 150,000 284.67ms 474.11ms 570.37ms scan and reclamation 10,000 2.20ms 47.84ms 79.17ms scan only, no reclamation 10,000 2.17ms 39.65ms 72.83ms Dropping the reclamation changes almost nothing, and a tenth of the slots makes the stall a tenth: the cost is the scan, linear in the slots, and it is there in every version of this library -- a long interval only makes it rarer, not cheaper. So the scan now walks a slice of the slot range per round and resumes where the previous round stopped, and the first round of each worker is offset by its worker id so the rounds spread over the interval instead of coinciding. Same workload: interval slots p99 p99.9 max 20s 150,000 2.07ms 4.36ms 27.76ms 60s 150,000 2.04ms 2.81ms 27.45ms 60s 400,000 2.06ms 5.74ms 40.32ms At 60s a round is now indistinguishable from not reclaiming at all, and the cost no longer grows with the number of metrics -- which is what the 60s default needed in order to be defensible. A full sweep of the slot range takes key_count / 10000 rounds. That only delays dropping the worker-local reference to a slot whose entry is already gone, and nothing reads it in the meantime: every dict API reports that slot as missing anyway. The range is bounded by key_count rather than by self.last, because a worker's own newest slots sit above the range it has synced. testRemoveExpiredKeysScansASlicePerRound covers the slicing: one round checks at most a slice and stops short of the range, the next picks up the rest, and each slot is checked once. Without the slice the first round checks everything and the test fails. --- CHANGELOG.md | 5 +++ README.md | 6 ++++ prometheus.lua | 23 ++++++++++--- prometheus_keys.lua | 81 ++++++++++++++++++++++++++++++++++++++++----- prometheus_test.lua | 39 ++++++++++++++++++++++ 5 files changed, 140 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc9a60e..baf56c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,11 @@ of changes. new series force-evicts live metrics (apache/apisix#13658); running the reclamation every 60s costs ~0.4s of dict lock time per hour per worker (#23). +- Bound the work one round of `remove_expired_keys()` does, and spread the + per-worker timers over the interval instead of firing them all at + `init_worker`. The local scan asks the dict for the ttl of every slot the + worker knows has one; with 150,000 of them across 8 workers that was a + ~480ms stall at p99.9 on a steady 20k rps, against 2.81ms once sliced (#23). - Reclaim expired shared-dict entries in batches, so no single `flush_expired()` call holds the dict lock for a whole backlog, refuse overlapping reclamation rounds, require `remove_expired_keys_interval` to be diff --git a/README.md b/README.md index 7ec622a..7b6059a 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,12 @@ section of nginx configuration. reclamation can occupy its timer for up to 10 seconds and must not take more than half of its own interval. + A round does bounded work -- it reclaims at most a batch of entries and + walks a slice of the slot range, resuming where the previous round stopped + -- so its cost does not grow with the number of metrics. A full sweep of + the slot range takes `key_count / 10000` rounds, which only delays dropping + the worker-local reference to a slot whose entry is already gone. + The interval decides how much backlog one round has to reclaim -- the expiry rate times the interval, times two, since a series leaves a value key and an index slot behind. Two things follow from that. A backlog larger diff --git a/prometheus.lua b/prometheus.lua index 00d8e6c..f3df2ee 100644 --- a/prometheus.lua +++ b/prometheus.lua @@ -127,11 +127,24 @@ local DEFAULT_SYNC_INTERVAL = 1 -- past that the tail starts to move. 60s keeps a round inside that zone for -- expiry rates up to ~100 series/s. -- --- Running often is cheap, which is what makes the choice asymmetric: a round --- with nothing to reclaim costs only the LRU walk -- 1.8-9.5ms depending on how --- full the dict is -- so 60s spends ~0.4s of lock time per hour per worker --- against ~0.06s at 3600s. Being too frequent costs fractions of a percent of a --- core; being too rare destroys live metrics. +-- Running often has to be cheap for any of this to hold, and it is only cheap +-- because a round does bounded work: both the reclamation and the local scan +-- are sliced (see FLUSH_EXPIRED_BATCH and REMOVE_EXPIRED_KEYS_SCAN_SLOTS), and +-- the per-worker timers are spread over the interval instead of all firing at +-- init_worker. Measured with 150,000 expiring series of which none actually +-- expires during the run, so every round is empty: +-- +-- interval p99 p99.9 max +-- 600s (no round fired, the control) 2.02ms 2.38ms 9.93ms +-- 60s, unbounded round 2.30ms 418.30ms 533.50ms +-- 20s, unbounded round 280.32ms 473.86ms 568.32ms +-- 60s, bounded round 2.04ms 2.81ms 27.45ms +-- 20s, bounded round 2.07ms 4.36ms 27.76ms +-- +-- With the round bounded, 60s is indistinguishable from not reclaiming at all, +-- and it also lets the local scan finish a full sweep in minutes rather than +-- days. Being too frequent then costs almost nothing, while being too rare +-- destroys live metrics -- which is what settles the choice. local DEFAULT_REMOVE_EXPIRED_KEYS_INTERVAL = 60 -- Shortest interval accepted: a round may occupy its timer for diff --git a/prometheus_keys.lua b/prometheus_keys.lua index 9253790..62e7347 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -45,6 +45,22 @@ local FLUSH_EXPIRED_MAX_BATCHES = 10 -- how long one round can occupy its timer: FLUSH_EXPIRED_MAX_BATCHES times this. local FLUSH_EXPIRED_BATCH_DELAY = 1 +-- Slot numbers the local scan of remove_expired_keys() may walk in one round. +-- +-- That scan asks the dict for the ttl of every slot the worker knows has one, +-- which is a lock acquisition per slot, and it is what a round actually costs: +-- with 150,000 such slots across 8 workers and a steady 20k rps it showed up as +-- a ~480ms stall at p99.9, of which the reclamation itself was 5%. A tenth of +-- the slots made it a ~40ms stall, so the cost is linear in them and has to be +-- bounded the way the reclamation is. +-- +-- The scan therefore walks a slice of the slot range per round and resumes +-- where the previous round stopped. A full sweep takes key_count divided by +-- this many rounds, which only delays dropping the worker-local reference to a +-- slot whose node is already gone -- nothing reads it in the meantime, because +-- every dict API reports that slot as missing anyway. +local REMOVE_EXPIRED_KEYS_SCAN_SLOTS = 10000 + -- Exposed so a caller that schedules the reclamation itself can reason about it: -- one round drains at most FLUSH_EXPIRED_BATCH * FLUSH_EXPIRED_MAX_BATCHES @@ -53,6 +69,7 @@ local FLUSH_EXPIRED_BATCH_DELAY = 1 KeyIndex.FLUSH_EXPIRED_BATCH = FLUSH_EXPIRED_BATCH KeyIndex.FLUSH_EXPIRED_MAX_BATCHES = FLUSH_EXPIRED_MAX_BATCHES KeyIndex.FLUSH_EXPIRED_BATCH_DELAY = FLUSH_EXPIRED_BATCH_DELAY +KeyIndex.REMOVE_EXPIRED_KEYS_SCAN_SLOTS = REMOVE_EXPIRED_KEYS_SCAN_SLOTS -- check and remove expired keys @@ -61,6 +78,33 @@ local function remove_expired_keys(_, self) end +-- Every worker runs init_worker at the same moment, so a plain ngx.timer.every +-- would have all of them reclaim at the same moment too and their stalls would +-- add up. Spread the first round evenly over the interval instead, by worker +-- id, and only then settle into the interval. +local function start_remove_expired_keys_timer(self, interval) + local id = tonumber(ngx.worker.id()) or 0 + local count = ngx.worker.count and tonumber(ngx.worker.count()) or 1 + if id < 0 or not count or count < 1 then + id, count = 0, 1 + end + + local offset = interval * id / count + if offset <= 0 then + ngx.timer.every(interval, remove_expired_keys, self) + return + end + + ngx.timer.at(offset, function(premature, s) + if premature then + return + end + ngx.timer.every(interval, remove_expired_keys, s) + s:remove_expired_keys() + end, self) +end + + function KeyIndex.new(shared_dict, prefix, remove_expired_keys_interval, auto_flush_expired) local self = setmetatable({}, KeyIndex) @@ -76,21 +120,40 @@ function KeyIndex.new(shared_dict, prefix, remove_expired_keys_interval, self.index = {} self.expire_keys = {} - ngx.timer.every(remove_expired_keys_interval or 600, remove_expired_keys, self) + self.scan_cursor = 0 + + start_remove_expired_keys_timer(self, remove_expired_keys_interval or 600) return self end -- check and remove expired keys function KeyIndex:remove_expired_keys() - for i, _ in pairs(self.expire_keys) do - -- Read i-th key. If it is nil or ttl is < 0, it means it was expired - local ttl, err = self.dict:ttl(self.key_prefix .. i) - if not (ttl and ttl >= 0 or err and err ~= "not found") then - if self.keys[i] then - self.index[self.keys[i]] = nil - self.keys[i] = nil + -- A slice of the slot range per round, resuming where the last round stopped, + -- so a round costs the same whatever the index has grown to. The range is + -- bounded by key_count rather than by self.last: a worker's own newest slots + -- are above the range it has synced, and they have to be scanned too. + local last = self.dict:get(self.key_count) or 0 + local from = self.scan_cursor + if from > last then + from = 0 + end + local to = from + REMOVE_EXPIRED_KEYS_SCAN_SLOTS - 1 + if to > last then + to = last + end + self.scan_cursor = to + 1 + + for i = from, to do + if self.expire_keys[i] then + -- Read i-th key. If it is nil or ttl is < 0, it means it was expired + local ttl, err = self.dict:ttl(self.key_prefix .. i) + if not (ttl and ttl >= 0 or err and err ~= "not found") then + if self.keys[i] then + self.index[self.keys[i]] = nil + self.keys[i] = nil + end + self.expire_keys[i] = nil end - self.expire_keys[i] = nil end end diff --git a/prometheus_test.lua b/prometheus_test.lua index e721160..fa2b59c 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -126,6 +126,7 @@ end function Nginx.sleep() end Nginx.timer = {} function Nginx.timer.every(_, _, _) end +function Nginx.timer.at(_, _, _) end function Nginx.get_phase() return 'init_worker' end @@ -990,6 +991,44 @@ function TestKeyIndex:testFlushExpiredRunsInBatches() luaunit.assertNotNil(self.dict.dict["permanent"]) end +-- The local scan asks the dict for the ttl of every slot the worker knows has +-- one, which is a lock acquisition per slot and is what a round actually costs. +-- It must therefore walk a bounded slice per round and resume where the +-- previous round stopped. +function TestKeyIndex:testRemoveExpiredKeysScansASlicePerRound() + local slice = self.key_index.REMOVE_EXPIRED_KEYS_SCAN_SLOTS + local keys = {} + for i = 1, slice + 10 do + keys[i] = "sliced_" .. i + end + luaunit.assertEquals(self.key_index:add(keys, "eviction_err", 600), nil) + + local checked = 0 + local ttl = SimpleDict.ttl + self.dict.ttl = function(dict, k) + checked = checked + 1 + return ttl(dict, k) + end + + self.key_index:remove_expired_keys() + local first = checked + luaunit.assertTrue(first <= slice) + -- it really did stop short of the whole range + luaunit.assertTrue(first < slice + 10) + + checked = 0 + self.key_index:remove_expired_keys() + -- the next round picks up the rest, and the slots are each checked once + luaunit.assertEquals(first + checked, slice + 10) + + -- a live slot is kept, whichever round reached it + luaunit.assertNotNil(self.key_index.index["sliced_1"]) + luaunit.assertNotNil(self.key_index.index["sliced_" .. (slice + 10)]) + + self.dict.ttl = nil + luaunit.assertEquals(ngx.logs, nil) +end + -- One round yields between batches, so the timer can fire again before it -- finishes -- ngx.timer.every does not wait for the previous run. The second -- round must step aside instead of racing this one for the dict lock. From 7374a4e52d9b31f368d3875b0523dfb1633554f1 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Sat, 10 Oct 2026 11:49:59 +0800 Subject: [PATCH 6/9] perf(keys): skip the slots the local scan cannot learn anything about Slicing the scan bounded its cost but did not reduce it: a round still asked the dict for the ttl of up to a slice of slots, and the answer is one the worker mostly already has. When add() renews a slot it knows when that slot will expire, so while the recorded deadline is still in the future there is nothing to ask -- nothing but a forced eviction takes a node before its ttl, and a local reference that outlives its slot is harmless until add() next touches that key. So expire_keys now holds the deadline instead of just a flag, and the scan only reaches for the dict once it has passed. A slot that turns out to have been renewed by another worker gets its deadline refreshed from the ttl it just read, rather than being asked about again every round. Isolated by turning the reclamation off, so a round is only the scan -- 8 workers, a 20s interval, a steady 20k rps, 180s, nothing actually expiring: slots sliced only sliced and deadline-aware 150,000 p99.9 3.40ms max 22.40ms p99.9 2.38ms max 15.01ms 400,000 p99.9 5.84ms max 31.92ms p99.9 2.48ms max 17.84ms which is the control (p99.9 2.40ms), and no longer grows with the index. With auto_flush_expired left on the gain is masked, because the round is then dominated by flush_expired()'s walk of the LRU queue: slots sliced only sliced and deadline-aware 150,000 p99.9 2.81ms max 27.45ms p99.9 3.34ms max 26.72ms 400,000 p99.9 5.74ms max 40.32ms p99.9 8.53ms max 37.60ms That walk cannot be made cheaper from here -- it is what flush_expired() does -- so it is the caller's remaining lever: with the reclamation moved to a single process, every other worker's round is left with only the scan, which is now free. Two alternatives were measured and dropped. Dropping the scan and relying on add() and sync_range() to clean up as they go leaves the local reference of a series that never returns in place for good, which is the common case under label churn. Broadcasting "something was reclaimed" through a shared counter so that a round can skip the scan entirely cannot work either: the passive expiry nginx performs on every write frees nodes at the LRU tail without telling Lua, so the counter would miss exactly the removals that happen when nothing else is reclaiming. --- CHANGELOG.md | 9 +++++--- prometheus_keys.lua | 56 ++++++++++++++++++++++++++++++++------------- prometheus_test.lua | 55 +++++++++++++++++++++++++++++++++++++------- 3 files changed, 93 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baf56c2..ef48ac1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,9 +14,12 @@ of changes. (#23). - Bound the work one round of `remove_expired_keys()` does, and spread the per-worker timers over the interval instead of firing them all at - `init_worker`. The local scan asks the dict for the ttl of every slot the - worker knows has one; with 150,000 of them across 8 workers that was a - ~480ms stall at p99.9 on a steady 20k rps, against 2.81ms once sliced (#23). + `init_worker`. Asking the dict for a slot's ttl is a lock acquisition, and + the scan did it for every slot the worker knew had one: with 150,000 of them + across 8 workers that was a ~480ms stall at p99.9 on a steady 20k rps. It now + skips the slots whose recorded deadline has not passed and walks a bounded + slice of the rest, which brings the same case to 2.38ms -- the same as not + reclaiming at all (#23). - Reclaim expired shared-dict entries in batches, so no single `flush_expired()` call holds the dict lock for a whole backlog, refuse overlapping reclamation rounds, require `remove_expired_keys_interval` to be diff --git a/prometheus_keys.lua b/prometheus_keys.lua index 62e7347..b92b338 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -47,18 +47,22 @@ local FLUSH_EXPIRED_BATCH_DELAY = 1 -- Slot numbers the local scan of remove_expired_keys() may walk in one round. -- --- That scan asks the dict for the ttl of every slot the worker knows has one, --- which is a lock acquisition per slot, and it is what a round actually costs: --- with 150,000 such slots across 8 workers and a steady 20k rps it showed up as --- a ~480ms stall at p99.9, of which the reclamation itself was 5%. A tenth of --- the slots made it a ~40ms stall, so the cost is linear in them and has to be --- bounded the way the reclamation is. +-- Asking the dict for a slot's ttl is a lock acquisition, and doing it for +-- every slot the worker knows has one is what a round used to cost: with +-- 150,000 such slots across 8 workers and a steady 20k rps it showed up as a +-- ~480ms stall at p99.9, of which the reclamation itself was 5%. A tenth of the +-- slots made it a ~40ms stall, so the cost was linear in them. -- --- The scan therefore walks a slice of the slot range per round and resumes --- where the previous round stopped. A full sweep takes key_count divided by --- this many rounds, which only delays dropping the worker-local reference to a --- slot whose node is already gone -- nothing reads it in the meantime, because --- every dict API reports that slot as missing anyway. +-- Two things bound it now. The scan skips any slot whose recorded deadline has +-- not passed, without touching the dict, so the lock acquisitions are +-- proportional to what has actually lapsed rather than to the index. And it +-- walks only a slice of the slot range per round, resuming where the previous +-- round stopped, so even a round where everything has lapsed stays bounded. +-- +-- A full sweep then takes key_count divided by this many rounds, which only +-- delays dropping the worker-local reference to a slot whose node is already +-- gone -- nothing reads it in the meantime, because every dict API reports that +-- slot as missing anyway. local REMOVE_EXPIRED_KEYS_SCAN_SLOTS = 10000 @@ -143,8 +147,17 @@ function KeyIndex:remove_expired_keys() end self.scan_cursor = to + 1 + local now = ngx.now() for i = from, to do - if self.expire_keys[i] then + -- self.expire_keys[i] is when this worker last saw slot i expire. While + -- that is still in the future the slot was there and nothing but a forced + -- eviction can have taken it, so there is nothing to ask the dict -- which + -- is what makes a round cost nothing when nothing has lapsed. Should a + -- forced eviction have taken it anyway, the only consequence is a local + -- reference that outlives its slot, and add() drops it on the next + -- observation of that key. + local deadline = self.expire_keys[i] + if deadline and deadline <= now then -- Read i-th key. If it is nil or ttl is < 0, it means it was expired local ttl, err = self.dict:ttl(self.key_prefix .. i) if not (ttl and ttl >= 0 or err and err ~= "not found") then @@ -153,6 +166,10 @@ function KeyIndex:remove_expired_keys() self.keys[i] = nil end self.expire_keys[i] = nil + else + -- still there: another worker renewed it, so wait out the new ttl + -- instead of asking again every round + self.expire_keys[i] = now + ttl end end end @@ -255,11 +272,11 @@ function KeyIndex:sync_range(first, last) self.keys[i] = key self.index[key] = i - -- if it is nil and ttl not is 0, set expire_keys map + -- if it is nil and ttl not is 0, record when the slot expires if not self.expire_keys[i] then local ttl, _ = self.dict:ttl(self.key_prefix .. i) if ttl and ttl ~= 0 then - self.expire_keys[i] = true + self.expire_keys[i] = ngx.now() + ttl end end elseif self.keys[i] then @@ -307,6 +324,10 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) keys = { key_or_keys } end + -- when the slot's expiry is renewed or set below, remember when it lands, so + -- that remove_expired_keys() does not have to ask the dict about it + local now = exptime and exptime > 0 and ngx.now() + for _, key in pairs(keys) do local retried = false local repairs = 0 @@ -318,6 +339,9 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) local expired = false if exptime then local ok, err = self.dict:expire(self.key_prefix .. self.index[key], exptime) + if ok and now then + self.expire_keys[self.index[key]] = now + exptime + end if not ok then if err == "not found" then -- The slot already expired in the shared dict. Drop the stale @@ -363,8 +387,8 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) local _, _, forcible2 = self.dict:incr(self.key_count, 1, 0) self.keys[N] = key self.index[key] = N - if exptime and exptime > 0 then - self.expire_keys[N] = true + if now then + self.expire_keys[N] = now + exptime end if forcible or forcible2 or repair_forcible then return (err_msg_lru_eviction .. "; key index: add key: idx=" .. diff --git a/prometheus_test.lua b/prometheus_test.lua index fa2b59c..350eff0 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -124,6 +124,10 @@ function Nginx.worker.id() return 'testworker' end function Nginx.sleep() end +function Nginx.now() + -- the SimpleDict mock keeps expiry in os.time() units + return os.time() +end Nginx.timer = {} function Nginx.timer.every(_, _, _) end function Nginx.timer.at(_, _, _) end @@ -991,10 +995,35 @@ function TestKeyIndex:testFlushExpiredRunsInBatches() luaunit.assertNotNil(self.dict.dict["permanent"]) end --- The local scan asks the dict for the ttl of every slot the worker knows has --- one, which is a lock acquisition per slot and is what a round actually costs. --- It must therefore walk a bounded slice per round and resume where the --- previous round stopped. +-- The local scan is what a round actually costs: a lock acquisition per slot +-- the worker knows has a ttl. It avoids most of them by remembering when it +-- recorded each slot expiring, and asking the dict only once that has passed -- +-- so while the metrics are being renewed a round touches the dict not at all. +function TestKeyIndex:testRemoveExpiredKeysSkipsSlotsThatCannotHaveLapsed() + local keys = {} + for i = 1, 100 do + keys[i] = "live_" .. i + end + luaunit.assertEquals(self.key_index:add(keys, "eviction_err", 600), nil) + + local checked = 0 + local ttl = SimpleDict.ttl + self.dict.ttl = function(dict, k) + checked = checked + 1 + return ttl(dict, k) + end + + self.key_index:remove_expired_keys() + luaunit.assertEquals(checked, 0) + luaunit.assertEquals(self.key_index.index["live_1"], 1) + + self.dict.ttl = nil + luaunit.assertEquals(ngx.logs, nil) +end + +-- The slots whose deadline has passed do have to be asked about, and there can +-- be as many of them as the index is large, so the scan walks a bounded slice +-- per round and resumes where the previous round stopped. function TestKeyIndex:testRemoveExpiredKeysScansASlicePerRound() local slice = self.key_index.REMOVE_EXPIRED_KEYS_SCAN_SLOTS local keys = {} @@ -1003,6 +1032,12 @@ function TestKeyIndex:testRemoveExpiredKeysScansASlicePerRound() end luaunit.assertEquals(self.key_index:add(keys, "eviction_err", 600), nil) + -- what a round running after the exptime sees, without waiting for it: every + -- recorded deadline has passed, so every slot has to be asked about + for i in pairs(self.key_index.expire_keys) do + self.key_index.expire_keys[i] = 0 + end + local checked = 0 local ttl = SimpleDict.ttl self.dict.ttl = function(dict, k) @@ -1018,12 +1053,16 @@ function TestKeyIndex:testRemoveExpiredKeysScansASlicePerRound() checked = 0 self.key_index:remove_expired_keys() - -- the next round picks up the rest, and the slots are each checked once + -- the next round picks up the rest, and each slot is asked about once luaunit.assertEquals(first + checked, slice + 10) - -- a live slot is kept, whichever round reached it - luaunit.assertNotNil(self.key_index.index["sliced_1"]) - luaunit.assertNotNil(self.key_index.index["sliced_" .. (slice + 10)]) + -- the slots are all still live, so they are kept, and their deadlines are + -- refreshed rather than re-checked on every round from now on + luaunit.assertEquals(self.key_index.index["sliced_1"], 1) + luaunit.assertEquals(self.key_index.index["sliced_" .. (slice + 10)], slice + 10) + checked = 0 + self.key_index:remove_expired_keys() + luaunit.assertEquals(checked, 0) self.dict.ttl = nil luaunit.assertEquals(ngx.logs, nil) From 3a0e9464b9c7362844aca3f78f9fd3e263af1ae2 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 12 Oct 2026 09:01:12 +0800 Subject: [PATCH 7/9] fix(keys): do not pause after the last batch of a round The pause belongs between two batches. When a round uses all FLUSH_EXPIRED_MAX_BATCHES of them the loop slept once more and then ended, so the timer was held for a second over a round that was already finished -- and at a short interval that is enough to make the next round be skipped by the re-entry guard. Raised by CodeRabbit on the first revision of this PR. The capped path itself is not covered by a test: reaching it needs a backlog larger than FLUSH_EXPIRED_BATCH * FLUSH_EXPIRED_MAX_BATCHES, which is half a million entries in the mock dict. testFlushExpiredRunsInBatches now asserts the invariant that holds either way, that a round pauses once less than it batches. --- prometheus_keys.lua | 8 +++++--- prometheus_test.lua | 5 +++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/prometheus_keys.lua b/prometheus_keys.lua index b92b338..ced1a67 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -207,12 +207,14 @@ end -- The batch loop itself, so that the public entry point below can guard it. local function flush_expired_batches(self) local total = 0 - for _ = 1, FLUSH_EXPIRED_MAX_BATCHES do + for batch = 1, FLUSH_EXPIRED_MAX_BATCHES do local freed = self.dict:flush_expired(FLUSH_EXPIRED_BATCH) total = total + freed -- freeing less than a full batch means this call has already walked the - -- whole queue, so there is nothing left to reclaim - if freed < FLUSH_EXPIRED_BATCH then + -- whole queue, so there is nothing left to reclaim. The pause belongs + -- between two batches, so there is none after the last one either -- it + -- would just hold up the timer for a round that is already over. + if freed < FLUSH_EXPIRED_BATCH or batch == FLUSH_EXPIRED_MAX_BATCHES then break end diff --git a/prometheus_test.lua b/prometheus_test.lua index 350eff0..2e7c584 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -980,11 +980,16 @@ function TestKeyIndex:testFlushExpiredRunsInBatches() batches[#batches + 1] = n return flush_expired(dict, n) end + local slept = 0 + ngx.sleep = function() slept = slept + 1 end -- the backlog is one entry past a batch, so reaching all of it takes a -- second call, which then comes back short luaunit.assertEquals(self.key_index:flush_expired(), expired) luaunit.assertEquals(batches, {batch, batch}) + -- the pause belongs between two batches, so there is one fewer of them + luaunit.assertEquals(slept, #batches - 1) + ngx.sleep = nil -- nothing left: the loop must stop after the call that comes back short batches = {} From 6acda871f792a53bdefc799e258b0648f3765279 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 12 Oct 2026 09:07:34 +0800 Subject: [PATCH 8/9] fix: default the reclaim interval to 1800s rather than 60s 60s was derived from one side of the trade only. Reclaiming is also what makes a metric give up its slot number -- one whose entry is still in the dict comes back on the same number, one whose entry has been reclaimed has to take a new one -- and a short interval spends that. Measured over rounds of 3000 series expiring and being observed again: interval new slot numbers per round 3600s 879 on average, and the last rounds 9, 8, 4, 3 -- it converges 60s 2003 -- all of them, every round The latency side turns out to be the weaker constraint, because a round does bounded work now: even a backlog filling two thirds of the dict costs ~10ms at p99.9 (350,000 entries: p99 2.13ms, p99.9 10.09ms, max 16.91ms). What a long interval risks is the memory, not the requests, and that risk is what 1800s halves relative to the 3600s this library used to default to. The expiry rate the bound tolerates on a 128m dict: live entries 1800s 3600s 60s 100,000 117 series/s 58 series/s 3,508 series/s 200,000 89 series/s 45 series/s 2,675 series/s 400,000 34 series/s 17 series/s 1,069 series/s So the default halves the backlog 3600s accumulates while leaving the reclamation rare enough that a returning metric usually still finds its entry. Deployments whose churn is higher than that table should lower it; those whose metrics have no exptime can raise it freely, since nothing ever expires. --- CHANGELOG.md | 13 ++++++------ README.md | 28 +++++++++++++++----------- prometheus.lua | 48 +++++++++++++++++++++++++++++++-------------- prometheus_test.lua | 4 ++-- 4 files changed, 59 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef48ac1..570e970 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,12 +6,13 @@ of changes. ## Unreleased -- **Changed the default `remove_expired_keys_interval` from 3600 to 60 - seconds**, and removed its upper bound. At 3600s an expiry rate of only ~44 - series/s fills a default-sized dict with dead entries, after which creating a - new series force-evicts live metrics (apache/apisix#13658); running the - reclamation every 60s costs ~0.4s of dict lock time per hour per worker - (#23). +- **Changed the default `remove_expired_keys_interval` from 3600 to 1800 + seconds**, and removed its upper bound. At 3600s an expiry rate of 17-58 + series/s, depending on how much of the dict is live, fills a default-sized + dict with dead entries, after which creating a new series force-evicts live + metrics (apache/apisix#13658). Halving the interval doubles the rate that is + tolerated while keeping the reclamation rare enough that a returning metric + usually still finds its entry and keeps its slot number (#23). - Bound the work one round of `remove_expired_keys()` does, and spread the per-worker timers over the interval instead of firing them all at `init_worker`. Asking the dict for a slot's ttl is a lock acquisition, and diff --git a/README.md b/README.md index 7b6059a..adb8a3a 100644 --- a/README.md +++ b/README.md @@ -114,8 +114,8 @@ section of nginx configuration. metrics or need to minimize memory usage of this library. * `remove_expired_keys_interval` (number): how often, in seconds, each worker drops the metrics that have expired and reclaims the shared dict entries - they left behind. Defaults to 60. There is no upper bound; values below the - minimum of 20 are raised to it, with a warning, because one round of + they left behind. Defaults to 1800. There is no upper bound; values below + the minimum of 20 are raised to it, with a warning, because one round of reclamation can occupy its timer for up to 10 seconds and must not take more than half of its own interval. @@ -127,15 +127,21 @@ section of nginx configuration. The interval decides how much backlog one round has to reclaim -- the expiry rate times the interval, times two, since a series leaves a value - key and an index slot behind. Two things follow from that. A backlog larger - than the dict's spare capacity makes the writes that create new series - force-evict live ones, so the interval has a hard ceiling of - `(dict entries - live entries) / (2 * expiry rate)`. And a backlog past - roughly ten thousand entries starts to show up in the request tail while - the lock is held, which puts a softer ceiling at `6000 / expiry rate`. The - default is sized for the second one at about 100 expiring series per - second; raise it if your churn is lower, and give the dict more room rather - than raising it if your churn is higher. + key and an index slot behind. A backlog larger than the dict's spare + capacity makes the writes that create new series force-evict live ones, so + the interval has a ceiling of + `(dict entries - live entries) / (2 * expiry rate)`. On a 128m dict, which + holds about 520,000 entries of a typical shape, the default tolerates + roughly 117 expiring series per second with 100,000 live entries, 89 with + 200,000, and 34 with 400,000. Lower it if your churn is higher than that, + and raise it freely if your metrics have no `exptime` at all, since then + nothing ever expires. + + Pulling the other way, reclaiming is also what makes a metric give up its + slot number: one whose entry is still in the dict comes back on the same + number, one whose entry has been reclaimed has to take a new one. A short + interval therefore buys memory headroom with slot numbers, and the default + is the middle of that trade. * `auto_flush_expired` (boolean): whether that reclamation also happens on the interval above, in every worker. Defaults to true. Set it to false to schedule [`prometheus:flush_expired()`](#prometheusflush_expired) yourself, diff --git a/prometheus.lua b/prometheus.lua index f3df2ee..19f674e 100644 --- a/prometheus.lua +++ b/prometheus.lua @@ -110,24 +110,47 @@ local DEFAULT_SYNC_INTERVAL = 1 -- 450,000 521,184 71,184 -- 28,816 live metrics destroyed -- 700,000 521,184 0 -- all of them -- --- At 3600s that bound is reached by an expiry rate of ~44 series/s, which is --- ordinary for a gateway with an exptime configured; this is the shape of --- apache/apisix#13658. At 60s it takes ~2,675 series/s. +-- The expiry rate that bound tolerates, on a 128m dict: -- --- The soft bound is request latency. Same dict, 8 workers, a steady 20k rps, --- one reclaim round in the middle: +-- live entries 1800s 3600s 60s +-- 100,000 117 series/s 58 series/s 3,508 series/s +-- 200,000 89 series/s 45 series/s 2,675 series/s +-- 400,000 34 series/s 17 series/s 1,069 series/s +-- +-- 3600s is what this library used to default to, and 17-58 series/s is ordinary +-- for a gateway with an exptime configured: this is the shape of +-- apache/apisix#13658. +-- +-- The soft bound is request latency, and it turns out to be the weaker of the +-- two. Same dict, 8 workers, a steady 20k rps, one reclaim round in the middle: -- -- backlog calls one hold p99 p99.9 max -- none - - 2.02ms 2.32ms 4.52ms -- 12,000 1 4.3ms 2.03ms 2.33ms 5.97ms -- 60,000 2 15.1ms 2.20ms 7.64ms 22.67ms -- 240,000 5 24.8ms 7.13ms 14.94ms 26.29ms +-- 350,000 8 15.2ms 2.13ms 10.09ms 16.91ms +-- +-- Even a backlog that fills two thirds of the dict costs ~10ms at p99.9, so +-- what a long interval risks is the memory, not the requests. -- --- A backlog of ~12,000 entries is indistinguishable from not reclaiming at all; --- past that the tail starts to move. 60s keeps a round inside that zone for --- expiry rates up to ~100 series/s. +-- Pulling the other way is that reclaiming is also what gives up slot numbers. +-- A metric whose entry is still in the dict comes back on its own number; one +-- whose entry has been reclaimed has to take a new one. Measured over rounds of +-- 3000 series expiring and being observed again: -- --- Running often has to be cheap for any of this to hold, and it is only cheap +-- interval new slot numbers per round +-- 3600s 879 on average, and the last rounds 9, 8, 4, 3 -- it converges +-- 60s 2003 -- all of them, every round +-- +-- So a short interval trades slot numbers for memory headroom, and 1800s is +-- picked to keep both: it halves the backlog 3600s accumulates while leaving +-- the reclamation rare enough that a returning metric usually still finds its +-- entry. Deployments whose churn is higher than the table above should lower +-- it, and those whose metrics never expire can raise it freely -- there is no +-- upper bound. +-- +-- None of this would hold if a round were expensive, and it is only cheap -- because a round does bounded work: both the reclamation and the local scan -- are sliced (see FLUSH_EXPIRED_BATCH and REMOVE_EXPIRED_KEYS_SCAN_SLOTS), and -- the per-worker timers are spread over the interval instead of all firing at @@ -140,12 +163,7 @@ local DEFAULT_SYNC_INTERVAL = 1 -- 20s, unbounded round 280.32ms 473.86ms 568.32ms -- 60s, bounded round 2.04ms 2.81ms 27.45ms -- 20s, bounded round 2.07ms 4.36ms 27.76ms --- --- With the round bounded, 60s is indistinguishable from not reclaiming at all, --- and it also lets the local scan finish a full sweep in minutes rather than --- days. Being too frequent then costs almost nothing, while being too rare --- destroys live metrics -- which is what settles the choice. -local DEFAULT_REMOVE_EXPIRED_KEYS_INTERVAL = 60 +local DEFAULT_REMOVE_EXPIRED_KEYS_INTERVAL = 1800 -- Shortest interval accepted: a round may occupy its timer for -- FLUSH_EXPIRED_MAX_BATCHES * FLUSH_EXPIRED_BATCH_DELAY seconds and must not diff --git a/prometheus_test.lua b/prometheus_test.lua index 2e7c584..52bbaa8 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -240,9 +240,9 @@ function TestPrometheus:testInitBoundsTheRemoveExpiredKeysInterval() -- the default applies to both forms of the second argument local p3 = require('prometheus').init("metrics", {}) - assert(p3.remove_expired_keys_interval == 60) + assert(p3.remove_expired_keys_interval == 1800) local p4 = require('prometheus').init("metrics", "pref_") - assert(p4.remove_expired_keys_interval == 60) + assert(p4.remove_expired_keys_interval == 1800) luaunit.assertEquals(ngx.logs, nil) end From a5a79a38a664f3c17aad8acbb6c4439f5892391f Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Wed, 14 Oct 2026 15:50:17 +0800 Subject: [PATCH 9/9] fix(keys): scan every lapsed slot per round, yielding between steps Walking a bounded slice per round could fall behind the rate metrics expire at. Instead, collect every slot whose recorded deadline has passed, confirm each with ttl() and yield after every 500 of them, so a round keeps up with expiry without stalling the worker. The scan stops when the worker is exiting and a guard keeps two rounds from overlapping. --- CHANGELOG.md | 7 +-- README.md | 11 +++-- prometheus.lua | 4 +- prometheus_keys.lua | 115 ++++++++++++++++++++++++++++---------------- prometheus_test.lua | 115 ++++++++++++++++++++++++++++++++++++-------- 5 files changed, 180 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 570e970..49ae959 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,9 +18,10 @@ of changes. `init_worker`. Asking the dict for a slot's ttl is a lock acquisition, and the scan did it for every slot the worker knew had one: with 150,000 of them across 8 workers that was a ~480ms stall at p99.9 on a steady 20k rps. It now - skips the slots whose recorded deadline has not passed and walks a bounded - slice of the rest, which brings the same case to 2.38ms -- the same as not - reclaiming at all (#23). + skips the slots whose recorded deadline has not passed and yields after every + 500 of the rest, so one round covers every lapsed slot without stalling the + worker. With 200,000 expiring series across 10 workers at 20k rps this keeps + p99 at 2.16ms, against 746ms when every worker scans in one go (#23). - Reclaim expired shared-dict entries in batches, so no single `flush_expired()` call holds the dict lock for a whole backlog, refuse overlapping reclamation rounds, require `remove_expired_keys_interval` to be diff --git a/README.md b/README.md index adb8a3a..78238d9 100644 --- a/README.md +++ b/README.md @@ -119,11 +119,12 @@ section of nginx configuration. reclamation can occupy its timer for up to 10 seconds and must not take more than half of its own interval. - A round does bounded work -- it reclaims at most a batch of entries and - walks a slice of the slot range, resuming where the previous round stopped - -- so its cost does not grow with the number of metrics. A full sweep of - the slot range takes `key_count / 10000` rounds, which only delays dropping - the worker-local reference to a slot whose entry is already gone. + A round does bounded work at a time -- it reclaims at most a batch of + entries, only asks the dict about the slots whose recorded deadline has + passed, and yields after every 500 of them -- so it never holds up the + worker for long, however many metrics there are. Each round still covers + every slot that has lapsed, so the worker-local index keeps up with the + rate the metrics expire at. The interval decides how much backlog one round has to reclaim -- the expiry rate times the interval, times two, since a series leaves a value diff --git a/prometheus.lua b/prometheus.lua index 19f674e..4bd98b6 100644 --- a/prometheus.lua +++ b/prometheus.lua @@ -151,8 +151,8 @@ local DEFAULT_SYNC_INTERVAL = 1 -- upper bound. -- -- None of this would hold if a round were expensive, and it is only cheap --- because a round does bounded work: both the reclamation and the local scan --- are sliced (see FLUSH_EXPIRED_BATCH and REMOVE_EXPIRED_KEYS_SCAN_SLOTS), and +-- because a round does bounded work: the reclamation is batched and the local +-- scan yields (see FLUSH_EXPIRED_BATCH and REMOVE_EXPIRED_KEYS_YIELD_EVERY), and -- the per-worker timers are spread over the interval instead of all firing at -- init_worker. Measured with 150,000 expiring series of which none actually -- expires during the run, so every round is empty: diff --git a/prometheus_keys.lua b/prometheus_keys.lua index ced1a67..2ccd9ff 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -45,25 +45,24 @@ local FLUSH_EXPIRED_MAX_BATCHES = 10 -- how long one round can occupy its timer: FLUSH_EXPIRED_MAX_BATCHES times this. local FLUSH_EXPIRED_BATCH_DELAY = 1 --- Slot numbers the local scan of remove_expired_keys() may walk in one round. +-- Slots the local scan of remove_expired_keys() asks the dict about between two +-- yields. -- -- Asking the dict for a slot's ttl is a lock acquisition, and doing it for -- every slot the worker knows has one is what a round used to cost: with -- 150,000 such slots across 8 workers and a steady 20k rps it showed up as a --- ~480ms stall at p99.9, of which the reclamation itself was 5%. A tenth of the --- slots made it a ~40ms stall, so the cost was linear in them. +-- ~480ms stall at p99.9, of which the reclamation itself was 5%. With 200,000 +-- slots and 10 workers whose rounds fire together it was a ~1s stall of every +-- worker at once, all of them spinning on the dict lock. -- -- Two things bound it now. The scan skips any slot whose recorded deadline has -- not passed, without touching the dict, so the lock acquisitions are -- proportional to what has actually lapsed rather than to the index. And it --- walks only a slice of the slot range per round, resuming where the previous --- round stopped, so even a round where everything has lapsed stays bounded. --- --- A full sweep then takes key_count divided by this many rounds, which only --- delays dropping the worker-local reference to a slot whose node is already --- gone -- nothing reads it in the meantime, because every dict API reports that --- slot as missing anyway. -local REMOVE_EXPIRED_KEYS_SCAN_SLOTS = 10000 +-- yields after every this many of them, so a round where everything has lapsed +-- still keeps the worker's event loop running: ~0.2ms of work between two +-- yields. Each round still covers every lapsed slot, so the worker-local index +-- cannot fall behind the rate the metrics expire at. +local REMOVE_EXPIRED_KEYS_YIELD_EVERY = 500 -- Exposed so a caller that schedules the reclamation itself can reason about it: @@ -73,7 +72,7 @@ local REMOVE_EXPIRED_KEYS_SCAN_SLOTS = 10000 KeyIndex.FLUSH_EXPIRED_BATCH = FLUSH_EXPIRED_BATCH KeyIndex.FLUSH_EXPIRED_MAX_BATCHES = FLUSH_EXPIRED_MAX_BATCHES KeyIndex.FLUSH_EXPIRED_BATCH_DELAY = FLUSH_EXPIRED_BATCH_DELAY -KeyIndex.REMOVE_EXPIRED_KEYS_SCAN_SLOTS = REMOVE_EXPIRED_KEYS_SCAN_SLOTS +KeyIndex.REMOVE_EXPIRED_KEYS_YIELD_EVERY = REMOVE_EXPIRED_KEYS_YIELD_EVERY -- check and remove expired keys @@ -124,47 +123,57 @@ function KeyIndex.new(shared_dict, prefix, remove_expired_keys_interval, self.index = {} self.expire_keys = {} - self.scan_cursor = 0 - start_remove_expired_keys_timer(self, remove_expired_keys_interval or 600) return self end --- check and remove expired keys -function KeyIndex:remove_expired_keys() - -- A slice of the slot range per round, resuming where the last round stopped, - -- so a round costs the same whatever the index has grown to. The range is - -- bounded by key_count rather than by self.last: a worker's own newest slots - -- are above the range it has synced, and they have to be scanned too. - local last = self.dict:get(self.key_count) or 0 - local from = self.scan_cursor - if from > last then - from = 0 - end - local to = from + REMOVE_EXPIRED_KEYS_SCAN_SLOTS - 1 - if to > last then - to = last +-- Drops the worker-local references to the slots that have expired. +local function remove_expired_slots(self) + -- self.expire_keys[i] is when this worker last saw slot i expire. While + -- that is still in the future the slot was there and nothing but a forced + -- eviction can have taken it, so there is nothing to ask the dict -- which + -- is what makes a round cost nothing when nothing has lapsed. Should a + -- forced eviction have taken it anyway, the only consequence is a local + -- reference that outlives its slot, and add() drops it on the next + -- observation of that key. + -- + -- The lapsed slots are collected first, in plain Lua: the scan below yields, + -- and a pairs() traversal must not span a yield, during which add() may + -- insert into self.expire_keys. + local now = ngx.now() + local due, n = {}, 0 + for i, deadline in pairs(self.expire_keys) do + if deadline <= now then + n = n + 1 + due[n] = i + end end - self.scan_cursor = to + 1 - local now = ngx.now() - for i = from, to do - -- self.expire_keys[i] is when this worker last saw slot i expire. While - -- that is still in the future the slot was there and nothing but a forced - -- eviction can have taken it, so there is nothing to ask the dict -- which - -- is what makes a round cost nothing when nothing has lapsed. Should a - -- forced eviction have taken it anyway, the only consequence is a local - -- reference that outlives its slot, and add() drops it on the next - -- observation of that key. + for j = 1, n do + if j > 1 and (j - 1) % REMOVE_EXPIRED_KEYS_YIELD_EVERY == 0 then + ngx.sleep(0) + -- do not hold up a reload: what is left is gone with the worker anyway + if ngx.worker.exiting() then + return + end + now = ngx.now() + end + + -- re-read: while this round yielded the slot may have been renewed, or + -- dropped by add() + local i = due[j] local deadline = self.expire_keys[i] if deadline and deadline <= now then -- Read i-th key. If it is nil or ttl is < 0, it means it was expired local ttl, err = self.dict:ttl(self.key_prefix .. i) if not (ttl and ttl >= 0 or err and err ~= "not found") then - if self.keys[i] then - self.index[self.keys[i]] = nil - self.keys[i] = nil + local key = self.keys[i] + -- the key may have moved to another slot meanwhile; that mapping is + -- live and is not this slot's to drop + if key and self.index[key] == i then + self.index[key] = nil end + self.keys[i] = nil self.expire_keys[i] = nil else -- still there: another worker renewed it, so wait out the new ttl @@ -173,8 +182,30 @@ function KeyIndex:remove_expired_keys() end end end +end + + +-- check and remove expired keys +function KeyIndex:remove_expired_keys() + -- A round yields, so the timer can fire again before the previous run + -- finished (ngx.timer.every does not wait for it). Two rounds would only ask + -- the dict about the same slots twice, so the second one steps aside. + if self.removing then + return + end + + self.removing = true + -- pcall so that a throwing ngx.sleep -- a caller reaching this from a phase + -- that does not allow yielding -- cannot leave the guard set and stop the + -- local scan for the life of the worker. + local ok, err = pcall(remove_expired_slots, self) + self.removing = false + + if not ok then + ngx.log(ngx.ERR, "failed to remove expired keys: ", tostring(err)) + end - -- The loop above only drops worker-local references, so the expired entries + -- The scan above only drops worker-local references, so the expired entries -- still have to be reclaimed from the dict itself. Callers that schedule -- flush_expired() themselves -- in a single process rather than in every -- worker -- turn this off with auto_flush_expired. diff --git a/prometheus_test.lua b/prometheus_test.lua index 52bbaa8..532a9c0 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -123,6 +123,9 @@ Nginx.worker = {} function Nginx.worker.id() return 'testworker' end +function Nginx.worker.exiting() + return false +end function Nginx.sleep() end function Nginx.now() -- the SimpleDict mock keeps expiry in os.time() units @@ -1027,13 +1030,13 @@ function TestKeyIndex:testRemoveExpiredKeysSkipsSlotsThatCannotHaveLapsed() end -- The slots whose deadline has passed do have to be asked about, and there can --- be as many of them as the index is large, so the scan walks a bounded slice --- per round and resumes where the previous round stopped. -function TestKeyIndex:testRemoveExpiredKeysScansASlicePerRound() - local slice = self.key_index.REMOVE_EXPIRED_KEYS_SCAN_SLOTS +-- be as many of them as the index is large. One round asks about all of them, +-- so the local index keeps up with the expiry rate, but yields every +-- REMOVE_EXPIRED_KEYS_YIELD_EVERY of them so the worker keeps serving. +local function lapse_all_slots(self, prefix, count) local keys = {} - for i = 1, slice + 10 do - keys[i] = "sliced_" .. i + for i = 1, count do + keys[i] = prefix .. i end luaunit.assertEquals(self.key_index:add(keys, "eviction_err", 600), nil) @@ -1043,36 +1046,108 @@ function TestKeyIndex:testRemoveExpiredKeysScansASlicePerRound() self.key_index.expire_keys[i] = 0 end - local checked = 0 + local counter = {checked = 0} local ttl = SimpleDict.ttl self.dict.ttl = function(dict, k) - checked = checked + 1 + counter.checked = counter.checked + 1 return ttl(dict, k) end + return counter +end - self.key_index:remove_expired_keys() - local first = checked - luaunit.assertTrue(first <= slice) - -- it really did stop short of the whole range - luaunit.assertTrue(first < slice + 10) +function TestKeyIndex:testRemoveExpiredKeysYieldsWhileScanning() + local every = self.key_index.REMOVE_EXPIRED_KEYS_YIELD_EVERY + local counter = lapse_all_slots(self, "yield_", 2 * every + 10) - checked = 0 + local yields, checked_at_yield = 0, {} + ngx.sleep = function() + yields = yields + 1 + checked_at_yield[yields] = counter.checked + end self.key_index:remove_expired_keys() - -- the next round picks up the rest, and each slot is asked about once - luaunit.assertEquals(first + checked, slice + 10) + ngx.sleep = nil + + -- every lapsed slot is asked about in this one round, in steps of `every` + luaunit.assertEquals(counter.checked, 2 * every + 10) + luaunit.assertEquals(checked_at_yield, {every, 2 * every}) -- the slots are all still live, so they are kept, and their deadlines are -- refreshed rather than re-checked on every round from now on - luaunit.assertEquals(self.key_index.index["sliced_1"], 1) - luaunit.assertEquals(self.key_index.index["sliced_" .. (slice + 10)], slice + 10) - checked = 0 + luaunit.assertEquals(self.key_index.index["yield_1"], 1) + luaunit.assertEquals(self.key_index.index["yield_" .. (2 * every + 10)], + 2 * every + 10) + counter.checked = 0 self.key_index:remove_expired_keys() - luaunit.assertEquals(checked, 0) + luaunit.assertEquals(counter.checked, 0) + + self.dict.ttl = nil + luaunit.assertEquals(ngx.logs, nil) +end + +-- A round yields, so the timer can fire again before it finishes. The second +-- round must step aside instead of asking the dict about the same slots again. +function TestKeyIndex:testRemoveExpiredKeysDoesNotOverlap() + local every = self.key_index.REMOVE_EXPIRED_KEYS_YIELD_EVERY + local counter = lapse_all_slots(self, "overlap_", every + 10) + + local checked_by_reentry + ngx.sleep = function() + if checked_by_reentry == nil then + local before = counter.checked + self.key_index:remove_expired_keys() + checked_by_reentry = counter.checked - before + end + end + self.key_index:remove_expired_keys() + ngx.sleep = nil + + luaunit.assertEquals(checked_by_reentry, 0) + luaunit.assertEquals(counter.checked, every + 10) self.dict.ttl = nil luaunit.assertEquals(ngx.logs, nil) end +-- A worker that is shutting down stops scanning at the next yield, so it does +-- not hold up a reload. +function TestKeyIndex:testRemoveExpiredKeysStopsWhenExiting() + local every = self.key_index.REMOVE_EXPIRED_KEYS_YIELD_EVERY + local counter = lapse_all_slots(self, "exiting_", every + 10) + + ngx.worker.exiting = function() return true end + self.key_index:remove_expired_keys() + ngx.worker.exiting = function() return false end + + luaunit.assertEquals(counter.checked, every) + + self.dict.ttl = nil + luaunit.assertEquals(ngx.logs, nil) +end + +-- A throwing ngx.sleep -- a caller reaching remove_expired_keys() from a phase +-- that does not allow yielding -- must not leave the guard set, or the worker +-- would stop scanning for good. +function TestKeyIndex:testRemoveExpiredKeysReleasesTheGuardAfterAnError() + local every = self.key_index.REMOVE_EXPIRED_KEYS_YIELD_EVERY + local counter = lapse_all_slots(self, "error_", every + 10) + + ngx.sleep = function() error("API disabled in the current context") end + self.key_index:remove_expired_keys() + ngx.sleep = nil + + luaunit.assertEquals(#ngx.logs, 1) + luaunit.assertStrContains(ngx.logs[1], "failed to remove expired keys") + ngx.logs = nil + luaunit.assertEquals(counter.checked, every) + + -- the guard is released, so the next round asks about the rest + counter.checked = 0 + self.key_index:remove_expired_keys() + luaunit.assertEquals(counter.checked, 10) + + self.dict.ttl = nil +end + -- One round yields between batches, so the timer can fire again before it -- finishes -- ngx.timer.every does not wait for the previous run. The second -- round must step aside instead of racing this one for the dict lock.