From 318fdba64ccd1e824cac4cb55ce144ca09eded06 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 18 Sep 2023 16:32:57 +0200 Subject: [PATCH] [7.17](backport #35945) Decrease Clones (#36593) * Decrease Clones (#35945) * Decrease Clones * Adding changelog * CodeReview changes (cherry picked from commit faf88b7d697ae708d1ecaa18214801717f2f014f) # Conflicts: # libbeat/beat/event.go # libbeat/beat/event_test.go --------- Co-authored-by: Evgeniy Belyi Co-authored-by: Denis Rechkunov --- CHANGELOG.next.asciidoc | 8 +- libbeat/beat/event.go | 77 ++++++++------ libbeat/beat/event_test.go | 199 +++++++++++++++++++++++++------------ 3 files changed, 181 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index c4222839a9d..abe22daabcd 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -30,7 +30,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d *Affecting all Beats* -*Filebeat* +- Eliminate cloning of event in deepUpdate {pull}35945[35945] *Auditbeat* @@ -141,9 +141,3 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d *Functionbeat* ==== Known Issue - - - - - - diff --git a/libbeat/beat/event.go b/libbeat/beat/event.go index 7fd016a14dc..68b015dbd46 100644 --- a/libbeat/beat/event.go +++ b/libbeat/beat/event.go @@ -98,45 +98,62 @@ func (e *Event) deepUpdate(d common.MapStr, overwrite bool) { if len(d) == 0 { return } - fieldsUpdate := d.Clone() // so we can delete redundant keys - var metaUpdate common.MapStr + // It's supported to update the timestamp using this function. + // However, we must handle it separately since it's a separate field of the event. + timestampValue, timestampExists := d[timestampFieldKey] + if timestampExists { + if overwrite { + _ = e.setTimestamp(timestampValue) + } - for fieldKey, value := range d { - switch fieldKey { + // Temporary delete it from the update map, + // so we can do `e.Fields.DeepUpdate(d)` or + // `e.Fields.DeepUpdateNoOverwrite(d)` later + delete(d, timestampFieldKey) + } - // one of the updates is the timestamp which is not a part of the event fields - case timestampFieldKey: - if overwrite { - _ = e.setTimestamp(value) - } - delete(fieldsUpdate, fieldKey) + // It's supported to update the metadata using this function. + // However, we must handle it separately since it's a separate field of the event. + metaValue, metaExists := d[metadataFieldKey] + if metaExists { + var metaUpdate common.MapStr + + switch meta := metaValue.(type) { + case common.MapStr: + metaUpdate = meta + case map[string]interface{}: + metaUpdate = common.MapStr(meta) + } - // some updates are addressed for the metadata not the fields - case metadataFieldKey: - switch meta := value.(type) { - case common.MapStr: - metaUpdate = meta - case map[string]interface{}: - metaUpdate = common.MapStr(meta) + if metaUpdate != nil { + if e.Meta == nil { + e.Meta = common.MapStr{} + } + if overwrite { + e.Meta.DeepUpdate(metaUpdate) + } else { + e.Meta.DeepUpdateNoOverwrite(metaUpdate) } - - delete(fieldsUpdate, fieldKey) } + + // Temporary delete it from the update map, + // so we can do `e.Fields.DeepUpdate(d)` or + // `e.Fields.DeepUpdateNoOverwrite(d)` later + delete(d, metadataFieldKey) } - if metaUpdate != nil { - if e.Meta == nil { - e.Meta = common.MapStr{} + // At the end we revert all changes we made to the update map + defer func() { + if timestampExists { + d[timestampFieldKey] = timestampValue } - if overwrite { - e.Meta.DeepUpdate(metaUpdate) - } else { - e.Meta.DeepUpdateNoOverwrite(metaUpdate) + if metaExists { + d[metadataFieldKey] = metaValue } - } + }() - if len(fieldsUpdate) == 0 { + if len(d) == 0 { return } @@ -145,9 +162,9 @@ func (e *Event) deepUpdate(d common.MapStr, overwrite bool) { } if overwrite { - e.Fields.DeepUpdate(fieldsUpdate) + e.Fields.DeepUpdate(d) } else { - e.Fields.DeepUpdateNoOverwrite(fieldsUpdate) + e.Fields.DeepUpdateNoOverwrite(d) } } diff --git a/libbeat/beat/event_test.go b/libbeat/beat/event_test.go index 2f4f226ecf9..1e03268a4a7 100644 --- a/libbeat/beat/event_test.go +++ b/libbeat/beat/event_test.go @@ -18,6 +18,7 @@ package beat import ( + "crypto/rand" "testing" "time" @@ -26,29 +27,77 @@ import ( "github.com/elastic/beats/v7/libbeat/common" ) +const ( + propSize = 1024 * 2014 * 10 +) + +var largeProp string + +func init() { + b := make([]byte, propSize) + _, _ = rand.Read(b) + largeProp = string(b) +} + func newEmptyEvent() *Event { return &Event{Fields: common.MapStr{}} } -func TestEventPutGetTimestamp(t *testing.T) { +func newEvent(e common.MapStr) *Event { + n := &common.MapStr{ + "Fields": common.MapStr{ + "large_prop": largeProp, + }, + } + n.DeepUpdate(e) + var ts time.Time + var meta common.MapStr + var fields common.MapStr + var private common.MapStr + + v, ex := (*n)["Timestamp"] + if ex { + ts = v.(time.Time) + } + v, ex = (*n)["Meta"] + if ex { + meta = v.(common.MapStr) + } + v, ex = (*n)["Fields"] + if ex { + fields = v.(common.MapStr) + } + v, ex = (*n)["Private"] + if ex { + private = v.(common.MapStr) + } + return &Event{ + Timestamp: ts, + Meta: meta, + Fields: fields, + Private: private, + } +} + +func BenchmarkTestEventPutGetTimestamp(b *testing.B) { evt := newEmptyEvent() ts := time.Now() - evt.PutValue("@timestamp", ts) + _, _ = evt.PutValue("@timestamp", ts) v, err := evt.GetValue("@timestamp") if err != nil { - t.Fatal(err) + b.Fatal(err) } - assert.Equal(t, ts, v) - assert.Equal(t, ts, evt.Timestamp) + assert.Equal(b, ts, v) + assert.Equal(b, ts, evt.Timestamp) // The @timestamp is not written into Fields. - assert.Nil(t, evt.Fields["@timestamp"]) + assert.Nil(b, evt.Fields["@timestamp"]) } -func TestDeepUpdate(t *testing.T) { +func BenchmarkTestDeepUpdate(b *testing.B) { ts := time.Now() cases := []struct { @@ -60,37 +109,43 @@ func TestDeepUpdate(t *testing.T) { }{ { name: "does nothing if no update", - event: &Event{}, + event: newEvent(common.MapStr{}), update: common.MapStr{}, - expected: &Event{}, + expected: newEvent(common.MapStr{}), }, { name: "updates timestamp", - event: &Event{}, + event: newEvent(common.MapStr{}), update: common.MapStr{ timestampFieldKey: ts, }, overwrite: true, expected: &Event{ Timestamp: ts, + Fields: common.MapStr{ + "large_prop": largeProp, + }, }, }, { name: "does not overwrite timestamp", - event: &Event{ - Timestamp: ts, - }, + event: newEvent(common.MapStr{ + "Timestamp": ts, + }), update: common.MapStr{ timestampFieldKey: time.Now().Add(time.Hour), }, overwrite: false, expected: &Event{ Timestamp: ts, + Fields: common.MapStr{ + "large_prop": largeProp, + }, }, }, { name: "initializes metadata if nil", - event: &Event{}, + event: newEvent(common.MapStr{}), update: common.MapStr{ metadataFieldKey: common.MapStr{ "first": "new", @@ -102,15 +157,18 @@ func TestDeepUpdate(t *testing.T) { "first": "new", "second": 42, }, + Fields: common.MapStr{ + "large_prop": largeProp, + }, }, }, { name: "updates metadata but does not overwrite", - event: &Event{ - Meta: common.MapStr{ + event: newEvent(common.MapStr{ + "Meta": common.MapStr{ "first": "initial", }, - }, + }), update: common.MapStr{ metadataFieldKey: common.MapStr{ "first": "new", @@ -123,15 +181,18 @@ func TestDeepUpdate(t *testing.T) { "first": "initial", "second": 42, }, + Fields: common.MapStr{ + "large_prop": largeProp, + }, }, }, { name: "updates metadata and overwrites", - event: &Event{ - Meta: common.MapStr{ + event: newEvent(common.MapStr{ + "Meta": common.MapStr{ "first": "initial", }, - }, + }), update: common.MapStr{ metadataFieldKey: common.MapStr{ "first": "new", @@ -144,15 +205,18 @@ func TestDeepUpdate(t *testing.T) { "first": "new", "second": 42, }, + Fields: common.MapStr{ + "large_prop": largeProp, + }, }, }, { name: "updates fields but does not overwrite", - event: &Event{ - Fields: common.MapStr{ + event: newEvent(common.MapStr{ + "Fields": common.MapStr{ "first": "initial", }, - }, + }), update: common.MapStr{ "first": "new", "second": 42, @@ -160,18 +224,19 @@ func TestDeepUpdate(t *testing.T) { overwrite: false, expected: &Event{ Fields: common.MapStr{ - "first": "initial", - "second": 42, + "first": "initial", + "second": 42, + "large_prop": largeProp, }, }, }, { name: "updates metadata and overwrites", - event: &Event{ - Fields: common.MapStr{ + event: newEvent(common.MapStr{ + "Fields": common.MapStr{ "first": "initial", }, - }, + }), update: common.MapStr{ "first": "new", "second": 42, @@ -179,123 +244,125 @@ func TestDeepUpdate(t *testing.T) { overwrite: true, expected: &Event{ Fields: common.MapStr{ - "first": "new", - "second": 42, + "first": "new", + "second": 42, + "large_prop": largeProp, }, }, }, { name: "initializes fields if nil", - event: &Event{}, + event: newEvent(common.MapStr{}), update: common.MapStr{ "first": "new", "second": 42, }, expected: &Event{ Fields: common.MapStr{ - "first": "new", - "second": 42, + "first": "new", + "second": 42, + "large_prop": largeProp, }, }, }, } for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { + b.Run(tc.name, func(b *testing.B) { tc.event.deepUpdate(tc.update, tc.overwrite) - assert.Equal(t, tc.expected.Timestamp, tc.event.Timestamp) - assert.Equal(t, tc.expected.Fields, tc.event.Fields) - assert.Equal(t, tc.expected.Meta, tc.event.Meta) + assert.Equal(b, tc.expected.Timestamp, tc.event.Timestamp) + assert.Equal(b, tc.expected.Fields, tc.event.Fields) + assert.Equal(b, tc.expected.Meta, tc.event.Meta) }) } } -func TestEventMetadata(t *testing.T) { +func BenchmarkTestEventMetadata(b *testing.B) { const id = "123" newMeta := func() common.MapStr { return common.MapStr{"_id": id} } - t.Run("put", func(t *testing.T) { + b.Run("put", func(b *testing.B) { evt := newEmptyEvent() meta := newMeta() - evt.PutValue("@metadata", meta) + _, _ = evt.PutValue("@metadata", meta) - assert.Equal(t, meta, evt.Meta) - assert.Empty(t, evt.Fields) + assert.Equal(b, meta, evt.Meta) + assert.Empty(b, evt.Fields) }) - t.Run("get", func(t *testing.T) { + b.Run("get", func(b *testing.B) { evt := newEmptyEvent() evt.Meta = newMeta() meta, err := evt.GetValue("@metadata") - assert.NoError(t, err) - assert.Equal(t, evt.Meta, meta) + assert.NoError(b, err) + assert.Equal(b, evt.Meta, meta) }) - t.Run("put sub-key", func(t *testing.T) { + b.Run("put sub-key", func(b *testing.B) { evt := newEmptyEvent() - evt.PutValue("@metadata._id", id) + _, _ = evt.PutValue("@metadata._id", id) - assert.Equal(t, newMeta(), evt.Meta) - assert.Empty(t, evt.Fields) + assert.Equal(b, newMeta(), evt.Meta) + assert.Empty(b, evt.Fields) }) - t.Run("get sub-key", func(t *testing.T) { + b.Run("get sub-key", func(b *testing.B) { evt := newEmptyEvent() evt.Meta = newMeta() v, err := evt.GetValue("@metadata._id") - assert.NoError(t, err) - assert.Equal(t, id, v) + assert.NoError(b, err) + assert.Equal(b, id, v) }) - t.Run("delete", func(t *testing.T) { + b.Run("delete", func(b *testing.B) { evt := newEmptyEvent() evt.Meta = newMeta() err := evt.Delete("@metadata") - assert.NoError(t, err) - assert.Nil(t, evt.Meta) + assert.NoError(b, err) + assert.Nil(b, evt.Meta) }) - t.Run("delete sub-key", func(t *testing.T) { + b.Run("delete sub-key", func(b *testing.B) { evt := newEmptyEvent() evt.Meta = newMeta() err := evt.Delete("@metadata._id") - assert.NoError(t, err) - assert.Empty(t, evt.Meta) + assert.NoError(b, err) + assert.Empty(b, evt.Meta) }) - t.Run("setID", func(t *testing.T) { + b.Run("setID", func(b *testing.B) { evt := newEmptyEvent() evt.SetID(id) - assert.Equal(t, newMeta(), evt.Meta) + assert.Equal(b, newMeta(), evt.Meta) }) - t.Run("put non-metadata", func(t *testing.T) { + b.Run("put non-metadata", func(b *testing.B) { evt := newEmptyEvent() - evt.PutValue("@metadataSpecial", id) + _, _ = evt.PutValue("@metadataSpecial", id) - assert.Equal(t, common.MapStr{"@metadataSpecial": id}, evt.Fields) + assert.Equal(b, common.MapStr{"@metadataSpecial": id}, evt.Fields) }) - t.Run("delete non-metadata", func(t *testing.T) { + b.Run("delete non-metadata", func(b *testing.B) { evt := newEmptyEvent() evt.Meta = newMeta() err := evt.Delete("@metadataSpecial") - assert.Error(t, err) - assert.Equal(t, newMeta(), evt.Meta) + assert.Error(b, err) + assert.Equal(b, newMeta(), evt.Meta) }) }