Skip to content

UN-3973 [DEV] Cut dashboard cron DB time by deriving monthly metrics from the daily tier - #2255

Open
kirtimanmishrazipstack wants to merge 7 commits into
UN-3883-Optimize-DB-cron-queries-causing-high-DB-loadfrom
UN-3973-optimize-db-queries-reduce-monthly-metrics
Open

UN-3973 [DEV] Cut dashboard cron DB time by deriving monthly metrics from the daily tier#2255
kirtimanmishrazipstack wants to merge 7 commits into
UN-3883-Optimize-DB-cron-queries-causing-high-DB-loadfrom
UN-3973-optimize-db-queries-reduce-monthly-metrics

Conversation

@kirtimanmishrazipstack

@kirtimanmishrazipstack kirtimanmishrazipstack commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

What

Monthly dashboard totals are now added up from the daily figures we already store, instead of
re-reading the raw processing history. Each run looks at the last 2 days instead of up to two
months, and a once-a-day pass looks back 7 days to repair anything the cron missed while down.

Why

Every 15 minutes, for each of 38 organisations, the cron re-read up to two months of processing
history — just so it could total the month. Monthly figures do not need that freshness, and the
daily totals are already stored.

Worth about 460 s of database time per 6 h. It will not move the two most expensive queries;
those are #2264's.

How

  • Constants: source window 2 days, reconciliation 7 days, active-org lookback 7 days.
    The reconciliation pass is the same task at a wider bound — a second schedule row, no new code path.
  • _aggregate_single_metric / _aggregate_llm_combined drop monthly_start / monthly_agg; the
    DAY query binds daily_start and the redundant Python guard goes.
  • _bulk_upsert_monthly_rollup_monthly_from_daily() — one ORM aggregate over
    event_metrics_daily for all orgs, so the monthly tier stops scaling with tenant count.
    metric_type is aggregated (Min), not grouped, or a mid-month type change breaks the upsert.
  • The active-org prefilter is decoupled from daily_start and pinned at 7 days.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why.

Monthly totals now inherit any holes in the stored daily figures. The once-a-day pass repairs the
last 7 days; older holes need the backfill command below, which is why it runs once at deploy.

Deleting old source data no longer corrects totals beyond that same window.

Nothing already counted changes, and a slow-finishing file cannot land in the wrong day.

Database Migrations

dashboard_metrics/0005_add_reconciliation_task.py — adds the once-a-day 04:00 UTC schedule row.
No table changes. Safe to re-run and removable on rollback.

Run once at deploy, before the first monthly rollup: manage.py backfill_metrics --days 60 --skip-hourly --skip-monthly. Monthly is built from the daily figures now, so those have to be
complete for this month and last month first.

Migration Order

The three UN-3883 PRs stack on the same integration branch. Merge in this order.

Order PR Migration Depends on This PR?
1 #2255 · UN-3973 dashboard_metrics/0005_add_reconciliation_task 0004_pg_periodic_tasks (UN-3445, already on main) Yes — this PR
2 #2264 · UN-3972 file_execution/0007_wfe_status_created_idx file_execution/0006_… No — merge after this PR
3 #2265 · UN-3974 dashboard_metrics/0006_split_aggregation_schedule 0005_add_reconciliation_task (#2255) No — merge after this PR

#2264's migration is in a different app and has no interaction with the other two. The one
hard dependency is #2265 on #2255 — merged out of order, #2265 fails at graph build:

NodeNotFoundError: Migration dashboard_metrics.0006_split_aggregation_schedule
dependencies reference nonexistent parent node ('dashboard_metrics', '0005_add_reconciliation_task')

Its tests are unaffected — the backend suite runs with --no-migrations.

Verified on a throwaway Postgres: 000400050006 applies from an empty database,
reverses, and re-applies, with makemigrations --check clean at each step.

Env Config

None.

Relevant Docs

UN-3883 analysis §6.2, §6.3, §7 step 1, §8.

Related Issues or PRs

Parent UN-3883. #2264 (UN-3972), #2265 (UN-3974) — see Migration Order above.

Dependencies Versions

None.

Notes on Testing

38 tests. Detail in Jira comment 45429.

# Acceptance criterion Verdict
1 Monthly totals derived from event_metrics_daily, not source tables Met
2 Per-run daily source window is 2 days Met
3 A once-daily 7-day reconciliation pass exists and is scheduled Met
4 Daily and monthly figures match pre-change values, incl. across a month boundary Met
5 Tests cover the month-boundary case and the reconciliation pass Met

Screenshots

n/a — backend only.

Checklist

I have read and understood the Contribution Guidelines.

…dow 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
@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR derives current and previous-month dashboard metrics from the stored daily tier, narrows routine source aggregation to two days, and adds a daily seven-day reconciliation pass.

  • Adds migration 0005_add_reconciliation_task after the existing 0004_pg_periodic_tasks migration.
  • Schedules the reconciliation task for 04:00 UTC with a seven-day source window.
  • Performs monthly upsert and orphan cleanup atomically from daily aggregates.
  • Adds coverage for rollups, month boundaries, reconciliation, migration idempotency, and query shape.

Confidence Score: 5/5

The PR appears safe to merge with the revised migration order.

No blocking failure remains; the revised migration follows the existing dashboard_metrics migration graph, and the previously reported rollup concerns were fixed, withdrawn, or explicitly accepted as documented behavior.

Important Files Changed

Filename Overview
backend/dashboard_metrics/migrations/0005_add_reconciliation_task.py Adds an idempotent, reversible reconciliation schedule with the corrected dependency on dashboard_metrics migration 0004.
backend/dashboard_metrics/tasks.py Narrows source-query windows and replaces raw-source monthly aggregation with an atomic daily-tier rollup.
backend/dashboard_metrics/tests/test_tasks.py Covers rollup correctness, reconciliation behavior, query boundaries, orphan handling, and migration scheduling.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[15-minute aggregation] --> B[Read last 2 days of source data]
    C[Daily 04:00 reconciliation] --> D[Read last 7 days of source data]
    B --> E[Upsert hourly and daily tiers]
    D --> E
    E --> F[Aggregate current and previous month from daily tier]
    F --> G[Atomically upsert monthly rows and remove obsolete keys]
Loading

Reviews (11): Last reviewed commit: "Merge remote-tracking branch 'origin/UN-..." | Re-trigger Greptile

Comment thread backend/dashboard_metrics/tasks.py
@kirtimanmishrazipstack
kirtimanmishrazipstack marked this pull request as draft August 25, 2026 14:11
@kirtimanmishrazipstack
kirtimanmishrazipstack marked this pull request as ready for review August 25, 2026 14:38
Comment thread backend/dashboard_metrics/tasks.py
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
Comment thread backend/dashboard_metrics/tasks.py
@kirtimanmishrazipstack

Copy link
Copy Markdown
Contributor Author

@greptile-apps please review

…iation 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
Comment thread backend/dashboard_metrics/tasks.py
@kirtimanmishrazipstack

Copy link
Copy Markdown
Contributor Author

@greptile-apps please review again

Comment thread backend/dashboard_metrics/tasks.py
@kirtimanmishrazipstack

Copy link
Copy Markdown
Contributor Author

@greptile-apps On the summary verdict — the mechanism is right, "not safe to merge" isn't. This is the trade the ticket asks for, and it was in the PR description before the review.

It's documented and accepted. Under "Can this PR break any existing features", item 1:

Monthly now inherits gaps in event_metrics_daily. […] the reconciliation pass heals 7 days back. Downtime longer than 7 days needs the backfill_metrics management command. This is the trade the ticket accepts by design.

Monthly stopped self-healing over 62 days deliberately — that 62-day re-derivation from source tables is the DB load UN-3973 exists to remove. Widening the rollup's source window to make orphan cleanup safe against arbitrarily old gaps would revert the PR.

You did surface one thing the description understated, and I've fixed it. "Inherits gaps" reads as undercounting; the deletion case is worse than that:

  • some daily rows for a key survive the gap → monthly recomputed low
  • every daily row for that key missing → key never enters fresh_keys → monthly row deleted outright

Confirmed against the test DB (SKIP_DAILY_NO_BACKING -> None). Both are bounded to the current and previous month by month__gte=month_start; a month outside the window is untouched (OLD_MONTH -> 777.0). Item 1 now states the deletion case explicitly rather than folding it into "gaps".

The one-time exposure is real, and it's a deploy step rather than a code change. Gaps already sitting in the daily tier would propagate into monthly on the first rollup after this ships. Added a ## Deploy Steps section:

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

That rebuilds daily from source across the rollup window; the scheduled rollup then derives monthly from a complete tier. --skip-monthly is deliberate — repair daily and let the rollup derive monthly, rather than writing monthly from source here. Needed once; the 04:00 reconciliation pass carries it forward after that.

Ongoing gaps beyond 7 days remain the accepted trade, with backfill_metrics as the documented remedy.

@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

You're right. Given that this tradeoff is explicit, ticket-approved, bounded to the current and previous month, and addressed operationally by the one-time daily backfill, it should not be a merge blocker. The orphan-deletion behavior is a real consequence, but it is now documented rather than an unaddressed regression. I withdraw the “not safe to merge” verdict. The remaining operational caveat is to run the documented backfill before the first rollup and use backfill_metrics for outages or source changes older than the 7-day reconciliation window.

@kirtimanmishrazipstack

Copy link
Copy Markdown
Contributor Author

@greptile-apps can you review and generate new confidence score?

…hange

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
kirtimanmishrazipstack added a commit that referenced this pull request Aug 31, 2026
…d 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
kirtimanmishrazipstack and others added 3 commits August 31, 2026 21:08
…into UN-3973-optimize-db-queries-reduce-monthly-metrics
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
…-causing-high-DB-load' into UN-3973-optimize-db-queries-reduce-monthly-metrics
@sonarqubecloud

sonarqubecloud Bot commented Sep 1, 2026

Copy link
Copy Markdown

@kirtimanmishrazipstack

Copy link
Copy Markdown
Contributor Author

@greptile-apps migration order is revised. Review again.

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