[backend-scheduler] redaction: caller-specified [start,end] time window - #7702
Open
zalegrala wants to merge 24 commits into
Open
[backend-scheduler] redaction: caller-specified [start,end] time window#7702zalegrala wants to merge 24 commits into
zalegrala wants to merge 24 commits into
Conversation
3 tasks
zalegrala
force-pushed
the
redaction-window
branch
from
August 11, 2026 16:09
2003d22 to
fc57b24
Compare
zalegrala
marked this pull request as ready for review
August 12, 2026 20:04
zalegrala
requested review from
carles-grafana,
electron0zero,
ie-pham,
javiermolinar,
knylander-grafana,
mapno,
mattdurham,
mdisibio,
ruslan-mikhailov,
stoewer,
yvrhdn and
zhxiaogg
as code owners
August 12, 2026 20:04
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends backend-scheduler redaction with an optional caller-specified absolute [start,end] time window (unix nanos), enabling large tenants to be redacted in time slices (reducing the duration compaction is held off) while also bounding the per-block scan range.
Changes:
- Add
start_time_unix_nano/end_time_unix_nanoto backend-scheduler redaction API + persisted batch/job details, and propagate them through scheduler → worker → storage. - Implement window-aware block selection/rescan filtering in the scheduler and window-bounded per-block fetch in
tempodbredaction for the query-selector path. - Add CLI flags (
tempo-cli redact --start/--end) with consistent resolution of relative times, plus documentation, tests, and changelog entries.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tempodb/tempodb.go | Extends RedactBlock signature and applies window bounds to query-based fetch requests. |
| tempodb/redaction.go | Introduces RedactionWindow type with validation and fetch-bound normalization. |
| tempodb/redaction_test.go | Adds window-focused storage-layer tests (selection correctness, one/two-sided bounds, guardrails). |
| pkg/tempopb/backendwork.proto | Adds window fields to SubmitRedactionRequest, RedactionBatch, and per-block RedactionDetail. |
| pkg/tempopb/backendwork.pb.go | Regenerates protobuf bindings for the new window fields. |
| modules/backendworker/backendworker.go | Reads window fields from jobs and passes them into store.RedactBlock. |
| modules/backendscheduler/redaction_window.go | Implements window-aware block selection helpers, request validation, and batch-load rejection for unusable windows. |
| modules/backendscheduler/redaction_window_test.go | Adds scheduler-level unit tests for window validation, selection, propagation, and rescan filtering. |
| modules/backendscheduler/redaction_integration_test.go | Updates redaction integration path to pass the window into RedactBlock. |
| modules/backendscheduler/redaction_covered_test.go | Adds tests for covered-range audit computations used for blast-radius reporting. |
| modules/backendscheduler/backendscheduler.go | Wires window validation, selection filtering, window propagation into jobs, and rescan filtering. |
| docs/sources/tempo/operations/tempo_cli.md | Documents new CLI flags and operational semantics/warnings for windowed redaction. |
| cmd/tempo-cli/shared.go | Adds parseTimeAt/parseRelativeTimeAt to resolve relative bounds against a single shared instant. |
| cmd/tempo-cli/cmd-redact.go | Adds --start/--end flags, resolves/validates a coherent window, and sets proto fields on submit. |
| cmd/tempo-cli/cmd-redact-window_test.go | Tests window-bound parsing (omitted/epoch/range guards) independent of RPC submission. |
| cmd/tempo-cli/cmd-redact_test.go | Extends validation tests and adds an end-to-end check that resolved nanos reach the request. |
| .chloggen/redaction-window.yaml | Changelog entry for user-facing windowed redaction feature. |
| .chloggen/redaction-block-window-signature.yaml | Changelog entry noting the breaking Compactor.RedactBlock signature change. |
Files not reviewed (1)
- pkg/tempopb/backendwork.pb.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zalegrala
force-pushed
the
redaction-window
branch
from
August 12, 2026 20:58
a489210 to
276ed03
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pkg/tempopb/backendwork.pb.go: Generated file
Suppressed comments (1)
modules/backendscheduler/backendscheduler.go:534
- MEDIUM:
filtered := metas[:0:0]forces capacity 0, so everyappend(filtered, meta)allocates a new backing array and copies the slice. For large tenants (the main target of windowed redactions), this adds avoidable CPU/GC overhead during submission.
Using an in-place filter (metas[:0]) is safe here because the range metas iteration only reads indices >= the current loop index, while append writes to indices < the current loop index when filtering, so it won’t corrupt iteration.
skippedJobSet := make(map[string]struct{})
filtered := metas[:0:0]
// Counted separately from the busy-block skip so the two reasons are never conflated in the logs.
Contributor
|
did you mean to have two changelogs? |
…the flags The window had unit tests for blockOverlapsWindow and the CLI time parser, but nothing asserted the three points where it takes effect. Each was verified by mutation to be silently unprotected: - block selection in SubmitRedaction: with the filter removed, every block gets a job and a request to redact one day becomes a whole-tenant redaction - window propagation in Next(): without it jobs carry 0/0 and each block is scanned in full - the scan bound in RedactBlock: without it, traces outside the window that satisfy the query are matched and dropped as well, which is over-deletion on a path with no recovery Each new test now fails when its line is reverted. Also adds a traceWithResourceAttrAtTime helper: MakeBatchWithAttributes leaves span timestamps at their defaults, so a fixture built on it cannot distinguish traces by time at all. Documents --start/--end and adds a time-window section covering the inclusive bounds, the any-part-of-the-trace matching rule, and the resolve-at-submit behaviour.
…e window
A one-sided window narrowed block selection while leaving the per-block scan unbounded: vparquet{3,4,5}
install the trace-time predicate only when both bounds are set, so setting one bound removed the filter
rather than half-applying it, and every query-matching trace in a selected block was deleted regardless
of its timestamp. The CLI documented one-sided windows as supported.
A negative bound was worse: block selection treats any non-zero value as a bound while the scan bound
treats non-positive as absent, so a bound that overflowed negative selected every block and then scanned
each unbounded — a typo resolving to a far-future date redacted the whole tenant.
Requiring both bounds, non-negative, and a non-empty range makes the unsafe shapes unrepresentable
rather than guarded, and keeps the scan bound always engaged.
The previous validation test passed for the wrong reason: its tenant ID came from the subtest name and
failed tenant validation first, so no window was ever evaluated.
… is set
vparquet{3,4,5} install the trace-time predicate only under `if start > 0 && end > 0`, so assigning one
field and leaving the other zero REMOVED the filter rather than narrowing it: the block was scanned in
full and every query match dropped regardless of timestamp. Confirmed by test on both sides.
SubmitRedaction now rejects one-sided windows, but that check runs at submission — a batch persisted by
an older scheduler, or any future caller, can still deliver one here. Materialising the open side (1ns /
MaxInt64) keeps the predicate installed whatever the source, so the bound cannot silently disappear.
Also drops two always-constant parameters from the window fixture that unparam flagged, and names it for
what it controls: the span timestamps, which MakeBatchWithAttributes gives no control over.
…nclusion
blockOverlapsWindow compared raw UnixNano values and treated only the window bounds as sentinels, so a
block whose recorded range is unset was silently dropped from every window with a lower bound:
time.Time{}.UnixNano() is a large negative number, not zero. That state is reachable — ObjectAdded skips
zero timestamps, so a block completed from a replayed WAL carries none, and the compactor propagates a
zero start into its output. The result was under-selection with no job, no log and no metric, which for a
deletion request means the data survives and the batch still reports success.
It also compared nanosecond bounds against second-granularity metadata. ObjectAdded builds the times from
uint32 epoch seconds, so the recorded range understates the real data by up to a second and a window
opening inside a block's final truncated second missed it. Padding outward matches the reasoning already
recorded in traceql.TrimToBlockOverlap.
Both now resolve toward inclusion: the per-block scan bound decides what is actually deleted, so an extra
block costs I/O while a missing one loses data. Indeterminate blocks are reported to the caller so they
can be counted rather than assumed silently.
Also corrects the CompactedTime rationale, which described rejecting a field that is not on BlockMeta at
all — it is a note about not consulting the compacted metas, not a choice available at this call site.
SubmitRedaction filters blocks by the window, but the rescan enqueues jobs for compaction OUTPUT blocks discovered later by ID, with no filter. A compaction merging an in-window input with an out-of-window one produces an output spanning both, so the rescan re-widened the redaction past what was requested — over exactly the data the submit-time filter had excluded. The rescan now looks the output metas up and applies the same test. A block not yet visible in the blocklist is treated as in scope, matching blockOverlapsWindow's resolve-doubt-toward-inclusion rule; the per-block scan bound still limits what is deleted.
…ally did skippedBlocks was 'blocks in active compaction' before the window filter shared the same loop, so a routine one-day slice of a 90-day tenant warned that nearly every block was mid-compaction, with skipped_compaction_jobs:0 contradicting it in the same line. That warning is the operator's signal for a real coverage gap on a destructive operation, so the two reasons are now counted and logged separately, and the warning only fires when something genuinely is busy. Blocks whose recorded range could not be judged are included (see blockOverlapsWindow) but now warn, since they were taken on trust and only the scan bound limits what they lose. A window overlapping no block is refused instead of creating an empty batch, which previously reported jobs_created:0 with a success status, gated the tenant's compaction for a quiescence cycle for no work, and rejected the corrected resubmission with AlreadyExists. An empty set with a non-empty skippedJobSet is left alone: every candidate is mid-compaction and the batch must exist for the rescan to reach them. The span and success log now carry the requested window alongside the covered range of the selected blocks. A bound far outside retention is a clumsy way to say 'everything', so the covered range is the honest description of the blast radius.
…elog
The docs advertised one-sided windows ('omit for no upper bound') and claimed each block is scanned only
over the requested range. Neither held: a one-sided window is now rejected, and the per-block scan bound
applies to --query only — with --trace-id the listed traces are removed from every selected block whatever
their timestamps, so a windowed --trace-id run is partial by design and needs the remaining slices.
Also documents what a single pass cannot cover, since this is how an operator confirms a compliance
deletion: blocks not yet visible at submit are excluded, and search results are cached, so re-running the
same search can list redacted traces until the entry expires. Verify by trace ID, by varying the time
range, or from the match count the redaction reports.
Adds the inter-slice wait (AlreadyExists during quiescence), the dry-run-first recommendation, and the
rollout caveat that an out-of-date worker ignores the window — which dry-run surfaces as an inflated count.
Options list gains --query and --dry-run, both of which shipped undocumented, and --trace-id is no longer
marked required since exactly one selector is.
Changelog splits in two: the window as an enhancement, and RedactBlock's signature change as breaking,
since Compactor is embedded in storage.Store and external mocks need updating.
…vacuously TestSubmitRedactionOnlySelectsBlocksInWindow slept 150ms against a 100ms blocklist poll and then asserted JobsCreated == 1. If the poll had picked up only the in-window block, that assertion held with the filter deleted — a false green on the most safety-critical line in the change. It now waits on all three blocks being visible, and asserts which block was selected rather than only how many, since a filter choosing the wrong single block also produces a count of one. The filter mutation is now killed on 5 of 5 runs. Renames TestRedactBlockWindowBoundsTheScan to name the two-sided case it actually covers, now that a one-sided sibling exists; the old name read as general proof and is what hid the one-sided bug. Drops a JobTimeout override from TestNextPropagatesWindowToJob that nothing in the test depends on and which implied a timing dependence it does not have.
Two adjacent int64 bounds transpose silently: an inverted window matches nothing, the job reports success, and the operator is told the block was processed when nothing was removed. RedactionWindow gives each bound a name at all six call sites. Pure refactor -- fetchBounds holds the same normalisation the call site did inline.
A transposed or zero-width window selects nothing. Scanning with it lets the job complete, report zero found, and advance the batch -- the operator is told the block was processed when nothing was removed. Under-deletion reported as success is the redaction failure with no external signal, so refuse the window instead. One-sided windows stay accepted: an older scheduler's batch can carry one, and fetchBounds materialises the open side to keep the scan bounded.
The one-sided window tests asserted only found==1 and TotalObjects==1. With two candidate traces that both satisfy the query and differ only in time, that is equally true of the intended victim and the intended survivor -- so transposing the two bounds inside fetchBounds left both subtests passing while deleting the opposite trace. Verified: the transposed-bounds mutant now fails all three window tests; before this it failed only the two-sided one.
Each bound took its own time.Now(), so --start now-7d --end now-7d resolved a few nanoseconds apart. That passes the client and server ordering checks and submits a window nothing can match: jobs are created, the batch reports success, and nothing is deleted -- the under-deletion-reported-as-success case the ordering check exists to prevent. parseTime gains an -At variant taking the instant; parseTime itself delegates, so the five query commands are unchanged. Also on this path, because they are the same resolution step: - Bound the resolved instant, not its year. UnixNano() is undefined past 2262-04-11T23:47:16Z, so a year check leaked ~8 months of 2262 (and all of 1678-1969), which wrap to negatives that preserve their order and pass the ordering check. - Refuse a bound resolving to exactly the epoch; 0 is the sentinel every layer reads as 'no bound', so a single-instant window there widened to the whole tenant. - Report which flag was supplied rather than inferring it from a resolved 0. - Cover the two assignments that put the window on the wire. Deleting them previously left every test passing while the CLI submitted an unbounded whole-tenant redaction.
…an honest
Three defects on the reporting and rescan paths, each with a test that fails without the fix.
Covered range understated the blast radius. time.Time{} was both the unseeded sentinel and a
legal StartTime -- and block selection deliberately includes blocks with unusable ranges, so
this was reachable by construction. Metas [1000s, zero, 9000s] reported a covered start of
02:30 against a true minimum of 00:16:40, and a trailing zero reported year 1. Extracted as
coveredRange so it is testable, and unusable ranges now contribute nothing; a range no block
could supply reports 'unknown' rather than year 1.
Rescan trusted the persisted window. The batch window comes off the manifest, not a
just-validated request. A negative bound makes blockOverlapsWindow report no overlap for
every real block, dropping every output block with only a debug log -- silent under-deletion.
Revalidate and fall back to unwindowed, which over-includes; the per-block scan bound still
decides what a query redaction deletes.
A dry-run with every in-window block busy built a batch that never scans. The zero-match
guard exempts deferred blocks so an apply batch survives for its rescan, but a dry-run arms
no rescan -- leaving zero jobs, compaction held off for a quiescence cycle, and AlreadyExists
on the retry.
Also here, same lines: hoist the block-meta index out of the generation loop and skip it
entirely for an unwindowed batch, flatten the classification chain onto continue, drop the
single-use rescanBlockInWindow wrapper, and stop claiming the rescan filter prevents
over-deletion -- an output merging an in-window input overlaps the window and is enqueued, so
the filter is an I/O saving.
…x the fixes A review of the previous batch found defects in the fixes themselves. Refuse a window alongside an explicit trace-ID list, at the CLI, at SubmitRedaction, and in RedactBlock. The ID path resolves traces with no time bound, so the window can only scope which BLOCKS are read: the listed traces were deleted from the overlapping blocks and left in the rest, under a SUCCEEDED status. Guarding at the storage layer too because that is where the deleting happens and RedactBlock is on an exported interface. The rescan's unusable-window fallback was inert and its log lied. It sanitized only a local copy; Next() still stamped the raw batch window onto every job, which RedactBlock then rejected -- so every block the fallback over-included hard-failed while the warn reported 'treating the batch as unwindowed'. Worse, the fallback pointed the wrong way: with no valid scan bound, unwindowed means deleting every query match regardless of time. Validate once at the load boundary instead and discard a batch whose persisted window is unusable, which destroys nothing and says so. A dry-run refused for all-busy blocks now reports FailedPrecondition 'being compacted' rather than NotFound 'no blocks overlap' -- the blocks did overlap, and the old message sent the operator to widen the window when the fix is to retry. coveredRange discarded the usable half of a half-zero meta, understating the very blast radius it exists to report; the bounds now accumulate independently. An inverted meta range contributes neither. Tests, each verified by re-running the mutation that survived before it: - fetchBounds directly. Deleting its unbounded early-return installs the trace-time predicate on every whole-tenant redaction, silently sparing traces with a zero recorded time -- and the whole package stayed green. - a dry-run arms no rescan, the premise the zero-match guard rests on. - an exact 24h window width, since any tolerance passed under per-bound time.Now(). - deleted TestWindowBoundNanoResolvesAgainstOneInstant: it asserted only that a pure function is pure, and mutation showed it blind to the regression it claimed to guard. Also: RedactBlock and RedactionWindow now document overlap-not-containment and the query-only scope; resolveWindow no longer leaves a half-resolved window on the receiver; the range error prints RFC3339Nano so the stated ceiling matches what is accepted; and the metaByID hoist comment no longer claims the blocklist is immutable within a tick.
… commentary No behaviour change. This is a subtractive pass to make the diff reviewable. SubmitRedaction's validation moves to validateRedactionRequest, taking the function from cyclomatic complexity 40 back to 27. Three reviews flagged it and I deferred it each time as 'structure, not correctness'; deferring is what let each subsequent fix add another inline guard to it. Comment density in production code drops from 0.82 lines per line of code to 0.51. The rationale blocks had grown into essays because every review finding felt like it needed its reasoning preserved in place. Each now states what the code does and how it fails, not the full history of why. Docs corrections, all claims that were false rather than merely verbose: - the one-sided rejection is policy, not a storage limit; fetchBounds does materialise an open side, so the old justification described a hazard the code does not have - a windowed --trace-id run is refused now, not 'partial by design' - the rescan does enqueue compaction output discovered after submission, so 'only blocks visible when you submit' overstated how bounded a run is -- in a cannot-be-undone note - --dry-run does not print a match count; it names the metric instead - the inserted window prose had terminated the options list, detaching the --tls options - the version-skew hazard is now a warning admonition stating the outcome plainly, since documentation is the only mitigation for it
The warning belaboured a tautology: a worker that does not know about the window does not respect the window. Says that once, in the house style of the file's other admonitions, and keeps the part that is not obvious -- the job still reports success, so there is no signal afterwards. Also softens survivor/victim framing in a test comment.
…elative to The submission record carried jobs_created, blocks_skipped_compacting and blocks_out_of_window but not the denominator, so jobs_created could not be reconciled against anything. Reconciling a live znet run took a metric query, two log queries and a read of the poller to establish that the missing blocks were neither skipped nor out-of-window -- the scheduler had simply seen fewer. tempodb_blocklist_length is not that denominator: it is written only at poll completion, while compaction mutates the live blocklist in between, so it can disagree with what a submission read seconds later. Capture the count at selection time and report it. Also stops re-deriving the total by summing two counters after metas has been reassigned to the filtered set.
Both from Copilot review, both real.
Validate now judges the MATERIALISED range rather than the raw fields. A one-sided window is
pinned before the scan, so {EndNano: 1} resolves to [1,1] and {StartNano: MaxInt64} to
[max,max]: ordered raw fields, but neither can install a trace-time predicate. fetchBounds
reported ok=false for those, so redactionIDsFromQuery installed no predicate and scanned the
block in full -- dropping every query match whatever its timestamp. The guard that produced
that ok=false was added last round to prevent silent under-deletion, and failed open instead.
The materialisation is now one function both methods share, so they cannot disagree about
which windows are usable, and fetchBounds has no fail-open branch at all: for a validated
non-zero window it always installs bounds. TestRedactionWindowValidateImpliesBoundedScan
pins that link, since it is the invariant the safety argument rests on.
blockOverlapsWindow now treats an inverted recorded range as indeterminate. Only zero times
were checked, so StartTime after EndTime fell through to the padded comparisons and could
EXCLUDE the block -- silent under-deletion, against the resolve-doubt-toward-inclusion rule
this function documents. coveredRange already handled inverted ranges; the two are now
consistent.
Neither is reachable through the CLI or SubmitRedaction, which reject one-sided windows. Both
are reachable through the exported Compactor interface and through a persisted batch, which
is the layer these guards exist for.
The signature description was still accurate; the error sentence had gone stale.
It said RedactBlock errors on 'negative bounds, or a start at or after the end'. Validate now
judges the RESOLVED range, so {EndNano: 1} is refused even though its raw start (0) precedes
its end (1). An external implementer following the old wording would check the raw fields and
reintroduce the fail-open hole that check exists to close: such a window installs no time
predicate, so the block is scanned in full and every query match dropped.
It also omitted an error case entirely -- a window alongside an explicit traceIDs list is now
refused, where the natural assumption would be that the window is simply ignored.
zalegrala
force-pushed
the
redaction-window
branch
from
August 13, 2026 13:39
276ed03 to
db212d5
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pkg/tempopb/backendwork.pb.go: Generated file
Suppressed comments (1)
modules/backendscheduler/backendscheduler.go:552
- MEDIUM:
indeterminateBlocksis incremented before the busy-block skip, so the subsequent log message ("redaction included blocks...") and theindeterminate_blocksspan attribute can count blocks that were actually skipped due to active compaction. That makes the audit counts inconsistent with the comment “Record what will actually be touched” (sincemetasis later reassigned to the filtered/enqueued set).
Could we either (a) count only indeterminate blocks that are actually enqueued, or (b) keep two separate counters (in-window vs enqueued) and label them accordingly in logs/attributes? The minimal fix below implements (a).
// on the block's real StartTime/EndTime — see blockOverlapsWindow on why not CompactedTime.
overlaps, indeterminate := blockOverlapsWindow(meta, req.StartTimeUnixNano, req.EndTimeUnixNano)
if indeterminate {
indeterminateBlocks++
}
There are no implementations of tempodb.Compactor anywhere -- no mocks, no test fakes -- and one caller of RedactBlock. storage.Store embeds the interface and blockbuilder consumes it; neither implements it. Tempo's breaking section is operator-facing: all 86 historical entries are config keys, CLI flags, defaults, removed receivers, or a wire incompatibility between components on rollout. None is a Go signature change. Extending TraceRedactor -- an interface actually designed for outside implementations -- was logged as a FEATURE, so a change to one with no implementers is comfortably below that bar. Left in, it would render at the top of the changelog under 'Breaking changes' beside entries that genuinely require action on upgrade, sending operators looking for work that does not exist. The enhancement entry already states what they need: both bounds required, the --trace-id exclusion, and the version-skew hazard. The Go-level error semantics live in the RedactBlock and RedactionWindow godoc, where an implementer would look.
Contributor
Author
|
Thanks @ie-pham. I've dropped on of the changelogs, which was added for an intermediate change which the final version doesn't carry. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pkg/tempopb/backendwork.pb.go: Generated file
Suppressed comments (1)
modules/backendscheduler/backendscheduler.go:553
- MEDIUM:
indeterminateBlocksis incremented before the busy-block check, but the warning below says these blocks were "included, not skipped". If an indeterminate block is currently in a compaction job, it will be skipped (deferred) yet still counted/logged as included, which can mislead operators and makes theindeterminate_blocksspan attribute ambiguous. Consider counting indeterminate blocks only after they’ve passed the busy-block filter (i.e., blocks that will actually get redaction jobs).
overlaps, indeterminate := blockOverlapsWindow(meta, req.StartTimeUnixNano, req.EndTimeUnixNano)
if indeterminate {
indeterminateBlocks++
}
…nqueued The counter was incremented at the overlap test, before the busy-block skip, so a block with an unusable range that was also mid-compaction was counted and then deferred. The warning says those blocks were 'included, not skipped', and the indeterminate_blocks span attribute is meant to line up with the jobs created -- both were wrong for that block. Counted after the busy check instead, so it means 'enqueued on trust'. Nothing goes unaccounted: an indeterminate block that is deferred is still reported by the busy-block warning, and blockOverlapsWindow returns overlaps=true whenever indeterminate is set, so the out-of-window branch could never have dropped one. Raised by Copilot, and by a local review pass that had it as a low-severity note I did not act on at the time.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pkg/tempopb/backendwork.pb.go: Generated file
Suppressed comments (1)
modules/backendscheduler/redaction_window.go:155
- MEDIUM: The comment says
start == endmatches nothing, but the underlying time-range predicate is inclusive and checks span overlap (span start <= end && span end >= start), so a zero-width window can still match traces spanning that instant. Consider rewording the comment to justify the strictstart < endrequirement as a safety/policy choice (e.g., likely operator typo) rather than claiming it can’t match anything.
// start == end matches only traces spanning that exact instant, i.e. nothing, while reporting success.
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 an optional
[start, end]time window to backend-scheduler redaction, so a large tenant can be redacted in time slices rather than one all-blocks batch. A redaction holds the tenant's compaction off for its whole run, so a single pass over every block keeps compaction paused long enough for the block list to grow. Slicing lets compaction recover between passes.FetchSpansRequest.Start/EndTimeUnixNanos), so each selected block is read only over the requested range.tempo-cli redact --start/--endtakesnow,now-7d, or RFC3339.0/0(the default) is the whole tenant, unchanged.Both bounds are inclusive, and a trace is redacted if any part of it overlaps the window — a trace starting before the window and ending inside it is removed in full. A window cannot be combined with
--trace-id: that path resolves traces with no time bound, so the pair would remove each listed trace only from the blocks that happen to overlap and leave the rest in place while reporting success.Warning
A worker that predates these fields does not respect the window, and the job still reports success — so wait until the cell's workers are on a version that supports it before submitting a windowed redaction. Unwindowed redactions are unaffected. Capability gating is tracked separately.
Reading order — the diff is 2228 lines but only ~300 are production logic; tests are 58% and
backendwork.pb.gois generated:tempodb/redaction.go— theRedactionWindowtype, its validation, and how it resolves to fetch bounds. Start here; everything else consumes it.tempodb/tempodb.go—RedactBlock: where the window bounds the scan, and where it is refused.modules/backendscheduler/redaction_window.go— block selection (blockOverlapsWindow), request validation, and the covered-range audit record.modules/backendscheduler/backendscheduler.go—SubmitRedactionfiltering and the rescan.cmd/tempo-cli/cmd-redact.go— flag resolution.Why not
CompactedTime: selection keys strictly on the block's data range (StartTime/EndTime). The poller fudgesCompactedTimeto "now" at compaction discovery to save a backend read, so keying on it could under-select and silently skip an in-window block — leaving data the tenant asked to delete in place.Two deliberate asymmetries, both because under-deletion is the failure with no external signal:
SubmitRedactionrejects one. vparquet installs its trace-time predicate only when both bounds are non-zero, so a half-set window would remove the filter rather than narrow it; requiring both at the API edge is policy, and materialising at the storage layer is the backstop.Coverage: every place the window takes effect has a test verified to fail when that line is reverted — selection, propagation in
Next(), the scan bound, the rescan filter, and the CLI's two wire assignments. Assertions are on which trace remains, not how many: with two candidates differing only in time, a count of one holds whichever of them was removed.Follows the merged query selector (#7663), quiescence (#7695), metric (#7699), dry-run lifecycle (#7700), and in-flight accounting (#7703). Takes
RedactionBatchfields 10/11; the parked cancel PR (#7701) renumbers around it.Which issue(s) this PR fixes:
N/A (tracked internally).
Checklist
.chloggen/