QUIC deferred logging: add retransmission rate#27699
QUIC deferred logging: add retransmission rate#27699alyssawilk merged 10 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
|
/assign @alyssawilk |
Signed-off-by: Paul Sohn <paulsohn@google.com>
|
/wait on CI |
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
alyssawilk
left a comment
There was a problem hiding this comment.
This looks great overall! nits follow
/wait
| added :ref:`CEL <envoy_v3_api_msg_extensions.formatter.cel.v3.Cel>` access log formatter to print CEL expression. | ||
| - area: access_log | ||
| change: | | ||
| (QUIC only) Added support for %BYTES_RETRANSMITTED% and %PACKETS_RETRANSMITTED%. |
There was a problem hiding this comment.
can you update logging docs to cover these as well?
envoy/stream_info/stream_info.h
Outdated
| virtual void addBytesRetransmitted(uint64_t bytes_retransmitted) PURE; | ||
|
|
||
| /** | ||
| * @return the number of body bytes retransmitted by the stream. |
There was a problem hiding this comment.
nothing else in here calls out it's body bytes. do we not track header bytes retransmitted? if not thoughts on naming?
There was a problem hiding this comment.
Based on the relevant QUICHE code, I believe it indeed only tracks body bytes retransmitted. Do you mean we should rename the function to be specific about body bytes?
There was a problem hiding this comment.
If you're planning to implement this for HTTP/1 and 2 I assume they're going to track all bytes, in which case I'd be inclined to keep the naming the same and comment that for HTTP/3 it only tracks body bytes?
actually what's the plan for H2 given things are intermingled on the same TCP connection?
cc @RyanTheOptimist
There was a problem hiding this comment.
If you're planning to implement this for HTTP/1 and 2 I assume they're going to track all bytes, in which case I'd be inclined to keep the naming the same and comment that for HTTP/3 it only tracks body bytes?
Agreed, sounds good.
actually what's the plan for H2 given things are intermingled on the same TCP connection?
This is still a totally open question.
Signed-off-by: Paul Sohn <paulsohn@google.com>
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM. Throwing over to @zuercher for logs review to get a thumbs up on handling of (currently) H3-only options.
|
@zuercher ping? |
Commit Message: [QUIC only] Log retransmitted packets and bytes. Additional Description: Adds to the deferred logging implementation from envoyproxy#23648 and implements the existing OnPacketRetransmitted function. Risk Level: Low Testing: Integration test Docs Changes: N/A Release Notes: added Signed-off-by: Paul Sohn <paulsohn@google.com> Signed-off-by: asheryer <asheryer@amazon.com>
Commit Message: [QUIC only] Log retransmitted packets and bytes. Additional Description: Adds to the deferred logging implementation from envoyproxy#23648 and implements the existing OnPacketRetransmitted function. Risk Level: Low Testing: Integration test Docs Changes: N/A Release Notes: added Signed-off-by: Paul Sohn <paulsohn@google.com> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
Commit Message: [QUIC only] Log retransmitted packets and bytes.
Additional Description: Adds to the deferred logging implementation from #23648 and implements the existing OnPacketRetransmitted function.
Risk Level: Low
Testing: Integration test
Docs Changes: N/A
Release Notes: added