Use packet capture timestamps for RTT accounting#136
Conversation
Compute probe RTT from the kernel/BPF packet capture timestamp when the packet source can provide one, instead of the userspace time at which the packet happens to be read and processed. This avoids RTT inflation from capture delivery batching or scheduler delay. ReadAndParse now returns the timestamp of the received packet, preferring a TimestampedSource-provided capture timestamp and falling back to time.Now() for sources that don't support it (Linux, Windows). macOS's PcapSource implements TimestampedSource using libpcap's CaptureInfo.Timestamp. All four drivers (ICMP, UDP, TCP-SYN, SACK) now compute RTT from this timestamp. Closes #135 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves traceroute RTT accuracy by computing RTT using packet-capture/kernel timestamps when available (notably on macOS via libpcap), falling back to userspace time.Now() when capture timestamps aren’t provided.
Changes:
- Introduces
packets.TimestampedSourceand updatespackets.ReadAndParseto return areceivedAttimestamp. - Updates ICMP/UDP/TCP-SYN/SACK drivers to compute RTT via
receivedAt.Sub(sendTime)instead oftime.Since(sendTime). - Implements timestamp reporting in the macOS
PcapSourceand adds unit tests for timestamp selection behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| udp/udp_driver.go | Uses receivedAt from ReadAndParse for RTT calculation. |
| tcp/tcp_driver.go | Uses receivedAt from ReadAndParse for RTT calculation. |
| icmp/icmp_driver.go | Threads receivedAt through ICMP RTT computation helpers. |
| sack/sack_driver.go | Threads receivedAt through SACK RTT computation helpers. |
| packets/packet_source.go | Adds TimestampedSource and returns a receive timestamp from ReadAndParse. |
| packets/bpfdev_darwin.go | Stores libpcap CaptureInfo.Timestamp and exposes it via LastPacketTimestamp. |
| packets/packet_source_test.go | New unit tests asserting kernel timestamp preference and fallback behavior. |
| packets/bpfdev_darwin_test.go | Updates call sites for new ReadAndParse return signature. |
| packets/tcp_filter_test.go | Updates call sites for new ReadAndParse return signature. |
| packets/pcap_filter_test.go | Updates call sites for new ReadAndParse return signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Kernel/BPF capture timestamps carry no monotonic clock reading, so receivedAt.Sub(sendTime) falls back to wall-clock-only subtraction (unlike the previous time.Since(sendTime), which was immune to clock steps via the monotonic reading). If the system wall clock steps backward between sendTime and the packet being captured, this could previously yield a negative RTT. Add packets.RTT(receivedAt, sendTime), which falls back to time.Since(sendTime) whenever the capture timestamp predates the send time, and use it at all four RTT computation sites.
- ReadAndParse's error messages referred to "ConnHandle", a stale name from before it operated on the generic Source interface. - Drop the flaky elapsed-time bounds in TestReadAndParseUsesKernelTimestampOverDelayedDelivery; the exact timestamp equality check already proves the kernel timestamp is used.
Local experiment results (macOS)Measured the accuracy impact of preferring the kernel/BPF capture timestamp over Method — same-run dual RTT computation (avoids run-to-run jitter confounds): for every received packet, computed both Against
Against
Takeaway: the delta between kernel-timestamp RTT and userspace-timestamp RTT is a direct measurement of scheduler/delivery jitter, not a network property — it's small (sub-ms p50/p90) when the system is idle, but grows to tens of milliseconds under load (background CPU contention, many concurrent probes, etc.). This is the same class of jitter #133/#134 exposed for macOS pcap's old 100ms-cadence delivery; capture timestamps remove this source of RTT inflation entirely rather than just reducing its likelihood. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0229c0a49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the kernel/BPF timestamp propagation through PcapSource, ReadAndParse, and all four traceroute drivers. The implementation is structurally sound, but the production macOS timestamp path is not asserted by a test.
Overall assessment: needs fixes (test coverage gap only; no security or correctness defect found).
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
packets/bpfdev_darwin_test.go:64 |
The real PcapSource kernel timestamp path is untested |
Checks run successfully: go test ./..., go vet ./..., go build ./..., and go test -race ./packets ./icmp ./tcp ./udp.
… test TestPcapSourceDeliversPacketsWithoutReadTimeoutDelay discarded ReadAndParse's returned timestamp, so it wouldn't catch a regression where PcapSource stopped implementing TimestampedSource or Read() stopped copying CaptureInfo.Timestamp -- exactly the class of regression this PR guards against. Now it retains and asserts on the returned timestamp, and bpfdev_darwin.go asserts PcapSource satisfies TimestampedSource at compile time. Also add TestTCPDriverUsesKernelTimestampForRTT, which wraps MockSource with a TimestampedSource implementation and a deliberately delayed Read() to prove the tcp driver computes RTT from the reported capture timestamp rather than delivery time.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
Summary
TimestampedSourceinterface;ReadAndParsenow returns the received-at timestamp, preferring the capture timestamp and falling back totime.Now()for sources that don't support it (Linux, Windows unaffected).PcapSourceimplementsTimestampedSourcevia libpcap'sCaptureInfo.Timestamp.time.Since(sendTime).Closes #135. This is separate from #134 (macOS immediate-mode fix for the ~100ms RTT floor), per the issue's own scoping.
Test plan
go build ./...go vet ./...go test ./...(full suite)packets/packet_source_test.go: simulate delayed packet delivery with a valid capture timestamp and assert RTT uses the capture timestamp, not delivery time; and assert fallback totime.Now()when no capture timestamp is available.8.8.8.8, andwww.sakura.ad.jpfor all four drivers using instrumented before/after RTT comparisons — kernel timestamps eliminate userspace scheduling jitter from RTT (up to tens of ms under load).🤖 Generated with Claude Code