Allow ConcatFields for nodes of type object #10959

simitt · 2019-02-27T09:13:41Z

Allow ConcatFields to concatenate two fields when the first field contains a node's parent key but not the node itself and it is of type object.

Current behaviour (simplified):

a := a.b.c
b := a.b.c.d
f.ConcatFields(a, b) => error

Behaviour after merging this PR:

a := a.b.c
b := a.b.c.d
f.ConcatFields(a, b) => error

a := a.b.c, type(c)=object
b := a.b.c.d
f.ConcatFields(a, b) => success

This needs to be backported to >=6.6 as it leads to a concrete bug in APM, see apm-server#1959

Return false if field contains a nodes parent key but not the node itself.

simitt · 2019-02-27T14:24:17Z

jenkins, retest this please

libbeat/common/field.go

graphaelli · 2019-02-27T19:35:56Z

libbeat/common/field.go

@@ -304,7 +344,7 @@ func ConcatFields(a, b Fields) (Fields, error) {

 	// check for duplicates
 	for _, k := range b.GetKeys() {
-		if a.HasNode(k) {
+		if !a.CanConcat(k) {


This was the only caller of HasNode, can that be removed now? Can this implementation replace the previous one instead of introducing a new function?

I didn't remove it as it is an exported method, and we should backport the bugfix to 6.x (best 6.6).

graphaelli · 2019-02-27T19:36:25Z

libbeat/common/field_test.go

@@ -27,6 +27,58 @@ import (
 	"github.com/elastic/go-ucfg/yaml"
 )

+func TestFieldsHasNode(t *testing.T) {


is this actually supposed to test CanConcat?

~~Yes, will change.~~
There were no explicit tests for HasNode so I added them. Will add dedicated CanConcat tests though to make it more clear.

libbeat/common/field.go

simitt · 2019-02-28T16:09:25Z

@elastic/beats could someone please review and lmk if you are fine with backporting to 7.0, 6.7 and 6.6 branches?

urso · 2019-03-04T18:40:12Z

libbeat/common/field.go

@@ -292,6 +292,46 @@ func (f Fields) getKeys(namespace string) []string {
 	return keys
 }

+// canConcat checks if inside fields the given node can be concatenated.
+// If the node already exists, this is only allowed for nodes of type object.
+func (f Fields) CanConcat(key string) bool {


Can we clarify the comment for when fields are concatenable. How about naming it conflicts ? This is what the for loop on ConcatFields does, look for conflicts between 2 field arrays.

canConcat checks that no node is reachable by key AND that the common prefix between all fields and the path given by key resolves into an object-node. Did I get this right?

We should return an error describing why key conflicts with f. Moving the loop from ConcatFields into a separate loop allows us to cleanly collect N errors into a multierror.

Do we need to export CanConcat? We seem to export too many symbols already. Let's try to not export symbols unless we know we will need them.

urso · 2019-03-04T18:44:23Z

libbeat/common/field.go

+			//// It's the last key to compare
+			if len(keys) == 0 {
+				return false
+			}


nit:

////

lets not add extra whitespace between for and if statements or beginning of function body. In most cases indentation should be enough to distinguish blocks.

urso · 2019-03-04T18:48:35Z

Logic LGTM.

urso · 2019-03-04T18:48:45Z

Jenkins, test this.

simitt · 2019-03-05T12:52:33Z

@urso I followed your suggestions, please have a look again.

urso · 2019-03-05T13:14:49Z

Note: Metricbeat windows tests didn't run due to build failure. All other tests have been green.

Checks if given field conflicts with current fields. Fixes an error where `append_fields` could not add valid field.

…11094) Checks if given field conflicts with current fields. Fixes an error where `append_fields` could not add valid field.

Fix field.HasNode method.

ca4979e

Return false if field contains a nodes parent key but not the node itself.

simitt added bug needs_backport PR is waiting to be backported to other branches. labels Feb 27, 2019

simitt requested a review from a team as a code owner February 27, 2019 09:13

simitt closed this Feb 27, 2019

Allow inserting fields when node is object.

4a85ca2

simitt changed the title ~~Fix field.HasNode method.~~ Allo ConcatFields for nodes of type object Feb 27, 2019

simitt reopened this Feb 27, 2019

simitt mentioned this pull request Feb 27, 2019

append_fields in template leads to invalid template if parent key is already contained elastic/apm-server#1959

Closed

graphaelli changed the title ~~Allo ConcatFields for nodes of type object~~ Allow ConcatFields for nodes of type object Feb 27, 2019

graphaelli reviewed Feb 27, 2019

View reviewed changes

cleanup

153ff19

houndci-bot reviewed Feb 28, 2019

View reviewed changes

libbeat/common/field.go Outdated Show resolved Hide resolved

Add changelog entry.

8eb7ac7

graphaelli approved these changes Feb 28, 2019

View reviewed changes

Merge branch 'master' into fix-field-has-node

cf96b88

urso reviewed Mar 4, 2019

View reviewed changes

Adress PR comments.

445398f

urso approved these changes Mar 5, 2019

View reviewed changes

Merge remote-tracking branch 'elastic/master' into fix-field-has-node

2e38d63

simitt merged commit 527fbd4 into elastic:master Mar 5, 2019

simitt added a commit to simitt/beats that referenced this pull request Mar 5, 2019

Allow ConcatFields for nodes of type object (elastic#10959)

29ec1b7

Checks if given field conflicts with current fields. Fixes an error where `append_fields` could not add valid field.

simitt mentioned this pull request Mar 5, 2019

[7.0] Allow ConcatFields for nodes of type object (#10959) #11092

Merged

simitt added a commit to simitt/beats that referenced this pull request Mar 5, 2019

Allow ConcatFields for nodes of type object (elastic#10959)

86b1a17

Checks if given field conflicts with current fields. Fixes an error where `append_fields` could not add valid field.

simitt mentioned this pull request Mar 5, 2019

[6.7] Allow ConcatFields for nodes of type object (#10959) #11093

Merged

simitt added a commit to simitt/beats that referenced this pull request Mar 5, 2019

Allow ConcatFields for nodes of type object (elastic#10959)

8d3acfa

Checks if given field conflicts with current fields. Fixes an error where `append_fields` could not add valid field.

simitt added a commit that referenced this pull request Mar 5, 2019

Allow ConcatFields for nodes of type object (#10959) (#11094)

9d5f9a9

Checks if given field conflicts with current fields. Fixes an error where `append_fields` could not add valid field.

simitt added a commit that referenced this pull request Mar 6, 2019

Allow ConcatFields for nodes of type object (#10959) (#11093)

b1d6005

Checks if given field conflicts with current fields. Fixes an error where `append_fields` could not add valid field.

simitt added a commit that referenced this pull request Mar 6, 2019

Allow ConcatFields for nodes of type object (#10959) (#11092)

d2ea0db

Checks if given field conflicts with current fields. Fixes an error where `append_fields` could not add valid field.

simitt deleted the fix-field-has-node branch May 6, 2019 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ConcatFields for nodes of type object #10959

Allow ConcatFields for nodes of type object #10959

simitt commented Feb 27, 2019 •

edited

Loading

simitt commented Feb 27, 2019

graphaelli Feb 27, 2019

simitt Feb 28, 2019

graphaelli Feb 27, 2019

simitt Feb 28, 2019 •

edited

Loading

simitt commented Feb 28, 2019

urso Mar 4, 2019 •

edited

Loading

urso Mar 4, 2019

urso commented Mar 4, 2019

urso commented Mar 4, 2019

simitt commented Mar 5, 2019

urso commented Mar 5, 2019

Allow ConcatFields for nodes of type object #10959

Allow ConcatFields for nodes of type object #10959

Conversation

simitt commented Feb 27, 2019 • edited Loading

simitt commented Feb 27, 2019

graphaelli Feb 27, 2019

Choose a reason for hiding this comment

simitt Feb 28, 2019

Choose a reason for hiding this comment

graphaelli Feb 27, 2019

Choose a reason for hiding this comment

simitt Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

simitt commented Feb 28, 2019

urso Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

urso Mar 4, 2019

Choose a reason for hiding this comment

urso commented Mar 4, 2019

urso commented Mar 4, 2019

simitt commented Mar 5, 2019

urso commented Mar 5, 2019

simitt commented Feb 27, 2019 •

edited

Loading

simitt Feb 28, 2019 •

edited

Loading

urso Mar 4, 2019 •

edited

Loading