Add new field to access log#3907
Conversation
This new field, `TIME_TO_LAST_BYTE_DURATION`, represents the duration between the first upstream byte received and the last downstream byte sent. Similarly to RESPONSE_DURATION, we can leave it undocumented for now until it proofs generally useful. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
|
@zuercher mind taking a look? |
zuercher
left a comment
zuercher
left a comment
There was a problem hiding this comment.
I think the change itself is fine, but I think the field name should more clearly identify what the value represents.
| field_extractor_ = [](const RequestInfo::RequestInfo& request_info) { | ||
| return AccessLogFormatUtils::durationToString(request_info.firstUpstreamRxByteReceived()); | ||
| }; | ||
| } else if (field_name == "TIME_TO_LAST_BYTE_DURATION") { |
There was a problem hiding this comment.
Shouldn't "time to last byte" report the time from the start of the downstream request until the last tx byte? (Which is just DURATION.) This is measuring how long the response takes to transit through Envoy, so more like RESPONSE_TRANSIT_DURATION or RESPONSE_TX_DURATION or something?
There was a problem hiding this comment.
I like RESPONSE_TX_DURATION — I think it's the closes one to "the time between the first upstream byte and the last downstream byte sent".
cc: @derekargueta
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
|
Note for when merging: the commit message needs update to replace |
| field_extractor_ = [](const RequestInfo::RequestInfo& request_info) { | ||
| return AccessLogFormatUtils::durationToString(request_info.firstUpstreamRxByteReceived()); | ||
| }; | ||
| } else if (field_name == "RESPONSE_TX_DURATION") { |
There was a problem hiding this comment.
I think this needs a doc update and a release note? https://www.envoyproxy.io/docs/envoy/latest/configuration/access_log?
There was a problem hiding this comment.
@mattklein123 %RESPONSE_DURATION% isn't public either... so my thinking was lets have it be private for now and then promote it as publicly available interface/variable? I am happy either way though.
There was a problem hiding this comment.
What do you mean not public? I think if it's not documented that's a doc bug. Can you fix both? :)
There was a problem hiding this comment.
Haha, sure — diff coming up.
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
|
Can we add a release note for both options since they were never "released" previously? |
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
|
@mattklein123 ah yes, sorry about that — done now. Is that only done in |
3 participants
This new field,
RESPONSE_TX_DURATION, represents the durationbetween the first upstream byte received and the last downstream
byte sent. Similarly to
RESPONSE_DURATION, we can leave it undocumentedfor now until it proofs to be generally useful.
For context, we rely on this metric heavily internally. We tried to implemented
from a regular filter but it's harder to get right.
Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com