diff --git a/apisix/plugins/prometheus/exporter.lua b/apisix/plugins/prometheus/exporter.lua index fbb600e18687..0fdfbb3049ca 100644 --- a/apisix/plugins/prometheus/exporter.lua +++ b/apisix/plugins/prometheus/exporter.lua @@ -89,6 +89,21 @@ end -- Default refresh interval local DEFAULT_REFRESH_INTERVAL = 15 +-- Default interval for reclaiming expired entries from the metrics shared dict +local DEFAULT_FLUSH_EXPIRED_INTERVAL = 60 + +-- Entries a single flush_expired() call may reclaim. The call holds the shared +-- dict lock until it returns, so this is what bounds how long the workers can +-- be kept waiting on that lock: measured at ~1ms per 10000 entries. +local FLUSH_EXPIRED_BATCH = 10000 + +-- Upper bound on the batches run by one tick, so the loop always finishes well +-- within one interval. Whatever 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 + local CACHED_METRICS_KEY = "cached_metrics_text" local metrics = {} @@ -99,6 +114,10 @@ local exporter_timer_running = false local exporter_timer_created = false +local flush_expired_timer_running = false + +local flush_expired_timer_created = false + local function gen_arr(...) clear_tab(inner_tab_arr) for i = 1, select('#', ...) do @@ -1201,6 +1220,105 @@ local function exporter_timer(premature, yieldable, cache_exptime) end +-- Expired entries in the metrics shared dict are only logically dead: every +-- dict API reports them as missing, but their slab pages stay allocated until +-- something reclaims them. The passive per-write expiry scan cannot do it, +-- 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. Left alone, the dict fills up with +-- dead entries and starts evicting live ones (apache/apisix#13658). The metrics +-- library reclaims them from every worker once an hour with an unbounded +-- flush_expired() call, which walks the whole LRU queue with the dict lock +-- held. Draining them here instead -- in the privileged agent, in bounded +-- batches -- keeps that backlog, and therefore the lock hold of any single +-- call, small. +local function flush_expired_metrics() + local dict = ngx.shared["prometheus-metrics"] + if not dict then + return 0 + end + + local total = 0 + for _ = 1, FLUSH_EXPIRED_MAX_BATCHES do + -- a return value below the batch size means this call has already + -- walked the whole LRU queue, so there is nothing left to reclaim + local freed = dict:flush_expired(FLUSH_EXPIRED_BATCH) + total = total + freed + if freed < FLUSH_EXPIRED_BATCH then + break + end + + ngx.sleep(FLUSH_EXPIRED_BATCH_DELAY) + end + + return total +end +_M.flush_expired_metrics = flush_expired_metrics + + +local function flush_expired_timer(premature) + if premature then + return + end + + if flush_expired_timer_running then + core.log.warn("Previous metrics flush still running, skipping") + return + end + + flush_expired_timer_running = true + + local ok, err = pcall(flush_expired_metrics) + if not ok then + core.log.error("Failed to flush expired metrics: ", err) + end + + flush_expired_timer_running = false +end + + +-- Nothing in the dict ever becomes expired unless at least one metric is +-- registered with an expire, so the timer is only worth running then. +local function metrics_expire_enabled(attr) + local metrics_attr = attr and attr.metrics + if type(metrics_attr) ~= "table" then + return false + end + + for _, conf in pairs(metrics_attr) do + local expire = type(conf) == "table" and tonumber(conf.expire) + if expire and expire > 0 then + return true + end + end + + return false +end + + +local function init_flush_expired_timer(attr) + if flush_expired_timer_created or not metrics_expire_enabled(attr) then + return + end + + local interval = DEFAULT_FLUSH_EXPIRED_INTERVAL + if attr and attr.flush_expired_interval then + interval = attr.flush_expired_interval + end + + if interval <= 0 then + return + end + + local ok, err = ngx_timer_every(interval, flush_expired_timer) + if ok then + flush_expired_timer_created = true + else + core.log.error("Failed to start the metrics flush timer: ", err) + end +end + + local function init_exporter_timer() if process.type() ~= "privileged agent" then return @@ -1214,6 +1332,8 @@ local function init_exporter_timer() local cache_exptime = refresh_interval * 2 + init_flush_expired_timer(attr) + exporter_timer(false, false, cache_exptime) if exporter_timer_created then diff --git a/conf/config.yaml.example b/conf/config.yaml.example index 6a1361aec496..bb93121fdc38 100644 --- a/conf/config.yaml.example +++ b/conf/config.yaml.example @@ -724,6 +724,9 @@ plugin_attr: # Plugin attributes ip: 127.0.0.1 # Set the IP. port: 9091 # Set the port. refresh_interval: 15 # Set the interval for refreshing cached metric data. unit: second. + flush_expired_interval: 60 # Set the interval for reclaiming expired entries from the + # `prometheus-metrics` shared dict. Only used when a metric + # below is configured with `expire`. unit: second. # metrics: # Create extra labels from nginx variables: https://nginx.org/en/docs/varindex.html # http_status: # expire: 0 # The expiration time after which metrics are removed. unit: second. diff --git a/t/plugin/prometheus-metric-expire.t b/t/plugin/prometheus-metric-expire.t index caad85ea04a5..dd30201ff390 100644 --- a/t/plugin/prometheus-metric-expire.t +++ b/t/plugin/prometheus-metric-expire.t @@ -130,3 +130,34 @@ plugin_attr: GET /t --- response_body passed + + + +=== TEST 2: expired entries are reclaimed in bounded batches +--- config + location /t { + content_by_lua_block { + local exporter = require("apisix.plugins.prometheus.exporter") + local dict = ngx.shared["prometheus-metrics"] + + -- a permanent entry kept at the LRU tail stops the passive + -- per-write expiry scan, which is what lets expired entries pile + -- up in the first place + dict:set("flush_expired_tail", 1) + for i = 1, 20000 do + dict:set("flush_expired_" .. i, "v", 0.1) + end + ngx.sleep(0.2) + + -- more than one batch, so the loop has to run again after the + -- delay instead of stopping at the first batch + local freed = exporter.flush_expired_metrics() + ngx.say("reclaimed all: ", freed >= 20000) + ngx.say("left over: ", exporter.flush_expired_metrics()) + ngx.say("tail kept: ", dict:get("flush_expired_tail")) + } + } +--- response_body +reclaimed all: true +left over: 0 +tail kept: 1