telemetry: add TCP RTT info collection #4745
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4745 +/- ##
=======================================
Coverage 54.68% 54.69%
=======================================
Files 414 416 +2
Lines 53550 53596 +46
=======================================
+ Hits 29286 29313 +27
- Misses 21836 21856 +20
+ Partials 2428 2427 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Could you attach a sample of what the new PeerConnections payload looks like? (or, highlights of what changes)? |
|
Example output from the test I added, from a Linux CI build: |
| // underlying network implementation, using a system call on Linux and Mac | ||
| // and returning an error for unsupported platforms. | ||
| func GetConnTCPInfo(conn net.Conn) (*TCPInfo, error) { | ||
| sysconn, ok := conn.(syscall.Conn) |
There was a problem hiding this comment.
Maybe this is the answer: unwrap until you get a syscall.Conn not as long as you the type implements underlyingConn.
There was a problem hiding this comment.
Yeah I could change it so GetConnTCPInfo required a syscall.Conn and then the loop in wsNetwork resolved and broke out of the loop with a syscall.Conn
network/wsNetwork.go
Outdated
| @@ -1821,6 +1830,18 @@ func (wn *WebsocketNetwork) sendPeerConnectionsTelemetryStatus() { | |||
| InstanceName: peer.InstanceName, | |||
| DuplicateFilterCount: peer.duplicateFilterCount, | |||
| } | |||
| // unwrap websocket.Conn, requestTrackedConnection, rejectingLimitListenerConn | |||
| var uconn net.Conn = peer.conn.UnderlyingConn() | |||
| for { | |||
There was a problem hiding this comment.
Is there an expected upper bound on this? If so, will be safer if this loop is bounded to say 10x that expected bound, just to avoid the infinite loop situation.
There was a problem hiding this comment.
Agree with Shant and JJ, need to bound here
There was a problem hiding this comment.
I added a for-1-to-10 around it, WDYT
network/wsNetwork.go
Outdated
| @@ -1821,6 +1830,18 @@ func (wn *WebsocketNetwork) sendPeerConnectionsTelemetryStatus() { | |||
| InstanceName: peer.InstanceName, | |||
| DuplicateFilterCount: peer.duplicateFilterCount, | |||
| } | |||
| // unwrap websocket.Conn, requestTrackedConnection, rejectingLimitListenerConn | |||
| var uconn net.Conn = peer.conn.UnderlyingConn() | |||
| for { | |||
There was a problem hiding this comment.
Agree with Shant and JJ, need to bound here
Summary
This collects RTT information from TCP and reports it in PeerConnectionDetails, if telemetry is enabled.
Test Plan
Added test for setting up a pair of networks connected to each other and gathering telemetry from open and closed connections. Reduced some code duplication in wsNetwork_test.go for setting up two networks.