[Ingest Manager] New structure of agent configuration by michalpristas · Pull Request #19128 · elastic/beats

michalpristas · 2020-06-11T12:09:18Z

What does this PR do?

This PR implements all 4 phases of configuration change described here #19082

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

elasticmachine · 2020-06-11T12:09:21Z

Pinging @elastic/ingest-management (Team:Ingest Management)

elasticmachine · 2020-06-11T12:40:26Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #19128 updated]
Start Time: 2020-06-17T11:47:13.856+0000
Duration: 52 min 13 sec

Test stats 🧪

Test	Results
Failed	0
Passed	538
Skipped	127
Total	665

blakerouse · 2020-06-11T12:59:54Z

x-pack/elastic-agent/pkg/agent/transpiler/rules.go

-	for _, datasourceNode := range datasourcesList.value {
-		nsNode, found := datasourceNode.Find("namespace")
+	for _, inputNode := range inputsNodeList.value {
+		nsNode, found := inputNode.Find("dataset.namespace")


Will Find("dataset.namespace") handle the fact that dataset could be a dictionary in the configuration:

dataset: namespace: default name: other

no in fact it will not, i added support for both just to be sure

blakerouse · 2020-06-11T13:00:55Z

x-pack/elastic-agent/pkg/agent/program/testdata/single_config.yml

+    vars:
+      var: value
+  - type: apache/metrics
+    dataset.namespace: testing


Probably good to add one of these tests files to use:

dataset: namespace: testing

do you mean with invalid key?
keys we dont know are passed to input by default

No I mean add a test to ensure that dataset.namespace: testing or

dataset: namespace: testing

Both work

blakerouse · 2020-06-11T13:02:17Z

x-pack/elastic-agent/pkg/agent/transpiler/rules.go

+			addFieldsMap := &Dict{value: []Node{&Key{"add_fields", processorMap}}}
+			processorsList.value = mergeStrategy(r.OnConflict).InjectItem(processorsList.value, addFieldsMap)
+
+			// add this for backwards compatibility remove later


Just a general question: When is the remove later?

up to stack when they remove support for this

later is now :-) this was only a short transition where we needed it.

blakerouse

Looks good.

michalpristas · 2020-06-12T06:36:25Z

NOTE: do not merge until @ruflin gives a green light

ruflin · 2020-06-16T08:10:52Z

x-pack/elastic-agent/elastic-agent.reference.yml

-  - namespace: default
+inputs:
+  - type: system/metrics
+    dataset.namespace: default


Do you already have support for dataset.type? I don't think we need it in this PR but curious if you added it?

yep dataset.type is supported

ruflin · 2020-06-16T08:12:00Z

x-pack/elastic-agent/pkg/agent/program/testdata/constraints_config-filebeat.yml

    paths:
      - /var/log/hello1.log
      - /var/log/hello2.log
-    dataset: generic


Shouldn't this become dataset.name instead of removing it?

yes this became dataset.name thing is we injected it previously which i think is incorrect as we're providing processor to enrich event. if you think this information is helpful in a final beat config i will add it back but for now i filtered it out

Even if the Beat does not understand it today, I think in the future the Beat should so having it there already would be helpful. Can happen in a later PR.

ruflin · 2020-06-16T08:12:59Z

x-pack/elastic-agent/pkg/agent/program/testdata/single_config-filebeat.yml

+            type: testtype
+            name: generic
+            namespace: default
+      - add_fields:


Not related to this PR but can you follow up to make sure we clean up the stream fields?

ruflin · 2020-06-16T08:13:29Z

x-pack/elastic-agent/pkg/agent/program/testdata/single_config.yml

-        id: apache-metrics-id
+    streams:
+      - metricset: status
+        dataset:


I assume dataset.name and

dataset: name: foo

work?

yeah it was a bit tricky as transpiler understands dataset.name as a single key but i made both versions work

ruflin · 2020-06-16T08:13:43Z

x-pack/elastic-agent/pkg/agent/program/testdata/single_config.yml

+      var: value
+  - type: logs
+    dataset:
+      type: testtype


ruflin · 2020-06-16T08:14:17Z

x-pack/elastic-agent/pkg/agent/transpiler/rules.go

-			if !ok {
-				continue
+			dsNode, found := inputNode.Find("dataset")
+			if found {


This looks pretty scary! Would be nice if ew can find a way to simplify this.

ruflin · 2020-06-16T08:15:14Z

x-pack/elastic-agent/pkg/agent/transpiler/rules.go

+			addFieldsMap := &Dict{value: []Node{&Key{"add_fields", processorMap}}}
+			processorsList.value = mergeStrategy(r.OnConflict).InjectItem(processorsList.value, addFieldsMap)
+
+			// add this for backwards compatibility remove later


later is now :-) this was only a short transition where we needed it.

ruflin · 2020-06-16T08:16:08Z

x-pack/elastic-agent/pkg/agent/transpiler/rules.go

+func datasetNamespaceFromInputNode(inputNode Node) string {
+	const defaultNamespace = "default"
+
+	if namespaceNode, found := inputNode.Find("dataset.namespace"); found {


Looks like we have very similar code in several places. Two things I'm wondering: Could go-ucfg to this for you and if not, could we extract it into a method?

ruflin · 2020-06-16T08:58:36Z

Tried to test it together with elastic/kibana#69226 but didn't work yet. Not sure if the problem is in KB, Agent or a general issue. Overall changes LGTM.

michalpristas · 2020-06-16T15:10:07Z

tested and setting dataset.type to system/metrics causes auth issues. when i set it to metrics it works fine

ruflin · 2020-06-17T07:39:56Z

Makes sense as dataset.type: system/metrics is not a valid type. Does it work with the most recent Kibana changes?

michalpristas · 2020-06-17T15:22:30Z

Tested with latest kibana changes ok

* phase 1 * phase 2 * phase 4 * updated configuration * fixed compact form (depends on aprser) * configuration update * fixed tests * mod

michalpristas added 5 commits June 11, 2020 11:16

phase 1

4e79b49

phase 2

a044e9c

phase 4

3f2e35e

updated configuration

94741ed

fixed compact form (depends on aprser)

bd8f87c

michalpristas added review needs_backport PR is waiting to be backported to other branches. Team:Ingest Management Ingest Management:beta1 Group issues for ingest management beta1 labels Jun 11, 2020

michalpristas self-assigned this Jun 11, 2020

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 11, 2020

blakerouse reviewed Jun 11, 2020

View reviewed changes

michalpristas added 2 commits June 11, 2020 15:09

configuration update

e8b58ba

fixed tests

c57a694

blakerouse approved these changes Jun 11, 2020

View reviewed changes

ruflin reviewed Jun 16, 2020

View reviewed changes

conflicts

85ad9c5

michalpristas added 2 commits June 17, 2020 13:45

Merge branch 'master' of github.com:elastic/beats into agent-ds-config

299bf28

mod

903bc2e

michalpristas merged commit 3e4b0c6 into elastic:master Jun 17, 2020

ruflin mentioned this pull request Jun 17, 2020

[Elastic Agent] Change structure of Elastic Agent across the Stack #19082

Closed

15 tasks

michalpristas mentioned this pull request Jun 18, 2020

Cherry-pick #19128 to 7.x: New structure of agent configuration #19270

Merged

6 tasks

michalpristas added a commit that referenced this pull request Jun 24, 2020

[Ingest Manager] New structure of agent configuration (#19128) (#19270)

d5da517

* phase 1 * phase 2 * phase 4 * updated configuration * fixed compact form (depends on aprser) * configuration update * fixed tests * mod

This was referenced Jun 24, 2020

[Ingest Manager] Stream fields removed from injected configurations #19356

Merged

Cherry-pick #19356 to 7.x: Stream fields removed from injected configurations #19389

Merged

Conversation

michalpristas commented Jun 11, 2020

What does this PR do?

Checklist

Uh oh!

elasticmachine commented Jun 11, 2020

Uh oh!

elasticmachine commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blakerouse left a comment

Choose a reason for hiding this comment

Uh oh!

michalpristas commented Jun 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruflin commented Jun 16, 2020

Uh oh!

michalpristas commented Jun 16, 2020

Uh oh!

ruflin commented Jun 17, 2020

Uh oh!

michalpristas commented Jun 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elasticmachine commented Jun 11, 2020 •

edited

Loading