Fix macOS pcap traceroute ~100 ms latency floor - #134
Conversation
|
Version impact audit for datadog-agent consumers:
So the current release impact looks limited to macOS development/custom Agent builds from |
|
The affected Agent builds are macOS Network Path builds that import For
So the practical answer is: no official released Agent 7 tag I scanned is affected; affected Agent builds are development/custom builds from Reference: DataDog/datadog-agent@a229565 The current Agent release metadata at the checked Also: Agent appears to have skipped importing traceroute |
There was a problem hiding this comment.
Pull request overview
This PR addresses a macOS-specific RTT inflation issue by configuring the libpcap/BPF capture path to deliver packets immediately instead of batching delivery on the read timeout cadence, which previously introduced an artificial ~100ms latency floor in traceroute RTT accounting.
Changes:
- Enable
pcap.InactiveHandle.SetImmediateMode(true)for the Darwin pcap source prior to activation. - Introduce a named
pcapReadTimeoutconstant for clarity and reuse. - Add a Darwin/root regression test asserting captured loopback SYN/ACK delivery occurs well before the prior 100ms floor; refactor packet-source test helpers to avoid requiring an unused sink on macOS.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packets/test_source_notdarwin_test.go | Adds a non-Darwin helper to construct/close a loopback test Source via NewSourceSink. |
| packets/pcap_filter_test.go | Switches to the new loopback-source helper and centralizes close logic. |
| packets/bpfdev_darwin.go | Names the read timeout and enables pcap immediate mode before activation. |
| packets/bpfdev_darwin_test.go | Adds a macOS/root regression test validating packet delivery latency is not dominated by the pcap read timeout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a542ac5e3b
ℹ️ 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".
| inactive.CleanUp() | ||
| return nil, fmt.Errorf("NewBpfDevice failed to set timeout: %w", err) | ||
| } | ||
| if err := inactive.SetImmediateMode(true); err != nil { |
There was a problem hiding this comment.
When a probe is dropped or the BPF filter matches no packets, this can make macOS traceroutes hang indefinitely. PcapSource.Read() only checks p.deadline before/after ReadPacketData() returns, and gopacket documents SetImmediateMode as overriding SetTimeout (https://pkg.go.dev/github.com/google/gopacket/pcap#InactiveHandle.SetImmediateMode), so the 100ms timeout no longer wakes the loop to return ReceiveProbeNoPktError.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[AI Generated]
I do not think this is a valid issue in this PR. SetImmediateMode(true) does override libpcap's packet-buffer timeout behavior, but gopacket still preserves its Go-side timeout after activation: InactiveHandle.Activate copies p.timeout to the active handle and calls setNonBlocking() when the timeout is positive. ReadPacketData then polls via waitForPacket() using that Go-side timeout and returns NextErrorTimeoutExpired; PcapSource.Read already converts repeated timeout wakeups into ReceiveProbeNoPktError once its deadline has elapsed.
So immediate mode removes the 100 ms batching delay for packets, but it should not make dropped/no-match reads block indefinitely. No code change needed.
Summary
Fixes #133.
On macOS, libpcap/BPF capture was configured with a 100 ms read timeout but not immediate mode. In practice, captured packets could be delivered to userspace on that timeout cadence, so traceroute RTT accounting included capture delivery delay. This produced an artificial ~100 ms RTT floor, including for directly reachable LAN targets.
This PR enables pcap immediate mode for the macOS BPF source so packets are delivered as soon as they arrive, while keeping the existing timeout as a fallback/read timeout setting.
Changes
InactiveHandle.SetImmediateMode(true)in the Darwin pcap source before activation.NewBpfDevicedirectly instead of requiring an unused raw packet sink.Validation
Reproduced before the fix:
datadog-traceroutereported ~101-105 ms RTT to a local gateway over TCP/80.curlTCP connect timing to the same gateway was ~5 ms.tcpdump -tttshowed SYN to SYN/ACK at ~3 ms.Checks run after the fix:
All passed.
Risk
Scoped to macOS packet capture setup. Linux uses AF_PACKET and Windows uses raw socket/driver completion paths, so this does not change their capture behavior.
Follow-up
This PR intentionally keeps the fix scoped to immediate mode plus regression coverage. Timestamp-based RTT accounting with packet capture metadata is broader hardening and is tracked separately in #135.