Skip to content

telemetry: count and correctly report samples dropped when account is full - #4145

Open
elitegreg wants to merge 1 commit into
gm/telemetry-drop-samples-metricfrom
gm/telemetry-account-full-metric
Open

telemetry: count and correctly report samples dropped when account is full#4145
elitegreg wants to merge 1 commit into
gm/telemetry-drop-samples-metricfrom
gm/telemetry-account-full-metric

Conversation

@elitegreg

@elitegreg elitegreg commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Resolves: #4127

Stacked on #4144 (gm/telemetry-drop-samples-metric), which adds the shared samples_dropped_total counter this reuses. Base is that branch, so the diff shown is just this change; merge #4144 first.

Summary of Changes

  • Account-full drops are now visible in metrics. SubmitSamples returns nil on ErrSamplesAccountFull so Tick counts it as a successful submission, which means data loss previously left nothing but a warning. It now increments doublezero_device_telemetry_agent_samples_dropped_total{reason="account_full"} and submitter_account_full on the errors counter.
  • The logged count is fixed. The old warning reported len(samples), the whole flushed partition, rather than what was actually lost, overstating the loss whenever a partition spans more than one batch (a 300-sample partition that filled the account on its second batch logged droppedSamples=300 when 61 samples were lost).
  • The two duplicated account-full blocks (direct write and post-initialize) are factored into handleAccountFull.
  • Control flow is unchanged: the account cannot accept further writes, so returning an error would only buy five pointless retries. The partition is still removed and the caller still treats it as done.

Note on the dropped count

The issue proposed counting len(batch). That undercounts for the same reason len(samples) overcounts: when the account fills mid-partition, the batches behind the failing one are never attempted and are discarded along with it, and Buffer.Remove also throws away whatever the collector buffered since the flush. handleAccountFull therefore counts len(samples)-i (the failing batch plus everything after it) and adds the partition's buffered length, read before the removal. The log reports the two separately as unsubmittedSamples and bufferedSamples.

Diff Breakdown

Category Files Lines (+/-) Net
Tests 1 +140 / -0 +140
Core logic 1 +23 / -4 +19
Scaffolding 1 +3 / -1 +2
Docs 1 +1 / -0 +1
Total 4 +167 / -5 +162

Mostly tests; the fix itself is one extracted helper and two call sites.

Key files (click to expand)

Testing Verification

  • New test adds a sample from inside the write callback (after the flush, so it is only reachable via Buffer.Remove) and asserts the drop counter rises by 3 for a 2-sample flush, the error counter by 1, and the log carries unsubmittedSamples=2 bufferedSamples=1.
  • New multi-batch test flushes 300 samples with the first batch succeeding and the second hitting a full account, and asserts exactly 61 drops and no unsubmittedSamples=300. Against the pre-fix submitter this test sees the old droppedSamples=300, confirming the miscount.
  • New test covers the post-initialize account-full branch, which the extracted helper now shares.
  • All three fail against the pre-fix submitter. Existing account-full tests (drops_samples_if_account_full, initializes_then_drops_samples_if_account_full) still pass unchanged.
  • The new subtests run serially and compare counter deltas, since the metrics are package-level and shared with those existing parallel subtests.

@elitegreg
elitegreg force-pushed the gm/telemetry-account-full-metric branch from 5c5438a to f6bded5 Compare August 3, 2026 15:33
@elitegreg
elitegreg force-pushed the gm/telemetry-drop-samples-metric branch from 9f60fe6 to 03ff35e Compare August 3, 2026 15:34
@elitegreg
elitegreg force-pushed the gm/telemetry-account-full-metric branch from f6bded5 to 282d4ad Compare August 3, 2026 15:35
@elitegreg
elitegreg marked this pull request as ready for review August 3, 2026 15:39
@elitegreg
elitegreg force-pushed the gm/telemetry-account-full-metric branch from 282d4ad to a51bf2a Compare August 3, 2026 16:55
elitegreg added a commit that referenced this pull request Aug 3, 2026
Resolves: #4128

Independent of #4144 and #4145 (different file), so this one branches
from `main`.

## Summary of Changes

- `ledgerPeerDiscovery.refresh` no longer empties the peer cache before
doing work that can fail. It cleared `p.peers` under the lock and then
called `LocalNet.Interfaces()`, so a transient failure there returned
with zero peers and `Pinger.Tick` iterated an empty slice, probing
nothing until a later refresh succeeded.
- The cache is now replaced only once the new list is built, and the
lock covers just that assignment rather than the whole build. The clear
was redundant with the existing assignment at the end of the happy path.
- Success path is unchanged.

## Diff Breakdown

| Category   | Files | Lines (+/-) | Net  |
|------------|-------|-------------|------|
| Tests      |     1 | +79 / -0    |  +79 |
| Core logic |     1 | +6 / -5     |   +1 |
| Docs       |     1 | +3 / -0     |   +3 |
| **Total**  |     3 | +88 / -5    |  +83 |

A one-line behavioral fix plus the regression test that pins it.

<details>
<summary>Key files (click to expand)</summary>

-
[`controlplane/telemetry/internal/telemetry/peers.go`](https://github.com/malbeclabs/doublezero/pull/4146/files#diff-9c369dff3cb79259b8bc34d8d952b923103baeef1515402614c23a997a06c286)
— drops the `p.peers = make(...)` clear, moves the mutex to wrap only
`p.peers = peers`, and leaves a comment that nothing in the build may
clear the cache

</details>

## Testing Verification

- New test lets the first refresh discover a peer, then fails every
subsequent `LocalNet.Interfaces()` call, and asserts `GetPeers()` still
returns the fully populated peer (link, device, tunnel, TWAMP port)
after at least three failed refreshes. It fails against the pre-fix
code, which returns an empty list.
- Existing peer discovery tests pass unchanged, covering the success
path and the skip cases.
- Package passes under `-race`, since the change moves what the mutex
covers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant