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

Update the HTTP field set with ECS definitions as of beta 2 #9645

Merged
merged 5 commits into from
Dec 21, 2018

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Dec 18, 2018

Caveat

  • We cannot alias the Packetbeat .body fields for this migration. This is a situation similar to Filebeat's source field.
  • http.request.method is not lowercased in this PR. This affects many Fb modules, Packetbeat, etc. I think it should be implemented via an index tokenizer, not by modifying _source

TODO

@webmat webmat requested a review from a team as a code owner December 18, 2018 21:15
@webmat webmat self-assigned this Dec 18, 2018
@webmat webmat added in progress Pull request is currently in progress. ecs labels Dec 18, 2018
@ruflin ruflin mentioned this pull request Dec 18, 2018
@ruflin
Copy link
Member

ruflin commented Dec 19, 2018

Looks like this breaks some packetbeat tests?

@webmat
Copy link
Contributor Author

webmat commented Dec 19, 2018

@ruflin Yeah that's possible. I had problems running the test suite locally, so I decided to ask Jenkins and Travis for help. Hence the WIP / "in progress" ;-)

@webmat
Copy link
Contributor Author

webmat commented Dec 19, 2018

Error in question:

======================================================================
ERROR: Check that the http body is exported only for some http messages that have the
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/gopath/src/github.com/elastic/beats/packetbeat/tests/system/test_0063_http_body.py", line 49, in test_include_body_for_both_request_response
    objs = self.read_output()
  File "/home/travis/gopath/src/github.com/elastic/beats/packetbeat/tests/system/packetbeat.py", line 122, in read_output
    self.all_fields_are_expected(jsons, self.expected_fields)
  File "/home/travis/gopath/src/github.com/elastic/beats/packetbeat/tests/system/../../../libbeat/tests/system/beat/beat.py", line 471, in all_fields_are_expected
    .format(key))
Exception: Unexpected key 'http.response.body' found

@webmat
Copy link
Contributor Author

webmat commented Dec 19, 2018

@andrewkroh ECS 1 Beta 2 introduced the http field set with the bodies nested a bit deeper: http.*.body.content instead of http.*.body. I've made the change in Packetbeat, and the tests are now passing. I'd like it if you could look at this, to make sure I haven't forgotten anything.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

That looks like everything w.r.t to the Packetbeat code. I'm really hoping the test coverage for http is good in Packetbeat 🤞

I think there are some mentions in the documentation that also need updated. Like here.

@webmat
Copy link
Contributor Author

webmat commented Dec 20, 2018

I've modified the documentation and added changelog entries in two places (Pb/breaking and All beats/Added).

I'd like to merge this as is. There's a few caveats listed in the body of this PR. One for awareness / discussion (can't alias Packetbeat's body changes), and one is for follow-up work.

@webmat webmat requested a review from ruflin December 20, 2018 20:42
@webmat webmat added review and removed in progress Pull request is currently in progress. labels Dec 20, 2018
@webmat
Copy link
Contributor Author

webmat commented Dec 21, 2018

jenkins, test this

@@ -629,7 +629,7 @@ func (http *httpPlugin) collectHeaders(m *message) interface{} {

func (http *httpPlugin) setBody(result common.MapStr, m *message) {
if m.sendBody && len(m.body) > 0 {
result["body"] = string(m.body)
result["body.content"] = string(m.body)
Copy link
Member

Choose a reason for hiding this comment

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

Probably better user here result.Put("body.content", string(m.body) so it creates the correct object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Although the rest of this file use the square bracket notation to set fields. I'm not adjusting the rest of this file.

@webmat
Copy link
Contributor Author

webmat commented Dec 21, 2018

@ruflin One last look needed. My understanding is that these metricbeat failures are unrelated, correct?

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

Once I merge this PR, the rebase conflicts that leave x-pack/auditbeat/include/fields.go broken will be a thing of the past.

@webmat
Copy link
Contributor Author

webmat commented Dec 21, 2018

Ok just confirmed metricbeat failure is unrelated. Just got fixed by #9749

@webmat webmat merged commit caf07a3 into elastic:master Dec 21, 2018
@webmat webmat deleted the ecs-http-update branch December 21, 2018 16:49
webmat pushed a commit to webmat/beats that referenced this pull request Dec 21, 2018
- With the exception of http, captured in elastic#9645
- With the exception of os, which must be updated in a bunch of places
webmat added a commit that referenced this pull request Dec 21, 2018
This addresses most of the differences, with the exception of http (#9645) and os (PR coming)

Changes:

- `client/server`, `source/destination`
  - Update each field set's definition
  - Add the `.address` field (except for `source`, which already had it)
- `network`: update definitions and examples wrt lowercase directive
- `user_agent.original` index is now `keyword` indexed (See elastic/ecs#262)
- Update `ecs.version` example
@ruflin
Copy link
Member

ruflin commented Dec 27, 2018

@webmat Nit: Could you adjust the title of the PR so if people come back to this PR it does not say WIP and it's merged ;-)

@ruflin
Copy link
Member

ruflin commented Dec 27, 2018

@webmat Should this also show up in the ecs migration yml file?

@webmat webmat changed the title WIP Update the HTTP field set with ECS definitions as of beta 2 Update the HTTP field set with ECS definitions as of beta 2 Jan 3, 2019
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.
@webmat
Copy link
Contributor Author

webmat commented Jan 3, 2019

@ruflin Yes, good point about ecs-migration. I've opened #9878

webmat added a commit that referenced this pull request Jan 7, 2019
…up. (#9878)

Details:

* Add two migrated fields to ecs-migration from #9645. Can't be aliased since `.body` is moving to `.body.content`.
* Remove dupe headers for what's after the 'processors' section. Likely a rebase hiccup.
* Fix typo in processors entries: `form` => `from`.
* Move the processors heading above the docker processor section
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
…tic#9645)

- Introduces fields for http size metrics
- HTTP body field is now nested deeper:
  - `http.request.body` moves to `http.request.body.content`
  - `http.response.body` moves to `http.response.body.content`
  - packetbeat has been adjusted accordingly
- Introduces missing field definition updates (mainly to lowercase `method`)
- Unrelated: delete `x-pack/auditbeat/include/fields.go` which should have been deleted in elastic#9724.
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
This addresses most of the differences, with the exception of http (elastic#9645) and os (PR coming)

Changes:

- `client/server`, `source/destination`
  - Update each field set's definition
  - Add the `.address` field (except for `source`, which already had it)
- `network`: update definitions and examples wrt lowercase directive
- `user_agent.original` index is now `keyword` indexed (See elastic/ecs#262)
- Update `ecs.version` example
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.

4 participants