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

Convert Filebeat elasticsearch.* to ECS #9293

Merged
merged 11 commits into from
Jan 17, 2019

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Nov 29, 2018

Very little appears to be required for this module.

Caveats

  • elasticsearch.deprecation, elasticsearch.gc and elasticsearch.server look good already, no need to modify.
  • Slowlog has two representations for event duration. One is an int in milliseconds, which is what I used to populate event.duration. But if we parse the textual slowlog.took instead, we can get much more precise durations. Outside of the scope of this ECS mapping, however.

Renames

  • elasticsearch.audit.event_type => event.type
  • elasticsearch.audit.origin_address => source.ip
  • elasticsearch.audit.uri => url.original
  • elasticsearch.audit.request_body => http.request.body
  • elasticsearch.audit.principal => user.name

Copies

  • Populate event.duration (ns) without removing slowlog.took and slowlog.took_millis

TODO

  • Generate a few more `-expected.json` files prior to making changes
  • Save slowlog.took_millis * 1000000 to event.duration
  • Make sure to address the http.*.body.content move in ecs-migration and the code as well
    • Delete http.request.body from ecs-migration
  • Alias renamed fields to their ECS counterpart
  • Document field migrations in ecs-migration.yml
  • Changelog
  • Migrate principal to user.name

@ruflin ruflin mentioned this pull request Nov 29, 2018
@webmat webmat self-assigned this Nov 29, 2018
@webmat webmat added in progress Pull request is currently in progress. module Filebeat Filebeat ecs labels Nov 29, 2018
@ruflin
Copy link
Contributor

ruflin commented Dec 4, 2018

+1 on the Caveats

@webmat
Copy link
Contributor Author

webmat commented Dec 18, 2018

I'll be able to start on this again once #9645 is merged.

webmat pushed a commit to webmat/beats that referenced this pull request Jan 3, 2019
Notes:

- Can't be aliased since `body` is moving to `body.content`.
- Currently only affects Packetbeat, so it's been listed only there,
even if these are ECS field defs.
- This will affect the ES Filebeat module logs as well. A note as been
added to elastic#9293, so it doesn't get forgotten.
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

@ycombinator You should probably also have a look at this one.

@ycombinator
Copy link
Contributor

Changes LGTM but other related PRs got merged recently so conflicts will need to be resolved on this one. Let me know if you need help with any of those.

@webmat
Copy link
Contributor Author

webmat commented Jan 8, 2019

Yeah I want to get more module migrations going and get those discussions going first. I'll fix these conflicts in a few days

@webmat webmat force-pushed the ecs-fb-elasticsearch branch from 7d03d37 to 78f1b0f Compare January 15, 2019 14:45
@webmat webmat requested a review from a team as a code owner January 15, 2019 14:45
@webmat webmat force-pushed the ecs-fb-elasticsearch branch from 78f1b0f to 63d4141 Compare January 15, 2019 14:52
@webmat webmat requested a review from a team as a code owner January 15, 2019 15:38
@webmat webmat added review and removed in progress Pull request is currently in progress. labels Jan 15, 2019
@webmat
Copy link
Contributor Author

webmat commented Jan 15, 2019

jenkins, test this

1 similar comment
@webmat
Copy link
Contributor Author

webmat commented Jan 15, 2019

jenkins, test this

@ruflin
Copy link
Contributor

ruflin commented Jan 16, 2019

jenkins, test this

Mathieu Martin added 6 commits January 16, 2019 09:15
- elasticsearch.audit.event_type => event.type
- elasticsearch.audit.origin_address => source.ip
- elasticsearch.audit.uri => url.original
- elasticsearch.audit.request_body => http.request.body
Not removing original, as there's a 1000000x scale difference
@webmat webmat force-pushed the ecs-fb-elasticsearch branch from 8a25060 to 1dc24da Compare January 16, 2019 14:15
@webmat
Copy link
Contributor Author

webmat commented Jan 16, 2019

@ruflin @ycombinator Ready for a final review. All elasticsearch-related tests are successful here, even in Jenkins.

The failing filebeat testsuite in there is actually puzzling to me: all tasks are [success], and it concludes that the test run has failed.

But all of this is obviously unrelated to the ES Filebeat module :-)

Only caveat I'd like confirmation on: right now it would be possible to populate event.duration with nanoseconds precision, but I'd have to parse the textual field slowlog.took. I say we can call that an improvement for later. The important part for FF is to move things to the right fields. Do you agree?

@ruflin
Copy link
Contributor

ruflin commented Jan 16, 2019

jenkins, test this

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM but I would really want to see CI pass at least for Filebeat.

@webmat
Copy link
Contributor Author

webmat commented Jan 16, 2019

This last run only had flakiness for libbeat and metricbeat, so we're good.

@ruflin
Copy link
Contributor

ruflin commented Jan 17, 2019

@webmat Let's get it in. Super unhappy about the CI failures but filebeat is excluded as you mentioned.

Can you also remove the WIP from the PR title?

@webmat webmat changed the title WIP Convert Filebeat elasticsearch.* to ECS Convert Filebeat elasticsearch.* to ECS Jan 17, 2019
@webmat webmat merged commit c4b2ad8 into elastic:master Jan 17, 2019
@webmat webmat deleted the ecs-fb-elasticsearch branch January 17, 2019 19:58
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.

3 participants