Skip to content

UN-3883 [FEAT] Cut dashboard metrics cron DB load: narrower source windows, monthly from daily, two new indexes - #2276

Open
kirtimanmishrazipstack wants to merge 4 commits into
mainfrom
UN-3883-Optimize-DB-cron-queries-causing-high-DB-load
Open

UN-3883 [FEAT] Cut dashboard metrics cron DB load: narrower source windows, monthly from daily, two new indexes#2276
kirtimanmishrazipstack wants to merge 4 commits into
mainfrom
UN-3883-Optimize-DB-cron-queries-causing-high-DB-load

Conversation

@kirtimanmishrazipstack

Copy link
Copy Markdown
Contributor

What

Three changes to the dashboard metrics cron, already reviewed and merged individually as #2255, #2264 and #2265:

  • Monthly figures are now added up from the daily figures instead of being recalculated from the raw tables every time.
  • Each run looks back 2 days instead of ~2 months. A separate pass at 04:40 UTC looks back 7 days once a day, so a short outage still repairs itself.
  • The expensive half of the work runs hourly instead of every 15 minutes. Hourly figures keep their 15-minute cadence; daily and monthly move to hourly at :20.
  • Two new database indexes, on the two tables the cron reads most.

Why

The cron was using roughly 55 minutes of database time every 6 hours on production. It re-read between 32 and 62 days of raw data for all 38 organisations, 96 times a day, to produce figures that only change once a month.

Nothing a customer sees changes. Hourly dashboard numbers are still at most 15 minutes old; daily and monthly numbers are now at most an hour old instead of 15 minutes, which is the one deliberate trade.

How

  • _rollup_monthly_from_daily replaces the per-org monthly source queries with one INSERT ... ON CONFLICT DO UPDATE over event_metrics_daily for every org. Upsert-only, per the design agreed on UN-3973: a monthly row the daily tier no longer produces is left in place, because a stale total is recoverable with backfill_metrics and a deleted one is not.
  • aggregate_metrics_from_sources(tier, source_window_days) is called by three schedule rows. Both kwargs travel on both transports — Beat calls the Django task directly, the PG scheduler goes worker proxy → internal endpoint → the same function.
  • Lock keys are one per granularity written, namespaced by source window. ALL takes both, so it genuinely excludes a concurrent hourly run; distinct windows are distinct jobs, so the once-daily reconciliation pass can never be starved by the 15-minute schedule it is never retried after.
  • Both indexes are built CONCURRENTLY under atomic = False, with AddIndex confined to state_operations. Each migration asserts the index is valid and has the expected definition before recording itself applied.

Can this PR break any existing features?

The realistic risks, and what bounds each:

  • Monthly figures go stale if the daily tier has a gap. Monthly is now only as good as event_metrics_daily. Gaps shorter than 7 days repair themselves on the next reconciliation pass; anything older needs backfill_metrics. Bounded to the current and previous month.
  • Rolling the code back past this release breaks aggregation until migration 0006 is reversed — the schedule rows carry a tier kwarg the previous release's signature rejects. Documented in the migration docstring; migrate dashboard_metrics 0005 restores it.
  • A pod still on the old image during the rollout receives tier and raises TypeError until it rolls. Self-healing, and no aggregation is lost because the next tick succeeds.
  • Both index builds are non-blocking, and both migrations are reversible.

Database Migrations

Four, in three apps. No schema changes to any metrics table.

Migration What it does
dashboard_metrics/0005_add_reconciliation_task Adds the 04:40 UTC reconciliation schedule, on both Beat and the PG scheduler
dashboard_metrics/0006_split_aggregation_schedule Splits the aggregation into two rows by tier
file_execution/0007_wfe_status_created_idx Index on workflow_file_execution (status, created_at)
workflow_v2/0029_we_created_at_idx Index on workflow_execution (created_at)

Both index migrations no-op via IF NOT EXISTS if the index was built out of band first, which is the preferred production path — the exact statement is in each migration's docstring.

Env Config

None.

Deploy Steps

Run once, before the first aggregation after deploy:

python manage.py backfill_metrics --days 60 --skip-hourly --skip-monthly

Monthly is now derived from the daily tier, so that tier has to be complete across the rollup window first. --skip-monthly is deliberate: repair daily and let the rollup derive monthly.

Relevant Docs

backend/dashboard_metrics/README.md is updated — schedules, windows, staleness bounds, and the ownership overlap with backfill_metrics.

Related Issues or PRs

Merged into this branch: #2255 (UN-3973), #2264 (UN-3972), #2265 (UN-3974). Parent: UN-3883.

Dependencies Versions

No dependency changes.

Notes on Testing

Ticket Acceptance criterion State
UN-3973 Monthly derived from event_metrics_daily, not source tables Met
UN-3973 Per-run daily source window is 2 days Met
UN-3973 A once-daily 7-day reconciliation pass exists and is scheduled Met
UN-3973 Daily and monthly match pre-change values, incl. across a month boundary Met
UN-3973 Tests cover the month boundary and the reconciliation pass Met
UN-3972 Index present, indisvalid = t Met
UN-3972 Non-atomic + CONCURRENTLY, no write-blocking lock Met
UN-3972 get_documents_processed free of a seq scan on workflow_file_execution Confirm on prod
UN-3972 get_failed_pages free of a seq scan on workflow_execution Met
UN-3972 get_recent_activity under 1 s Flagged — belongs to the (created_at DESC) index descoped in comment 45015
UN-3974 Hourly stays 15 min, daily/monthly go hourly Met
UN-3974 Hourly figures unchanged, daily/monthly lag ≤ 1 h Met
UN-3974 Prefilter leaves the Query Insights top 10 Confirm on prod

The two Confirm on prod rows are post-deploy readings against Query Insights, not outstanding work.

Screenshots

Not applicable — no UI change.

Checklist

I have read and understood the Contribution Guidelines.

…from the daily tier (#2255)

* UN-3973 Derive monthly metrics from the daily tier, narrow source window to 2 days

The dashboard aggregation widened its DAY-granularity query to the first of
the previous month so monthly buckets could be summed in Python from the same
rows. Every run re-read 32-62 days of source data per metric, per org, 96
times a day.

Monthly is now rolled up from event_metrics_daily in one statement for all
orgs, so the source queries only need the daily window. That window drops to
2 days, sized against the measured worst created_at -> terminal-status lag of
~2h. A once-daily 7-day pass reruns the same task at a wider bound to repair
gaps left by cron downtime.

The active-org prefilter is decoupled from the daily window and pinned at 7
days: metrics filtered on another column (hitl_completions on approved_at) can
land for an org whose executions are older than the source window.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 Address Sonar and Greptile review findings

Sonar:
- S117: rename apps.get_model() locals in 0004 to snake_case
- S3776: cut _run_aggregation cognitive complexity from 22 by hoisting the
  static metric config tables to module level and extracting the per-org
  body, the active-org prefilter and the result shape into helpers

Greptile:
- Monthly rows in the rebuilt window whose daily rows are gone are now
  deleted alongside the upsert, so the two tiers cannot disagree. An empty
  daily tier still short-circuits, so a wiped tier cannot cascade into
  deleting monthly history.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 Add tests covering the source window, rollup SQL and reconciliation schedule

Closes the acceptance criteria that had no automated check:

- the monthly rollup issues no source-table SQL, asserted by capturing the
  queries it actually sends
- the window ladder at 2 / 7 / 62 days, including a row that finishes after
  the narrow window has moved past its created_at and so never re-enters it
- the reconciliation schedule row, its idempotency and its reverse

The schedule tests call the migration's function directly. The suite runs with
--no-migrations, so data migrations never execute and asserting on the beat row
would fail regardless of the migration being correct.

Also moves the dotenv load in settings/base.py above the Celery block.
CELERY_BROKER_BASE_URL, _USER and _PASS were read above it, so they could not be
supplied by an env file at all and had to be ambient. Ambient values still take
precedence, so deployed behaviour is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 Trim comments in tasks.py and revert the unrelated settings change

Cut the verbose comments and docstrings down to the purpose and the
non-obvious bits. Code is unchanged.

Restore backend/settings/base.py to main — moving the dotenv load ahead of
get_required_setting was a local test convenience, not part of this change.
The test rig exports the broker vars itself, so CI never needed it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 Renumber the reconciliation migration to 0005

UN-3445 landed 0004_pg_periodic_tasks on main after this branch was cut, leaving
dashboard_metrics with two 0004s depending on 0003 and nothing depending on either.
Django saw two leaf nodes and refused to build the graph, so `migrate` failed before
applying anything — every app, not just this one.

Depend on 0004_pg_periodic_tasks and renumber to match, so the short prefix form
stays usable for a rollback.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 [FIX] Address review: PG transport, scoped orphan sweep, retry posture

The reconciliation row could not run on the PG transport — two functions share the
task name dashboard_metrics.aggregate_from_sources and the worker one took no
arguments, so the mirrored row dispatched source_window_days into a zero-arg function
and the message was dropped. The worker proxy and the internal endpoint now plumb it,
and 0005 declares the PG twin rather than leaving the mirror to invent one.

The orphan sweep is scoped to the (organization, month) partitions the rollup actually
produced: an incomplete daily tier passed the empty-tier guard and deleted monthly rows
it could not vouch for. Its deletion count now reaches the task result and a WARNING.

DatabaseError and OperationalError propagate from the monthly rollup so the configured
autoretry fires, instead of being logged once behind success: True.

The prefilter is never narrower than the query window, so a widened source_window_days
cannot skip the orgs it exists to repair. bulk_create takes an explicit batch_size.

The Beat/PG drift guard named 0002 and 0004, so it kept comparing three schedules
against three while this PR added a fourth. It now discovers every migration in the
app, replays their RunPython forwards in order, derives the Beat cadence from the
schedule row, binds every declared kwarg to its task signature, and asserts every
post-install Beat write bumps PeriodicTasks.last_update.

The reconcile row moves to 04:40 UTC: */15 fires at minute 0, and the per-tier lock
keys do not block each other, so 04:00 started two full aggregations at once.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 [FIX] Address Athul's review: upsert-only monthly, per-schedule lock, honest success

The orphan delete is removed. The design agreed on this ticket (comments 44768/45016)
is a pure INSERT ... ON CONFLICT DO UPDATE; the delete was scope beyond it, and it
converts a recoverable undercount into unrecoverable loss — the daily rows that would
rebuild a deleted monthly row are exactly the ones that were missing. A stale total is
recoverable with backfill_metrics.

The reconciliation pass no longer shares a lock key with the 15-minute schedule. The
15-minute row is an IntervalSchedule and drifts against a fixed crontab, so on a shared
key the once-daily repair loses the race roughly one day in seven, returns
skipped=True and is never retried.

A run in which every metric for every org failed no longer reports success: True. The
result's success now reflects the error count, the completion log rises to WARNING, and
the worker-side guard reads skipped_reason and errors as well as skipped — it saw none
of these three did-nothing shapes before.

A failed monthly rollup is distinguishable from an empty one: upserted=0 collided with
the legitimate no-op and the no_active_orgs return.

source_window_days is validated and bounded. It arrives as JSON from a Beat row that is
editable in the admin: negative puts the window in the future, 0 never refreshes
yesterday, 365 restores the multi-month scan this ticket exists to remove.

Tests: a golden test seeds source rows, lets the real aggregation populate daily, and
compares the rolled-up monthly against the pre-change derivation computed independently
from get_documents_processed — AC-4 was claimed Met and had no equivalence assertion.
Fixture offsets derive from the month boundary rather than fixed day counts, which land
in the wrong month for the last days of any month.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ecution on (status, created_at) (#2264)

* UN-3972 [PERF] Index workflow_file_execution on (status, created_at)

The dashboard metrics cron's documents_processed and failed_pages queries
filter this table on status + a created_at window, but all four existing
indexes lead with workflow_execution_id. With no entry point here the planner
drives top-down from the org and sequentially scans all 1.28M rows of
workflow_execution — 83% of the cron's DB time on production.

Built CONCURRENTLY with atomic = False; a plain AddIndex would hold a SHARE
lock over a 3.4GB table taking live inserts. Guarded against a leftover
INVALID index from an interrupted build, which IF NOT EXISTS would otherwise
keep silently.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3972 [PERF] Trim the migration docstring to the project ceiling

The docstring restated the prod plan, deployment runbook and recovery steps.
That detail belongs in the PR, not in a file every future agent scans.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3972 [PERF] Guard the index migration's non-atomic CONCURRENTLY shape with tests

The suite runs with --no-migrations, so 0007 is never executed in CI. Regenerating it
with makemigrations, or dropping atomic = False / CONCURRENTLY while tidying, would land
a plain AddIndex — a SHARE lock held for the whole build on a 3.4 GB table that takes
live inserts — with every test still green.

Five DB-free assertions on the migration module and the model's Meta.indexes: non-atomic,
concurrent in both directions, the INVALID-index guard present, AddIndex confined to
state_operations, and model/migration agreement.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3972 [FIX] Assert the index definition, not just its validity, and pin reversibility

The CREATE INDEX CONCURRENTLY IF NOT EXISTS matches on name alone, so a hand-built
index with different columns was kept while Django recorded (status, created_at) into
model state — a permanent, invisible divergence that makemigrations --check cannot
see. The guard now compares pg_get_indexdef against the expected btree definition and
qualifies the lookup by current_schema(), since app tables live in the unstract schema.

Also pin that every database_operation is reversible: dropping the guard's
reverse_sql=noop killed the whole rollback path with all five tests still green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3972 [FIX] Address Athul's review: guard semantics, whole-migration assertions

Three mutations that were green are now caught: appending a bare AddIndex after the
SeparateDatabaseAndState (a real lock-taking build on a 3.4 GB table, invisible because
every assertion read operations[0]); flipping the guard's NOT indisvalid polarity, which
either raises on every healthy deploy or never fires at all; and a typo in reverse_sql,
which makes rollback a silent no-op through IF EXISTS while Django unapplies the
migration.

The CREATE assertion matches the column order by regex instead of an exact byte
sequence — removing one space used to fail it, a false-failure mode whose only outcome
is someone loosening the assertion.

Docstrings: the plan citation now points at UN-4045, which supersedes the earlier
workflow_file_execution reading; "every existing index leads with workflow_execution_id"
was false (the PK leads with id); the exact CREATE statement an operator should run out
of band is spelled out, with a warning off the struck two-index variant; and the models.py
comment no longer implies the index fixes both cron queries when it fixes one until
UN-3973 narrows the window.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…edule by tier and indexing workflow_execution on created_at (#2265)

* UN-3973 Derive monthly metrics from the daily tier, narrow source window to 2 days

The dashboard aggregation widened its DAY-granularity query to the first of
the previous month so monthly buckets could be summed in Python from the same
rows. Every run re-read 32-62 days of source data per metric, per org, 96
times a day.

Monthly is now rolled up from event_metrics_daily in one statement for all
orgs, so the source queries only need the daily window. That window drops to
2 days, sized against the measured worst created_at -> terminal-status lag of
~2h. A once-daily 7-day pass reruns the same task at a wider bound to repair
gaps left by cron downtime.

The active-org prefilter is decoupled from the daily window and pinned at 7
days: metrics filtered on another column (hitl_completions on approved_at) can
land for an org whose executions are older than the source window.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 Address Sonar and Greptile review findings

Sonar:
- S117: rename apps.get_model() locals in 0004 to snake_case
- S3776: cut _run_aggregation cognitive complexity from 22 by hoisting the
  static metric config tables to module level and extracting the per-org
  body, the active-org prefilter and the result shape into helpers

Greptile:
- Monthly rows in the rebuilt window whose daily rows are gone are now
  deleted alongside the upsert, so the two tiers cannot disagree. An empty
  daily tier still short-circuits, so a wiped tier cannot cascade into
  deleting monthly history.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 Add tests covering the source window, rollup SQL and reconciliation schedule

Closes the acceptance criteria that had no automated check:

- the monthly rollup issues no source-table SQL, asserted by capturing the
  queries it actually sends
- the window ladder at 2 / 7 / 62 days, including a row that finishes after
  the narrow window has moved past its created_at and so never re-enters it
- the reconciliation schedule row, its idempotency and its reverse

The schedule tests call the migration's function directly. The suite runs with
--no-migrations, so data migrations never execute and asserting on the beat row
would fail regardless of the migration being correct.

Also moves the dotenv load in settings/base.py above the Celery block.
CELERY_BROKER_BASE_URL, _USER and _PASS were read above it, so they could not be
supplied by an env file at all and had to be ambient. Ambient values still take
precedence, so deployed behaviour is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 Trim comments in tasks.py and revert the unrelated settings change

Cut the verbose comments and docstrings down to the purpose and the
non-obvious bits. Code is unchanged.

Restore backend/settings/base.py to main — moving the dotenv load ahead of
get_required_setting was a local test convenience, not part of this change.
The test rig exports the broker vars itself, so CI never needed it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3974 [PERF] Split the dashboard metrics schedule by tier and index workflow_execution on created_at

Schedule split. One schedule ran every 15 minutes and wrote all three metric
tiers. Dashboard daily and monthly figures do not need 15-minute freshness, so
they move to hourly — 96 runs a day becomes 24 for the expensive
DAY-granularity half of the work, while the hourly tier keeps its cadence.

Both schedule rows point at the same task and differ only in a `tier` kwarg; a
second task name would need its own worker registration and internal endpoint
for the PG path. The lock is now keyed per tier, so the two runs that collide
at the top of every hour do not starve each other. Omitting `tier` still writes
all three tiers, so a manual trigger never silently writes nothing.

Prefilter index. The active-org prefilter measures 1,849ms per call on
production — the slowest single query on the instance. Nothing on
workflow_execution leads with created_at: the two composite indexes are
date-ordered only within one workflow or pipeline, and the partial index is
empty in steady state. The split raises this query's call count, and UN-4045
will leave three more metric queries on the same bare date-range shape, so the
index lands with the split rather than after it.

Built CONCURRENTLY with atomic = False and guarded against a leftover INVALID
index, matching migration 0026.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3974 [PERF] Keep scheduler ownership out of the split migration and cut _run_aggregation's complexity

Migration 0005 used update_or_create for the PG row of the schedule it was only
re-keying, which reset pg_owned to False. converge_pg_scheduler disables a row's
Beat twin when the PG scheduler adopts it, so on an adopted deployment the
migration would have left the aggregation with no firer at all — Beat disabled,
PG no longer owning it. It now updates only task_kwargs on that row, leaving
enabled and pg_owned to the scheduler that owns them. Rollback is symmetric.

Threading the tier through _run_aggregation took its cognitive complexity from
25 to 27 against a limit of 15. Extracted _collect_org_metrics and
_aggregate_org, and hoisted the two static metric tables to module level so they
are not rebuilt per call. Names match the same extraction on #2255 so the two
reconcile cleanly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3974 [PERF] Trim comments and docstrings to the project ceiling

The two index migrations carried 50-60 line docstrings restating the prod plan,
deployment runbook and recovery steps. That detail belongs in the PR, not in
files every future agent scans. Cut to purpose and key behaviour.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3974 [PERF] Cover all three acceptance criteria with tests

The suite runs with --no-migrations, so neither 0005 nor 0029 ever executes in
CI, and nothing pinned the schedule split's behaviour at all. 46 tests, at least
one per acceptance criterion.

AC-1 — cadence, and the tier reaching the task. 0005 creates one row and rewrites
one, both scheduler tables agreeing, and the rewrite touches neither pg_owned nor
enabled: on an adopted deployment converge_pg_scheduler has already disabled the
Beat twin, so handing ownership back would leave the aggregation with no firer.
Separately the internal endpoint and the worker proxy are pinned to carry `tier`
— that leg fails silently, since _call_internal builds a body only when a tier is
given and the existing worker test called the task without one.

AC-2 — the split changes no figure. Runs the real _run_aggregation three times
and diffs the metrics tables: `hourly` reproduces the pre-split hourly figures
exactly, and hourly + daily_monthly reproduce every row `all` writes. Two guards
keep it from going vacuous, the second because mutation testing caught the first
version passing while _aggregate_single_metric was broken — the fixture produced
only LLM metrics, leaving half the split unverified.

AC-3 — the index. Migration shape (non-atomic, CONCURRENTLY both directions, the
INVALID guard, AddIndex confined to state_operations), plus an integration test
that EXPLAINs the query the aggregation actually issues, captured rather than
rewritten: a hand-copied queryset would keep passing after the prefilter changed,
which is the one thing it is for. Rows are inserted in ascending created_at order
so the heap matches production's append order. The Query Insights half of AC-3 is
a production reading and is deliberately not faked here.

Every test verified to fail when the thing it guards breaks.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 Renumber the reconciliation migration to 0005

UN-3445 landed 0004_pg_periodic_tasks on main after this branch was cut, leaving
dashboard_metrics with two 0004s depending on 0003 and nothing depending on either.
Django saw two leaf nodes and refused to build the graph, so `migrate` failed before
applying anything — every app, not just this one.

Depend on 0004_pg_periodic_tasks and renumber to match, so the short prefix form
stays usable for a rollback.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3974 Renumber the schedule-split migration to 0006 behind UN-3973's 0005

UN-3445's 0004_pg_periodic_tasks is the parent of both this migration and UN-3973's
reconciliation migration, so landing both would leave dashboard_metrics with two leaf
nodes and no applicable graph.

Depend on 0005_add_reconciliation_task instead, which puts the intended merge order
(UN-3973 then UN-3974) in the graph rather than in the merge queue. This branch cannot
migrate on its own until UN-3973 lands; its tests are unaffected, since the suite runs
with --no-migrations.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 [FIX] Address review: PG transport, scoped orphan sweep, retry posture

The reconciliation row could not run on the PG transport — two functions share the
task name dashboard_metrics.aggregate_from_sources and the worker one took no
arguments, so the mirrored row dispatched source_window_days into a zero-arg function
and the message was dropped. The worker proxy and the internal endpoint now plumb it,
and 0005 declares the PG twin rather than leaving the mirror to invent one.

The orphan sweep is scoped to the (organization, month) partitions the rollup actually
produced: an incomplete daily tier passed the empty-tier guard and deleted monthly rows
it could not vouch for. Its deletion count now reaches the task result and a WARNING.

DatabaseError and OperationalError propagate from the monthly rollup so the configured
autoretry fires, instead of being logged once behind success: True.

The prefilter is never narrower than the query window, so a widened source_window_days
cannot skip the orgs it exists to repair. bulk_create takes an explicit batch_size.

The Beat/PG drift guard named 0002 and 0004, so it kept comparing three schedules
against three while this PR added a fourth. It now discovers every migration in the
app, replays their RunPython forwards in order, derives the Beat cadence from the
schedule row, binds every declared kwarg to its task signature, and asserts every
post-install Beat write bumps PeriodicTasks.last_update.

The reconcile row moves to 04:40 UTC: */15 fires at minute 0, and the per-tier lock
keys do not block each other, so 04:00 started two full aggregations at once.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3974 [FIX] Address review: Beat reload, inherited ownership, boundary validation

Rewriting live Beat rows through historical models fires no post_save, so
DatabaseScheduler never reloaded: the existing row kept firing with no tier and the
new row never fired at all. 0006 now bumps PeriodicTasks.last_update in both
directions, as scheduler/ownership.py and mirror_pg_periodic_tasks.py already do.

The new row inherits pg_owned and both enabled flags from the row it is split from
instead of hardcoding Beat. In a PG-adopted environment the daily and monthly tiers
had no firer at all while the hourly run still returned success.

It also moves to minute 20. Minute 0 collides with */15 — and so does the suggested
minute 30, since */15 fires at :00 :15 :30 :45 — and the per-tier locks are built so
the two runs cannot block each other.

An unrecognised tier is now rejected in post(), and the blanket except ValueError in
_run is gone, so a ValueError from inside the aggregation reaches the logged 500 path
rather than reading as a bad request body.

Tests: the JSON round-trip assertion was a stdlib tautology that never read what the
migration writes — replaced with an assertion on the updated row, the one firing the
hourly tier in production. The ALL default is pinned off inspect.signature. The RunSQL
table and column are derived from the model rather than grepped. The planner-choice
assertion is deleted: a cost model on 12,000 rows is not production evidence.

Also drops a full Organization count that ran on every tier for one log field, and
gives two test modules the Django bootstrap their siblings carry.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3973 [FIX] Address Athul's review: upsert-only monthly, per-schedule lock, honest success

The orphan delete is removed. The design agreed on this ticket (comments 44768/45016)
is a pure INSERT ... ON CONFLICT DO UPDATE; the delete was scope beyond it, and it
converts a recoverable undercount into unrecoverable loss — the daily rows that would
rebuild a deleted monthly row are exactly the ones that were missing. A stale total is
recoverable with backfill_metrics.

The reconciliation pass no longer shares a lock key with the 15-minute schedule. The
15-minute row is an IntervalSchedule and drifts against a fixed crontab, so on a shared
key the once-daily repair loses the race roughly one day in seven, returns
skipped=True and is never retried.

A run in which every metric for every org failed no longer reports success: True. The
result's success now reflects the error count, the completion log rises to WARNING, and
the worker-side guard reads skipped_reason and errors as well as skipped — it saw none
of these three did-nothing shapes before.

A failed monthly rollup is distinguishable from an empty one: upserted=0 collided with
the legitimate no-op and the no_active_orgs return.

source_window_days is validated and bounded. It arrives as JSON from a Beat row that is
editable in the admin: negative puts the window in the future, 0 never refreshes
yesterday, 365 restores the multi-month scan this ticket exists to remove.

Tests: a golden test seeds source rows, lets the real aggregation populate daily, and
compares the rolled-up monthly against the pre-change derivation computed independently
from get_documents_processed — AC-4 was claimed Met and had no equivalence assertion.
Fixture offsets derive from the month boundary rather than fixed day counts, which land
in the wrong month for the last days of any month.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

* UN-3974 [FIX] Address Athul's review: lock covers what is written, both kwargs, graph guard

The lock is keyed by granularity written, not by enum member. ALL took a third key that
excluded nothing, so an ALL run and the scheduled hourly run wrote EventMetricsHourly
concurrently — reachable from the documented manual trigger and from the endpoint's own
"omit tier" contract. ALL now takes both keys and releases whatever it took if it cannot
take them all. Keys are namespaced by source window so the once-daily reconciliation
pass, which is never retried, is not starved by the 15-minute schedule.

source_window_days is accepted on all three legs. 0006 hard-depends on 0005, so the
reconciliation row is a certainty rather than a hypothetical, and this branch's
signatures rejected the kwarg it dispatches.

The tier predicates come from one membership table, so a member added without an entry
raises instead of acquiring the lock, iterating every org, writing nothing and returning
success.

The migration's bulk updates check their row counts. A filtered update matching nothing
reported success while leaving the old row on kwargs="{}" — every tier every 15 minutes
— alongside the new hourly row: strictly more load than before, silently.

tier is validated at the request boundary with a warning log, and an explicit null is
treated as omitted.

New test_migration_graph.py builds the migration graph, which is what catches 0006's
dependency on a node that is not on this branch; --no-migrations means nothing else does.
Lock behaviour is now exercised rather than its key string asserted, merge_schedules has
coverage at all, the equivalence file carries one absolute expectation and a frozen
clock, and the prefilter asserts the index is usable under enable_seqscan=off rather
than that the planner chose it on 12,000 synthetic rows.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Sep 2, 2026

Copy link
Copy Markdown

@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR reduces dashboard aggregation database load by splitting hourly and daily/monthly schedules, narrowing source windows, rolling monthly metrics up from daily data, and adding supporting indexes.

  • Adds 2-day aggregation and daily 7-day reconciliation windows.
  • Moves daily/monthly aggregation to an hourly schedule while retaining 15-minute hourly metrics.
  • Adds concurrent indexes for workflow execution queries.
  • Introduces a WorkflowExecution-based organization prefilter that can omit organizations with activity only in other metric sources.

Confidence Score: 4/5

The PR should not merge until the active-organization prefilter includes organizations with qualifying activity in every metric source.

The new prefilter gates all aggregation on recent WorkflowExecution rows, so organizations with only PageUsage, Usage, or HITLQueue activity are skipped and retain stale or missing dashboard metrics.

Files Needing Attention: backend/dashboard_metrics/tasks.py

Important Files Changed

Filename Overview
backend/dashboard_metrics/tasks.py Implements tiered aggregation, narrower windows, monthly rollup, locking, and active-organization filtering; the prefilter can omit valid non-WorkflowExecution metric activity.
backend/dashboard_metrics/internal_views.py Validates and forwards aggregation tier and source-window parameters across the internal scheduler boundary.
backend/dashboard_metrics/migrations/0005_add_reconciliation_task.py Adds the daily seven-day reconciliation schedule for both Beat and PostgreSQL transports.
backend/dashboard_metrics/migrations/0006_split_aggregation_schedule.py Splits hourly and daily/monthly aggregation schedules while preserving existing scheduler ownership.
backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py Adds a concurrent status-and-created-at index with separate migration state handling.
backend/workflow_manager/workflow_v2/migrations/0029_we_created_at_idx.py Adds a concurrent created-at index with explicit validity and definition checks.
workers/scheduler/dashboard_metrics_tasks.py Propagates tier and source-window scheduler arguments to the backend aggregation endpoint.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  S[Metric source tables] --> H[Hourly aggregation every 15 minutes]
  S --> D[Daily aggregation hourly]
  S --> R[Seven-day reconciliation daily]
  D --> DR[(EventMetricsDaily)]
  R --> DR
  DR --> M[Monthly rollup]
  H --> HR[(EventMetricsHourly)]
  M --> MR[(EventMetricsMonthly)]
Loading

Fix all with Greploop Fix All in Claude Code

Prompt To Fix All With AI
### Issue 1
backend/dashboard_metrics/tasks.py:631-634
**Prefilter drops valid metric activity**

When an organization has recent `PageUsage`, `Usage`, or `HITLQueue` activity but no `WorkflowExecution` created within the lookback, `_active_org_ids` excludes it from every metric query, causing its pages-processed, LLM, or HITL dashboard rows to remain stale or absent even after reconciliation.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "Merge branch 'main' into UN-3883-Optimiz..." | Re-trigger Greptile

Comment on lines +631 to +634
return set(
WorkflowExecution.objects.filter(created_at__gte=cutoff)
.values_list("workflow__organization_id", flat=True)
.distinct()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Prefilter drops valid metric activity

When an organization has recent PageUsage, Usage, or HITLQueue activity but no WorkflowExecution created within the lookback, _active_org_ids excludes it from every metric query, causing its pages-processed, LLM, or HITL dashboard rows to remain stale or absent even after reconciliation.

Knowledge Base Used: Restore dashboard metrics and navigation

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/dashboard_metrics/tasks.py
Line: 631-634

Comment:
**Prefilter drops valid metric activity**

When an organization has recent `PageUsage`, `Usage`, or `HITLQueue` activity but no `WorkflowExecution` created within the lookback, `_active_org_ids` excludes it from every metric query, causing its pages-processed, LLM, or HITL dashboard rows to remain stale or absent even after reconciliation.

**Knowledge Base Used:** [Restore dashboard metrics and navigation](https://app.greptile.com/zipstack/-/custom-context/knowledge-base/zipstack/unstract/-/reverts/incident-mitigation_1813-20260302-dashboard-metrics-sidebar-440ae49.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Unstract test results

Per-group results

Status Group Tier Passed Failed Errors Skipped Duration (s)
e2e-api-deployment e2e 3 0 0 0 16.7
e2e-coowners e2e 1 0 0 0 1.8
e2e-etl e2e 1 0 0 0 20.9
e2e-login e2e 2 0 0 0 1.3
e2e-prompt-studio e2e 1 0 0 0 5.9
e2e-smoke e2e 2 0 0 0 1.5
e2e-workflow e2e 1 0 0 0 20.5
frontend unit 0 1 0 0 0.0
integration-backend integration 358 0 0 26 48.7
integration-connectors integration 1 0 0 7 8.0
integration-workers integration 157 0 0 1 51.0
ui e2e 0 1 0 0 0.0
unit-backend unit 1185 0 8 1 34.8
unit-connectors unit 63 0 0 0 9.7
unit-core unit 33 0 0 0 1.0
unit-platform-service unit 15 0 0 0 1.8
unit-rig unit 120 0 0 0 3.7
unit-runner unit 5 0 0 0 3.8
unit-sdk1 unit 563 0 0 0 28.4
unit-workers unit 1376 0 0 1 114.2
TOTAL 3887 2 8 36 373.9

Critical paths

⚠️ Critical paths not yet covered

  • workflow-execution-fan-out — Multi-file workflow execution fans out to file-processing workers and rejoins. (declared coverage: no groups declared)
✅ Covered critical paths
  • auth-login — covered by e2e-login
  • adapter-register-llm — covered by integration-backend
  • workflow-author — covered by integration-backend
  • co-owner-manage — covered by integration-backend, e2e-coowners
  • workflow-create-execute — covered by e2e-workflow
  • api-deployment-provision — covered by integration-backend
  • api-deployment-auth — covered by integration-backend
  • api-deployment-run — covered by e2e-api-deployment
  • mcp-server-auth — covered by integration-backend
  • mcp-platform-auth — covered by integration-backend
  • prompt-studio-author — covered by integration-backend
  • prompt-studio-fetch-response — covered by e2e-prompt-studio
  • connector-register-test — covered by integration-backend
  • pipeline-etl-execute — covered by e2e-etl
  • usage-aggregate-read — covered by integration-backend
  • usage-token-tracking — covered by e2e-api-deployment
  • callback-result-delivery — covered by e2e-api-deployment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant