[http]: count sent and received bytes for http stream#17521
[http]: count sent and received bytes for http stream#17521mattklein123 merged 100 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
|
Thanks for working on this! /assign @KBaichoo |
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
|
/assign @yanavlasov |
|
This pull request is ready for another round of review. |
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
chaoqin-li1123
changed the title
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this LGTM modulo small requested changes. @yanavlasov?
Also, what about my larger comment about splitting into purely headers bytes and data bytes, and then maybe having connection wire bytes? Thoughts?
/wait
Signed-off-by: chaoqin-li1123 <[email protected]>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM pending the discussion on whether this approach satisfies @yanavlasov concerns about pipelining, etc. If we keep this approach I would augment the documentation with the limitations for HTTP/1.
/wait
@mattklein123 I'm not quite sure I understand your suggestion. Are you suggesting to track header/body bytes after they were parsed by the codec (i.e. the size of the Envoy's header map structure?). This will be hard to convert to headers representation on the wire for H/2, given header compression. There could be a case for tracking body bytes, where it is easier to infer framing overhead, and total bytes and determine header wire bytes from the difference. But I think we may still need to go to codec level for H/1 when partially received chunks are dispatched. |
Yeah basically I was suggesting to count wire bytes at the connection level only, and then you could access those in either the connection access log or possibly the current value in the stream access log. Then you could count (decompressed) header bytes as well as body bytes. All of it together would give you a pretty complete picture and I think work around the pipeline issue not being accounted for (since you get wire bytes in the connection log). IIRC we already track bytes send / received at the connection level, or if not would be easy to fix for HTTP (it might only work for TCP right now I'm not sure). I'm fine keeping this, but I still think we should document the limitations. |
yanavlasov
left a comment
yanavlasov
left a comment
There was a problem hiding this comment.
LGTM. Seems like extra file was added as well as document behavior with pipelined requests for HTTP/1.1
test/server/server_corpus/0
Outdated
| @@ -0,0 +1,42 @@ | |||
| static_resources { | |||
There was a problem hiding this comment.
Do you need this file, or is this bad merge?
There was a problem hiding this comment.
Bad merge, cleaned up.
|
|
||
| %DOWNSTREAM_WIRE_BYTES_RECEIVED% | ||
| HTTP | ||
| Total number of bytes received from the downstream by the http stream. |
There was a problem hiding this comment.
Let's add the following to document H/1 pipelining behavior. "Envoy over counts sizes of received HTTP/1.1 pipelined requests by adding up bytes of requests in the pipeline to the one currently being processed".
There was a problem hiding this comment.
Comments added.
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
I see. I'm not quite sure how this work for multiplexed protocols since the requirement is to attribute bytes to specific requests. We can revisit this if we find some way to improve this design. |
Integrates envoyproxy/envoy#17479 & envoyproxy/envoy#17521 Envoy diff: envoyproxy/envoy@a5b3af2...c687308 This PR: * Creates a new `extension_registry_platform_additions` library where platform-specific additions to the extensions can be configured. Currently only registers the Apple DNS resolver extension factory and no-ops on non-Apple platforms but more can be added here in the future. * Implements the new `Stream::bytesMeter()` function on `DirectStream`. * Increases the max binary size from 7.2MB to 7.3MB. We were already within 1% of that on `main` so it didn't take much new code to go over this limit.
| upstreamToString(params.param.upstream_protocol)); | ||
| } | ||
|
|
||
| void HttpProtocolIntegrationTest::expectUpstreamBytesSentAndReceived( |
There was a problem hiding this comment.
This change omitted H3 bytes accounting. And it could break existing monitoring. It would be great if things could have been communicated earlier, i.e. a tracking issue or cc'ing H3 code owners.
Integrates #17479 & #17521 Envoy diff: a5b3af2...c687308 This PR: * Creates a new `extension_registry_platform_additions` library where platform-specific additions to the extensions can be configured. Currently only registers the Apple DNS resolver extension factory and no-ops on non-Apple platforms but more can be added here in the future. * Implements the new `Stream::bytesMeter()` function on `DirectStream`. * Increases the max binary size from 7.2MB to 7.3MB. We were already within 1% of that on `main` so it didn't take much new code to go over this limit. Signed-off-by: JP Simard <[email protected]>
Integrates #17479 & #17521 Envoy diff: a5b3af2...c687308 This PR: * Creates a new `extension_registry_platform_additions` library where platform-specific additions to the extensions can be configured. Currently only registers the Apple DNS resolver extension factory and no-ops on non-Apple platforms but more can be added here in the future. * Implements the new `Stream::bytesMeter()` function on `DirectStream`. * Increases the max binary size from 7.2MB to 7.3MB. We were already within 1% of that on `main` so it didn't take much new code to go over this limit. Signed-off-by: JP Simard <[email protected]>
5 participants
Commit Message: Count the number of bytes sent to and received from upstream server per request and store them in stream_info for better observability.
Additional Description:NA
Risk Level:NA
Testing:will add testing for the new metric.
Docs Changes:add doc for the new metric.
Release Notes:NA
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]