Skip to content

Commit

Permalink
Don't unnecessarily copy AST when rendering inputs
Browse files Browse the repository at this point in the history
# Conflicts:
#	internal/pkg/agent/transpiler/ast.go
  • Loading branch information
swiatekm committed Dec 2, 2024
1 parent 928f1e1 commit f0bf34d
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 6 deletions.
32 changes: 32 additions & 0 deletions changelog/fragments/1733158776-avoid-copies-config-gen.yaml
Original file line number Diff line number Diff line change
@@ -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
10 changes: 5 additions & 5 deletions internal/pkg/agent/transpiler/ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ type Node interface {
// the capacity of the array had to be changed.
Vars([]string) []string

// 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.
Expand Down Expand Up @@ -176,7 +176,7 @@ func (d *Dict) Vars(vars []string) []string {
return vars
}

// 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 {
Expand Down Expand Up @@ -299,7 +299,7 @@ func (k *Key) Vars(vars []string) []string {
return k.value.Vars(vars)
}

// 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
Expand Down Expand Up @@ -427,7 +427,7 @@ func (l *List) Vars(vars []string) []string {
return vars
}

// 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 {
Expand Down Expand Up @@ -512,7 +512,7 @@ func (s *StrVal) Vars(vars []string) []string {
return vars
}

// 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)
}
Expand Down
37 changes: 37 additions & 0 deletions internal/pkg/agent/transpiler/ast_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion internal/pkg/agent/transpiler/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,15 @@ 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
}
hadStreams := false
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
Expand Down

0 comments on commit f0bf34d

Please sign in to comment.