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

Introduce size metrics for HTTP. #239

Merged
merged 5 commits into from
Dec 11, 2018
Merged

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Dec 5, 2018

This PR introduces the concept of metrics for HTTP events. It currently adds only the metrics for body and total (which includes headers). It's missing the obvious third wheel, header sizes. I wouldn't mind adding them if people feel strongly about it, but I haven't seen it come up in the few web server logging directives I've reviewed.

Speaking of which, here are some equivalencies:

  • http.request.bytes.total: Apache httpd's %I, NGINX $request_length
  • http.request.bytes.body: content-length header, NGINX $content_length, Traefik RequestContentSize
  • http.response.bytes.total: Apache httpd's %O, NGINX $bytes_sent
  • http.response.bytes.body: Apache httpd's %B, NGINX $body_bytes_sent, Traefik DownstreamContentSize (I think)

I do think logging both makes sense, for people who want detailed metrics.

In recent ECS migrations of Beats modules, I've seen a few modules using a content_length field. So there's a need for the metrics. However I feel like the term "content_length" comes with a bit too much meaning to start from there and define the other fields.

I'm open to discussing other ways of naming the fields. We could use "size" instead of bytes, or not nest so much (e.g. .size, .body_size and if we go there, .header_size).

@webmat
Copy link
Contributor Author

webmat commented Dec 5, 2018

I'm flagging this for beta2. I'd like it to get in there, but I don't think it's the end of the world if it doesn't.

fields.yml Outdated
Size in bytes of the response body and headers combined.
example: 1437

- name: response.bytes.body
Copy link
Contributor

Choose a reason for hiding this comment

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

We have above response.bodywhich contains the content of the body and here we have the size of it. What about response.body.content and response.body.size?

Copy link
Contributor Author

@webmat webmat Dec 7, 2018

Choose a reason for hiding this comment

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

I like the grouping this enables: .header.bytes, .body.bytes. Which eventually leaves room for other metadata, if we ever want to track .body.encoding & such.

However do we use http.[request|response].body anywhere? Would moving that to .body.content be another "concrete field" => object transition, like source?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would also allow adding something like body.hash if the body should be compared but not the full body stored.

We use http.request.body today in Metricbeat in the http module. So an alias would not be possible but I think it's fine in this case as it is with source.

@webmat webmat removed the 1.0.0-beta2 label Dec 6, 2018
@webmat
Copy link
Contributor Author

webmat commented Dec 7, 2018

Ok, I could have sworn we had added http.request.body already, but we haven't. If we go forward with this PR, we need to make sure to revisit it if http.request.body goes in (#263).

@webmat webmat force-pushed the http-metrics branch 2 times, most recently from 1ef4b7e to aceaad8 Compare December 7, 2018 21:08
@ruflin ruflin mentioned this pull request Dec 10, 2018
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.

This will need a changelog entry (or multiple).

@webmat
Copy link
Contributor Author

webmat commented Dec 10, 2018

As discussed elsewhere, I will incorporate the changes from #263 into this PR and introduce all of this in one go.

@webmat webmat requested a review from MikePaquette December 10, 2018 16:20
@webmat
Copy link
Contributor Author

webmat commented Dec 10, 2018

@ruflin As discussed, the addition of the body field is now in this PR. I've also added changelog entries for everything that's happening. I'm nesting them all under the "Reintroducing http" entry. Since http was not part of beta 1, I would say that the body => body.content rename is not an ECS breaking change. WDYT?

@MikePaquette Can you check out this PR? I'd love it if we were able to get this into Beta 2. This adds fields for HTTP size metrics. Total size and body size for both request and response. As discussed earlier, this structure also leaves room to eventually add support for headers (and header size metrics). The PR also adds one missing field: we already had a field for the response body, and now we're adding a field for the request body.

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 except changelog.

It's important that this is not a breaking change is it happened between releases. Otherwise we could never revert or experiment with things between releases.

@webmat webmat merged commit 5d62e2e into elastic:master Dec 11, 2018
@webmat webmat deleted the http-metrics branch December 11, 2018 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants