[libbeat/processing] add a new shared processor wrapper - #50713
Conversation
🤖 GitHub commentsJust comment with:
|
02a95af to
c9457c0
Compare
c9457c0 to
61b4346
Compare
This comment has been minimized.
This comment has been minimized.
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
TL;DR
Remediation
Investigation detailsRoot CauseThe header check stage ( Evidence
Verification
Follow-up
Note 🔒 Integrity filter blocked 3 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | noneWhat is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
90bb633 to
c65f38f
Compare
c0ea245 to
1c43f6d
Compare
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
1c43f6d to
9cb291f
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a shared processor wrapper that caches processor instances by a configuration hash with reference counting and lifecycle-managed Close. The kubernetes metadata processor is registered using shared.New(New) and a test helper signature was fixed. New concurrency tests for both the shared wrapper and the kubernetes annotator validate caching, reference-counted Close, and event isolation under concurrent Run/Close and concurrent cache mutations. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libbeat/processors/shared/shared.go`:
- Around line 59-61: The sharedProcessors map currently hands out the same
wrapper instance so duplicate Close() calls from different borrowers can
decrement p.refCount multiple times; fix by introducing a per-borrower handle
type (e.g., sharedHandle) that holds a pointer to the shared entry (e.g.,
sharedEntry with proc and refCount) plus its own closed flag and mutex, have
Borrow/lookup (the code around sharedProcessors[hash] and p.refCount) increment
only sharedEntry.refCount and return a new sharedHandle; implement
sharedHandle.Close() to be idempotent by checking its closed flag, setting it
once, then decrementing sharedEntry.refCount and when that hits zero close the
underlying processor and remove the sharedEntry from sharedProcessors (also
protect shared map and refCount with the existing lock used in the code paths
around lines 59 and 79-90).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9b242bef-2171-4fa8-825f-a107ce0ae187
📒 Files selected for processing (5)
libbeat/processors/add_kubernetes_metadata/kubernetes.golibbeat/processors/add_kubernetes_metadata/kubernetes_concurrent_test.golibbeat/processors/add_kubernetes_metadata/kubernetes_test.golibbeat/processors/shared/shared.golibbeat/processors/shared/shared_test.go
9cb291f to
609a0c0
Compare
…or wrapper (#50780) * [libbeat/processing] add a new shared processor wrapper (#50713) * chore: add a new shared wrapper for processors to use * refcount * simplify and add tests * license * lint * double close test * testing and review comments (cherry picked from commit 54f7600) # Conflicts: # libbeat/processors/add_kubernetes_metadata/kubernetes_test.go * merge conflicts --------- Co-authored-by: Vihas Makwana <121151420+VihasMakwana@users.noreply.github.com> Co-authored-by: Vihas Makwana <vihas.makwana@elastic.co>
…r wrapper (#50781) * [libbeat/processing] add a new shared processor wrapper (#50713) * chore: add a new shared wrapper for processors to use * refcount * simplify and add tests * license * lint * double close test * testing and review comments (cherry picked from commit 54f7600) # Conflicts: # libbeat/processors/add_kubernetes_metadata/kubernetes_test.go * merge conflicts --------- Co-authored-by: Vihas Makwana <121151420+VihasMakwana@users.noreply.github.com> Co-authored-by: Vihas Makwana <vihas.makwana@elastic.co>
|
I'm also backporting this to 9.3. |
…r wrapper (#50830) * [libbeat/processing] add a new shared processor wrapper (#50713) * chore: add a new shared wrapper for processors to use * refcount * simplify and add tests * license * lint * double close test * testing and review comments (cherry picked from commit 54f7600) # Conflicts: # libbeat/processors/add_kubernetes_metadata/kubernetes_test.go * merge conflicts --------- Co-authored-by: Vihas Makwana <121151420+VihasMakwana@users.noreply.github.com> Co-authored-by: Vihas Makwana <vihas.makwana@elastic.co>
…tic#50713)" This reverts commit 54f7600.
Cherry-pick of elastic#50586 (squashed). Replaces the approach from elastic#50713 (reverted in the previous commit): instead of a dedicated shared processor wrapper opted into per-processor, SafeWrap now dedupes processor instances globally by (name, config hash) with reference counting, so per-input processors are no longer instantiated per harvester. Fixes memory/goroutine blow-up with many harvesters (elastic#50376). Note: the lifecycle contract tests introduced in the first commit of this PR fail at this commit (TestContractConcurrentPipelines, TestContractInputConcurrentHarvesters): closing a group twice releases another owner's reference and the shared instance is closed under a live holder. The next commit restores the contract.
…tic#50713)" This reverts commit 54f7600.
Cherry-pick of elastic#50586 (squashed). Replaces the approach from elastic#50713 (reverted in the previous commit): instead of a dedicated shared processor wrapper opted into per-processor, SafeWrap now dedupes processor instances globally by (name, config hash) with reference counting, so per-input processors are no longer instantiated per harvester. Fixes memory/goroutine blow-up with many harvesters (elastic#50376). Note: the lifecycle contract tests introduced in the first commit of this PR fail at this commit (TestContractConcurrentPipelines, TestContractInputConcurrentHarvesters): closing a group twice releases another owner's reference and the shared instance is closed under a live holder. The next commit restores the contract.
Deduplicate processor instances in SafeWrap using (name, hash).
Constructor calls with an identical configuration return handles to one
shared, reference-counted instance.
Beat-level global processors were already shared before this PR so most
processors should already work with that level of concurrency.
Design points:
- Exceptions:
1. PathSetter implementations
2. The rate_limit processor: a shared token bucket would divide the
configured per-input limit by the number of owners.
- Each constructor call returns a handle; Close is idempotent per
handle.
- Concurrent constructors sharing the same key wait for a single
instance to initialize. Construction is done outside the global lock
to avoid one processor blocking all constructors.
This reverts #50713, which shared only add_kubernetes_metadata through a
dedicated wrapper package, cherry-picks and hardens the approach from
#50586.
Processors that need to opt-out of being shared can implement the
Unshareable interface.
Closes #50376
Co-authored-by: Vihas Makwana <vihas.makwana@elastic.co>
Proposed commit message
Right now, per input processors are instantiated for every new harvester created.
beats/filebeat/channel/runner.go
Lines 136 to 150 in c37e794
In environments with a large number of log files, this will lead to excessive memory usage and can cause memory exhaustion/OOM issues.
Fix this by adding a shared processor wrapper, that will create one processor instance for a given config, no matter how many times it was instantiated.
Why is it important
Processors interested in using a "shared" state can opt-in by wrapping their constructors:
Before:
After:
This will result in much less memory allocation over long term.
Checklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.How to test this PR locally
You can test this with following config:
With new files being created, the memory increase will still be there, but slow and steady. Without my changes, the usage is drastic.
It is more drastic if you're testing this in k8s environment with
add_kubernetes_metadataprocessor.Related issues