Add agent.{id,ephemeral_id} to all beat events by andrewvc · Pull Request #9404 · elastic/beats

andrewvc · 2018-12-05T18:56:56Z

This adds the ECS fields agent.id and agent.ephemeral_id to all beats.

Heartbeat in particular needs these to execute aggregations queries in dashboards.

I could use some feedback here on the best place to add tests. AFAICT existing similar functionality for these baseline fields is not tested. Thoughts @urso ?

elasticmachine · 2018-12-05T18:56:58Z

Pinging @elastic/uptime

ruflin

+1 on adding this feature. @urso your input would be good here.

ruflin · 2018-12-06T09:11:40Z

libbeat/beat/info.go

We already have a ephemeral id: https://github.com/elastic/beats/blob/master/libbeat/cmd/instance/metrics_common.go#L31

Yes, not easy to find. I think here is a better place for it.

Errr, this isn't a new UUID, we pull it from https://github.com/elastic/beats/pull/9404/files#diff-73bd591578392eb2d281f4778ac9a47eR88

Oops, I meant to say: https://github.com/elastic/beats/pull/9404/files#diff-f7226280e7734d6e3d88e7dde232602eR217

Actually, it already is pulled from where you mentioned (this line is the type declaration)

urso

+1 on this change.

As we convert them to strings anyway (which needs an alloc) how about changing the fields to string and name them ID and EphemeralID (yeah I know this would be a breaking change)?

andrewvc · 2018-12-06T21:08:27Z

@urso yeah, I'm glad to rename UUID -> EphemeralID

andrewvc · 2018-12-06T21:15:52Z

@urso @ruflin I've done the rename. That wound up touching a number of files FYI

webmat

Wearing my ECS hat, this LGTM.

I can't comment much on the Go code (although it looks good to me).

ruflin

This also needs a changelog entry. Overall LGTM.

ruflin · 2018-12-07T10:34:43Z

libbeat/beat/info.go

Why did you change thsi to beat machine from instance? This is unique for each instance.

andrewvc · 2018-12-12T01:46:16Z

Hit a flaky test. This PR is now blocked on the SkipTest PR for that here: #9503

andrewvc · 2018-12-12T01:49:10Z

@ruflin changelog entry added. Let me know if we're LGTM (though we'll still be blocked on the Flaky test merge)

ruflin · 2018-12-12T07:18:00Z

@andrewvc The tests that are failing are golden files which do not seem to be up-to-date. But I could not reproduce it locally. Any chance you could rebase on master and see if it still happens?

ruflin · 2018-12-13T10:29:39Z

@andrewvc I just realised now that the failing tests are actually correct. The reason is that we have added a new field to Beats so it affects the golden files. To update them run the following commands:

cd testing/environment
make start
cd ../../filebeat
make update && make filebeat.test
. ./build/python-env/bin/activate
INTEGRATION_TESTS=1 GENERATE=1 nosetests tests/system/test_modules.py

Hope it has no typos inside. Please let me know if you need some help with the above.

andrewvc · 2018-12-14T01:03:09Z

@ruflin I ran those commands but it only updated some of the stuff in c246f83 . Still getting test failures: https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-linux/2507/beat=filebeat,label=ubuntu/

ruflin · 2018-12-14T10:31:23Z

The problem is that the agent_id and ephemeral id change every time. So we should skip these two fields. Let me fix it locally and provide you with the code.

ruflin · 2018-12-14T10:34:16Z

Can you try to replace line https://github.com/elastic/beats/blob/master/filebeat/tests/system/test_modules.py#L189 with the following content?

    # These keys are host dependent
    host_keys = ["host.name", "agent.hostname", "agent.type", "agent.ephemeral_id", "agent.id"]

This exclude the two ids from being generated. Afterwards you need to run the command again.

ruflin · 2018-12-17T08:15:47Z

I did run the branch locally and on my end it removes the ids also from the files which still contain it in this PR. Not sure what happened? Can you run it again?

andrewvc · 2018-12-17T19:24:36Z

@ruflin the tests were failing for me at the last line your command list (where nosetests is run). Weirdly, after trying a few things, the cause seemed to be line breaks in the formatting of the new geo.name field's description. I removed them in dd19ce0 . WFG to see if that did it.

andrewvc · 2018-12-18T01:41:20Z

Hmmm, still getting errors on filebeat. Looks like it's still looking for agent.id?

https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-linux/2605/

ruflin · 2018-12-18T08:36:37Z

@andrewvc Yes, some files still have agent.id inside: https://github.com/elastic/beats/pull/9404/files#diff-adbf1bb07ce8f5f874891602efc0f5e7R4

andrewvc · 2018-12-19T15:50:14Z

jenkins, please retest this

andrewvc · 2018-12-20T01:31:47Z

OK , just rebased with a make update (again).

Anything else needed here @ruflin?

FYI, this PR has a high chance of needing extra rebases due to it being sensitive to any field changes in any other beat.

ruflin

I cleaned up the changelog but found one more change that should be reverted and needs make update afterwards.

libbeat/_meta/fields.ecs.yml

ruflin · 2018-12-27T07:20:28Z

@andrewvc Can you rebase this one?

@kuisathaverat One interesting thing I saw in this PR: It seems the Check step in the Jenkinsfile fails here because of a merge conflict. Does this mean what the Jenkinsfile tests is the PR merged with master?

kuisathaverat · 2019-01-02T19:20:19Z

Does this mean what the Jenkinsfile tests is the PR merged with master?

yes, the default configuration makes a merge before building the PR.

This adds the ECS fields `agent.id` and `agent.ephemeral_id` to all beats.

andrewvc · 2019-01-03T02:19:16Z

Failure unrelated

andrewvc · 2019-01-03T02:20:13Z

@ruflin @urso do y'all think this should be backported at all? 7.0 does seem like a fine time to introduce this.

ruflin · 2019-01-07T08:47:14Z

I don't see a need to backport this at the moment.

urso · 2019-01-07T20:45:17Z

No need to backport.

We found this change to break indexing if %{[beat.version]} was used. Will need to investigate why the code string formatter didn't produce an error here.

* Add agent.{id,ephemeral_id} to all beat events This adds the ECS fields `agent.id` and `agent.ephemeral_id` to all beats. * Bring back whitespace * Use CHANGELOG.next * Update fb tests

andrewvc added libbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Dec 5, 2018

andrewvc requested a review from ruflin December 5, 2018 18:56

andrewvc added enhancement needs_backport PR is waiting to be backported to other branches. labels Dec 5, 2018

andrewvc requested a review from urso December 5, 2018 18:57

ruflin reviewed Dec 6, 2018

View reviewed changes

urso reviewed Dec 6, 2018

View reviewed changes

andrewvc force-pushed the add-beat-ids-to-agent branch from fc4b6a9 to 66b03ba Compare December 6, 2018 21:16

webmat added the ecs label Dec 6, 2018

webmat approved these changes Dec 6, 2018

View reviewed changes

ruflin mentioned this pull request Dec 6, 2018

Transition Beats to ECS #8655

Closed

andrewvc self-assigned this Dec 7, 2018

ruflin reviewed Dec 7, 2018

View reviewed changes

andrewvc force-pushed the add-beat-ids-to-agent branch from 66b03ba to a0d9141 Compare December 7, 2018 21:12

andrewvc force-pushed the add-beat-ids-to-agent branch from 00bf3c2 to f48a716 Compare December 13, 2018 02:22

andrewvc force-pushed the add-beat-ids-to-agent branch from f48a716 to 4ac324d Compare December 13, 2018 21:29

andrewvc mentioned this pull request Dec 14, 2018

Fix race detector detected race in heartbeat #9539

Merged

andrewvc requested a review from a team as a code owner December 14, 2018 16:29

andrewvc force-pushed the add-beat-ids-to-agent branch from cf1c63e to 853ee4d Compare December 14, 2018 19:24

andrewvc force-pushed the add-beat-ids-to-agent branch from 853ee4d to dd19ce0 Compare December 17, 2018 19:15

andrewvc mentioned this pull request Dec 18, 2018

[META] Heartbeat 7.0 TODO #9017

Closed

7 tasks

andrewvc force-pushed the add-beat-ids-to-agent branch from 30a0f8b to 0f3bc0c Compare December 19, 2018 14:26

andrewvc force-pushed the add-beat-ids-to-agent branch from 0f3bc0c to f0745af Compare December 20, 2018 01:31

andrewvc force-pushed the add-beat-ids-to-agent branch from f0745af to fb2e6f1 Compare December 20, 2018 18:13

ruflin reviewed Dec 21, 2018

View reviewed changes

libbeat/_meta/fields.ecs.yml Outdated Show resolved Hide resolved

andrewvc force-pushed the add-beat-ids-to-agent branch from 4862126 to 9499a05 Compare December 21, 2018 21:47

andrewvc force-pushed the add-beat-ids-to-agent branch 2 times, most recently from 3e745a5 to 5b46ed4 Compare December 31, 2018 21:29

urso approved these changes Jan 2, 2019

View reviewed changes

andrewvc added 3 commits January 2, 2019 17:38

Add agent.{id,ephemeral_id} to all beat events

731c337

This adds the ECS fields `agent.id` and `agent.ephemeral_id` to all beats.

Bring back whitespace

739b735

Use CHANGELOG.next

153ed37

andrewvc force-pushed the add-beat-ids-to-agent branch from 5b46ed4 to 153ed37 Compare January 2, 2019 23:39

Update fb tests

2e4918e

andrewvc merged commit 700e982 into elastic:master Jan 3, 2019

andrewvc removed the needs_backport PR is waiting to be backported to other branches. label Jan 3, 2019

Conversation

andrewvc commented Dec 5, 2018

Uh oh!

elasticmachine commented Dec 5, 2018

Uh oh!

ruflin left a comment

Choose a reason for hiding this comment

Uh oh!

ruflin Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

andrewvc Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

andrewvc Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

andrewvc Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

urso left a comment

Choose a reason for hiding this comment

Uh oh!

andrewvc commented Dec 6, 2018

Uh oh!

andrewvc commented Dec 6, 2018

Uh oh!

webmat left a comment

Choose a reason for hiding this comment

Uh oh!

ruflin left a comment

Choose a reason for hiding this comment

Uh oh!

ruflin Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

andrewvc commented Dec 12, 2018

Uh oh!

andrewvc commented Dec 12, 2018

Uh oh!

ruflin commented Dec 12, 2018

Uh oh!

ruflin commented Dec 13, 2018

Uh oh!

andrewvc commented Dec 14, 2018

Uh oh!

ruflin commented Dec 14, 2018

Uh oh!

ruflin commented Dec 14, 2018

Uh oh!

ruflin commented Dec 17, 2018

Uh oh!

andrewvc commented Dec 17, 2018

Uh oh!

andrewvc commented Dec 18, 2018

Uh oh!

ruflin commented Dec 18, 2018

Uh oh!

andrewvc commented Dec 19, 2018

Uh oh!

andrewvc commented Dec 20, 2018

Uh oh!

ruflin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ruflin commented Dec 27, 2018

Uh oh!

kuisathaverat commented Jan 2, 2019

Uh oh!

andrewvc commented Jan 3, 2019

Uh oh!

andrewvc commented Jan 3, 2019

Uh oh!

ruflin commented Jan 7, 2019

Uh oh!

urso commented Jan 7, 2019

Uh oh!

Reviewers

Assignees