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 to ECS Filebeat modules populating http size and duration metrics #10188

Merged
merged 11 commits into from
Jan 23, 2019

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Jan 18, 2019

This PR finishes the migration of http size and time metrics for all relevant Filebeat modules. This had already started happening in the more recent module transitions. This PR finishes up the work all at once.

Questions

  • In all module transitions so far, I've left the module-specific total request time fields alone. I think we can get rid of them, as event.duration should be the single place to record the overall duration. I would remove them, and mark then in ecs-migration.yml as migrated, with alias: false. Any disagreements?
    • Note that any additional timing metrics such as various wait times are unaffected by this.
  • Take a look at how I'm reducing Painless compilations across access log modules, when calculating event.duration. I think it's worth doing a follow-up PR to spread this practice around, and reduce the impact of calculating event.duration across all Fb modules even further. Objections?

Modules/filesets affected

  • apache.access (apache2.access.body_sent.bytes)
  • iis.access (iis.access.body_sent.bytes, iis.access.body_received.bytes, iis.access.request_time_ms)
  • kibana.log (http.response.content_length)
  • nginx.access (nginx.access.body_sent.bytes)
  • traefik (traefik.access.body_sent.bytes, traefik.access.duration)

List of aliased field migrations

response size

  • http.response.content_length => http.response.body.bytes (Kibana module)
  • apache2.access.body_sent.bytes => http.response.body.bytes
  • iis.access.body_sent.bytes => http.response.body.bytes
  • nginx.access.body_sent.bytes => http.response.body.bytes
  • traefik.access.body_sent.bytes => http.response.body.bytes

request size

  • iis.access.body_received.bytes => http.request.body.bytes

Event duration

No aliases, as scale is different. Original fields removed.

  • http.response.elapsed_time (Kibana module)
  • iis.access.request_time_ms
  • traefik.access.duration

TODO

  • Change all HTTP size metrics
  • Compute all event durations
  • Look for other usage of http.response.content_length
  • Look for other usage of http.response.elapsed_time
  • Alias the fields for the migration
  • Document in ecs-migration.yml
    • Delete any other web access event durations, and document as alias: false in ecs-migration.yml as well
  • Changelog

@webmat webmat requested review from a team as code owners January 18, 2019 21:35
@webmat webmat added in progress Pull request is currently in progress. module Filebeat Filebeat ecs labels Jan 18, 2019
@webmat
Copy link
Contributor Author

webmat commented Jan 18, 2019

@ruflin I'd like your opinion on the two questions outlined in the PR body. For both questions, you can look at the code, because I went ahead and did what I'm suggesting. Open to discussion, however.

This is pretty much ready to go, except for the changelog.

@webmat webmat added review and removed in progress Pull request is currently in progress. labels Jan 18, 2019
@ruflin ruflin mentioned this pull request Jan 18, 2019
@webmat webmat changed the title WIP Migrate to ECS Filebeat modules populating http size and duration metrics Migrate to ECS Filebeat modules populating http size and duration metrics Jan 18, 2019
@webmat
Copy link
Contributor Author

webmat commented Jan 19, 2019

Unrelated test failure (heartbeat)

Copy link
Member

@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.

+1 on on the painless part. For the event.duration not 100% sure I can follow. So we map the existing fields to event.duration or not use it at all?

filebeat/module/traefik/access/test/test.log-expected.json Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor Author

webmat commented Jan 21, 2019

@ruflin Ok ready for another review, including for both caveats. Now that my painless is working, the caveats will make more sense :-)

@cachedout
Copy link
Contributor

From the @elastic/stack-monitoring perspective this LGTM but let's see if @ycombinator can come by and just verify that as well.

@@ -8,8 +8,7 @@
"fileset.name": "log",
"http.request.method": "get",
"http.request.referrer": "http://localhost:5601/app/kibana",
"http.response.content_length": 9,
"http.response.elapsed_time": 26,
Copy link
Contributor

@ycombinator ycombinator Jan 22, 2019

Choose a reason for hiding this comment

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

We should mention the removal of this field these two fields in the breaking changes section of the CHANGELOG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, agreed. I've often glossed over the details, for the module-specific fields (e.g. "migrate apache.access.*").

But these two deserve a special mention indeed, since they could have been mistaken for ECS fields.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

Just a minor comment about mentioning breaking changes in the CHANGELOG. Otherwise LGTM.

@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

Changelog too chatty? Rest of the PR is approved and hasn't been touched.

Mathieu Martin added 8 commits January 22, 2019 23:12
I'm also reusing the same pattern of using a temp field, prior to computing `event.duration`. This will reduce compilations, as both the `script` and `if` arguments are exactly the same code, from one module to the other. Only the `scale` param needs to be adjusted, depending if one has millis, micros, etc.
Not nested under the special `_ingest`
@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

Rebased over master.

I'm starting another PR on top of this one, to finish the event.duration migration in other modules the same way I'm doing here.

Copy link
Member

@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.

For the Changelog: If we document every single field we changed, it definitively gets too chatty. What I have in mind is that we will have a doc page with all the fields that we changed so users can go there to look it up.

@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

@ruflin Yeah we should definitely have a generated page that explains "the ECS migration" in plain terms, and is followed by the exhaustive list of fields migrated (aliases, durations that moved to event.duration with a different scale, etc).

Then the changelogs related to ECS could be simplified, and all link to that "ECS migration" documentation page.

I'll rewrite the changelog a bit, then we should be good.

@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

Ok, pushed one changelog entry per field (duration and body.bytes). Merging right away. Hit me up for a follow-up PR, if these are still problematic.

@webmat webmat merged commit 262fd04 into elastic:master Jan 23, 2019
@webmat webmat deleted the ecs-http-size-metrics-fb branch January 23, 2019 16:10
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