Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate beat.* to agent.* #8873

Merged
merged 1 commit into from
Nov 6, 2018
Merged

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Nov 1, 2018

The beat.* fields map to agent.* fields in ECS. This PR converts the following fileds:

  • beat.hostname -> agent.hostname
  • beat.version -> agent.version

agent.type is added to indicate which Beat the data comes from. The field beat.name is converted to agent.name but is only set when it is not the same as the hostname. We already feed the field at the moment into host.name. The main discussion is which field we use by default in our dashboards moving forward.

Changed:

  • Remove old fields from field.yml
  • Updated changelog
  • Updated tests and generated files

@ruflin ruflin added in progress Pull request is currently in progress. ecs labels Nov 1, 2018
#


- from: beat.name
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bleskes This is the file I have in mind to track the migration of the fields so it can be used later to do automatic migrations.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@webmat webmat Nov 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the meaning of this structure? I'm wondering about the direction of the alias this is suggesting:

  • Does it mean field offset is type alias, and points to the data in log.offset?
    • This will be appropriate for 7.x, for backwards compatibility
  • Does it mean field log.offset is type alias, and points to the data in offset?
    • This would be appropriate if we want to start preparing for ECS in 6.x

Since we may actually do both, I think we should rename the keys of the structures slightly.

I currently read them as "old field is in from" and "new field is in to". However when you read with the strategy in mind, it reads like "alias from offset to log.offset", which is confusing.

What do you think about the following?

- old: offset
  new: log.offset
  strategy: alias # or copy_to

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I messed up the meaning of which strategy made sense in which version. Updated my comment above :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR, just to be clear. Just maybe a thing to rethink in a separate PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure can still change, what is important right now is that we have a structure way of documenting it. We can focus on naming later.

@@ -80,8 +80,8 @@ func Load(
Annotations: Annotations{
Event: config.EventMetadata,
Builtin: common.MapStr{
"beat": common.MapStr{
"name": name,
"agent": common.MapStr{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@graphaelli I think we should must make it possible for the apm-server to overwrite these fields with the agent information. Meaning we should only set them if they are not already in the event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@graphaelli Will make this happen in a follow up PR. Marked it in the ECS tracking issue: #8655

ruflin added a commit to ruflin/beats that referenced this pull request Nov 1, 2018
This file should help to track the migration of all files to ECS and then have potential automation on top to convert dashboards and other files which use the fields.

This file is extracted from elastic#8873 to make it available in multiple PRs.
ruflin added a commit that referenced this pull request Nov 1, 2018
This file should help to track the migration of all files to ECS and then have potential automation on top to convert dashboards and other files which use the fields.

This file is extracted from #8873 to make it available in multiple PRs.
@ruflin ruflin force-pushed the ecs-migration-kickoff branch 2 times, most recently from 2326942 to 92ad691 Compare November 5, 2018 13:00
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a few discrepancies

@@ -18,6 +18,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
*Auditbeat*

- Use `initial_scan` action for new paths. {pull}7954[7954]
- Rename beat.name to beat.type, beat.hostname to agent.hostname, beat.version to agent.version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should "Rename beat.name to beat.type" read "Rename beat.name to agent.type"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.



- from: beat.name
to: agent.name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not aligned with the changelog. Shouldn't "beat.name" become "beat.type"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, agent.type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I messed up my correction. Yes, "agent.type" :-)

@@ -79,6 +79,6 @@ def test_beat_fields(self):

output = self.read_output()
doc = output[0]
assert doc["beat.name"] == "testShipperName"
assert doc["beat.hostname"] == socket.gethostname()
assert doc["host.name"] == "testShipperName"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be updated to "host.hostname", no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have host.name in Beats today since 6.4 and is what is used at the moment everywhere.

@@ -190,7 +190,7 @@ def _test_expected_events(self, test_file, objects):

def clean_keys(obj):
# These keys are host dependent
host_keys = ["host.name", "beat.hostname", "beat.name"]
host_keys = ["host.name", "agent.hostname", "agent.type"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same: "host.name" => "host.hostname"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the intention is to stick to "host.name" in 6.x and move to "host.hostname" in 7.x?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to figure that one out. This PR keeps the same behaviour as we had.

@ruflin ruflin changed the title [WIP] Migrate beat.* to agent.* Migrate beat.* to agent.* Nov 6, 2018
@ruflin ruflin mentioned this pull request Nov 6, 2018
The beat.* fields map to agent.* fields in ECS. This PR converts the following fileds:

* beat.hostname -> agent.hostname
* beat.version -> agent.version

`agent.type` is added to indicate which Beat the data comes from. The field `beat.name` is converted to `agent.name` but is only set when it is not the same as the hostname. We already feed the field at the moment into host.name. The main discussion is which field we use by default in our dashboards moving forward.

Changed:

* Remove old fields from field.yml
* Updated changelog
* Updated tests and generated files
@ruflin ruflin added review and removed in progress Pull request is currently in progress. labels Nov 6, 2018
@ruflin ruflin merged commit 1767034 into elastic:master Nov 6, 2018
@ruflin ruflin deleted the ecs-migration-kickoff branch November 6, 2018 18:26
@andrewkroh
Copy link
Member

@ruflin @webmat We don't have agent.hostname listed in ECS. Is that an oversight? Should we add it?

@webmat
Copy link
Contributor

webmat commented Nov 14, 2018

Yeah it's an oversight. It was mentioned in #8873, but I promptly forgot about that :-) So I created issue elastic/ecs#178 to make sure it gets addressed.

This is not a concept that's only useful to Beats, I think it also belongs in ECS. See the ECS issue for details, or to discuss the idea.

DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
The beat.* fields map to agent.* fields in ECS. This PR converts the following fileds:

* beat.hostname -> agent.hostname
* beat.version -> agent.version

`agent.type` is added to indicate which Beat the data comes from. The field `beat.name` is converted to `agent.name` but is only set when it is not the same as the hostname. We already feed the field at the moment into host.name. The main discussion is which field we use by default in our dashboards moving forward.

Changed:

* Remove old fields from field.yml
* Updated changelog
* Updated tests and generated files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants