controlplane/telemetry: keep newest samples on partial buffer overcapacity - #4233
Open
ayushsingh82 wants to merge 1 commit into
Open
controlplane/telemetry: keep newest samples on partial buffer overcapacity#4233ayushsingh82 wants to merge 1 commit into
ayushsingh82 wants to merge 1 commit into
Conversation
…acity Tick()'s failed-retry path used to drop the entire unwritten batch the moment it didn't fully fit back into the buffer. Replace that binary check with room := capacity - bufLen, and requeue only the newest `room` samples (unwritten is already oldest-first), dropping just the oldest excess instead of the whole backlog. Also fixes an off-by-one where an exact-fit backlog (bufLen + len(unwritten) == capacity) was treated as over capacity and dropped in full. Updates the tests whose assertions baked in the old boundary bug.
Contributor
Author
|
@elitegreg PR is up, ready for CI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4149.
Tick()'s failed-retry path (controlplane/telemetry/internal/telemetry/submitter.go) replaced the binary "does the whole unwritten remainder fit, yes/no" check withroom := capacity - bufLen.roomsamples (the tail — batches submit oldest-first, sounwrittenis already chronological) and drops only the oldest excess, instead of discarding the entire backlog.bufLen + len(unwritten) == capacity) was treated as over capacity and dropped in full; it now requeues in full.Test coverage
Rewrote 4 pre-existing tests whose assertions baked in the old boundary bug, rather than just adding new ones:
failed_retries_drop_when_over_capacity→failed_retries_keep_newest_when_over_capacity: was asserting full drop at a boundary that should now partially keep.no_backpressure_when_drop_on_overcapacity→kept_samples_never_exceed_capacity_after_partial_drop: retargeted at the invariant that actually matters post-fix (buffer never exceeds capacity), since "producer never blocks" isn't true in general once partial-keep can fill a buffer to exactly capacity.drops_failed_samples_when_requeue_would_meet_capacity→requeues_failed_samples_when_they_exactly_meet_capacity: exact-fit (bufLen + len(unwritten) == capacity) now requeues in full instead of dropping — that was the off-by-one in the old>=check.logs_and_counts_dropped_samples_when_over_capacity: updated to a genuine 3-samples-into-capacity-2 case so it exercises partial drop and asserts the newkeptSampleslog field.Test plan
go test ./internal/telemetry/...— 8/8 top-level tests pass, 0 failures (includes all 3 renamed subtests)go vet ./internal/telemetry/...gofmt -lcleangolangci-lint run ./internal/telemetry/...— 0 issues