tcp_proxy: add COMMON_DURATION access log support#45449
Conversation
agrawroh
force-pushed
the
tcp-proxy-common-duration
branch
from
a83d263 to
fba91e8
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for the COMMON_DURATION access log command operator to the TCP proxy, populating connection time points (DS_CX_BEG, DS_CX_END, US_CX_BEG, and US_CX_END) for TCP connections. Feedback suggests modifying onDownstreamConnectionEnd to only record the first close event instead of overwriting it, which prevents artificial inflation of connection duration metrics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Implement the COMMON_DURATION access log command operator for the TCP proxy, which previously rendered as "-" for TCP connections. The DS_CX_BEG, DS_CX_END, US_CX_BEG and US_CX_END connection time points are now populated, which helps estimate added latency for TLS termination and TCP proxying. DS_CX_BEG and DS_CX_END are added as well-known COMMON_DURATION names, backed by startTimeMonotonic() and a new DownstreamTiming connection-end field. US_CX_BEG and US_CX_END are populated on the downstream StreamInfo via the existing upstream connect timing setters. Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com> Co-authored-by: Isaac Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
agrawroh
force-pushed
the
tcp-proxy-common-duration
branch
from
fba91e8 to
60b8b9c
Compare
botengyao
left a comment
botengyao
left a comment
There was a problem hiding this comment.
thanks for adding this support, and just have one major concern.
/wait
| auto& time_source = read_callbacks_->connection().dispatcher().timeSource(); | ||
| initial_upstream_connection_start_time_.emplace(time_source.monotonicTime()); | ||
| // Record the upstream connect begin time point for COMMON_DURATION access logging. | ||
| getStreamInfo().upstreamInfo()->upstreamTiming().onUpstreamConnectStart(time_source); |
There was a problem hiding this comment.
There is a onUpstreamConnectStart after the connection.connect() in the upstream client connection, for http, the upstream info is plumbed back to the downstream stream_info. Here, for tcp_proxy, this looks to me a little different, and it is more on the start of the connect attempt for the connection pool, which also could fail due to overflow, or other reasons.
I think there are two options:
- to really match what HTTP is doing, we can plumb the upstream connection stream_info to the downstream.
- update the naming/docs to say this is the upstream connection attempt begin from the pool.
up to you.
There was a problem hiding this comment.
Thanks, I like the 1st option to align with HTTP. I will remove the onUpstreamConnectStart call at the pool‑attempt site in establishUpstreamConnection, and instead plumb upstream_connect_start_/upstream_connect_complete_ from the upstream connection's StreamInfo in onGenericPoolReady.
It's gonna be similar to what Router::UpstreamRequest::onPoolReady does.
…y-common-duration
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
3 participants
Description
This PR adds a new
COMMON_DURATIONaccess log command operator for the TCP Proxy, which was previously rendered as-for TCP connections. TheDS_CX_BEG,DS_CX_END,US_CX_BEGandUS_CX_ENDconnection time points would now get populated.This could helps estimate the added latency for TLS termination and TCP proxying.
Commit Message: tcp_proxy: add COMMON_DURATION access log support
Additional Description: Added a new
COMMON_DURATIONaccess log command operator for the TCP Proxy.Risk Level: Low
Testing: Added Tests
Docs Changes: Added
Release Notes: Added