feat(web-analytics): cache warmer demand selection to widen the lookback window - #73493
Conversation
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/web_analytics/dags/cache_warming.py:372-380
**Keep payload validation fail-open**
A decodable cache object with an unexpected structure or field type raises during the unchecked field access or timestamp arithmetic, so the selection op fails instead of falling back to a fresh scan and the hourly warming run is skipped.
Reviews (1): Last reviewed commit: "chore(ci): cover posthog/storage in dags..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91221a6a29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🤖 CI report |
|
Note 🤖 stamphog reviewed Gates denied this PR (CI workflow path edit hit the infra/CI deny-list, and size/cross-team scope classified it T2-never), which routes to human review regardless of my own read; the change also alters fleet-wide ClickHouse scan cost/behavior and constance settings, which is risky territory best confirmed by a human even though prior agent review comments were addressed.
Gate mechanics and policy version
|
3 participants
Problem
The web analytics cache warmer picks "hot" query shapes to keep precomputed by scanning
system.query_logover a lookback window. That window was 2 days, which only catches teams that use web analytics daily. Demand analysis shows most teams return every few days to weekly: widening the window to 14 days nearly triples the teams with a warmable shape (≈8.3k → ≈24k), and team acquisition is still climbing at the 14-day edge.The window couldn't just be bumped. The selection scans terabytes of
log_commentfleet-wide and runs hourly, so a 14-day window would multiply that cost ~7× and exceed the default read cap (the query would fail, not just cost more).Changes
WEB_ANALYTICS_WARMING_SELECTION_TTL_SECONDS, default 6h). The hourly warmer replays the cached list; the expensive scan only re-runs when the cache expires — roughly 4×/day instead of 24×. Fails open on Redis errors (degrades to a fresh scan).WEB_ANALYTICS_WARMING_DAYSdefault2→14. The scan's read cap is lifted (max_bytes_to_read=0, bounded bymax_execution_time) since 14 days exceeds the 15 TiB default.normalizedQueryHash(query), which read the entirequerySQL-text column (~550 GiB compressed fleet-wide) purely for a logging id. The id is now derived from the JSON already read out oflog_comment. This also removes a per-row SQL-normalization CPU cost across ~600M rows — most of the speedup.PREWHEREthe cheap native columns (type,is_initial_query,query_kind = 'Select') so the biglog_commentstring is read only for surviving rows.is_initial_queryalone prunes ~9/10 of the window;query_kind = 'Select'also excludes the warmer's own INSERT replays without a JSON parse.Realizing the broader coverage also needs
WEB_ANALYTICS_WARMING_MAX_SHAPES(currently 20,000) raised, since selection isLIMIT max_shapesordered by demand and weekly shapes rank below the cut. That's a monitored runtime ramp, deliberately not part of this PR.How did you test this code?
Automated (
products/web_analytics/dags/tests/test_cache_warming.py, 35 passing):test_second_run_reuses_cached_selection— guards the whole point of the cache: a second warming run must not re-run the terabyte scan. Fails if the cache read regresses.test_redis_failure_falls_back_to_scan— Redis being down degrades to a fresh scan instead of breaking warming.test_op_reads_instance_settingsto force a cache miss so it stays deterministic regardless of Redis state.test_selection_sql_survives_driver_percent_formattingstill guards%-escaping (it caught a literal%in a new comment during development).Also benchmarked the new vs old selection query directly against prod
query_log(2-day window): 2.2× faster wall time, −24% peak memory, identical 2,000-shape result. No manual UI testing.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code. Skills invoked:
/writing-tests.The decoupling was implemented as a cache-with-TTL inside the selection op rather than a second Dagster schedule, so it self-heals: a cache miss just recomputes, with no ordering dependency between a selection job and the warming job. The query-cost work was driven by measuring in prod —
query_logcolumn sizes (queryis the largest column afterlog_comment) and prefilter selectivity (is_initial_queryprunes ~90% of rows) — which is what pointed at dropping thequery-column read plus a PREWHERE as the win.