fix(batch-processor): bound the pending entries backlog by default - #13826
Open
nic-6443 wants to merge 5 commits into
Open
fix(batch-processor): bound the pending entries backlog by default#13826nic-6443 wants to merge 5 commits into
nic-6443 wants to merge 5 commits into
Conversation
max_pending_entries was only honoured when the plugin's metadata configured it, so out of the box the batch processor kept every undelivered entry in worker memory. A log server that is slow or unreachable therefore grew the worker's memory with the request rate until the container hit its limit, which is most visible when include_req_body / include_resp_body are on and each entry carries a copy of both bodies. The limit now defaults to 16384 entries, and the fallback lives in the batch processor manager rather than in each plugin, so a logger cannot be wired up without it. datadog, lago, loggly, sls-logger and syslog went through the same manager but never exposed the knob at all; they do now, along with the stream subsystem's syslog plugin. Two supporting changes come with it: - the manager reads the metadata itself, given the plugin name that new() now takes, instead of each plugin repeating the same three-line lookup and passing the value down; - discards are reported at most once per second with a running count. One line per discarded entry would turn the outage that causes the discards into a log flood. The default is what fits a 128 MB backlog budget for a representative request+response body of 1 KB each; docs/en/latest/batch-processor.md records the measured memory for other body sizes.
dosubot
Bot
added
the
size:L
Four defects in the previous commit, all from review: - the limit was off by one. `pending > max` was inherited from the original check, but this commit adds a default and describes the field as the maximum, so the two have to agree: a limit of 3 now retains 3 entries. - a discarded entry was counted twice. add_entry() returned nil, the caller fell through to add_entry_to_new_processor(), and that re-ran the check. Counting and reporting now happen once, in add_entry(); the second function still checks the backlog so a direct caller stays bounded. - summing the processed entries walked every buffer on every logged request, which is O(number of plugin configurations) and, before this series, did not run at all for the users who never set the limit. The count only grows, so the last one read bounds the backlog from above and the walk can be skipped until that bound reaches the limit -- roughly once every max_pending_entries entries on a healthy system. - lago built its manager without its plugin name, so it looked metadata up under "lago logger" and ignored any configured override. The tests grow to cover each of these: exact accepted counts at the limit, the reported discard counts across the reporting interval, metadata validation for every logger including the stream syslog plugin, an end-to-end case driving sls-logger through the Admin API, and a check that every logger's batch processor reads its own plugin's metadata -- which is what catches the lago mistake.
There was a problem hiding this comment.
Pull request overview
This PR prevents unbounded worker memory growth in batch-processor-based loggers by enforcing a default pending-entry backlog limit (16384) and centralizing the max_pending_entries metadata handling in batch-processor-manager, including rate-limited discard reporting.
Changes:
- Add a default
max_pending_entriescap (16384) inbatch-processor-managerand emit discard summaries at most once per second. - Move
max_pending_entriesmetadata lookup + schema injection into the manager, and update batch-processor loggers (and stream syslog) to use it. - Add tests and update docs (EN/ZH) to describe the new default and how to tune it.
Reviewed changes
Copilot reviewed 69 out of 69 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| t/utils/batch-processor-manager.t | New tests for default cap, metadata override, discard log throttling, and knob exposure across loggers |
| t/plugin/udp-logger.t | Update test hook for new manager function signature |
| t/plugin/tcp-logger.t | Update test hook for new manager function signature |
| t/plugin/syslog.t | Update test hook for new manager function signatures |
| t/plugin/sls-logger.t | Update test hook for new manager function signature |
| t/plugin/rocketmq-logger2.t | Update test hook for new manager function signatures |
| t/plugin/rocketmq-logger.t | Update test hook for new manager function signatures |
| t/plugin/rocketmq-logger-log-format.t | Update test hook for new manager function signatures |
| t/plugin/kafka-logger4.t | Update test hook for new manager function signatures |
| t/plugin/kafka-logger2.t | Update test hook for new manager function signatures |
| t/plugin/kafka-logger.t | Update test hook for new manager function signatures |
| t/plugin/kafka-logger-log-format.t | Update test hook for new manager function signatures |
| t/plugin/kafka-logger-large-body.t | Update test hook for new manager function signatures |
| t/plugin/ai-proxy-kafka-log.t | Update test hook for new manager function signatures |
| docs/zh/latest/plugins/udp-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/tencent-cloud-cls.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/tcp-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/syslog.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/splunk-hec-logging.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/sls-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/skywalking-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/rocketmq-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/loki-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/loggly.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/kafka-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/http-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/google-cloud-logging.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/elasticsearch-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/datadog.md | Document max_pending_entries default and behavior |
| docs/zh/latest/plugins/clickhouse-logger.md | Document max_pending_entries default and behavior |
| docs/zh/latest/batch-processor.md | Add backlog-limiting section and update integration example |
| docs/en/latest/plugins/udp-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/tencent-cloud-cls.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/tcp-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/syslog.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/splunk-hec-logging.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/sls-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/skywalking-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/rocketmq-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/loki-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/loggly.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/lago.md | Add Plugin Metadata section exposing max_pending_entries |
| docs/en/latest/plugins/kafka-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/http-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/google-cloud-logging.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/elasticsearch-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/datadog.md | Document max_pending_entries default and behavior |
| docs/en/latest/plugins/clickhouse-logger.md | Document max_pending_entries default and behavior |
| docs/en/latest/batch-processor.md | Add backlog-limiting section and update integration example |
| apisix/utils/batch-processor-manager.lua | Implement default cap + discard summary logging and centralize metadata lookup/schema injection |
| apisix/stream/plugins/syslog.lua | Use manager metadata schema wrapping and correct metadata name wiring |
| apisix/plugins/udp-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/tencent-cloud-cls.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/tcp-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/syslog/init.lua | Instantiate manager with explicit plugin name for shared http/stream syslog |
| apisix/plugins/syslog.lua | Use manager metadata schema wrapping |
| apisix/plugins/splunk-hec-logging.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/sls-logger.lua | Use manager metadata schema wrapping |
| apisix/plugins/skywalking-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/rocketmq-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/loki-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/loggly.lua | Use manager metadata schema wrapping |
| apisix/plugins/lago.lua | Add metadata schema exposure + metadata-type schema checking |
| apisix/plugins/kafka-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/http-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/google-cloud-logging.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/elasticsearch-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
| apisix/plugins/datadog.lua | Use manager metadata schema wrapping |
| apisix/plugins/clickhouse-logger.lua | Use manager metadata schema wrapping and remove per-plugin metadata lookup/plumbing |
Suppressed comments (1)
apisix/utils/batch-processor-manager.lua:180
- Same as add_entry(): add_entry_to_new_processor() returns nil on discard, which makes it hard for callers/tests to distinguish “discarded” from other failure modes and contributes to double-counting when both paths are attempted.
function _M:add_entry_to_new_processor(conf, entry, ctx, func)
if should_discard(self) then
return
end
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Three metadata tables carry a "Valid values" column, and the new rows left it blank where the neighbouring rows fill it in.
16384 was derived from the memory a pending entry holds on its own. The end-to-end measurement that followed showed the real cost is about 2.4x that, because batches already handed to the sender keep both their entries and the payload serialized from them: with 1 KB request and response bodies logged, a stalled log server grew the worker by 172 MB, over the 128 MB the value was chosen to fit. 8192 brings that case to 98 MB, inside the budget, and still leaves roughly eight times the backlog a healthy system carries -- which tracks batch_max_size rather than throughput, so the docs now also say to raise this limit alongside batch_max_size.
TEST 5 only checked that a batch processor's plugin name was some logger, so a logger reading another logger's metadata would have passed. Load each module on its own and require the managers it builds to name that module's plugin. Checked against both mistakes: dropping lago's plugin name, and pointing http-logger at kafka-logger's metadata. The end-to-end case also ignored its request results, which would let the discard assertion pass for the wrong reason if the route never applied.
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.
max_pending_entriesis only honoured when the plugin's metadata configures it, so out of the box the batch processor keeps every undelivered entry in worker memory. A log server that is slow or unreachable therefore grows the worker's memory with the request rate until the process hits its limit. It's most visible withinclude_req_body/include_resp_bodyon, since each entry then carries a copy of both bodies.The limit now defaults to 8192, and the fallback lives in
batch-processor-managerrather than in each plugin, so a logger can't be wired up without it. That also fixes the other half of the problem:datadog,lago,loggly,sls-loggerandsyslogall go through the same manager but never exposed the knob at all. They do now, as does the stream subsystem'ssyslog.Two things come along with it:
new()now takes, instead of twelve plugins repeating the same three lines and passing the value down;Picking the default
Two measurements. The first isolates what a single pending entry holds, by parking entries in the buffer so nothing ever flushes, against a log endpoint that accepts the connection and never answers. An equal-size control run over a route with no logger is subtracted, so the figure is the entry rather than the traffic behind it. Single worker,
http-logger, both bodies logged:The second runs the whole plugin with stock batch processor settings against the same dead endpoint, which is what the limit actually costs a worker. It comes out around 2.4x the entries alone, because batches already handed to the sender hold both their entries and the payload serialized from them — so the first table alone would have picked a default about twice too large.
8192 is the largest power of two whose end-to-end cost stays inside a 128 MB budget for a representative body-logging setup, 1 KB request plus 1 KB response.
pendingplateaus at exactly 8192 in every run, which is the cap holding.The right column is why a default this size is safe to ship: with a log server that keeps up, the backlog sits under 1000 entries whatever the rate, because it tracks
batch_max_sizerather than throughput. That leaves roughly eight times the room healthy operation needs. Anyone who raisesbatch_max_sizeshould raise this with it, whichbatch-processor.mdnow says.Larger bodies still exceed the budget at the default — 16 KB bodies reach 839 MB — so that table is in the docs with a note to lower the limit when logging bodies larger than a few KB.
Tests
t/utils/batch-processor-manager.tcovers the cap applying with no metadata configured, metadata overriding it, the once-per-second discard reporting with exact counts, every logger exposing and validating the knob, an end-to-end case through the Admin API onsls-logger, and a check that each logger's batch processor reads its own plugin's metadata. All of them fail on master.