Skip to content

perf(nodes): avoid materializing reset history in lists - #851

Merged
ImMohammad20000 merged 3 commits into
PasarGuard:devfrom
dr-hoseyn:codex/perf-node-list-usage-aggregate
Sep 7, 2026
Merged

perf(nodes): avoid materializing reset history in lists#851
ImMohammad20000 merged 3 commits into
PasarGuard:devfrom
dr-hoseyn:codex/perf-node-list-usage-aggregate

Conversation

@dr-hoseyn

@dr-hoseyn dr-hoseyn commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The dashboard node list is polled every 10 seconds. It previously selectinloaded every NodeUsageResetLogs row for the page and materialized those rows as ORM objects just to calculate lifetime_uplink and lifetime_downlink.

This change:

  • calculates reset uplink/downlink with indexed correlated SUM expressions in the node query
  • keeps reset-history rows unloaded for the dashboard list
  • preserves the existing default loading behavior for callers that actually need the history objects
  • adds regression coverage for aggregate values, empty history, and the operation wiring
  • adds a reproducible before/after benchmark

Not a duplicate

I checked all local/remote branches and open PRs for node list/reset-history aggregation. No existing change implements this. In particular, #615 targets pools, indexes, usage writes, and health checks; #724 fixed N+1 loading by eagerly loading relations but still materializes the complete node reset history.

Benchmark

Command: python -m scripts.benchmark_node_list_usage

SQLite in-memory, matching the dashboard page size of 15 nodes, with 1,000 reset records per node; median of 5 timed runs:

Metric Before After Change
Median query time 208.33 ms 7.49 ms -96.40%
Peak traced memory 25.39 MiB 0.13 MiB -99.49%
SELECTs 3 2 -1
Reset-log ORM objects 15,000 0 -100%
Session identity map 15,015 15 -99.90%

The benchmark also asserts that lifetime usage is identical in both paths.

Verification

  • focused node aggregate/loading tests: 5 passed
  • non-API suite excluding the pre-existing API-coupled review test: 166 passed, 2 skipped
  • Ruff check and format check: passed
  • git diff --check: passed

The local API test harness remains blocked by its existing database setup: the on-disk test DB references missing Alembic revision a4d8c7e91b32, while the in-memory override creates tables on a connection different from the app session (no such table: jwt).

Summary by CodeRabbit

  • New Features

    • Node lists now display accurate lifetime uplink and downlink usage totals, including usage accumulated across reset periods.
  • Performance Improvements

    • Node usage totals load more efficiently without loading the full usage-log history, reducing database and memory overhead for large datasets.
  • Bug Fixes

    • Corrected lifetime usage reporting for nodes with and without recorded usage history.

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 364afba0-578b-44c8-9f72-4d164f832107

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: fb6ada8f-a091-4cb9-9b75-f0fe87525e1c

📥 Commits

Reviewing files that changed from the base of the PR and between 234ab68 and 7d288d7.

📒 Files selected for processing (5)
  • app/db/crud/node.py
  • app/db/models.py
  • app/operation/node.py
  • scripts/benchmark_node_list_usage.py
  • tests/test_node_list_usage_aggregate.py

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


Walkthrough

Node lifetime usage can now load as correlated aggregates without loading usage-log history. The WEB node-list operation uses this mode. Tests and a benchmark validate totals, loading behavior, SQL activity, memory, and runtime.

Changes

Node lifetime usage aggregation

Layer / File(s) Summary
Query aggregation and model expressions
app/db/crud/node.py, app/db/models.py
get_nodes can attach correlated uplink and downlink totals. Node exposes these values through query expressions and falls back to usage logs when needed.
WEB node-list integration
app/operation/node.py
get_db_nodes disables usage-log loading and enables lifetime usage aggregation.
Aggregation validation and measurement
tests/test_node_list_usage_aggregate.py, scripts/benchmark_node_list_usage.py
Tests verify totals and query options. The benchmark compares usage-log loading with lifetime aggregation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 7d288

Node list views now calculate lifetime traffic through database aggregates without loading reset-history objects, preserving reported totals while reducing list-query resource use. No merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant WEB NodeOperation
  participant get_nodes
  participant NodeUsageResetLogs
  WEB NodeOperation->>get_nodes: Request lifetime usage without usage logs
  get_nodes->>NodeUsageResetLogs: Sum uplink and downlink per node
  NodeUsageResetLogs-->>get_nodes: Return correlated totals
  get_nodes-->>WEB NodeOperation: Return nodes with lifetime usage values
Loading

Suggested reviewers: immohammad20000, m03ed, x0sina

Poem

A rabbit queried totals with care
No log-history bundles filled the air
Uplink and downlink hopped in line
Tests checked each measured sign
The node list grew swift and fine

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: node-list queries no longer materialize reset history objects, and lifetime usage is aggregated instead.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dr-hoseyn

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@ImMohammad20000
ImMohammad20000 merged commit 472ecb8 into PasarGuard:dev Sep 7, 2026
9 checks passed
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.

2 participants