qlog: use PathEndpointInfo in connection_started#5368
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the StartedConnection qlog event to use a new PathEndpointInfo struct instead of net.UDPAddr pointers, providing better separation of IPv4 and IPv6 endpoint information.
- Replaces
SrcAddr,DestAddr, and connection ID fields withLocalandRemotePathEndpointInfostructs - Introduces
PathEndpointInfostruct that can hold both IPv4 and IPv6 address/port pairs - Updates JSON encoding to use separate IPv4/IPv6 fields with dedicated port fields
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| qlog/event.go | Introduces PathEndpointInfo struct and updates StartedConnection to use it |
| qlog/event_test.go | Updates test to use new PathEndpointInfo structure and verify new JSON format |
| connection_logging.go | Adds conversion function from net.UDPAddr to PathEndpointInfo |
| connection.go | Updates StartedConnection usage to use new PathEndpointInfo fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if addr.IP.To4() != nil { | ||
| addrPort := netip.AddrPortFrom(netip.AddrFrom4([4]byte(addr.IP.To4())), uint16(addr.Port)) | ||
| if addrPort.IsValid() { | ||
| info.IPv4 = addrPort | ||
| } | ||
| } else { | ||
| addrPort := netip.AddrPortFrom(netip.AddrFrom16([16]byte(addr.IP.To16())), uint16(addr.Port)) | ||
| if addrPort.IsValid() { | ||
| info.IPv6 = addrPort |
There was a problem hiding this comment.
The slice-to-array conversions [4]byte(addr.IP.To4()) and [16]byte(addr.IP.To16()) will panic if the slice length doesn't match the array size. Use explicit length checks or safer conversion methods.
| if addr.IP.To4() != nil { | |
| addrPort := netip.AddrPortFrom(netip.AddrFrom4([4]byte(addr.IP.To4())), uint16(addr.Port)) | |
| if addrPort.IsValid() { | |
| info.IPv4 = addrPort | |
| } | |
| } else { | |
| addrPort := netip.AddrPortFrom(netip.AddrFrom16([16]byte(addr.IP.To16())), uint16(addr.Port)) | |
| if addrPort.IsValid() { | |
| info.IPv6 = addrPort | |
| ip4 := addr.IP.To4() | |
| if ip4 != nil && len(ip4) == net.IPv4len { | |
| var ip4arr [4]byte | |
| copy(ip4arr[:], ip4) | |
| addrPort := netip.AddrPortFrom(netip.AddrFrom4(ip4arr), uint16(addr.Port)) | |
| if addrPort.IsValid() { | |
| info.IPv4 = addrPort | |
| } | |
| } else { | |
| ip16 := addr.IP.To16() | |
| if ip16 != nil && len(ip16) == net.IPv6len { | |
| var ip16arr [16]byte | |
| copy(ip16arr[:], ip16) | |
| addrPort := netip.AddrPortFrom(netip.AddrFrom16(ip16arr), uint16(addr.Port)) | |
| if addrPort.IsValid() { | |
| info.IPv6 = addrPort | |
| } |
| addrPort := netip.AddrPortFrom(netip.AddrFrom16([16]byte(addr.IP.To16())), uint16(addr.Port)) | ||
| if addrPort.IsValid() { | ||
| info.IPv6 = addrPort |
There was a problem hiding this comment.
The slice-to-array conversions [4]byte(addr.IP.To4()) and [16]byte(addr.IP.To16()) will panic if the slice length doesn't match the array size. Use explicit length checks or safer conversion methods.
| addrPort := netip.AddrPortFrom(netip.AddrFrom16([16]byte(addr.IP.To16())), uint16(addr.Port)) | |
| if addrPort.IsValid() { | |
| info.IPv6 = addrPort | |
| ip16 := addr.IP.To16() | |
| if ip16 != nil && len(ip16) == 16 { | |
| var arr16 [16]byte | |
| copy(arr16[:], ip16) | |
| addrPort := netip.AddrPortFrom(netip.AddrFrom16(arr16), uint16(addr.Port)) | |
| if addrPort.IsValid() { | |
| info.IPv6 = addrPort | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5368 +/- ##
==========================================
+ Coverage 83.02% 83.21% +0.18%
==========================================
Files 158 158
Lines 18904 18966 +62
==========================================
+ Hits 15695 15781 +86
+ Misses 2588 2566 -22
+ Partials 621 619 -2 ☔ View full report in Codecov by Sentry. |
35424d9 to
9ed2c94
Compare
f4119c8 to
9e60908
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
9e60908 to
3aa5d66
Compare
|
This is not entirely correct. I'm seeing events like this one: {
"time": 0.06425,
"name": "transport:connection_started",
"data": {
"local": {
"ip_v6": "::",
"port_v6": 58451
},
"remote": {
"ip_v4": "127.0.0.1",
"port_v4": 6121
}
},
"timeText": "0.06"
}Obviously, this is wrong. However, this was wrong before as well: {
"time": 0.043125,
"name": "transport:connection_started",
"data": {
"ip_version": "ipv6",
"src_ip": "::",
"src_port": 61547,
"dst_ip": "127.0.0.1",
"dst_port": 6121,
"src_cid": "f294cb83",
"dst_cid": "9bcf36d4f4d120bd2794264b4308d65eca0d"
},
"timeText": "0.04"
} |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Fixes #5347. Depends on #5367.