perf: skip unnecessary event.Clone() in processors for single-field ops#49777
Open
strawgate wants to merge 1 commit intoelastic:mainfrom
Open
perf: skip unnecessary event.Clone() in processors for single-field ops#49777strawgate wants to merge 1 commit intoelastic:mainfrom
strawgate wants to merge 1 commit intoelastic:mainfrom
Conversation
botelastic
bot
added
the
needs_team
Contributor
🤖 GitHub commentsJust comment with:
|
strawgate
changed the title
📝 WalkthroughWalkthroughThis pull request reduces unnecessary event cloning across libbeat processors. Processors that previously cloned whenever ✨ 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/rename.go`:
- Around line 79-80: The code deletes the source field before ensuring the
destination write succeeds, and because renameNeedsClone can return false for
single non-overlapping fields no backup is kept when f.config.FailOnError is
true; change the logic so that when f.config.FailOnError is true you always
preserve a backup (create backup = event.Clone()) before mutating, or alter
renameField to perform PutValue(to, value) first and only remove the original
(from) after PutValue succeeds, and if PutValue fails restore the backup; update
the same pattern in the other affected blocks (the similar logic around lines
90-92 and 140-151) to use the same safe sequence (backup on FailOnError or
write-then-delete) and reference f.config.FailOnError, renameNeedsClone, backup,
renameField, PutValue, from and to when making the changes.
🪄 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: 128dd85b-820c-4316-8e09-dc37698937ae
📒 Files selected for processing (12)
libbeat/processors/actions/append.golibbeat/processors/actions/clone_skip_test.golibbeat/processors/actions/copy_fields.golibbeat/processors/actions/decode_base64_field.golibbeat/processors/actions/decompress_gzip_field.golibbeat/processors/actions/rename.golibbeat/processors/actions/replace.golibbeat/processors/actions/truncate_fields.golibbeat/processors/decode_csv_fields/decode_csv_fields.golibbeat/processors/dissect/processor.golibbeat/processors/extract_array/extract_array.golibbeat/processors/urldecode/urldecode.go
💤 Files with no reviewable changes (3)
- libbeat/processors/actions/decompress_gzip_field.go
- libbeat/processors/actions/decode_base64_field.go
- libbeat/processors/actions/append.go
Comment on lines
+79
to
80
| if f.config.FailOnError && renameNeedsClone(f.config) { | ||
| backup = event.Clone() |
There was a problem hiding this comment.
Single-field rename can drop source data on failure with fail_on_error: true
For non-overlapping single-field configs, renameNeedsClone returns false, so no backup is kept. But renameField deletes from before PutValue(to, value). If PutValue fails, the processor returns a partially mutated event (source removed), which is a rollback/data-loss regression under fail-on-error.
Proposed safe fix
func renameNeedsClone(config renameFieldsConfig) bool {
- if len(config.Fields) > 1 {
+ if len(config.Fields) > 1 {
return true
}
if len(config.Fields) == 1 {
from := config.Fields[0].From
to := config.Fields[0].To
- if strings.HasPrefix(to, from+".") || strings.HasPrefix(from, to+".") {
+ // clone required when delete-before-put ordering is required
+ // (overlapping paths), otherwise renameField can do put-before-delete.
+ if strings.HasPrefix(to, from+".") || strings.HasPrefix(from, to+".") {
return true
}
}
return false
}func (f *renameFields) renameField(from string, to string, event *beat.Event) error {
_, err := event.GetValue(to)
if err == nil {
return fmt.Errorf("target field %s already exists, drop or rename this field first", to)
}
value, err := event.GetValue(from)
if err != nil {
...
}
- // Deletion must happen first to support cases where a becomes a.b
- err = event.Delete(from)
- if err != nil {
- return fmt.Errorf("could not delete key: %s, %w", from, err)
- }
-
- _, err = event.PutValue(to, value)
- if err != nil {
- return fmt.Errorf("could not put value: %s: %v, %w", to, value, err)
- }
+ overlap := strings.HasPrefix(to, from+".") || strings.HasPrefix(from, to+".")
+ if !overlap {
+ if _, err = event.PutValue(to, value); err != nil {
+ return fmt.Errorf("could not put value: %s: %v, %w", to, value, err)
+ }
+ if err = event.Delete(from); err != nil {
+ _, _ = event.Delete(to) // best-effort local rollback
+ return fmt.Errorf("could not delete key: %s, %w", from, err)
+ }
+ return nil
+ }
+
+ // overlap case still requires delete-first
+ if err = event.Delete(from); err != nil {
+ return fmt.Errorf("could not delete key: %s, %w", from, err)
+ }
+ if _, err = event.PutValue(to, value); err != nil {
+ return fmt.Errorf("could not put value: %s: %v, %w", to, value, err)
+ }
return nil
}Also applies to: 90-92, 140-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libbeat/processors/actions/rename.go` around lines 79 - 80, The code deletes
the source field before ensuring the destination write succeeds, and because
renameNeedsClone can return false for single non-overlapping fields no backup is
kept when f.config.FailOnError is true; change the logic so that when
f.config.FailOnError is true you always preserve a backup (create backup =
event.Clone()) before mutating, or alter renameField to perform PutValue(to,
value) first and only remove the original (from) after PutValue succeeds, and if
PutValue fails restore the backup; update the same pattern in the other affected
blocks (the similar logic around lines 90-92 and 140-151) to use the same safe
sequence (backup on FailOnError or write-then-delete) and reference
f.config.FailOnError, renameNeedsClone, backup, renameField, PutValue, from and
to when making the changes.
strawgate
added a commit
to strawgate/beats
that referenced
this pull request
Mar 30, 2026
The dissect event.Clone() skip is moving to elastic#49777 which applies the same pattern more broadly across all action processors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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/rename.go`:
- Line 86: The error string constructed in rename.go (variable errMsg created
via fmt.Errorf in the rename processor) uses a lowercase "failed to rename..."
which mismatches tests expecting "Failed to rename..."; update the fmt.Errorf
call that sets errMsg to use a capitalized "Failed to rename fields in
processor: %w" so the error message matches the assertions in rename_test.go (or
alternatively update the tests if you prefer changing expectations).
🪄 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: 87c29645-1654-4003-988e-bb655b87972c
📒 Files selected for processing (6)
changelog/fragments/1774840000-skip-unnecessary-event-clone.yamllibbeat/processors/actions/append.golibbeat/processors/actions/copy_fields.golibbeat/processors/actions/decompress_gzip_field.golibbeat/processors/actions/rename.golibbeat/processors/extract_array/extract_array.go
✅ Files skipped from review due to trivial changes (2)
- changelog/fragments/1774840000-skip-unnecessary-event-clone.yaml
- libbeat/processors/actions/append.go
🚧 Files skipped from review as they are similar to previous changes (2)
- libbeat/processors/actions/decompress_gzip_field.go
- libbeat/processors/actions/copy_fields.go
| err := f.renameField(field.From, field.To, event) | ||
| if err != nil { | ||
| errMsg := fmt.Errorf("Failed to rename fields in processor: %w", err) | ||
| errMsg := fmt.Errorf("failed to rename fields in processor: %w", err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if test expectations match the new lowercase error message
rg -n "Failed to rename fields in processor" libbeat/processors/actions/Repository: elastic/beats
Length of output: 383
🏁 Script executed:
# Check the actual error message in rename.go line 86
sed -n '85,87p' libbeat/processors/actions/rename.goRepository: elastic/beats
Length of output: 212
🏁 Script executed:
# Search for any lowercase "failed to rename" in the test file
rg -n "failed to rename" libbeat/processors/actions/rename_test.goRepository: elastic/beats
Length of output: 39
🏁 Script executed:
# Check the test assertions more closely to confirm they compare error messages
sed -n '95,105p' libbeat/processors/actions/rename_test.go
sed -n '178,204p' libbeat/processors/actions/rename_test.goRepository: elastic/beats
Length of output: 931
Tests will fail due to error message case mismatch
The error message in rename.go line 86 uses lowercase ("failed to rename..."), but test assertions in rename_test.go lines 99 and 198 expect capital F ("Failed to rename..."). These string comparisons via reflect.DeepEqual will fail. Either revert the error message to capital F or update both test assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libbeat/processors/actions/rename.go` at line 86, The error string
constructed in rename.go (variable errMsg created via fmt.Errorf in the rename
processor) uses a lowercase "failed to rename..." which mismatches tests
expecting "Failed to rename..."; update the fmt.Errorf call that sets errMsg to
use a capitalized "Failed to rename fields in processor: %w" so the error
message matches the assertions in rename_test.go (or alternatively update the
tests if you prefer changing expectations).
Eliminate event.Clone() in dissect (check-then-write), rename, copy, replace, truncate, urldecode, extract_array, decode_csv, decode_base64, decompress_gzip, and append processors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
strawgate
force-pushed
the
perf/skip-unnecessary-clone
branch
from
6d5ad2d to
7edb9a7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libbeat/processors/actions/rename.go (1)
79-80:⚠️ Potential issue | 🔴 Critical
fail_on_errorcan still lose data in single-field renameLine 79 skips backup for single-field non-overlapping renames, but Lines 118-127 still do delete-before-put. If
PutValuefails (e.g., blocked destination path), source data is already deleted and cannot be restored despitefail_on_error: true.Proposed fix
func (f *renameFields) renameField(from string, to string, event *beat.Event) error { @@ - // Deletion must happen first to support cases where a becomes a.b - err = event.Delete(from) - if err != nil { - return fmt.Errorf("could not delete key: %s, %w", from, err) - } - - _, err = event.PutValue(to, value) - if err != nil { - return fmt.Errorf("could not put value: %s: %v, %w", to, value, err) - } + overlap := strings.HasPrefix(to, from+".") || strings.HasPrefix(from, to+".") + if !overlap { + if _, err = event.PutValue(to, value); err != nil { + return fmt.Errorf("could not put value: %s: %v, %w", to, value, err) + } + if err = event.Delete(from); err != nil { + _, _ = event.Delete(to) // best-effort local rollback + return fmt.Errorf("could not delete key: %s, %w", from, err) + } + return nil + } + + // overlap case still requires delete-first + err = event.Delete(from) + if err != nil { + return fmt.Errorf("could not delete key: %s, %w", from, err) + } + _, err = event.PutValue(to, value) + if err != nil { + return fmt.Errorf("could not put value: %s: %v, %w", to, value, err) + } return nil }Also applies to: 118-127, 140-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libbeat/processors/actions/rename.go` around lines 79 - 80, The current logic in rename.go only creates a backup when renameNeedsClone(f.config) is true, which skips backups for single-field non-overlapping renames even though later code paths (uses of PutValue and delete-before-put in the blocks around where PutValue is called) perform a delete-before-put; to fix, ensure that when f.config.FailOnError is true you create a backup (e.g., call event.Clone into backup) for all rename cases that perform delete-before-put (including the single-field path), or alternatively change the operation order in the rename implementation so you perform PutValue (destination write) before deleting the source, and on failure restore from the backup; update the code paths referenced by renameNeedsClone, the backup variable, and the PutValue/delete logic (the blocks around PutValue and the delete-before-put sections) so fail_on_error truly preserves source data.
🤖 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/rename.go`:
- Around line 79-80: The current logic in rename.go only creates a backup when
renameNeedsClone(f.config) is true, which skips backups for single-field
non-overlapping renames even though later code paths (uses of PutValue and
delete-before-put in the blocks around where PutValue is called) perform a
delete-before-put; to fix, ensure that when f.config.FailOnError is true you
create a backup (e.g., call event.Clone into backup) for all rename cases that
perform delete-before-put (including the single-field path), or alternatively
change the operation order in the rename implementation so you perform PutValue
(destination write) before deleting the source, and on failure restore from the
backup; update the code paths referenced by renameNeedsClone, the backup
variable, and the PutValue/delete logic (the blocks around PutValue and the
delete-before-put sections) so fail_on_error truly preserves source data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 175d3224-5645-436b-9b15-696f89e8eb98
📒 Files selected for processing (13)
changelog/fragments/1774840000-skip-unnecessary-event-clone.yamllibbeat/processors/actions/append.golibbeat/processors/actions/clone_skip_test.golibbeat/processors/actions/copy_fields.golibbeat/processors/actions/decode_base64_field.golibbeat/processors/actions/decompress_gzip_field.golibbeat/processors/actions/rename.golibbeat/processors/actions/replace.golibbeat/processors/actions/truncate_fields.golibbeat/processors/decode_csv_fields/decode_csv_fields.golibbeat/processors/dissect/processor.golibbeat/processors/extract_array/extract_array.golibbeat/processors/urldecode/urldecode.go
💤 Files with no reviewable changes (1)
- libbeat/processors/actions/decode_base64_field.go
✅ Files skipped from review due to trivial changes (2)
- changelog/fragments/1774840000-skip-unnecessary-event-clone.yaml
- libbeat/processors/actions/replace.go
🚧 Files skipped from review as they are similar to previous changes (6)
- libbeat/processors/actions/truncate_fields.go
- libbeat/processors/actions/append.go
- libbeat/processors/extract_array/extract_array.go
- libbeat/processors/actions/copy_fields.go
- libbeat/processors/actions/decompress_gzip_field.go
- libbeat/processors/dissect/processor.go
Contributor
TL;DRAll 4 failed Buildkite jobs are the same deterministic test regression: error-message casing changed from Remediation
Investigation detailsRoot CauseThis is a code/test mismatch introduced by commit In that commit, these strings were changed to lowercase:
But tests still expect uppercase
Evidence
Verification
Follow-upIf lowercase messages are intentional, update the expected test outputs in Note 🔒 Integrity filtering filtered 3 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. |
pierrehilbert
added
the
Team:Elastic-Agent-Data-Plane
Contributor
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
botelastic
bot
removed
the
needs_team
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
3 participants
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.
Summary
Skip unnecessary
event.Clone()in 11 processors. The clone deep-copies the entire event for rollback on error, but in most cases there is nothing to roll back — the operation either fails before writing or performs a single atomic write.This brings all processors in line with
convert, which has had this optimization since 2019 (#11686).Changes
dissect — Restructure
mapper()to validate all target keys before writing any values. If a conflict is found, the event is returned unmodified. This eliminatesevent.Clone()entirely regardless of configuration. Dissect clones the full accumulated event (including all add_fields/host/cloud metadata) on every call, making this the highest-impact change.rename, copy_fields, replace, truncate_fields, urldecode, extract_array, decode_csv_fields — Skip clone when
len(fields) == 1. A single-field operation can't partially fail. For rename, path overlap detection (a → a.b) preserves the clone when needed.decode_base64_field, decompress_gzip_field, append — Remove clone entirely (always single operation).
Behavioral safety
renameNeedsClone()detects when from/to share a path prefix and preserves the clone.len(fields) > 1.event.Clone()NOT changed isalterFieldProcessor(in-place key mutation).Per-processor benchmarks
End-to-end filebeat benchmarks
GCE e2-standard-2, Docker cgroups, benchmark input → mock ES, 30s, 3 alternating runs:
Test plan
🤖 Generated with Claude Code