Skip to content

fix(prometheus): reclaim expired metric entries in bounded batches - #13981

Closed
AlinsRan wants to merge 1 commit into
apache:masterfrom
AlinsRan:fix/prometheus-flush-expired-in-privileged-agent
Closed

AlinsRan wants to merge 1 commit into
apache:masterfrom
AlinsRan:fix/prometheus-flush-expired-in-privileged-agent

Conversation

@AlinsRan

Copy link
Copy Markdown
Contributor

Description

Follow-up to #13754 / #13658.

Expired entries in the prometheus-metrics shared dict are only logically dead: every dict API reports them as missing, but their slab pages stay allocated. The passive per-write expiry scan cannot reclaim them, 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 expire) inevitably ends up sitting there. The dict then fills up with dead entries and starts force-evicting live ones, which is #13658.

nginx-lua-prometheus-api7 1.0.0 fixed that by calling flush_expired() from remove_expired_keys(). Two properties of that call are worth addressing, as raised in #13658 (comment):

  • it is unbounded, and it holds the shared dict lock for the whole LRU walk, so an hour's backlog is reclaimed in a single uninterrupted hold;
  • it runs in every worker, although one process reclaiming is enough.

This PR drains the dict from the privileged agent, in bounded batches:

  • flush_expired(10000) per call, with a 1s pause between batches, at most 30 batches per tick;
  • a call that frees less than a batch has already walked the whole queue, so the loop stops there;
  • the timer only starts when at least one metric is configured with an expire, since nothing in the dict expires otherwise;
  • the interval is configurable through plugin_attr.prometheus.flush_expired_interval, default 60s.

The library timer still runs hourly in each worker, but with the backlog already drained it finds nothing left to reclaim, so its walk is cheap.

Measured on OpenResty 1.29.2.4 with a 512m dict, a permanent entry pinning the LRU tail (single process, resty, no lock contention):

entries expired lock hold of one flush_expired()
301k 1k ~3ms (walks the whole queue)
750k 749.7k ~70ms, unbounded
750k 749.7k ~1ms per flush_expired(10000) call

Reclaim work is ~0.09µs per entry freed and ~0.01µs per live node walked, so a batch of 10000 is ~1ms.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Expired entries in the `prometheus-metrics` shared dict are only
logically dead: every dict API reports them as missing, but their slab
pages stay allocated. The passive per-write expiry scan cannot reclaim
them, 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 expire) inevitably ends up sitting there, so the dict fills
up with dead entries and starts force-evicting live ones (apache#13658).

nginx-lua-prometheus-api7 1.0.0 reclaims them from every worker on an
hour-long timer with an unbounded `flush_expired()` call. That call
holds the dict lock for the whole LRU walk, so a backlog that built up
over an hour is reclaimed in one uninterrupted hold, and every worker
repeats the walk.

Drain them from the privileged agent instead, in batches of 10000 with
a pause in between, so no single call can hold the lock for long: a
batch is ~1ms of reclaim work, and a call that finds less than a batch
has already walked the whole queue. The library timer still runs, but
by then there is nothing left for it to reclaim.

The timer only starts when at least one metric is configured with an
`expire`, since nothing in the dict expires otherwise. Its interval is
configurable through `plugin_attr.prometheus.flush_expired_interval`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant