Skip to content

perf(loggly): hoist per-request closure out of log phase - #13648

Merged
shreemaan-abhishek merged 2 commits into
apache:masterfrom
shreemaan-abhishek:fix/loggly-per-request-closure
Jul 6, 2026
Merged

shreemaan-abhishek merged 2 commits into
apache:masterfrom
shreemaan-abhishek:fix/loggly-per-request-closure

Conversation

@shreemaan-abhishek

@shreemaan-abhishek shreemaan-abhishek commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Description

The loggly logger reassigned the handle_http_payload closure on every request inside _M.log(), solely to capture conf. Each request therefore allocated a fresh function object, adding avoidable GC pressure in the log phase. Benchmarking loggers showed loggly carrying a disproportionately high per-request overhead compared to peers in the same class, and this per-request allocation was the cause (P50 is unaffected since the actual send happens in the async batch-processor timer).

This PR hoists the handler to module scope: conf is threaded into handle_log(entries, conf) directly, and the small binding closure is created once per batch processor (i.e. once per config version) rather than once per request. The separate handle_http_payload indirection is removed.

As a side effect this also removes a latent correctness bug: the shared module-level handle_http_payload was overwritten by whichever request ran _M.log() last, so with two routes using different loggly configs the async handler could send a batch with the wrong conf. Binding conf per processor fixes that.

Behavior is otherwise unchanged; existing t/plugin/loggly.t cases (including TEST 15, the http bulk path that asserts conf.tags) continue to cover both the syslog and http paths.

Which issue(s) this PR fixes:

Fixes #

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)

The log phase reassigned the handle_http_payload closure on every
request just to capture conf, allocating a new function object per
request and adding GC pressure. Thread conf into handle_log directly
and build the batch-processor handler once per config version instead
of per request. This also removes a latent bug where the shared
module-level closure could resolve the wrong conf when multiple routes
use different loggly configs.
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. performance generate flamegraph for the current PR labels Jul 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces per-request overhead in the loggly plugin by removing a per-request function allocation in the log phase and ensuring the async batch handler uses the correct plugin conf when multiple routes use different loggly configurations.

Changes:

  • Hoists the HTTP bulk-send logic into handle_log(entries, conf) so conf is passed explicitly instead of captured via a per-request closure.
  • Removes the mutable module-level handle_http_payload indirection to avoid cross-route config mix-ups in async processing.
  • Binds conf once per batch processor (via the batch handler closure passed to batch_processor_manager:add_entry_to_new_processor) rather than once per request.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Missing regression coverage for the cross-config wrong-conf bug

The code change looks right, but the PR claims to fix a bug where the module-level handle_http_payload could be overwritten by a later request from a different loggly configuration. The patch does not add a test that would fail on the old behavior: two routes/configs with different customer_token/tags, delayed batch flush, and interleaved requests.

Please add a Test::Nginx regression case that creates two distinct HTTP bulk loggly configs, sends interleaved requests before the first batch flushes, and asserts each batch is sent with its own token/tags. Without that coverage, this correctness fix can regress unnoticed.

Two http-bulk loggly configs with distinct tokens/tags buffer one
entry each before either batch flushes; asserts each batch is sent to
its own endpoint. Fails against the old shared module-level closure,
which sent both batches with the last request's conf.
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 3, 2026
@shreemaan-abhishek

Copy link
Copy Markdown
Contributor Author

Added a regression test (TEST 22 in t/plugin/loggly.t) covering exactly this case:

  • Two routes with distinct HTTP-bulk loggly configs (customer_token token-a/token-b, tags aaa/bbb).
  • Both send one entry each, which buffer before either batch flushes (inactive_timeout: 1, default batch size), so the two batch processors flush after both log() calls have run.
  • The mock bulk endpoint captures the token from the request path and the X-LOGGLY-TAG header, and the test asserts each batch arrives with its own token/tags.

This fails on the previous behavior: the shared module-level handle_http_payload was left holding whichever request ran log() last, so both batches were sent with token-b and the token-a assertion never matches. With this PR each batch processor binds its own conf, so both assertions pass.

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added regression coverage now addresses the previous cross-config concern. No merge-blocking issue remains from my side.

@shreemaan-abhishek
shreemaan-abhishek merged commit 4756428 into apache:master Jul 6, 2026
19 of 22 checks passed
@kayx23 kayx23 mentioned this pull request Aug 18, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance generate flamegraph for the current PR size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants