Conversation
A metric that expired and came back consumed a new slot number and bumped delete_count, which sends every worker through a full sync of key_count slots -- on the request path, since sync() is the first thing add() does and add() runs on every observation of a metric with an exptime. Measured on the shape of a gateway pod (10 workers, 512m dict, the three exporter.lua metrics, 140k label combinations, 200 req/s): one such return took the worker set from 14.9% to 30.2% CPU, and 50 of them over ten seconds put three workers at 98.6%, 93.3% and 87.9% of a core. Three changes, and none of them adds shared state: ttl() tells apart the two states get() reports alike. A slot past its ttl whose node is still in the dict is the key's own -- expire() resurrects it with its value intact -- so the reclaim round and sync_range leave it alone, and the key comes back on the same number. Listing it meanwhile costs nothing: metric_data() skips a key whose value has expired. Only a slot whose node is gone gives its number up. On its own this is most of the bloat: 100 metrics expiring and returning used to take 100 new numbers, or 2 when nothing disturbed the workers; now they take 0. Renewal reads its own slot and renews it there, instead of starting with sync(). That is one dict read where sync() is two, and it touches none of the shared counters, so an external bump cannot make the request path scan: 15.7% against 287.7% under a storm of 50. The broadcast is gone. What it protected was not the duplicate metrics of #11934 -- the index check in list() does that -- but a peer dropping the index entry of a key that had since moved to another slot, which hid a live metric from that peer's scrape. Dropping an index entry only when it still points at the slot being given up fixes that locally, in one line, and a fresh slot is above every worker's self.last anyway, so their incremental sync finds it. Verified on 10 workers at 300k series with 1,500 new series/s: no duplicates, no live value missing from the output, counter values exactly as driven.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
2 tasks
AlinsRan
marked this pull request as draft
September 29, 2026 06:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A metric that expired and came back consumed a new slot number and bumped
delete_count, which sends every worker through a full sync ofkey_countslots — on the request path, since
sync()is the first thingadd()does andadd()runs on every observation of a metric with anexptime.Reproduced on the shape of a gateway pod: 10 workers, a 512m dict, the three
metrics of
apisix/plugins/prometheus/exporter.luawith their label sets and thedefault latency buckets, 140k label combinations, 200 req/s of exactly what
exporter.http_log()does. CPU summed over the workers, 10s windows:delete_count+1)The third row is bumped externally, and this PR does not react at all: its
request path no longer reads the shared counters.
What this fixes
key_countclimbs monotonically, so a worker's catch-up sync after a reload gets slower and never comes back downexpire()could still have resurrectedttl(), notget()delete_countbump makes every worker forget every slot that is merely past its ttl, so all of those metrics take new numbers tooget(), which cannot tell that state from a reclaimed nodedelete_countbump: +100 numbers → +0toppicture in the reports: one worker at ~100% CPUdelete_count, which sends all 10 workers through a full sync ofkey_countslots — on the request path/metricsself.index[key]without checking that it still points at that slot — by then it points at the live oneforget_slot()A and B are
key_countgrowth; C is the CPU spike; D is what the broadcast wasactually protecting, and the reason it could not simply be deleted. The rest of
this description is the evidence behind each row.
Why the broadcast multiplies slot growth
Row B is the part that is easy to miss, so here it is step by step.
delete_countis not just a CPU cost: it is what turns one metric's return into hundreds of new
slot numbers.
Worker A observes metric
Kwhose slot's node has been reclaimed, soexpire()returns"not found". v1.0.0 givesKa fresh number andbumps
delete_count.Every other worker's next
sync()findsself.deleted ~= delete_count, so itruns
sync_range(0, key_count)— the whole range — instead ofsync_range(self.last, key_count).Inside that walk,
get()returns nil for a slot that is merely past itsttl exactly as it does for one whose node is gone. The old code cannot tell
them apart, so its
elseif self.keys[i]branch drops the local reference forboth:
So that worker now has no slot for every metric that happens to be expired at
that moment — even though every one of those nodes is still in the dict and
expire()would have resurrected it in place.Each of those metrics, on its next observation, finds
self.index[key] == nil,skips the renewal path entirely and allocates a new number at
key_count + 1.Two things make it worse. The worker that bumps does not pay: it raises its own
self.deletedfirst, so the walk — and the forgetting — lands on the other nine.And it compounds: every new number raises
key_count, which makes the next fullsync more expensive and leaves more dead slots behind, each of which is another
future
"not found"and another bump.The difference between the incremental and the full walk is the whole
multiplier. An incremental sync only covers
[self.last, key_count], so withouta bump the damage is confined to the top of the range; the bump widens it to
every slot the worker knows.
Measured on v1.0.0's own code — 100 metrics with a 1s exptime, all of them
expiring and then being observed again:
delete_countThe first row is what should happen: the metrics come back on their own numbers.
The second row is the broadcast turning that into a hundred new ones. The last
two are the residual case this PR does not address — a number whose node is gone
is not the key's any more, and reusing it is what #24 is for.
Three changes, no new shared state
ttl()tells apart the two statesget()reports alike. A slot past its ttlwhose node is still in the dict is the key's own —
expire()resurrects it withits value intact — so the reclaim round and
sync_rangeleave it alone and thekey comes back on the same number. Listing it meanwhile costs nothing:
metric_data()skips a key whose value has expired. Only a slot whose node isgone (
ttl()reports"not found") gives its number up.That one distinction is most of the bloat, as the table above shows.
Renewal reads its own slot, instead of starting with
sync():One dict read where
sync()is two, and it touches none of the shared counters,so an external bump cannot make the request path scan.
The broadcast is gone. What it protected was not the duplicate metrics of
apache/apisix#11934 — the
index[key] == idxcheck inlist()does that — but apeer dropping the index entry of a key that had since moved to another slot,
which hid a live metric from that peer's scrape. Measured on 1.0.0, with the
dead slot below the peer's incremental range:
The guard is one line: drop an index entry only while it still points at the slot
being given up. And a fresh slot is above every worker's
self.lastanyway, sotheir incremental sync finds it — nothing has to be broadcast.
What this leaves
A metric whose node was already reclaimed still takes a new number, and numbers
are never reused, so
key_countkeeps growing with genuinely new label sets andwith returns that come more than a reclaim interval late. That costs one thing:
the full catch-up sync a worker does at startup,
key_count× ~0.45µs. Slotreuse (#24) addresses it and stays deferred — it needs a way to tell the scrape
that a slot below its
lastchanged, which is the only part of this work thatwould add shared state.
Verification
10 workers plus the privileged agent, checks run inside the scraping process
against a walk of the dict itself, at 300k series with 1,500 new series/s:
[error]lineslisted_expired: 5138in that run is the deliberate trade: a key whose node isstill there stays listed, and
metric_data()skips it — 5k wasted value readsagainst 305k slots, in exchange for not consuming a number.
Unit tests: 50, 49 pass.
TestPrometheus.testPrintfTablefails onmainaswell, under LuaJIT, and is unrelated. The
SimpleDictmock now models the threeslot states (
get()no longer prunes,ttl()returns a negative number for anode past its ttl and
"not found"once it is freed,expire()resurrects one,add()replaces one in place), so the tests that assumed the old semantics wererewritten rather than patched.