internet-latency-collector: rank lossy ripe atlas targets last and split the probe blacklist by role - #4331
internet-latency-collector: rank lossy ripe atlas targets last and split the probe blacklist by role#4331elitegreg wants to merge 7 commits into
Conversation
A target probe that drops most of the pings aimed at it passed the staleness check, which only asks whether a measurement exported anything in the last hour. Columbus landed on a probe behind NAT that answers about 15% of the time: enough to keep exporting, far too little to keep all 30 of its circuits inside the 2 hour freshness window, so circuits took turns going absent and the account-not-found alert fired daily for a month. Tally pings against each target and mark it unresponsive when a full window closes above the loss threshold, which hands it to the existing rotation path. Also prefer directly reachable probes over NAT'd ones during selection, since a NAT'd probe answers on behalf of the CPE rather than the probe. NAT'd probes are still used where a location has nothing else, matching the anchor fallback.
Filtering NAT'd probes at fetch time would dark a location whose every direct probe is already blacklisted. Columbus is exactly that case: its one direct probe never answers and is re-blacklisted every 24 hours, leaving only NAT'd candidates for most of the day. Move the preference to target selection, after responsiveness filtering, so a NAT'd probe is still reachable as a last resort. Sources are left alone, since being behind NAT does not stop a probe sending outbound pings.
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds RIPE Atlas target-loss tracking and NAT-aware target selection to rotate unreliable probes.
Changes:
- Tracks target ping attempts and loss ratios.
- Prefers direct probes while preserving NAT fallback.
- Adds state and selection tests.
File summaries
| File | Description |
|---|---|
| controlplane/internet-latency-collector/internal/ripeatlas/state.go | Updated as part of this pull request. |
| controlplane/internet-latency-collector/internal/ripeatlas/state_test.go | Updated as part of this pull request. |
| controlplane/internet-latency-collector/internal/ripeatlas/collector.go | Updated as part of this pull request. |
| controlplane/internet-latency-collector/internal/ripeatlas/collector_test.go | Updated as part of this pull request. |
| controlplane/internet-latency-collector/internal/ripeatlas/client.go | Updated as part of this pull request. |
Review details
Suppressed comments (5)
controlplane/internet-latency-collector/internal/ripeatlas/collector.go:937
- Issue
AddUnresponsiveProbewrites this target-only failure into the shared blacklist thatfilterResponsiveProbesalso uses for source selection.
Context
When a NAT probe exceeds the loss threshold, the next measurement-generation cycle skips that probe as a source even though NAT does not affect its outbound pings. This can remove circuits for other target locations.
Proposed Fix
Keep target-loss exclusions separate from source-probe exclusions, or make source selection ignore entries created by this target-loss path.
measurementState.AddUnresponsiveProbe(meta.TargetProbeID)
controlplane/internet-latency-collector/internal/ripeatlas/collector.go:675
- Issue The collector path that records target attempts and successes has no test coverage; the new state tests call
RecordTargetResultsdirectly, while the export tests do not assert the counters after parsing results.
Context A regression that stops counting timeout results, counts a successful RTT incorrectly, or omits the RecordTargetResults call would leave all current tests green even though loss-based rotation would never trigger.
Proposed Fix Add an export test with successful and total-loss result objects, then assert the persisted attempt and success counts before exercising the one-hour evaluation path.
measurementState.RecordTargetResults(measurement.ID, targetAttempts, targetSuccesses, time.Now().Unix())
controlplane/internet-latency-collector/internal/ripeatlas/collector.go:927
- Issue
The new collector wiring has no test that exercises the full loss-rotation path. The added tests cover the evaluator and target filtering in isolation, but they do not verify that an exported loss-heavy batch is stored, that the management pass blacklists its target, and that reconciliation replaces the measurement.
Context
A regression in either call site or state persistence could leave a target returning 15 successful pings out of 100 selected indefinitely while all current unit tests still pass.
Proposed Fix
Add a collector test with mocked results and API calls that runs the export and configuration steps, then asserts the target is blacklisted and the old measurement is removed or replaced.
if lossy, attempts, successes := measurementState.EvaluateTargetLoss(measurement.ID, currentTime); lossy {
controlplane/internet-latency-collector/internal/ripeatlas/collector.go:675
- Issue
The loss counters update before the exporter confirms the batch. IfWriteRecordsfails, the timestamp cursor remains unchanged, so the next export fetches the same results and counts them again. Repeated exporter failures can therefore blacklist a healthy target from duplicated outcomes.
Context
When a batch contains at least one successful ping and ledger or CSV export fails, each retry adds the same batch to the one-hour window even though RIPE produced no new pings.
Proposed Fix
Update the loss counters only after the batch writes successfully, or persist a deduplicated cursor and counter update together.
measurementState.RecordTargetResults(measurement.ID, targetAttempts, targetSuccesses, time.Now().Unix())
controlplane/internet-latency-collector/internal/ripeatlas/state.go:290
- Issue
This reset runs before the minimum-attempt check, so a closed window with fewer than 30 attempts discards its evidence instead of remaining open.
Context
With the default 10-minute sampling interval, a target with four or fewer source probes produces at most 24 attempts per hour. A target that loses more than half of its pings is then reset every hour and is never classified as lossy.
Proposed Fix
Keep the counters and original window start when the attempt count is below the minimum, and reset them only after a window has enough attempts to be judged.
meta.TargetWindowStart = now
meta.TargetAttempts = 0
meta.TargetSuccesses = 0
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…acking Four fixes to the loss-rotation path, three of which could blacklist a usable target or fail to catch a bad one. Count each result once. The export cursor advances only past results carrying a latency, so every timeout newer than the last success came back from each incremental query and was tallied again, inflating the ratio for a target that was merely losing its most recent pings. A separate loss cursor tracks what has been counted; the export cursor is untouched, since stale detection depends on it. Count only after the batch is written. Recording before WriteRecords meant a failed export tallied the same results on every attempt. Keep a short window open. The window reset ran before the minimum-attempt check, discarding the evidence every hour; at the 10 minute sampling interval a measurement with fewer than five sources could never reach the minimum within one window, so it was never judged at all. Separate target exclusions from source exclusions. Both selections read one list, so a probe that failed as a target was also barred from sourcing, dropping its location's circuits to every other target. Failing to answer pings says nothing about sending them. Target failures now land in their own list honored by target selection only, while a probe that stopped running measurements altogether still bars both. Tests cover the export path end to end: tallies from a mixed batch, replayed timeouts not recounted, a failed export counting nothing, and a lossy target being barred from targeting while staying available as a source.
…state Load decodes into an intermediate struct that declared only metadata and unresponsive_probes, so the target list came back nil after every restart and the next Save wrote the nil back over the persisted entries, erasing them about ten minutes into each process lifetime. Also drops GetTargetLossCursor, which has no callers, and the duplicated opening sentence on RecordTargetResults.
…ad of excluding them filterSelectableTargets dropped every probe with a live unresponsive mark, so a metro whose candidates were all marked produced no wanted measurement, and reconciliation read the absent target as unwanted and deleted the existing measurement. A metro with one dead probe and one lossy probe went from partial data to none for up to 24h (#4182). Target selection now filters only hard invalidity and ranks candidates unmarked-then-marked by distance, so the nearest marked candidate is still selected when nothing better exists. The wider non-anchor fetch triggers on the absence of an unmarked candidate rather than of any candidate, so it still runs where it could help, and a fetch that finds nothing leaves the measurement alone. Expiry semantics are untouched; probe qualification is a separate design. Drops the NAT preference with it. Probe 55128 near cmh, the one the collector converges on at 0% loss, carries system-ipv4-rfc1918 and answers fine, so the predicate does not predict the outcome: it keys on a user-supplied tag with arbitrary coverage and caught 2 of the 13 bad candidates in the metro it was written for. The loss measurement covers the case directly. Probe.Tags stays (#4334 adds the same field).
nikw9944
changed the title
Summary
Loadnever decodingunresponsive_targets, which erased the persisted list on the first save after every restart.Probe.BehindNATand the unreadProbe.IsAnchor.Why
The shared list conflates two roles. On 2026-09-14 20:00 UTC, un-blacklisting cmh's dead target probe changed cmh's source probe in the ams, bom and chi measurements and tore all three down (
Measurement has outdated source probes, marking for recreation×3). Failing to answer pings says nothing about sending them.Excluding a marked target is worse than using it. An empty candidate set means no wanted measurement for the location, so reconciliation deletes the existing one as unwanted: a metro with one dead probe and one lossy probe goes from partial data to none for up to 24h.
A lossy target is invisible to the staleness check, which only asks whether anything was exported in the last hour. cmh's target 12651 ran at 13–26% success every hour for two days (
valid_latenciesagainstraw_resultsin the export logs) — too sparse to keep all 30 circuits inside the 2h freshness window, so they take turns going missing and the metro reads as intermittently absent rather than plainly broken.NAT does not predict the outcome. Probe 55128 near cmh — the one the collector converges on, at 0% loss — carries RIPE's
system-ipv4-rfc1918tag. The predicate keys onnat, a user-supplied tag of arbitrary coverage, and caught 2 of the 13 bad candidates in the metro it was written for.Notes
UnresponsiveProbeExpiry(24h) and retry behaviour are unchanged on purpose: walking back down a metro's dead candidates after each expiry needs a probe-qualification design, not a longer timer.MeasurementMeta, so it resets whenever a measurement is recreated and a new target starts clean.Probe.Tagsduplicates the field internet-latency-collector: exclude probes tagged system-ipv4-doesnt-work #4334 adds on main — trivial rebase. Left to their own PRs: internet-latency-collector: lossy RIPE Atlas circuits get their sample timestamps compressed onchain #4337 and internet-latency-collector: skip reconciliation when the measurement picture is incomplete #4339.Testing Verification
configureMeasurements: one candidate markednever_exported, the current target marked for excessive loss in the same cycle — noStopMeasurement, noCreateMeasurement, target intact. Reverting to exclusion panics on the empty set.Loadround-trip: a target mark survives save/load/save and does not bar sourcing; fails without the decode fix.