feat(dashboards): parallelize dashboard tile serialization behind a flag - #69660
feat(dashboards): parallelize dashboard tile serialization behind a flag#69660pauldambra wants to merge 20 commits into
Conversation
Dashboard GETs serialized tiles sequentially, so with force refresh each tile's insight computation blocked the next one. Dispatch tile serialization across a shared ThreadPoolExecutor (gated by the parallel_dashboard_tile_serialization flag, sequential fallback kept), propagate the task-chain context and contextvars into workers, and clean up DB connections in the long-lived pool. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
|
Hey @pauldambra! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
|
pauldambra
left a comment
There was a problem hiding this comment.
QA Swarm review complete. See inline comments.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: qa-team (specialists + generalists), paul-reviewer, xp-reviewer, security-audit Verdict:
|
| Reviewer | Assessment |
|---|---|
| 🔍 qa-team | HIGH risk; sound design intent, but error paths break the task-chain hand-back and the pool has no timeout/fairness. |
| 👤 paul | solid careful work; fix the drain-on-error path before shipping, add trace attributes so the flag flip is measurable. |
| 📐 xp | hard parts handled with well-explained comments; main reservation is the parallel path ships with zero end-to-end coverage. |
| 🛡 security-audit | No injection/auth/IDOR; one low-severity concurrency defect (same leak) — impact bounded to same-tenant task mis-scheduling. |
Automated by QA Swarm — not a human review
…onnections inline Pool threads are reused across requests, so a queued chained-refresh task must always be drained on exit instead of only on the success path. close_old_connections() should only run for actual pool workers, not the inline (flag-off/test) path which already shares the request's connection. Also add span attributes so we can measure whether the parallel path is actually taken and how many tiles it serialized. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
Inline path calls serialize_tile_with_context directly (worker wrapper is pool-only now), drop the redundant exit-side close_old_connections, and dedupe the org feature-flag lookups into a local helper. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
pauldambra
left a comment
There was a problem hiding this comment.
QA Swarm round 2 (fix-delta review) complete. See inline comments.
Fix mypy-breaking Optional[bool] return in _org_flag_enabled, restore exit-side close_old_connections() in serialize_tile_in_worker so pool threads don't hold idle DB connections, harden the drain/set_in_context ordering so a raising drain still clears the in_context flag, and add a regression test pinning the chain-drain-on-raise behavior. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
Early-return on empty dashboards before flag lookups, and hoist the request-thread chain lookup out of the tile-results loop. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
…lization Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
|
Note 🤖 stamphog reviewed This is a concurrency change to shared, process-wide dashboard tile serialization (thread pool, DB connection lifecycle, task-chain handoff, shared/anonymous dashboard rendering) — squarely risky territory — and an independent bot reviewer (veria-ai) has an unresolved current-head concern that an anonymous/shared-dashboard visitor could bypass the task-chain serialization and fan out background refresh jobs when the organization flag is off. That concern hasn't been addressed on the current head, so it blocks approval regardless of the extensive self-authored "QA Swarm" commentary, which is posted by the PR author's own account and doesn't count as independent assurance.
Gate mechanics and policy version
Updated in place — this replaces 6 earlier stamphog review(s) on this PR. |
Generated-By: PostHog Code Task-Id: 30411b8b-8f92-4c1b-976d-1b16e302756a
Address the deferred review findings: bound the wait on tile futures with a configurable timeout and cancel unstarted work, keep completed tiles' chained refresh tasks when a sibling fails, fail the query statuses of tasks orphaned by a worker error so pollers don't hang until the Redis TTL, pre-warm UserPermissions caches before fan-out so workers never race the lazy population, drop the pool default from 20 to an env-tunable 8, and make the test gate overridable so the parallel path can be exercised in tests. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/dashboards/backend/api/dashboard.py |
65.7% | 474, 476, 495, 498–499, 516–517, 519–520, 523, 2321, 2325–2326, 2334–2342, 2349–2354, 2364–2365, 2367–2368, 2372–2377, 2380–2382, 2384–2387, 2390–2391 |
posthog/clickhouse/client/async_task_chain.py |
80.0% | 87 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 30435271124 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
batch_exports |
████████░░░░░░░░░░░░ 39.6% |
8,602 / 21,741 |
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
warehouse_sources_queue |
████████████░░░░░░░░ 59.2% |
148 / 250 |
tasks |
██████████████░░░░░░ 69.6% |
31,303 / 44,945 |
data_tools |
██████████████░░░░░░ 70.0% |
63 / 90 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
cdp |
████████████████░░░░ 81.0% |
3,146 / 3,883 |
signals |
████████████████░░░░ 81.2% |
23,852 / 29,387 |
wizard |
█████████████████░░░ 84.8% |
935 / 1,102 |
data_modeling |
█████████████████░░░ 85.3% |
7,343 / 8,611 |
notebooks |
█████████████████░░░ 85.7% |
7,525 / 8,785 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
cohorts |
█████████████████░░░ 86.9% |
5,648 / 6,496 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
exports |
██████████████████░░ 88.4% |
6,950 / 7,862 |
data_warehouse |
██████████████████░░ 88.7% |
12,196 / 13,757 |
business_knowledge |
██████████████████░░ 89.0% |
4,391 / 4,936 |
dashboards |
██████████████████░░ 89.4% |
6,143 / 6,875 |
engineering_analytics |
██████████████████░░ 89.4% |
6,441 / 7,202 |
visual_review |
██████████████████░░ 89.5% |
5,837 / 6,522 |
conversations |
██████████████████░░ 89.5% |
17,117 / 19,124 |
alerts |
██████████████████░░ 90.0% |
4,342 / 4,827 |
mcp_analytics |
██████████████████░░ 90.2% |
2,883 / 3,198 |
links |
██████████████████░░ 90.6% |
183 / 202 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
slack_app |
██████████████████░░ 91.0% |
9,533 / 10,472 |
marketing_analytics |
██████████████████░░ 91.1% |
11,905 / 13,074 |
error_tracking |
██████████████████░░ 91.1% |
10,931 / 12,004 |
stamphog |
██████████████████░░ 91.1% |
4,056 / 4,450 |
mcp_store |
██████████████████░░ 92.3% |
4,279 / 4,634 |
product_analytics |
███████████████████░ 92.5% |
5,852 / 6,324 |
managed_migrations |
███████████████████░ 92.6% |
1,556 / 1,681 |
early_access_features |
███████████████████░ 92.6% |
1,287 / 1,390 |
notifications |
███████████████████░ 92.6% |
1,017 / 1,098 |
ai_observability |
███████████████████░ 93.0% |
15,354 / 16,517 |
surveys |
███████████████████░ 93.1% |
5,771 / 6,197 |
web_analytics |
███████████████████░ 93.2% |
14,829 / 15,906 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
approvals |
███████████████████░ 93.3% |
3,437 / 3,682 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
legal_documents |
███████████████████░ 93.8% |
1,628 / 1,736 |
workflows |
███████████████████░ 93.9% |
6,919 / 7,372 |
endpoints |
███████████████████░ 94.2% |
8,655 / 9,192 |
tracing |
███████████████████░ 94.5% |
2,670 / 2,826 |
review_hog |
███████████████████░ 94.6% |
6,912 / 7,303 |
messaging |
███████████████████░ 94.7% |
2,885 / 3,048 |
skills |
███████████████████░ 95.0% |
3,169 / 3,337 |
logs |
███████████████████░ 95.5% |
10,435 / 10,928 |
experiments |
███████████████████░ 95.5% |
25,446 / 26,637 |
growth |
███████████████████░ 96.1% |
3,245 / 3,376 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
replay_vision |
███████████████████░ 96.4% |
15,773 / 16,367 |
feature_flags |
███████████████████░ 96.4% |
17,371 / 18,023 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
access_control |
███████████████████░ 96.9% |
870 / 898 |
customer_analytics |
███████████████████░ 97.1% |
9,798 / 10,086 |
warehouse_sources |
███████████████████░ 97.2% |
346,709 / 356,568 |
data_catalog |
████████████████████ 97.7% |
2,555 / 2,615 |
analytics_platform |
████████████████████ 98.0% |
2,153 / 2,197 |
metrics |
████████████████████ 98.2% |
2,491 / 2,536 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
✅ Django migration risk — migration analysis complete
We've analyzed your migrations for potential risks.
Summary: 9 Safe | 4 Needs Review | 0 Blocked
⚠️ Needs Review
May have performance impact
agent_platform.0002_alter_agentapplication_slug
└─ #1 ⚠️ AlterField
Field alteration may cause table locks or data loss (check if changing type or constraints)
model: agentapplication, field: slug, field_type: SlugField
agent_platform.0003_agentapplication_global_slug_unique
└─ #1 ⚠️ RemoveConstraint: Unscored Django operation: RemoveConstraint (needs manual review)
└─ #2 ⚠️ AddConstraint
Adding constraint may lock table (use NOT VALID pattern)
model: agentapplication
agent_platform.0005_copy_encrypted_env_to_revisions
└─ #1 ⚠️ RunPython: RunPython data migration needs review for performance
agent_platform.0011_agentrevision_skill_refs
└─ #1 ⚠️ AddField
Adding NOT NULL field with callable default (list) - verify it's stable
model: agentrevision, field: skill_refs, default: list
✅ Safe
Brief or no lock, backwards compatible
agent_platform.0001_initial
└─ #1 ✅ CreateModel
Creating new table is safe
model: AgentApplication
└─ #2 ✅ CreateModel
Creating new table is safe
model: AgentRevision
└─ #3 ✅ AddField
Adding nullable field requires brief lock
model: agentapplication, field: live_revision
└─ #4 ✅ CreateModel
Creating new table is safe
model: AgentSandboxInstance
└─ #5 ✅ CreateModel
Creating new table is safe
model: AgentSession
└─ #6 ✅ CreateModel
Creating new table is safe
model: AgentSessionCredential
└─ #7 ✅ CreateModel
Creating new table is safe
model: AgentToolApprovalRequest
└─ #8 ✅ CreateModel
Creating new table is safe
model: AgentUser
│
└──> ℹ️ INFO:
ℹ️ Skipped operations on newly created tables (empty tables
don't cause lock contention).
agent_platform.0004_agentrevision_encrypted_env
└─ #1 ✅ AddField
Adding nullable field requires brief lock
model: agentrevision, field: encrypted_env
agent_platform.0006_remove_agentapplication_encrypted_env
└─ #1 ✅ SeparateDatabaseAndState: Only state operations (no database changes)
agent_platform.0007_agent_identity_linking
└─ #1 ✅ CreateModel
Creating new table is safe
model: AgentIdentityCredential
└─ #2 ✅ CreateModel
Creating new table is safe
model: AgentIdentityLinkState
│
└──> ℹ️ INFO:
ℹ️ Skipped operations on newly created tables (empty tables
don't cause lock contention).
agent_platform.0008_agent_identity_subject
└─ #1 ✅ SeparateDatabaseAndState: Only state operations (no database changes)
└─ #2 ✅ AddField
Adding nullable field requires brief lock
model: agentidentitycredential, field: subject
agent_platform.0009_agentsession_agenttoolapprovalrequest_is_preview
└─ #1 ✅ AddField
Adding NOT NULL field with constant default (safe in PG11+)
model: agentsession, field: is_preview
└─ #2 ✅ AddField
Adding NOT NULL field with constant default (safe in PG11+)
model: agenttoolapprovalrequest, field: is_preview
agent_platform.0010_remove_is_preview_state_only
└─ #1 ✅ SeparateDatabaseAndState: Only state operations (no database changes)
agent_platform.0012_agentsession_search_text_turn_count
└─ #1 ✅ AddField
Adding nullable field requires brief lock
model: agentsession, field: search_text
└─ #2 ✅ AddField
Adding NOT NULL field with constant default (safe in PG11+)
model: agentsession, field: turn_count
agent_platform.0013_agent_transport_binding
└─ #1 ✅ CreateModel
Creating new table is safe
model: AgentTransportBinding
│
└──> ℹ️ INFO:
ℹ️ Skipped operations on newly created tables (empty tables
don't cause lock contention).
📚 How to Deploy These Changes Safely
AddConstraint:
Add constraints in 2 phases without locking, using the PostHog helpers:
-
AddConstraintNotValid (instant, validates new rows only, no table scan)
-
ValidateConstraint in a separate migration (scans table with non-blocking lock)
from posthog.migration_helpers import AddConstraintNotValid, ValidateConstraint
See the migration safety guide
AddField:
This operation acquires a brief lock but doesn't rewrite the table.
Deployment uses lock timeouts with automatic retries, so lock contention will cause retries rather than connection pile-up.
RunPython:
Use batching for large data migrations:
- Use
.iterator()to avoid loading all rows into memory - Use
.bulk_update()instead of saving individual objects - Batch size: 1,000-10,000 rows per batch
- Add pauses between batches
- Consider background jobs for very large updates (millions of rows)
See the migration safety guide
Last updated: 2026-07-29 08:27 UTC (75da055)
✅ ClickHouse migration SQL — none
No ClickHouse migrations in the latest push.
…e hot path Production traces show two posthog_dashboardtile SELECTs per tile on every dashboard render (the nested InsightSerializer's dashboard_tiles field and dashboards field each evaluate insight.dashboard_tiles.all() unprefetched) — add the prefetch to get_tiles. Also add spans around InsightSerializer.to_representation, insight_result, and the dashboard filter/variable override application, so the per-tile Python self-time visible in traces gets attributed to a concrete block instead of reading as an uninstrumented gap. Note: dashboard query-count assertions (assertNumQueries / snapshot_postgres_queries) may shift by the prefetch; no local DB available to retune them, so CI output will show the exact new counts. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
pauldambra
left a comment
There was a problem hiding this comment.
QA Swarm round 3 (safety-rails + prefetch delta review) complete. See inline comments.
- skip UserPermissions pre-warm on shared/embedded dashboard renders, which build permissions with AnonymousUser and would crash the org-membership lookup - close pool-worker DB connections even when tests opt into parallel serialization via DASHBOARD_TILE_PARALLEL_IN_TESTS - clamp DASHBOARD_TILE_SERIALIZE_CONCURRENCY and DASHBOARD_TILE_SERIALIZE_TIMEOUT_SECONDS env knobs to safe minimums - prefer a real tile exception over the synthetic timeout error when both occur, and document the wait-to-slowest-tile behavior - narrow the pre-warm comment to what's actually warmed (UserPermissions, not UserAccessControl) Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
Dedupe the pool-active-under-tests gate into _tile_serialize_pool_active, reuse drain_task_chain inside execute_task_chain, and replace the tuple-padding comprehension on the inline path with an explicit loop. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
The helper means "pool usable in this environment" (always true outside tests, independent of the org flag), not "parallel is on" — rename so a future caller doesn't treat it as a runtime check. Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
…lization Generated-By: PostHog Code Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
The lambda-based list comprehension left mypy unable to infer the futures' element type. Use a typed helper function matching the other tests in the file so the future list type is inferable. Generated-By: PostHog Code Task-Id: 7a3e98ff-bdea-439d-9594-c8c80153a499
PR overviewThis pull request adds a feature-flagged path to parallelize dashboard tile serialization in the dashboards backend, including shared dashboard serialization handled by the dashboard API. One security issue remains open around shared dashboard requests: the worker path can lose the active task-chain context and schedule stale tile refreshes immediately when the organization flag is disabled. That means an anonymous shared-dashboard visitor could fan out background refresh jobs that were previously serialized, increasing load on the job system. One issue has already been addressed, but this remaining concurrency behavior should be fixed before the posture is considered clean. Open issues (1)
Fixed/addressed: 1 · PR risk: 6/10 |
… quota
The insight__dashboard_tiles Prefetch with a custom queryset in get_tiles conflicted with the viewset queryset's existing insight__dashboard_tiles__dashboard string lookup carried along by dashboard.tiles.all(), making every dashboard retrieve with insight tiles 500 ("lookup was already seen with a different queryset"). Use the identical string lookup instead: it dedupes on the retrieve path and still adds the prefetch on sharing/streaming renders.
Also add a per-team quota on the shared tile-serialization pool so one team cannot occupy every worker and starve other teams' dashboards on the same process. Tiles over the quota serialize inline on the request thread; slots release via future done callbacks (which fire on cancellation too).
Generated-By: PostHog Code
Task-Id: 38eb786d-b3e3-4cd5-940e-9bdf3a4aec0f
mypy could not infer the type of the done-callback lambda because of its defaulted parameter. team.id is constant across the loop, so hoist it to a local and use a plain single-parameter lambda that mypy infers from add_done_callback's expected type. Generated-By: PostHog Code Task-Id: 3b2ec9cb-241a-4763-aa22-c597345d8df4
# Conflicts: # products/dashboards/backend/api/dashboard.py
# Conflicts: # products/dashboards/backend/api/dashboard.py
| tile, | ||
| order, | ||
| self.context, | ||
| chained_tile_refresh_enabled, |
There was a problem hiding this comment.
Medium: Shared refresh jobs bypass the active task chain
The sharing endpoint unconditionally wraps dashboard serialization in task_chain_context(), but this argument is false whenever the organization flag is disabled. Pool workers then schedule each stale shared-tile refresh immediately, so an anonymous visitor can fan out refresh jobs that were previously serialized. Capture whether the request thread is already in a task-chain context before submitting work and propagate that state to the workers, rather than relying only on the feature flag.
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Problem
The dashboard endpoint serializes tiles one at a time in
DashboardSerializer.get_tiles.Each tile's serialization can trigger a full insight computation (notably with
refreshforcing a cache bypass), so a dashboard with N tiles pays N sequential ClickHouse round-trips.Wall-clock grows linearly with tile count, and busy dashboards get slow exactly when users force a refresh.
Changes
DashboardSerializer.get_tilesnow dispatches tile serialization across a sharedThreadPoolExecutor(20 workers), so per-tile insight computation overlaps instead of running back to back. In a synthetic model (20 tiles, 50ms per tile) this is ~1000ms → ~51ms.parallel_dashboard_tile_serializationfeature flag (org-scoped, same shape as the adjacentchained_dashboard_tile_refreshflag). Flag off keeps today's sequential behavior, so this can roll out gradually.task_chain_contextis a thread-local flag on the request thread, so workers now raise it themselves and hand queued tasks back via a newdrain_task_chainhelper, which the request thread re-queues in layout order.close_old_connections()on entry/exit (mirroringDatabaseSyncToAsync), since pool threads outlive requests and never get Django's request-finished cleanup.contextvars.copy_context().run, so tile spans stay attached to the request trace.settings.TESTtiles always serialize inline: worker threads use separate DB connections that can't see the test transaction and are invisible toassertNumQueries.Note
build:openapicouldn't run in the agent environment (no DB), but the diff adds no serializer fields or endpoints, so no schema change is expected.How did you test this code?
TestTaskChainThreadLocals(2SimpleTestCasetests, no DB) intest_execute_async.py, both run and passing locally:test_drain_task_chain_returns_and_clearscatches a regression wheredrain_task_chainstops clearing (chained tasks would execute twice) or stops returning (chained refreshes silently dropped). The helper is new, so nothing covered it.test_task_added_on_worker_thread_is_handed_back_via_drainlocks in the worker-thread hand-back mechanism the dashboard change depends on; if the thread-local plumbing breaks, chained refresh silently degrades to per-task dispatch.test_refresh_cachetests. I (Claude) could not run the DB-backed dashboard tests in this environment (no Postgres/ClickHouse); they collect cleanly and will run in CI.Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with Claude Code in an iterative optimization loop directed by Paul.
Skills invoked: /writing-tests.
Decisions along the way: started with a per-request
ThreadPoolExecutormatching the existing widget-query pattern in the same file, then moved to a shared module-level pool to avoid per-request thread spawn and to bound per-process concurrency. Consideredas_completedfor collection but dropped it since all tiles are needed before returning, so in-orderfuture.result()is equivalent and simpler. The feature-flag gate, test-mode inline fallback, task-chain hand-back, connection cleanup, and contextvars propagation were each added after identifying them as correctness or rollout risks of naively threading the loop.Timing numbers above come from a synthetic harness modeling per-tile latency, not the real endpoint; treat them as the shape of the win, not a measured production number.
Created with PostHog Code