UN-4046 [DOCS] Document the PG queue transport in the dashboard-metrics README - #2280
Merged
kirtimanmishrazipstack merged 3 commits intoSep 10, 2026
Merged
Conversation
The README described a Celery-only architecture and told readers to start the cron with `celery -A backend beat` and `celery -A backend worker -Q dashboard_metric_events`. Neither process is defined in the default compose stack any more: UN-3796 added the PG path and UN-4046 made it the default, deleting celery-beat and worker-metrics and adding worker-pg-metrics. Anyone onboarding from this file started a worker the fleet no longer runs, and had no way to find out why nothing fired. Add a Transport chapter covering the dual-written schedule rows, the full PG path from the leader-elected tick through worker-pg-metrics and the internal API to tasks.py, why the proxy hop exists (no Django in the workers image), the two env gates and their defaults, and the failure modes specific to this transport — fire-and-forget with MAX_ATTEMPTS=1, autoretry_for being inert on the PG path, and the silent no-firer state. Rename "Celery Tasks" to "Scheduled Tasks", since the task bodies are shared and only the transport differs. Keep the Celery path documented throughout as the rollback target rather than deleting it. Update the setup and troubleshooting steps to match: step 5 now prints pg_owned and cron_string from PgPeriodicTask alongside the Beat rows, which is the check that actually identifies which scheduler owns the cron. Docs only, no behaviour change.
Self-review found the new Setup section reproduced the exact failure the change was written to prevent. It listed only the metrics consumer as the process to start, and stated there was "no separate Beat-equivalent process to start". That is true of compose, where worker-pg-reaper is already declared, but false for the host-runner flow the same paragraph tells the reader to use: the periodic tick runs inside the reaper (reaper.py:1310 calls dispatch_due_periodic_tasks), so starting only `run-worker.sh pg-metrics` leaves the schedule with no firer — silently. Start both processes in Quick Commands and Setup step 4, and say why both are needed. Name worker-pg-reaper explicitly in the diagram and in the no-firer failure mode. "The PG scheduler" was ambiguous against the worker-pg-scheduler service named forty lines earlier, which is the pipeline-task consumer and fires no periodics — a reader debugging the silent state would have checked the wrong service and found it healthy. Also: - Qualify the "Celery tasks have max_retries=3" line by transport. It contradicted the new failure-modes section, which is correct for the live path (MAX_ATTEMPTS=1, autoretry_for inert on internal HTTP). - Rename the duplicated "Task Name" columns to Python Function / Registered Task Name. - Drop the suite count that disagreed with the tree printed below it. - Fix the TOC's broken #frontend-components anchor, whose heading is "UI Data Flow — What Shows Where". Pre-existing, but in a block this change already rewrites, and it was the file's only MD051.
…unbook
Second self-review pass, all verified against source.
The dual-write claim was wrong for most of the rows. It said every schedule
is written "from a single spec by the same migration so the two cannot
drift", citing 0006. That holds for the two aggregation rows and the
reconciliation row only. The three original rows come from two separate
migrations — 0002 for Beat and its PG twin 0004 — and 0006's own docstring
says so ("cannot drift the way 0002 and 0004 can"), as does the drift-guard
test that exists for it. Replace the sentence with a table splitting the two
cases and point at tests/test_pg_periodic_task_declarations.py.
Convergence does not run on "every backend start". The whole block in
entrypoint.sh is gated on --migrate, which compose passes but a deployment
running migrations as a separate job does not. The payoff clause was
"ownership is a values change rather than a remembered procedure", which is
exactly the belief that breaks there.
The rollback runbook could produce the outage it warns about. Releasing the
metrics rows back to Beat needs --periodics too, so
PG_SCHEDULER_ADOPT_PERIODICS must stay true while PG_SCHEDULER_ENABLED goes
false. Unsetting both — the intuitive "roll it all back" — leaves the rows
pg_owned with Beat disabled, i.e. no firer.
Also corrected:
- "logs at ERROR rather than raising" conflated two cases. A non-200 does
raise; only a 200 reporting per-org errors is logged instead.
- MAX_ATTEMPTS=1 is one service's env in compose, not a property of the
transport; the consumer default is 5. Attribute it, and mention the httpx
connection-establishment retries.
- The autoretry_for bullet had the right conclusion via the wrong mechanism.
The decorator IS in the call path; retry() re-raises when a task is called
directly instead of dispatched by a worker.
- Note the endpoint path is relative to INTERNAL_API_BASE_URL, which ends
/internal, so the diagram can't be pasted into curl as-is.
- --periodics covers every mirrored non-pipeline periodic, not just these.
- Stop quoting exact liveness numbers as fact: they are ${VAR:-default}
overrides and k8s takes its values from a chart in another repo.
Contributor
Author
Self-review (
|
| # | Finding | Resolution |
|---|---|---|
| 1 | The process that fires the cron was never listed as something to start. Setup listed only the metrics consumer and stated "there is no separate Beat-equivalent process to start". True of compose, false for the host-runner flow the same paragraph recommends: the tick runs in the reaper (reaper.py:1310 → dispatch_due_periodic_tasks). Following it literally reproduces the silent no-firer state this PR exists to document. |
0fa100ff — both processes listed in Quick Commands and Setup step 4, with the reason. |
| 2 | The dual-write claim was wrong for 3 of the 5 rows. I wrote that every schedule is written "from a single spec by the same migration so the two cannot drift", citing 0006. That holds only for the aggregation rows and the reconciliation row. The three original rows come from two migrations — 0002 (Beat) and its PG twin 0004. 0006's own docstring says so: "cannot drift the way 0002 and 0004 can". |
a32e7a14 — replaced with a table splitting the two cases, pointing at the drift-guard test tests/test_pg_periodic_task_declarations.py. |
| 3 | The rollback runbook could cause the outage it warns about. Releasing the metrics rows needs --periodics in both directions, so PG_SCHEDULER_ADOPT_PERIODICS must stay true while PG_SCHEDULER_ENABLED goes false. Unsetting both — the intuitive "roll it all back" — leaves the rows pg_owned with Beat disabled: no firer. |
a32e7a14 — callout rewritten to lead with the flag interaction. |
Important
entrypoint.shdoes not converge on "every backend start" — the block is gated on--migrate(entrypoint.sh:27). Compose passes it; a deployment running migrations as a separate job does not, which is exactly where the "ownership is a values change" promise breaks. Now stated precisely, with the caveat called out.- "the PG scheduler" was ambiguous against the
worker-pg-schedulerservice named earlier in the same chapter — which is the pipeline-task consumer and fires no periodics. Someone debugging the silent state would have checked the wrong service and found it healthy. Now namedworker-pg-reaperexplicitly, with the distinction spelled out. - A pre-existing
max_retries=3line contradicted the new failure-modes section. Qualified by transport.
Corrections to my own new text
MAX_ATTEMPTS=1is one service's env in compose, not a property of the transport — the consumer default is5. Now attributed, so nobody assumes it holds in k8s.- "logs at ERROR rather than raising" conflated two cases: a non-200 does raise; only a 200 reporting per-org
errorsis logged instead. - The
autoretry_forbullet had the right conclusion via the wrong mechanism — the decorator is in the call path;retry()re-raises when a task is called directly rather than dispatched by a worker. - The endpoint in the diagram is relative to
INTERNAL_API_BASE_URL(which ends/internal), so it can't be pasted into curl as-is. Noted. --periodicscovers every mirrored non-pipeline periodic, not onlydashboard_metrics.*. Hedged so it stays true when another is mirrored.- Stopped quoting exact liveness numbers as fact — they are
${VAR:-default}overrides and k8s takes its values from a chart in another repo.
Deliberately not changed
- The remaining Celery invocations are all inside explicitly labelled rollback/legacy blocks — intentional, per the stated scope of keeping Celery documented as the rollback target.
- Reviewer flagged the File Structure tree gaining migration entries as mild scope creep. Left as-is:
0004and0006are the transport's own migrations and the tree previously stopped at0002, which was itself misleading. run-worker.sh's usage text omits the validpg-metricsrole (run-worker.sh:227-235). Real, but a script bug and out of scope for a docs PR — worth a follow-up.
Verification
- Every claim traced to a source
file:lineonorigin/main@335b0a3a. - All internal anchors resolve.
markdownlint-cli@0.42.0 --disable MD013(the repo's own pre-commit config): 62 violations vs the base commit's 63 — none added, and the one removed is theMD051from the broken#frontend-componentsTOC anchor this PR repairs.
Not marking ready — leaving that to the author's call.
|
jaseemjaskp
marked this pull request as ready for review
September 10, 2026 05:54
jaseemjaskp
requested review from
kirtimanmishrazipstack and
muhammad-ali-e
September 10, 2026 05:55
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Unstract test resultsPer-group results
Critical paths
|
kirtimanmishrazipstack
approved these changes
Sep 10, 2026
kirtimanmishrazipstack
deleted the
UN-4046-dashboard-metrics-readme-pg-transport
branch
September 10, 2026 06:15
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
2 participants
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
Documentation only.
backend/dashboard_metrics/README.mddescribed a Celery-only architecture and told readers to start the metrics cron withcelery -A backend beatandcelery -A backend worker -Q dashboard_metric_events. Neither process exists in the default compose stack any more.## Celery Tasksto## Scheduled Tasks— the task bodies are shared, only the transport differs.Why
UN-3796 added the PG path; UN-4046 made it the default, deleted
celery-beatandworker-metricsfromdocker/docker-compose.yaml, and addedworker-pg-metrics. The README was never updated for either.The concrete cost: someone onboarding from this file starts a worker the fleet no longer runs, sees no error and no queue depth, and has nothing in the docs pointing at
pg_owned— which is the flag that actually decides whether anything fires. The README's setup step 5 checkeddjango_celery_beat.PeriodicTask.enabledonly, which on a current deploy readsFalseon healthy rows.How
The new chapter documents:
dashboard_metrics.*schedule is a pair —django_celery_beat.PeriodicTaskandpg_queue.PgPeriodicTask— written from one spec inmigrations/0006_split_aggregation_schedule.py.pg_ownedselects the firer.workers/queue_backend/pg_queue/pg_scheduler.py::dispatch_due_periodic_tasks, enqueue andnext_run_atadvance in one transaction) →pg_queue_messageondashboard_metric_events→worker-pg-metrics→ the HTTP proxy inworkers/scheduler/dashboard_metrics_tasks.py→internal_views.py→tasks.pyverbatim.worker-pg-metricsis its own service — the consumer's health heartbeat freezes while a task runs, soHEALTH_STALEbounds one task's wall clock; a minutes-long aggregation would trip the scheduler's 240s bound.PG_SCHEDULER_ENABLED(defaultstrue) andPG_SCHEDULER_ADOPT_PERIODICS(must betruefor the periodics), converged byconverge_pg_scheduleron every backend start.MAX_ATTEMPTS=1,autoretry_forbeing inert on the internal-HTTP path, and the silent no-firer state.Setup step 5 now prints
pg_ownedandcron_stringfromPgPeriodicTaskalongside the Beat rows.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No. One markdown file changed, no code, no schema, no config, no schedule rows. Nothing imports or renders this README.
Database Migrations
None.
Env Config
None. The PR documents two existing variables (
PG_SCHEDULER_ENABLED,PG_SCHEDULER_ADOPT_PERIODICS); it does not introduce or change either.Deploy Steps
None.
Relevant Docs
This PR is the doc change. Source of truth verified against
origin/mainat335b0a3a:backend/scheduler/ownership.py—PG_SCHEDULER_ENABLEDdefaultbackend/entrypoint.sh— convergence on start,--periodicsderivationbackend/pg_queue/models.py—PgPeriodicTaskfieldsbackend/pg_queue/management/commands/converge_pg_scheduler.py— ownership, both directionsworkers/queue_backend/pg_queue/pg_scheduler.py— the dispatch tickworkers/scheduler/dashboard_metrics_tasks.py— the proxyworkers/run-worker.sh— thepg-metricsroledocker/docker-compose.yaml—worker-pg-metrics,PG_SCHEDULER_ADOPT_PERIODICS=trueRelated Issues or PRs
Dependencies Versions
No dependency changes.
Notes on Testing
Docs only, so no tests. What was checked:
origin/mainsource listed above, not from memory.markdownlint-cli@0.42.0 --disable MD013(the repo's pre-commit config) reports the same 63 violations as the base commit — all pre-existing, zero added. Four were introduced mid-edit (3× MD033 from a<details>block, 1× MD040) and removed.MD051is the pre-existing[Frontend Components](#frontend-components)TOC entry, deliberately left alone as unrelated to this change.Screenshots
Not applicable.
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code