Repository navigation
perf(cohorts): attribute seeder scans and back off failed chunk reclaims - #91389
Conversation
|
😎 This pull request was merged. |
🤖 CI report
|
🦔 PostHog Review is reviewing this pull requestStep 5/6 · Validating findings · 7/12 Specialist review skills read the changed code in parallel each from their own perspective, a blind-spot sweep catches what they missed, and only validated findings are published back to this pull request. This comment updates as the review progresses. |
gustavohstrassburger
left a comment
There was a problem hiding this comment.
Looks solid. A few non-blocking suggestions inline; the metrics one is worth pairing with the charts changes you already listed.
| /// The two `seeder_reconcile_*` lag and age metrics take no entry on purpose: they are gauges, and | ||
| /// a bucket ladder would not apply to them. | ||
| fn configured_builder() -> Result<PrometheusBuilder, BuildError> { | ||
| let mut builder = PrometheusBuilder::new().set_buckets(DEFAULT_SECONDS_BUCKETS)?; |
There was a problem hiding this comment.
suggestion: configured_builder switches the exporter from rolling-window summaries to real bucketed histograms, and that changes the rendered shape of seven metrics master already exports. A panel querying seeder_chunk_scan_duration_seconds{quantile="0.99"} returns no data the moment this deploys, because the quantile series stops being emitted. histogram_quantile(0.99, rate(seeder_chunk_scan_duration_seconds_bucket[5m])) is what replaces it.
The full set that changes shape:
seeder_chunk_scan_duration_secondsandseeder_person_chunk_scan_duration_secondsseeder_person_planning_duration_secondsseeder_aggregate_entriesseeder_produce_ack_seconds,seeder_pacer_wait_seconds,seeder_reconcile_observation_pass_seconds
The last three take no HISTOGRAM_LADDERS entry, so they inherit DEFAULT_SECONDS_BUCKETS and flip too, even though nothing in the diff names them.
The description lists two charts changes this deploy needs, SEEDER_MAX_CHUNK_ATTEMPTS and the posthog_cohort_backfill_chunks_failed retarget, but not this one. A panel that goes blank reads as "the seeder stopped reporting" rather than "the metric changed shape". Worth a third line on the charts checklist naming these seven and the histogram_quantile rewrite, so whoever lands the charts PR fixes all three together.
|
This pull request was merged into |
Problem
An operator cannot tell what a cohort backfill scan cost. A slow scan cannot be traced back to a query in ClickHouse, nothing records how much data a chunk moved, and the seeder's latency quantiles read zero because they are rolling window summaries that drop a rare long scan.
Separately, a chunk that fails for a durable reason is claimed again on the next poll tick, fails the same way, and spends its whole attempt budget in minutes. The run then fails, and its cohort cannot start a new run until an operator intervenes.
Changes
Warning
This PR does not make the current backfill run faster. Everything here is measurement, and the retry backoff deliberately adds wall clock. The two changes that would speed a run up, more concurrency and more CPU, are chart config and are not in this diff. Deploying also rolls the singleton pod and loses the in-flight chunks' byte accumulators.
log_commentnaming the team, run, chunk, day, band and claim epoch.query_log_archiveextractsteam_id,product,featureandcohort_idfrom it, so seeder scans now archive under the team that caused them instead of under team 0.Scan semantics, tile wire format, claim ordering and the processor are unchanged.
The wait is a sum over attempts, not the cap
The drain for one durably failing chunk is the sum of its per-attempt ceilings, and the expected drain is half that, because the jitter draws uniformly from
[0, ceiling]. At base 30s and cap 1800s the doubling reaches the cap on the 7th attempt:The deployed
SEEDER_MAX_CHUNK_ATTEMPTSis far above the code default, so this needs a companion charts change lowering it before or with this deploy. A run holds its cohort's uniqueness slot for that whole window.Deploy ordering
Warning
Migration 0012 must land before the image.
claim_nextreadsnext_attempt_atunconditionally, so an image that rolls first makes every claim throwcolumn does not existand the live run stops claiming entirely. Thedb_defaulton the byte columns protects the other direction only.The
posthog_cohort_backfill_chunks_failedalert also needs retargeting in charts. A failed chunk now stays in that status for its whole wait, so an ordinary trickle of retryable failures holds the gauge above zero across the alert's 30-minute window.How did you test this code?
New tests and the regression each catches:
next_attempt_at. Confirmed to fail when the recovery path is changed to always ask for the first attempt's wait, which is the edit that would defeat the whole feature while every policy-level test stayed green.log_commentshape: pinned byte for byte, plus a check that every phase carries the four keysquery_log_archiveextracts. A dropped key archives the scan under team 0.mark_producedpersists both counts, and planning leaves them at the database-side default. The default is load-bearing, since planning inserts an explicit column list naming neither column.Not run: nothing in this diff was exercised against a real ClickHouse cluster. The
log_commentand byte counters are asserted against the types, not againstsystem.query_log.Docs update
No.