Repository navigation
Bound eviction batches, refuse exports over the bound, store identical resends once - #17
Draft
RhysSullivan wants to merge 4 commits into
Draft
RhysSullivan wants to merge 4 commits into
RhysSullivan wants to merge 4 commits into
Conversation
A full store of large traces evicted the 100 oldest in one uninterrupted batch, which took seconds and held every export behind it. Exports timed out, the exporter sent them again, and storing each resend rewrote every row and index entry of its spans, so the collector slowed further. Each batch now stops at MOTEL_OTEL_RETENTION_SPAN_BATCH spans (1000), so the pass budget bounds a pass; the deletes that read a whole table (the operation index and the log orphan sweeps) run once per pass rather than once per batch; and a resent span identical to the stored one is not written again.
…tion pass at once Capped eviction passes could fall behind ingest, and every export was still stored, so the store grew past its size bound without limit. An export that finds the store over its bound now runs one eviction pass and, if the store is still over, is refused with 429 and Retry-After, counted as overBound. Stored bytes stay within the bound plus the exports already admitted. A pass's deletes now commit in one transaction, so a failure between deleting spans and deleting their operation-index rows no longer leaves index rows for spans that are gone, and the operation-index delete runs once per pass for every evicted trace.
A deleted log's attributes and body index rows are most of its size and only the orphan sweeps remove them. Sweeping once after the batches left each batch measuring no room freed, so a size-bound pass deleted every log, the newest included (daemon test: configured size retention evicts oldest logs first). A batch that deletes logs now sweeps before it measures the store; a pass that deleted none still sweeps once.
…ge on running traces The over-bound check sat on the collector's exact /v1/traces and /v1/logs paths, and the typed routes also store from /v1/traces/, //v1/traces, /v1/Traces and /v1/%74races, so those spellings bypassed it. The check now runs in the store's ingestTraces and ingestLogs, in the same synchronous step as the write, behind a refuseOverBound option the workerd collector sets; the native daemon keeps storing every export and holding its bound with its retention loop. The collector also sends every spelling of an ingest path down its bounded route. A store over its bound whose traces all still have running spans now evicts them, oldest first, rather than refuse every export forever. The pass's deletes from the operation and log body indexes no longer swallow errors: a failure fails the pass, and its transaction rolls back.
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.
Superseded by the smaller resend-only change in UsefulSoftwareCo/motel#18: three review rounds each found a new failure mode in the batch cap and admission check, which together amount to a redesign of the storage policy; kept open as a draft for Rhys to close.
Problem
A full store under steady export load fell behind. On Executor's self-host beta.40 at 2.4 MCP calls a second (about 250 spans a second), the bundled collector reached its 1 GiB bound after about 13 minutes. In the next 7 minutes the product logged 92
Telemetry export failedwarnings (tracestransportwithtimed_out,interrupted; app relaytimeout). The collector refused exports asqueueFull(4 of 4 pending) and asinvalidwith "Network connection lost", which is the product hanging up mid-export. An idle full store is fine; #14 fixed that.Cause
Two parts that feed each other. The timings come from an instrumented copy of this collector serving that product under load:
DELETE FROM span_attributes WHERE trace_id IN …alone took 2.7–3.5 s. The 500 ms pass budget is checked only between batches. Every export waited behind the batch, past the exporter's 5-second timeout.Fix
MOTEL_OTEL_RETENTION_SPAN_BATCHspans (default 1,000).ingestTracesandingestLogs, which every stored export passes through, whatever route or path spelling it came by. The typed routes also store from/v1/traces/,//v1/traces,/v1/Tracesand/v1/%74races; before, these skipped the collector's checks.MOTEL_OTEL_MAX_DB_SIZE_MBruns one eviction pass. If the store is still over, the export is refused: 429 withRetry-After: 1on the bounded ingest route, which every spelling of an ingest path now takes, counted asoverBoundinGET /api/ingest.What is and is not bounded. The budget is checked between batches, so a pass's batches take at most the budget plus one batch. Several parts are not bounded by time:
So a pass has no hard worst case under a 5-second exporter timeout. In practice the product's traces are far below the cap, and a full store under load holds almost no logs.
What the change looks like:
Files worth reading:
src/services/TelemetryStore.ts(cleanupExpired,makeRoom,ingestTraces) andsrc/workerd.ts(write,ingestPath).How it was tested
New tests, each red before its fix:
stores an identical resent export without rewriting its rowsmain(rows deleted and reinserted)holds its size bound under ingest faster than its capped eviction passes: 1 ms budget, 200-span batches, 100 sequential 400-span exports, asserts stored bytesrolls back a retention pass that fails part-way: injects a failure after a batch's deletes on the native store/v1/traces,/v1/Traces,/v1/%74racesand//v1/traces/770fe2c: 75,796,480 bytes against at most 5,242,880over its bound with only running traces evicts them rather than refuse every export770fe2c: no export acceptedfails and rolls back a retention pass whose search-index delete fails770fe2c: spans deleted, index rows keptdaemon.test.tsconfigured size retention evicts oldest logs firstfailed on the first fix commit, in CI. It passes now.bun run test: 225 pass, 0 fail, onb8993a4. The benchmark below ran on770fe2c; the changes since touch only over-bound exports, eviction of running traces and index-delete failures, none of which its runs reach.Replay benchmark: Motel only, with beta.40's workerd and bindings, on copies of one full 1 GiB store of real product traces. The exporter behaves like the product's: 1 s interval, 1,000-span batches, 5 s timeout, three resends, 60 s pause after a lost batch. Runs alternate between
mainand this branch on a shared WSL2 host whose load changed from run to run, so compare neighbours, not rows far apart. The replay sends no logs, and the store holds none.mainmainmain15d8fbd)Peak stored MiB is read from the store's header every 10 s, so it can lag the WAL; end is exact. Every run stayed within the 1 GiB bound, and this branch refused nothing as
overBound: at shipped defaults, with 1,000-span exports, eviction kept up. The new bounds test shows the case where it does not. The first pair's host load changed during the runs, so read it as indicative only. On a quiet host at 500 spans/s both versions keep up. At 1,000 spans/smainloses 39 exports, this branch 3. A lost export also stops the exporter for 60 s, so "spans stored" also reflects those pauses. The first-commit-only row stores more because it never refuses; the bounds test shows what that costs in a store with a 1 ms budget.Not done:
spans.rowid, with a one-off rebuild on existing stores.idx_span_attributes_trace_spanduplicates the primary key's prefix; dropping it would save a write per attribute row.