[DNM] perf: use DeepCopyUpdate to eliminate Clone+DeepUpdate allocations#49762
[DNM] perf: use DeepCopyUpdate to eliminate Clone+DeepUpdate allocations#49762strawgate wants to merge 1 commit intoelastic:mainfrom
Conversation
botelastic
bot
added
the
needs_team
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
📝 WalkthroughWalkthroughThis pull request introduces performance optimizations across libbeat's event processing pipeline by replacing the ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libbeat/processors/actions/addfields/add_fields.go`:
- Around line 140-154: The current check around af.singleKey/af.singleKeyInner
short-circuits and returns early when the top-level key exists but nested leaves
may be missing; remove or replace that shortcut so DeepCopyUpdateNoOverwrite
always runs for this case. Specifically, eliminate the early-return branch that
inspects event.Fields[af.singleKey] and returns when all immediate child keys
exist, or change it to perform a full recursive existence check against
af.singleKeyInner before returning; otherwise always call
event.Fields.DeepCopyUpdateNoOverwrite(mapstr.M{af.singleKey:
af.singleKeyInner}) so nested missing leaves (e.g., host.os.version) are merged
in.
🪄 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: Pro
Run ID: 48d539a1-37c3-4b8e-9d6f-f57a9d3b0118
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
changelog/fragments/1774757000-deepcopyupdate-allocations.yamlgo.modheartbeat/eventext/eventext.golibbeat/processors/actions/addfields/add_fields.golibbeat/processors/actions/addfields/add_fields_benchmark_test.golibbeat/processors/actions/addfields/add_fields_test.golibbeat/processors/actions/rename.golibbeat/processors/add_cloud_metadata/add_cloud_metadata.golibbeat/processors/add_cloud_metadata/add_cloud_metadata_optimize_test.golibbeat/processors/add_host_metadata/add_host_metadata.golibbeat/processors/add_host_metadata/add_host_metadata_test.golibbeat/processors/add_observer_metadata/add_observer_metadata.golibbeat/publisher/processing/default.go
| if dstVal, ok := event.Fields[af.singleKey]; ok { | ||
| if dstMap, ok := dstVal.(mapstr.M); ok { | ||
| allExist := true | ||
| for sk := range af.singleKeyInner { | ||
| if _, ok := dstMap[sk]; !ok { | ||
| allExist = false | ||
| break | ||
| } | ||
| } | ||
| if allExist { | ||
| return event, nil | ||
| } | ||
| } | ||
| } | ||
| event.Fields.DeepCopyUpdateNoOverwrite(mapstr.M{af.singleKey: af.singleKeyInner}) |
There was a problem hiding this comment.
Don’t short-circuit recursive no-overwrite merges on top-level key presence.
Line 143 only checks whether the immediate child keys exist. That changes behavior for nested maps: if the destination already has host.os, this returns early even when nested leaves like host.os.version are still missing, while DeepCopyUpdateNoOverwrite would descend and add them. That can silently drop enrichment on partially populated objects.
Safe fix
- if dstVal, ok := event.Fields[af.singleKey]; ok {
- if dstMap, ok := dstVal.(mapstr.M); ok {
- allExist := true
- for sk := range af.singleKeyInner {
- if _, ok := dstMap[sk]; !ok {
- allExist = false
- break
- }
- }
- if allExist {
- return event, nil
- }
- }
- }
event.Fields.DeepCopyUpdateNoOverwrite(mapstr.M{af.singleKey: af.singleKeyInner})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if dstVal, ok := event.Fields[af.singleKey]; ok { | |
| if dstMap, ok := dstVal.(mapstr.M); ok { | |
| allExist := true | |
| for sk := range af.singleKeyInner { | |
| if _, ok := dstMap[sk]; !ok { | |
| allExist = false | |
| break | |
| } | |
| } | |
| if allExist { | |
| return event, nil | |
| } | |
| } | |
| } | |
| event.Fields.DeepCopyUpdateNoOverwrite(mapstr.M{af.singleKey: af.singleKeyInner}) | |
| event.Fields.DeepCopyUpdateNoOverwrite(mapstr.M{af.singleKey: af.singleKeyInner}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libbeat/processors/actions/addfields/add_fields.go` around lines 140 - 154,
The current check around af.singleKey/af.singleKeyInner short-circuits and
returns early when the top-level key exists but nested leaves may be missing;
remove or replace that shortcut so DeepCopyUpdateNoOverwrite always runs for
this case. Specifically, eliminate the early-return branch that inspects
event.Fields[af.singleKey] and returns when all immediate child keys exist, or
change it to perform a full recursive existence check against af.singleKeyInner
before returning; otherwise always call
event.Fields.DeepCopyUpdateNoOverwrite(mapstr.M{af.singleKey:
af.singleKeyInner}) so nested missing leaves (e.g., host.os.version) are merged
in.
strawgate
changed the title
strawgate
force-pushed
the
claude-perf-deepcopyupdate
branch
from
0072955 to
a40faea
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libbeat/processors/actions/addfields/add_fields.go (1)
140-154:⚠️ Potential issue | 🔴 CriticalNested no-overwrite merge early return still incorrectly short-circuits on top-level key presence.
This check only verifies immediate child keys exist in
dstMap, butDeepCopyUpdateNoOverwritewould recursively descend into nested maps. Ifaf.singleKeyInnercontains{"os": {"version": "1.0"}}and destination has{"os": {}}, this returns early andversionis never added.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libbeat/processors/actions/addfields/add_fields.go` around lines 140 - 154, The early-return incorrectly assumes presence of top-level child keys implies all nested fields exist; replace the shallow check around af.singleKey/af.singleKeyInner with a deep-existence check that recursively descends mapstr.M to verify every nested key from af.singleKeyInner exists in event.Fields[af.singleKey] (or implement a helper like deepAllKeysExist(dstMap, af.singleKeyInner) that walks nested maps and returns false if any key path is missing). Only return early when the recursive check confirms every nested path exists; otherwise call event.Fields.DeepCopyUpdateNoOverwrite(mapstr.M{af.singleKey: af.singleKeyInner}) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libbeat/processors/actions/addfields/add_fields.go`:
- Around line 140-154: The early-return incorrectly assumes presence of
top-level child keys implies all nested fields exist; replace the shallow check
around af.singleKey/af.singleKeyInner with a deep-existence check that
recursively descends mapstr.M to verify every nested key from af.singleKeyInner
exists in event.Fields[af.singleKey] (or implement a helper like
deepAllKeysExist(dstMap, af.singleKeyInner) that walks nested maps and returns
false if any key path is missing). Only return early when the recursive check
confirms every nested path exists; otherwise call
event.Fields.DeepCopyUpdateNoOverwrite(mapstr.M{af.singleKey:
af.singleKeyInner}) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64f08516-1c4b-4056-a1c1-e50c464cf010
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
changelog/fragments/1774757000-deepcopyupdate-allocations.yamlgo.modheartbeat/eventext/eventext.golibbeat/processors/actions/addfields/add_fields.golibbeat/processors/actions/addfields/add_fields_benchmark_test.golibbeat/processors/actions/addfields/add_fields_test.golibbeat/processors/actions/rename.golibbeat/processors/add_cloud_metadata/add_cloud_metadata.golibbeat/processors/add_cloud_metadata/add_cloud_metadata_optimize_test.golibbeat/processors/add_host_metadata/add_host_metadata.golibbeat/processors/add_host_metadata/add_host_metadata_test.golibbeat/processors/add_observer_metadata/add_observer_metadata.golibbeat/publisher/processing/default.go
✅ Files skipped from review due to trivial changes (3)
- changelog/fragments/1774757000-deepcopyupdate-allocations.yaml
- libbeat/processors/actions/addfields/add_fields_benchmark_test.go
- libbeat/processors/actions/addfields/add_fields_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- go.mod
- libbeat/publisher/processing/default.go
- heartbeat/eventext/eventext.go
- libbeat/processors/add_cloud_metadata/add_cloud_metadata.go
- libbeat/processors/actions/rename.go
- libbeat/processors/add_host_metadata/add_host_metadata_test.go
- libbeat/processors/add_cloud_metadata/add_cloud_metadata_optimize_test.go
This comment has been minimized.
This comment has been minimized.
TL;DRAll 4 failing Buildkite jobs are failing on the same Remediation
Investigation detailsRoot Cause
The failing cases are the ones that assert exact
But fixtures expect Evidence
Verification
Follow-up
Note 🔒 Integrity filtering filtered 2 itemsIntegrity filtering activated and filtered the following items during workflow execution.
What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Replace Clone()+DeepUpdate() with single-pass DeepCopyUpdate() in add_fields, cloud/host/observer metadata, rename, processing, and heartbeat eventext. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
strawgate
force-pushed
the
claude-perf-deepcopyupdate
branch
from
a40faea to
296ee8c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libbeat/processors/actions/addfields/add_fields.go (1)
140-154:⚠️ Potential issue | 🟠 MajorShallow key check may skip nested merges.
The early-return at line 149 checks only immediate child key presence (
dstMap[sk]), not nested structure. Ifaf.singleKeyInnerhas{"os": {"version": "5.4"}}anddstMapalready has{"os": {"family": "linux"}}, the check seesosexists and returns — skipping the merge that would addos.version.
DeepCopyUpdateNoOverwritedescends recursively; this shortcut does not.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libbeat/processors/actions/addfields/add_fields.go` around lines 140 - 154, The early-return uses a shallow presence check against dstMap (checking dstMap[sk]) which wrongly skips nested merges when af.singleKeyInner contains nested maps; update the logic around af.singleKey / af.singleKeyInner so you either remove the shallow early-return or replace it with a recursive existence check that descends maps and ensures all nested keys in af.singleKeyInner are present in the corresponding nested dstMap before returning; reference the same symbols (af.singleKey, af.singleKeyInner, dstMap, event.Fields.DeepCopyUpdateNoOverwrite) and ensure the new check mirrors DeepCopyUpdateNoOverwrite's recursion semantics so nested fields like {"os": {"version": "5.4"}} will be merged correctly instead of being skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libbeat/processors/actions/addfields/add_fields.go`:
- Around line 140-154: The early-return uses a shallow presence check against
dstMap (checking dstMap[sk]) which wrongly skips nested merges when
af.singleKeyInner contains nested maps; update the logic around af.singleKey /
af.singleKeyInner so you either remove the shallow early-return or replace it
with a recursive existence check that descends maps and ensures all nested keys
in af.singleKeyInner are present in the corresponding nested dstMap before
returning; reference the same symbols (af.singleKey, af.singleKeyInner, dstMap,
event.Fields.DeepCopyUpdateNoOverwrite) and ensure the new check mirrors
DeepCopyUpdateNoOverwrite's recursion semantics so nested fields like {"os":
{"version": "5.4"}} will be merged correctly instead of being skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e91b3a1d-f5c1-4ba1-a8e7-7aa1bd280832
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
changelog/fragments/1774757000-deepcopyupdate-allocations.yamlgo.modheartbeat/eventext/eventext.golibbeat/processors/actions/addfields/add_fields.golibbeat/processors/actions/addfields/add_fields_benchmark_test.golibbeat/processors/actions/addfields/add_fields_test.golibbeat/processors/add_cloud_metadata/add_cloud_metadata.golibbeat/processors/add_cloud_metadata/add_cloud_metadata_optimize_test.golibbeat/processors/add_host_metadata/add_host_metadata.golibbeat/processors/add_host_metadata/add_host_metadata_test.golibbeat/processors/add_observer_metadata/add_observer_metadata.golibbeat/publisher/processing/default.go
✅ Files skipped from review due to trivial changes (4)
- changelog/fragments/1774757000-deepcopyupdate-allocations.yaml
- libbeat/processors/add_host_metadata/add_host_metadata.go
- libbeat/processors/actions/addfields/add_fields_benchmark_test.go
- heartbeat/eventext/eventext.go
🚧 Files skipped from review as they are similar to previous changes (3)
- libbeat/processors/add_observer_metadata/add_observer_metadata.go
- libbeat/publisher/processing/default.go
- libbeat/processors/actions/addfields/add_fields_test.go
pierrehilbert
added
the
Team:Elastic-Agent-Data-Plane
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
botelastic
bot
removed
the
needs_team
pierrehilbert
added
Team:obs-ds-hosted-services
|
Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services) |
botelastic
bot
removed
the
needs_team
3 participants
Summary
Replace the two-step
Clone()+DeepUpdate()pattern with single-passDeepCopyUpdate()across hot-path processors and pipeline setup.DeepCopyUpdatemerges source into destination while creating fresh nested maps in one pass, eliminating the intermediate full-tree clone.Part 2 of processor allocation work (Part 1: #49761).
Changes
DeepCopyUpdate. Split@metadatahandling to bypassevent.deepUpdateoverhead. Skip no-overwrite merge when all destination keys already exist.metadata.Clone()with per-value deep copy — immutable values returned as-is, only nested maps cloned.atomic.Int64+mapstr.Pointer(12x faster than mutex for cache reads). UseDeepCopyUpdatefor cached data merge.Clone()+DeepUpdate()→DeepCopyUpdate().Clone()+DeepUpdate()→DeepCopyUpdate()at 3 pipeline setup sites.Clone()+DeepUpdate()→DeepCopyUpdate().Per-processor benchmarks
End-to-end filebeat benchmarks
Filebeat with
benchmarkinput → mock Elasticsearch,GOMAXPROCS=2, 30 seconds per run.6× add_fields only:
Dependencies
Requires elastic/elastic-agent-libs#390 (
DeepCopyUpdate/DeepCopyUpdateNoOverwrite). Temporarily pinned to fork viareplacein go.mod.Test plan
🤖 Generated with Claude Code