[libbeat/processing] Deduplicate processors based on config - #52277
Conversation
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
🤖 GitHub commentsJust comment with:
|
cmacknz
left a comment
There was a problem hiding this comment.
I think the approach makes sense, still digesting the concurrency interactions but found only some potential ref counting bugs on the first pass.
…cycle Black-box tests written against current main by an author with no knowledge of the upcoming processor-sharing implementation, so they pin the observable contract rather than implementation details: construction through the registry, lifecycle (double-Close tolerance, run after close), SetPaths semantics, cross-pipeline independence (closing one pipeline's processors must not affect another pipeline built from an identical config) and concurrent construct/run/close churn, at both the libbeat/processors and filebeat/channel level. They pass unmodified on this commit (pre-change) and must keep passing after the sharing change lands; any commit that needs to modify them is changing the contract and must show that in its diff. Also extend the channel runner mock to close the per-client processor groups, pinning that repeated Close of a group is tolerated and that owners release their processors on shutdown. Assisted-By: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…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.
490c668 to
f1a24a6
Compare
|
@cmacknz your comments are correct in the sense that the evicted I refactored some code around the construct-or-wait logic and commented the paths you highlighted. |
f1a24a6 to
de8136d
Compare
…-out, singleflight construction Rework of the sharing mechanism from the previous commit, addressing findings from adversarial review, contract tests and benchmarks: - Each constructor call returns its own handle to the shared instance. Close is idempotent per handle (the documented SafeWrap contract: one instance can be reachable from multiple processor groups that each close it): a duplicate Close can no longer release another owner's reference and close the shared instance under a live holder. - Processors are shared by default with two exceptions: PathSetter implementations (paths are owner-specific; in otel mode each beat receiver sets its own paths and pointer-compared paths would fail the second receiver) and Unshareable implementations, a new marker interface for processors whose semantics change when shared (rate_limit: one shared token bucket would divide the configured per-input limit by the number of owners). - The real constructor runs outside the global lock via per-key singleflight: a blocking constructor (add_kubernetes_metadata with wait_for_metadata waits for the API server, potentially forever with wait_for_metadata_timeout: 0) no longer stalls all processor construction and teardown process-wide, and a construction panic cannot leave concurrent waiters blocked. The underlying Close (which drains in-flight Runs) also runs outside the global lock. - Shared handles preserve the pdata fast path: a shared processor that implements PdataProcessor is returned as a handle that forwards RunPdata, and unshared Closer+PdataProcessor instances keep the safePdataProcessorWithClose wrapping. - The eviction of the last reference removes the cache entry, bounding the shared map under autodiscover config churn. Includes a concurrency churn stress test (same-config construct/Run/ Close churn racing a steady different-config holder of the same processor name) designed for script/stresstest.sh; 1000+ runs with 0 failures under -race.
de8136d to
9ccee8e
Compare
|
LGTM, will let @khushijain21 do the approval since she had some comments to address as well. |
|
Tick the box to add this pull request to the merge queue (same as
|
Proposed commit message
Note
I kept four commits for cleaner reviewing:
Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration filesstresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.Disruptive User Impact
None directly but indirectly, inputs with identical processor configurations now share one instance.
How to test this PR locally
e2e
Filebeat config:
Create files:
Run each binary separately:
mainUnit tests
The contract tests (first commit) were written black-box against main before the implementation existed and pass unmodified on both.
Related issues
add_fieldswith sharedprocessor #51616