From a6be95d122be514b52abac8bf70f63b2f3b29465 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Mon, 9 Dec 2024 16:06:56 +0000 Subject: [PATCH] Avoid unnecessary copies during config generation (#6184) * Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f3229b8af401f2c2db29352e50eaaec9e3f90) # Conflicts: # internal/pkg/agent/transpiler/ast.go --- .../1733158776-avoid-copies-config-gen.yaml | 32 +++++++++++++++ .../application/coordinator/coordinator.go | 2 +- internal/pkg/agent/transpiler/ast.go | 13 ++++--- internal/pkg/agent/transpiler/ast_test.go | 39 ++++++++++++++++++- internal/pkg/agent/transpiler/utils.go | 3 +- 5 files changed, 80 insertions(+), 9 deletions(-) create mode 100644 changelog/fragments/1733158776-avoid-copies-config-gen.yaml diff --git a/changelog/fragments/1733158776-avoid-copies-config-gen.yaml b/changelog/fragments/1733158776-avoid-copies-config-gen.yaml new file mode 100644 index 00000000000..e585edca413 --- /dev/null +++ b/changelog/fragments/1733158776-avoid-copies-config-gen.yaml @@ -0,0 +1,32 @@ +# Kind can be one of: +# - breaking-change: a change to previously-documented behavior +# - deprecation: functionality that is being removed in a later release +# - bug-fix: fixes a problem in a previous version +# - enhancement: extends functionality but does not break or fix existing behavior +# - feature: new functionality +# - known-issue: problems that we are aware of in a given version +# - security: impacts on the security of a product or a user’s deployment. +# - upgrade: important information for someone upgrading from a prior version +# - other: does not fit into any of the other categories +kind: enhancement + +# Change summary; a 80ish characters long description of the change. +summary: Avoid unnecessary copies when generating component configurations + +# Long description; in case the summary is not enough to describe the change +# this field accommodate a description without length limits. +# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. +#description: + +# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. +component: elastic-agent + +# PR URL; optional; the PR number that added the changeset. +# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. +# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. +# Please provide it if you are adding a fragment for a different PR. +#pr: https://github.com/owner/repo/1234 + +# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). +# If not present is automatically filled by the tooling with the issue linked to the PR number. +#issue: https://github.com/owner/repo/1234 diff --git a/internal/pkg/agent/application/coordinator/coordinator.go b/internal/pkg/agent/application/coordinator/coordinator.go index 12debd7b6ad..1533b7fb0a5 100644 --- a/internal/pkg/agent/application/coordinator/coordinator.go +++ b/internal/pkg/agent/application/coordinator/coordinator.go @@ -1282,7 +1282,7 @@ func (c *Coordinator) generateComponentModel() (err error) { c.setComponentGenError(err) }() - ast := c.ast.Clone() + ast := c.ast.ShallowClone() inputs, ok := transpiler.Lookup(ast, "inputs") if ok { renderedInputs, err := transpiler.RenderInputs(inputs, c.vars) diff --git a/internal/pkg/agent/transpiler/ast.go b/internal/pkg/agent/transpiler/ast.go index 1fae370ce40..b6490586ca1 100644 --- a/internal/pkg/agent/transpiler/ast.go +++ b/internal/pkg/agent/transpiler/ast.go @@ -58,7 +58,7 @@ type Node interface { // Hash compute a sha256 hash of the current node and recursively call any children. Hash() []byte - // Apply apply the current vars, returning the new value for the node. + // Apply apply the current vars, returning the new value for the node. This does not modify the original Node. Apply(*Vars) (Node, error) // Processors returns any attached processors, because of variable substitution. @@ -147,7 +147,8 @@ func (d *Dict) ShallowClone() Node { if i == nil { continue } - nodes = append(nodes, i) + // Dict nodes are key-value pairs, and we do want to make a copy of the key here + nodes = append(nodes, i.ShallowClone()) } return &Dict{value: nodes} @@ -162,7 +163,7 @@ func (d *Dict) Hash() []byte { return h.Sum(nil) } -// Apply applies the vars to all the nodes in the dictionary. +// Apply applies the vars to all the nodes in the dictionary. This does not modify the original dictionary. func (d *Dict) Apply(vars *Vars) (Node, error) { nodes := make([]Node, 0, len(d.value)) for _, v := range d.value { @@ -277,7 +278,7 @@ func (k *Key) Hash() []byte { return h.Sum(nil) } -// Apply applies the vars to the value. +// Apply applies the vars to the value. This does not modify the original node. func (k *Key) Apply(vars *Vars) (Node, error) { if k.value == nil { return k, nil @@ -397,7 +398,7 @@ func (l *List) ShallowClone() Node { return &List{value: nodes} } -// Apply applies the vars to all nodes in the list. +// Apply applies the vars to all nodes in the list. This does not modify the original list. func (l *List) Apply(vars *Vars) (Node, error) { nodes := make([]Node, 0, len(l.value)) for _, v := range l.value { @@ -472,7 +473,7 @@ func (s *StrVal) Hash() []byte { return []byte(s.value) } -// Apply applies the vars to the string value. +// Apply applies the vars to the string value. This does not modify the original string. func (s *StrVal) Apply(vars *Vars) (Node, error) { return vars.Replace(s.value) } diff --git a/internal/pkg/agent/transpiler/ast_test.go b/internal/pkg/agent/transpiler/ast_test.go index 098b6be9107..dd4f2f9d448 100644 --- a/internal/pkg/agent/transpiler/ast_test.go +++ b/internal/pkg/agent/transpiler/ast_test.go @@ -9,6 +9,8 @@ import ( "reflect" "testing" + "github.com/elastic/elastic-agent-libs/mapstr" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -852,6 +854,41 @@ func TestHash(t *testing.T) { } } +func TestApplyDoesNotMutate(t *testing.T) { + tests := map[string]struct { + input Node + }{ + "dict": { + &Dict{ + value: []Node{ + &Key{name: "str", value: &StrVal{value: "${var}"}}, + }, + }, + }, + "list": { + &List{ + value: []Node{ + &StrVal{value: "${var}"}, + }, + }, + }, + "key": { + &Key{name: "str", value: &StrVal{value: "${var}"}}, + }, + "str": {&StrVal{value: "${var}"}}, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + vars, err := NewVars("", map[string]any{"var": "value"}, mapstr.M{}) + require.NoError(t, err) + applied, err := test.input.Apply(vars) + require.NoError(t, err) + assert.NotEqual(t, test.input, applied) + }) + } +} + func TestShallowClone(t *testing.T) { tests := map[string]struct { input *AST @@ -909,7 +946,7 @@ func TestShallowClone(t *testing.T) { t.Run(name, func(t *testing.T) { cloned := test.input.ShallowClone() assert.Equal(t, test.input, cloned) - err := test.input.Insert(&AST{root: &BoolVal{value: true}}, "key") + err := test.input.Insert(&AST{root: &Key{name: "integer", value: &IntVal{value: 7}}}, "integer") if err == nil { assert.NotEqual(t, test.input, cloned) } else if list, ok := test.input.root.(*List); ok { diff --git a/internal/pkg/agent/transpiler/utils.go b/internal/pkg/agent/transpiler/utils.go index 05ee174f4d3..e726fd996a5 100644 --- a/internal/pkg/agent/transpiler/utils.go +++ b/internal/pkg/agent/transpiler/utils.go @@ -26,7 +26,7 @@ func RenderInputs(inputs Node, varsArray []*Vars) (Node, error) { nodesMap := map[string]*Dict{} for _, vars := range varsArray { for _, node := range l.Value().([]Node) { - dict, ok := node.Clone().(*Dict) + dict, ok := node.(*Dict) if !ok { continue } @@ -34,6 +34,7 @@ func RenderInputs(inputs Node, varsArray []*Vars) (Node, error) { if streams := getStreams(dict); streams != nil { hadStreams = true } + // Apply creates a new Node with a deep copy of all the values n, err := dict.Apply(vars) if errors.Is(err, ErrNoMatch) { // has a variable that didn't exist, so we ignore it