Repository navigation
Conversation
…B commit Every Push takes its own appender and calls Commit on it, and Commit writes to the WAL through wlog.(*WL).Log, which holds the tenant's WAL mutex for the whole record loop. When a distributor shards one large tenant's writes by series across many ingesters, each ingester sees a high rate of requests carrying only a few samples each, and takes that mutex once per request. Add an experimental, opt-in push coalescer, one per tenant TSDB. A request arriving while no commit is in flight commits on its own with no added latency. A request arriving while a commit is in flight joins the next batch; when the in-flight commit finishes, the first queued request leads that batch, appends every request in it into one appender, commits once and hands each request its own result. There is no timer. Joining a batch is a single compare-and-swap on a lock-free stack, so the merge point itself cannot fall into sync.Mutex starvation mode. The append phase of Push is unchanged; it is wrapped in a closure over the request's own locals, so per-request limits, active series, errors and discarded-sample counts stay attributed to the request they belong to, and metrics are still emitted by the request's own goroutine after the commit. A follower never returns before the leader has finished appending its samples, which keeps the pooled write request alive for as long as the leader reads it. A batch shares one appender, and Append only checks against committed state. So a sample that a separate commit would have rejected because of another request in the same batch is instead dropped at Commit without an error being returned: a duplicate timestamp, or an out-of-order sample beyond the window. The stored data is the same in both cases. In one narrow case the batch stores a sample a separate commit rejects as too old; the band of affected timestamps is as wide as the batch's own advance of the head max time. Tests pin each of these against separate commits. Signed-off-by: Ben Ye <benye@amazon.com>
Signed-off-by: Ben Ye <benye@amazon.com>
Address the modernize and ineffassign findings: range over int, WaitGroup.Go, max, and slices.Sort/SortFunc instead of sort.Slice; drop the dead first assignment of commitsBefore in the benchmark. Signed-off-by: Ben Ye <benye@amazon.com>
Move the Push-level tests, the attribution tests, the out-of-order bound test and the histogram and exemplar tests into push_coalesce_test.go, in two halves: pushCoalescer driven with fake appenders, then Ingester.Push against a real TSDB. Merge the duplicated helpers: one ingester start helper, which now enables exemplars too, and one head dump helper that handles float and histogram samples. Signed-off-by: Ben Ye <benye@amazon.com>
Run a distributor, one ingester, a store-gateway and a querier with push coalescing enabled, push 3,200 small requests from 64 concurrent clients, and check that every push succeeds, that the ingester served some requests as followers and issued fewer commits than requests, and that every sample reads back through the querier with its own value. Run it twice: once with every client on its own series, and once with eight clients sharing each series, so that batches hold several out-of-order requests for the same series. Signed-off-by: Ben Ye <benye@amazon.com>
This branch has not been deployed
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.
What this PR does:
Adds experimental, opt-in push coalescing to the ingester: concurrent write requests for the same tenant are merged into a single TSDB commit.
Problem
Every
Pushtakes its own appender and callsCommit()on it.Commit()writes the WAL throughwlog.(*WL).Log, which holds the tenant'sWL.mtxfor the whole record loop. When a distributor shards one large tenant's remote-write requests by series across many ingesters, every ingester sees a very high rate of requests that each carry only a few samples, and takes that one mutex once per request. Once any waiter has waited longer than 1ms,sync.Mutexswitches to starvation mode and hands the lock over in strict FIFO order; if woken waiters are then slow to get back on a CPU, each handoff costs far more than the work done under the lock, throughput drops below the arrival rate, and in-flight pushes pile up.Approach
One merge point per tenant TSDB (
userTSDB.pushCoalescer), so a batch never spans tenants and the WAL mutex it relieves is that tenant's.Commit()once, and hands each request its own result.-ingester.push-coalesce.max-batch-requests(default 32),-ingester.push-coalesce.max-batch-samples(default 2000) or-ingester.push-coalesce.max-batch-bytes(default off). Requests that disagree onDiscardOutOfOrdernever share a batch, because it is an appender-level option. Truncation keeps arrival order, so a request too large to join anyone else's batch still leads its own.Commit()failure fails every request in the batch. An append error that requires discarding the appender rolls the whole batch back; the offending request gets its error and the others get a retryable error. Label-set order is validated before a request joins a batch, so the one deterministic error on that path cannot take a whole batch down.The append phase of
Pushis unchanged: it is wrapped in a closure over the request's own locals, so per-request limits, active series, errors and discarded-sample counts stay attributed to the request they belong to, and metrics are still emitted by each request's own goroutine after the commit. A follower never returns before the leader has finished appending its samples, which keeps the pooled write request alive for as long as the leader reads it.Disabled by default (
-ingester.push-coalesce.enabled=false); with it off,Pushfollows exactly the existing path.Semantic differences when enabled
A batch shares one appender, and
Appendonly checks against committed state whileCommitre-checks against what earlier samples in the same batch left behind, counting rather than returning what it then rejects. When two requests in one batch write the same series:400 duplicate sample400 too oldThe third case can only ever store more, never less, and the affected timestamps lie in
[H − W, t_X − W)whereHis the head max time before the batch andt_Xthe batch's newest sample: a band as wide as the batch's advance of the head max (one scrape interval in steady state, against an OOO window of typically tens of minutes), and only for a series silent for longer than the window.For reference, Cortex already behaves this way within a single request: two samples for one series at the same timestamp in one request return
200and keep the first value, while the same two samples sent as two requests return400(pinned byTestIngesterPush_DuplicateIsAlreadySilentWithinOneRequest). Coalescing widens that from one request to one batch. While enabled, thenew-value-for-timestampandsample-too-oldreasons ofcortex_discarded_samples_totalcan under-report accordingly.Tests
TestIngesterPushCoalescing_DivergenceFromSeparateCommits: 8 scenarios, each comparing every request's error and the full head contents including the OOO head. Each scenario declares whether it expects errors or data to differ, and both directions are asserted, so a TSDB change either way fails the test.TestIngesterPushCoalescing_OutOfOrderEscapeIsBoundedByInBatchHeadAdvance: pins the band above with samples just inside, below and above it.TestIngesterPushCoalescing_ErrorsStayAttributedToTheirRequestand..._PerUserMetricTotalsMatchSeparateCommits: per-request errors and the ingested, failed and discarded-sample counters match separate commits, with the expected values also written out../pkg/ingester/...passes, also with-race.Benchmarks
BenchmarkIngesterPushCoalescing*inpkg/ingester/push_coalesce_bench_test.go. Single tenant, 5 samples per request, OOO window on, every series created before the clock starts. AMD EPYC 7R13, local NVMe, Go 1.27,GOMAXPROCS=8, median of 3 runs.When concurrent requests often write the same series (512 shared series): 100k → 204k pushes/s at 1,024 pushers, 78k → 159k at 4,096, still 32 requests per commit.
Mutex profile at 4,096 pushers: aggregate mutex delay drops from 12,817 s to 0.06 s;
wlog.(*WL).Logaccounts for 9,807 s of the former and does not appear in the latter.The cost is p50: a queued request waits for the in-flight commit (at 4,096 pushers, 0.03ms → 41ms), while p99 improves 3–13×. At high concurrency throughput levels off, because the leader appends a batch serially. In an earlier revision of this change,
-ingester.push-coalesce.max-batch-requests=128measured 15–20% faster than the default 32 at 1,024–4,096 pushers; the default is kept conservative.Which issue(s) this PR fixes:
None filed.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags