Skip to content

device/telemetry: account-full path reports success and logs the wrong count #4127

Description

@elitegreg

Tracked by #4130.

Problem

Two defects in the account-full handling in Submitter.SubmitSamples.

1. Data loss is reported as success.

// controlplane/telemetry/internal/telemetry/submitter.go:159
} else if errors.Is(err, telemetry.ErrSamplesAccountFull) {
	log.Warn("Partition account is full, dropping samples from buffer and moving on", "droppedSamples", len(samples))
	s.cfg.Buffer.Remove(partitionKey)
	return nil          // caller counts this as a successful submission
}

return nil means Tick records success = true, no error metric fires, and the whole partition
is removed from the buffer. The only trace is a Warn. There is no counter to alert on and no way
to see this in Grafana. The same block is duplicated at lines 151-155 for the post-initialize path.

2. The logged count is wrong.

SubmitSamples batches at telemetry.MaxDeviceLatencySamplesPerBatch (line 95) and the loop
variable is batch, but the log reports len(samples) — the full partition, not the batch that
actually hit the limit. The number in the log overstates the loss whenever a partition spans more
than one batch.

Proposed change

Use the shared drop counter from #4126, fix the count, and factor the duplicated
block:

func (s *Submitter) handleAccountFull(log *slog.Logger, partitionKey PartitionKey, batchLen int) {
	metrics.SamplesDropped.WithLabelValues(metrics.DropReasonAccountFull).Add(float64(batchLen))
	metrics.Errors.WithLabelValues(metrics.ErrorTypeSubmitterAccountFull).Inc()
	log.Warn("partition account is full, dropping partition",
		"droppedBatch", batchLen,
		"epoch", partitionKey.Epoch)
	s.cfg.Buffer.Remove(partitionKey)
}

Called from both branches, replacing len(samples) with len(batch).

Whether return nil should become a distinct sentinel is worth deciding during the fix. Returning
an error would trigger 5 pointless retries against an account that cannot accept writes, so nil
is probably right; the gap is the missing metric, not the control flow.

Worth checking separately

An account filling up mid-epoch is itself a signal. If this fires outside of a backlog-flush
scenario it may mean the account sizing does not cover a full epoch at the current 10s sampling
interval, which would be a separate bug.

Acceptance

  • Account-full increments a counter that can be alerted on.
  • The logged count matches the batch that was actually rejected.
  • The two duplicated blocks share one implementation.

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions