Skip to content

Commit 5f5774b

Browse files
strawgateclaude
andauthored
perf: optimize FileMetaReader hot path and EncoderReader map sizing (#50020)
FileMetaReader.Next() is called for every line harvested by filebeat. Replace DeepUpdate (recursive map merge) and dotted-key Put ("log.file.device_id") with direct map assignment. Lazy-cache the per-file metadata (path, device_id, inode, fingerprint) on first call since these values are constant for the file's lifetime. The cached interface{} values are copied via maps.Copy without re-boxing, which is where the alloc savings come from. The cache is built into a local variable and only assigned on success, so a failed setFileSystemMetadata retries on the next call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * perf: pre-size Fields map in EncoderReader to avoid grow on first write EncoderReader.Next() creates a Fields map for every line. Changing from mapstr.M{} (capacity 0) to make(mapstr.M, 1) avoids a runtime map grow when the first downstream consumer writes into it. Benefits all inputs using EncoderReader: filestream, legacy log, and S3. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: add changelog fragment and resolve lint issues Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: remove changelog fragment, using skip-changelog label Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: use require.IsType and consolidate benchmark into metafields_test.go Replace bare type assertion + require.True(ok) patterns with require.IsType in metafields_other_test.go and metafields_windows_test.go. Move benchmark functions from metafields_bench_test.go into metafields_test.go and delete the separate bench file, following the one-test-file-per-processor principle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: use comma-ok type assertions to satisfy errcheck linter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * perf: exact cachedMeta pre-sizing via platformFileFields constant Add a platformFileFields constant to each platform file (2 on non-Windows, 3 on Windows) so metafields.go can compute the exact map capacity without over-allocating. Add TestCachedMetaSizing on both platforms to verify the pre-allocation matches the actual entry count. Also document that the direct log key assignment is intentional, and fix TestMetaFieldsOwnerAndGroup to use the platform-correct group name (wheel on macOS, root on Linux). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix testifylint: use require.Len instead of len() comparison The linter requires require.Len(t, x, n) instead of require.Equal(t, n, len(x)). Assisted-By: Cursor Made-with: Cursor * fix testifylint: use require.Len in windows test file Same fix as previous commit, but for the Windows-specific test. Assisted-By: Cursor Made-with: Cursor --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 8b7236f commit 5f5774b

7 files changed

Lines changed: 254 additions & 116 deletions

File tree

libbeat/reader/readfile/encode.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func (r EncoderReader) Next() (reader.Message, error) {
6969
Ts: time.Now(),
7070
Content: bytes.TrimPrefix(c, []byte("\uFEFF")),
7171
Bytes: sz,
72-
Fields: mapstr.M{},
72+
Fields: make(mapstr.M, 1),
7373
}, err
7474
}
7575

libbeat/reader/readfile/fs_metafields_other.go

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -28,44 +28,38 @@ import (
2828
"github.com/elastic/elastic-agent-libs/mapstr"
2929
)
3030

31+
// platformFileFields is the number of fields setFileSystemMetadata always writes
32+
// into the log.file submap on this platform (device_id and inode).
33+
// Optional fields (owner, group) are counted separately in metafields.go.
34+
const platformFileFields = 2
35+
36+
// Keys written into the log.file submap by setFileSystemMetadata.
3137
const (
32-
deviceIDKey = "log.file.device_id"
33-
inodeKey = "log.file.inode"
34-
ownerKey = "log.file.owner"
35-
groupKey = "log.file.group"
38+
deviceIDKey = "device_id"
39+
inodeKey = "inode"
40+
ownerKey = "owner"
41+
groupKey = "group"
3642
)
3743

38-
func setFileSystemMetadata(fi file.ExtendedFileInfo, fields mapstr.M, includeOwner bool, includeGroup bool) error {
44+
func setFileSystemMetadata(fi file.ExtendedFileInfo, fileMap mapstr.M, includeOwner bool, includeGroup bool) error {
3945
osstate := fi.GetOSState()
40-
_, err := fields.Put(deviceIDKey, strconv.FormatUint(osstate.Device, 10))
41-
if err != nil {
42-
return fmt.Errorf("failed to set %q: %w", deviceIDKey, err)
43-
}
44-
_, err = fields.Put(inodeKey, osstate.InodeString())
45-
if err != nil {
46-
return fmt.Errorf("failed to set %q: %w", inodeKey, err)
47-
}
46+
fileMap[deviceIDKey] = strconv.FormatUint(osstate.Device, 10)
47+
fileMap[inodeKey] = osstate.InodeString()
4848

4949
if includeOwner {
5050
o, err := user.LookupId(strconv.FormatUint(osstate.UID, 10))
5151
if err != nil {
5252
return fmt.Errorf("failed to lookup uid %q: %w", osstate.UID, err)
5353
}
54-
_, err = fields.Put(ownerKey, o.Username)
55-
if err != nil {
56-
return fmt.Errorf("failed to set %q: %w", ownerKey, err)
57-
}
54+
fileMap[ownerKey] = o.Username
5855
}
5956

6057
if includeGroup {
6158
g, err := user.LookupGroupId(strconv.FormatUint(osstate.GID, 10))
6259
if err != nil {
6360
return fmt.Errorf("failed to lookup gid %q: %w", osstate.GID, err)
6461
}
65-
_, err = fields.Put(groupKey, g.Name)
66-
if err != nil {
67-
return fmt.Errorf("failed to set %q: %w", groupKey, err)
68-
}
62+
fileMap[groupKey] = g.Name
6963
}
7064
return nil
7165
}

libbeat/reader/readfile/fs_metafields_windows.go

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -18,33 +18,27 @@
1818
package readfile
1919

2020
import (
21-
"fmt"
2221
"strconv"
2322

2423
"github.com/elastic/beats/v7/libbeat/common/file"
2524
"github.com/elastic/elastic-agent-libs/mapstr"
2625
)
2726

27+
// platformFileFields is the number of fields setFileSystemMetadata always writes
28+
// into the log.file submap on this platform (idxhi, idxlo, and vol).
29+
const platformFileFields = 3
30+
31+
// Keys written into the log.file submap by setFileSystemMetadata.
2832
const (
29-
idxhiKey = "log.file.idxhi"
30-
idxloKey = "log.file.idxlo"
31-
volKey = "log.file.vol"
33+
idxhiKey = "idxhi"
34+
idxloKey = "idxlo"
35+
volKey = "vol"
3236
)
3337

34-
func setFileSystemMetadata(fi file.ExtendedFileInfo, fields mapstr.M, includeOwner bool, includeGroup bool) error {
38+
func setFileSystemMetadata(fi file.ExtendedFileInfo, fileMap mapstr.M, includeOwner bool, includeGroup bool) error {
3539
osstate := fi.GetOSState()
36-
_, err := fields.Put(idxhiKey, strconv.FormatUint(osstate.IdxHi, 10))
37-
if err != nil {
38-
return fmt.Errorf("failed to set %q: %w", idxhiKey, err)
39-
}
40-
_, err = fields.Put(idxloKey, strconv.FormatUint(osstate.IdxLo, 10))
41-
if err != nil {
42-
return fmt.Errorf("failed to set %q: %w", idxloKey, err)
43-
}
44-
_, err = fields.Put(volKey, strconv.FormatUint(osstate.Vol, 10))
45-
if err != nil {
46-
return fmt.Errorf("failed to set %q: %w", volKey, err)
47-
}
48-
40+
fileMap[idxhiKey] = strconv.FormatUint(osstate.IdxHi, 10)
41+
fileMap[idxloKey] = strconv.FormatUint(osstate.IdxLo, 10)
42+
fileMap[volKey] = strconv.FormatUint(osstate.Vol, 10)
4943
return nil
5044
}

libbeat/reader/readfile/metafields.go

Lines changed: 53 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,16 @@ package readfile
1919

2020
import (
2121
"fmt"
22+
"maps"
2223

2324
"github.com/elastic/beats/v7/libbeat/common/file"
2425
"github.com/elastic/beats/v7/libbeat/reader"
2526
"github.com/elastic/elastic-agent-libs/mapstr"
2627
)
2728

28-
// Reader produces lines by reading lines from an io.Reader
29-
// through a decoder converting the reader it's encoding to utf-8.
29+
// FileMetaReader enriches every message with per-file metadata.
30+
// OS metadata strings (device_id, inode, etc.) are cached after the
31+
// first call to Next because they are constant for the file's lifetime.
3032
type FileMetaReader struct {
3133
reader reader.Reader
3234
path string
@@ -35,12 +37,21 @@ type FileMetaReader struct {
3537
includeGroup bool
3638
fingerprint string
3739
offset int64
40+
cachedMeta mapstr.M // lazily populated on first Next()
3841
}
3942

4043
// New creates a new Encode reader from input reader by applying
4144
// the given codec.
4245
func NewFilemeta(r reader.Reader, path string, fi file.ExtendedFileInfo, includeOwner bool, includeGroup bool, fingerprint string, offset int64) reader.Reader {
43-
return &FileMetaReader{r, path, fi, includeOwner, includeGroup, fingerprint, offset}
46+
return &FileMetaReader{
47+
reader: r,
48+
path: path,
49+
fi: fi,
50+
includeOwner: includeOwner,
51+
includeGroup: includeGroup,
52+
fingerprint: fingerprint,
53+
offset: offset,
54+
}
4455
}
4556

4657
// Next reads the next line from it's initial io.Reader
@@ -54,25 +65,47 @@ func (r *FileMetaReader) Next() (reader.Message, error) {
5465
return message, err
5566
}
5667

57-
message.Fields.DeepUpdate(mapstr.M{
58-
"log": mapstr.M{
59-
"offset": r.offset,
60-
"file": mapstr.M{
61-
"path": r.path,
62-
},
63-
},
64-
})
65-
66-
err = setFileSystemMetadata(r.fi, message.Fields, r.includeOwner, r.includeGroup)
67-
if err != nil {
68-
return message, fmt.Errorf("failed to set file system metadata: %w", err)
68+
// On first call, compute and cache the per-file OS metadata (device_id,
69+
// inode, etc.) since they are constant for this file's lifetime.
70+
// Build into a local variable so a failed setFileSystemMetadata does not
71+
// leave r.cachedMeta in a partial state.
72+
if r.cachedMeta == nil {
73+
// Pre-size exactly: path + platform-invariant fields + optional fields.
74+
// platformFileFields is defined per-platform in fs_metafields_*.go.
75+
// On Windows, includeOwner/includeGroup are not supported so they add
76+
// no fields; the slight over-allocation is harmless.
77+
size := 1 + platformFileFields // path + platform fields
78+
if r.includeOwner {
79+
size++
80+
}
81+
if r.includeGroup {
82+
size++
83+
}
84+
if r.fingerprint != "" {
85+
size++
86+
}
87+
m := make(mapstr.M, size)
88+
m["path"] = r.path
89+
if err := setFileSystemMetadata(r.fi, m, r.includeOwner, r.includeGroup); err != nil {
90+
return message, fmt.Errorf("failed to set file system metadata: %w", err)
91+
}
92+
if r.fingerprint != "" {
93+
m["fingerprint"] = r.fingerprint
94+
}
95+
r.cachedMeta = m
6996
}
7097

71-
if r.fingerprint != "" {
72-
_, err = message.Fields.Put("log.file.fingerprint", r.fingerprint)
73-
if err != nil {
74-
return message, fmt.Errorf("failed to set fingerprint: %w", err)
75-
}
98+
// Copy cached fields into a fresh map for this event.
99+
fileMap := make(mapstr.M, len(r.cachedMeta))
100+
maps.Copy(fileMap, r.cachedMeta)
101+
// Direct assignment replaces any existing "log" key rather than merging.
102+
// This is intentional and safe: FileMetaReader is always the first component
103+
// in the pipeline to write log.* fields. Downstream readers (parsers,
104+
// LimitReader) run after this and merge into the map we write here via
105+
// AddFields() / Update(), so nothing is lost.
106+
message.Fields["log"] = mapstr.M{
107+
"offset": r.offset,
108+
"file": fileMap,
76109
}
77110
r.offset += int64(message.Bytes)
78111

libbeat/reader/readfile/metafields_other_test.go

Lines changed: 74 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,15 @@
2020
package readfile
2121

2222
import (
23+
"runtime"
2324
"syscall"
2425
"testing"
2526
"time"
2627

2728
"github.com/stretchr/testify/require"
2829

2930
"github.com/elastic/beats/v7/libbeat/common/file"
31+
"github.com/elastic/beats/v7/libbeat/reader"
3032
"github.com/elastic/elastic-agent-libs/mapstr"
3133
)
3234

@@ -42,53 +44,87 @@ func createTestFileInfo() file.ExtendedFileInfo {
4244
func checkFields(t *testing.T, expected, actual mapstr.M) {
4345
t.Helper()
4446

45-
dev, err := actual.GetValue(deviceIDKey)
46-
require.NoError(t, err)
47-
require.Equal(t, "17", dev)
48-
err = actual.Delete(deviceIDKey)
49-
require.NoError(t, err)
47+
require.IsType(t, mapstr.M{}, actual["log"], "expected log to be mapstr.M")
48+
logMap, _ := actual["log"].(mapstr.M)
49+
require.IsType(t, mapstr.M{}, logMap["file"], "expected log.file to be mapstr.M")
50+
fileMap, _ := logMap["file"].(mapstr.M)
5051

51-
inode, err := actual.GetValue(inodeKey)
52-
require.NoError(t, err)
53-
require.Equal(t, "999", inode)
54-
err = actual.Delete(inodeKey)
55-
require.NoError(t, err)
52+
require.Equal(t, "17", fileMap[deviceIDKey])
53+
delete(fileMap, deviceIDKey)
54+
require.Equal(t, "999", fileMap[inodeKey])
55+
delete(fileMap, inodeKey)
5656

57-
_, err = actual.GetValue(ownerKey)
58-
require.Error(t, err)
59-
60-
_, err = actual.GetValue(groupKey)
61-
require.Error(t, err)
57+
_, hasOwner := fileMap[ownerKey]
58+
require.False(t, hasOwner)
59+
_, hasGroup := fileMap[groupKey]
60+
require.False(t, hasGroup)
6261

6362
require.Equal(t, expected, actual)
6463
}
6564

65+
// TestCachedMetaSizing verifies that cachedMeta ends up with exactly the right
66+
// number of entries for each combination of optional fields on this platform.
67+
// Because make(mapstr.M, size) pre-allocates exactly `size` slots, matching
68+
// len(cachedMeta) to the formula proves no map growth occurred.
69+
func TestCachedMetaSizing(t *testing.T) {
70+
fi := createTestFileInfo()
71+
msg := reader.Message{Content: []byte("line"), Bytes: 4, Fields: mapstr.M{}}
72+
73+
tests := []struct {
74+
name string
75+
includeOwner bool
76+
includeGroup bool
77+
fingerprint string
78+
wantLen int
79+
}{
80+
{"base only", false, false, "", 1 + platformFileFields},
81+
{"with owner", true, false, "", 1 + platformFileFields + 1},
82+
{"with group", false, true, "", 1 + platformFileFields + 1},
83+
{"with owner and group", true, true, "", 1 + platformFileFields + 2},
84+
{"with fingerprint", false, false, "hash", 1 + platformFileFields + 1},
85+
{"all fields", true, true, "hash", 1 + platformFileFields + 3},
86+
}
87+
88+
for _, tc := range tests {
89+
t.Run(tc.name, func(t *testing.T) {
90+
r := &FileMetaReader{
91+
reader: msgReader([]reader.Message{msg}),
92+
path: "test/path",
93+
fi: fi,
94+
includeOwner: tc.includeOwner,
95+
includeGroup: tc.includeGroup,
96+
fingerprint: tc.fingerprint,
97+
}
98+
_, err := r.Next()
99+
require.NoError(t, err)
100+
require.Len(t, r.cachedMeta, tc.wantLen,
101+
"cachedMeta entry count should match the pre-allocated size")
102+
})
103+
}
104+
}
105+
66106
func checkFieldsWithOwnerGroup(t *testing.T, expected, actual mapstr.M) {
67107
t.Helper()
68108

69-
dev, err := actual.GetValue(deviceIDKey)
70-
require.NoError(t, err)
71-
require.Equal(t, "17", dev)
72-
err = actual.Delete(deviceIDKey)
73-
require.NoError(t, err)
74-
75-
inode, err := actual.GetValue(inodeKey)
76-
require.NoError(t, err)
77-
require.Equal(t, "999", inode)
78-
err = actual.Delete(inodeKey)
79-
require.NoError(t, err)
80-
81-
o, err := actual.GetValue(ownerKey)
82-
require.NoError(t, err)
83-
require.Equal(t, "root", o)
84-
err = actual.Delete(ownerKey)
85-
require.NoError(t, err)
86-
87-
g, err := actual.GetValue(groupKey)
88-
require.NoError(t, err)
89-
require.Equal(t, "root", g)
90-
err = actual.Delete(groupKey)
91-
require.NoError(t, err)
109+
require.IsType(t, mapstr.M{}, actual["log"], "expected log to be mapstr.M")
110+
logMap, _ := actual["log"].(mapstr.M)
111+
require.IsType(t, mapstr.M{}, logMap["file"], "expected log.file to be mapstr.M")
112+
fileMap, _ := logMap["file"].(mapstr.M)
113+
114+
require.Equal(t, "17", fileMap[deviceIDKey])
115+
delete(fileMap, deviceIDKey)
116+
require.Equal(t, "999", fileMap[inodeKey])
117+
delete(fileMap, inodeKey)
118+
119+
// macOS uses "wheel" for GID 0; Linux uses "root".
120+
expectedGroup := "root"
121+
if runtime.GOOS == "darwin" {
122+
expectedGroup = "wheel"
123+
}
124+
require.Equal(t, "root", fileMap[ownerKey])
125+
delete(fileMap, ownerKey)
126+
require.Equal(t, expectedGroup, fileMap[groupKey])
127+
delete(fileMap, groupKey)
92128

93129
require.Equal(t, expected, actual)
94130
}

0 commit comments

Comments
 (0)