Skip to content

Migrate bandwidth-history to accumulated HourlyBandwidth data#126

Merged
Yeraze merged 1 commit into
mainfrom
feature/125-bandwidth-accumulated-endpoint
Jun 25, 2026
Merged

Migrate bandwidth-history to accumulated HourlyBandwidth data#126
Yeraze merged 1 commit into
mainfrom
feature/125-bandwidth-accumulated-endpoint

Conversation

@Yeraze

@Yeraze Yeraze commented Jun 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • Migrates /devices/{mac}/bandwidth-history and /network/bandwidth-history from polling-based DeviceConnection rate snapshots (Mbps) to accumulated HourlyBandwidth byte totals
  • Changes API field names from download_mbps/upload_mbps to download_bytes/upload_bytes
  • Updates frontend to convert bytes→MB for display and removes the old rate×interval approximation
  • Fixes UTC timezone bug in signal quality history timestamps (signal_analysis_service.py)
  • Bumps version to 2.9.1

Closes #125

Test plan

  • Verify HourlyBandwidth table has per-device records (device_id IS NOT NULL)
  • Check /devices/{mac}/bandwidth-history returns download_bytes/upload_bytes fields
  • Check /network/bandwidth-history returns download_bytes/upload_bytes fields
  • Confirm hourly bandwidth charts render correctly on device detail pages
  • Confirm signal quality history timestamps display in local timezone

Authored by Merlin 🪄

…ta (#125)

Switch per-device and network hourly bandwidth endpoints from polling-based
DeviceConnection rate snapshots to accumulated HourlyBandwidth byte totals.
Also fixes UTC timezone bug in signal quality history timestamps.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown

Code Review

This PR migrates both bandwidth-history endpoints from polling-based DeviceConnection rate snapshots to accumulated HourlyBandwidth byte totals, updates the frontend to convert bytes→MB, and fixes a UTC timezone bug in signal quality history timestamps. The direction is correct and the overall structure is clean — a few issues worth addressing before merge.


🔴 Critical — Data integrity: collector upsert silently inserts duplicates instead of updating

File: src/services/data_usage_collector.py (pre-existing, but this PR now relies on the data)

The _store_hourly_values method filters for an existing network-wide record using:

HourlyBandwidth.device_id == device_id   # where device_id is None

In SQL, col = NULL always evaluates to NULL (never TRUE), so this SELECT always returns zero rows. Every collector run falls into the else branch and inserts a new duplicate row rather than updating the existing one. Since this PR's routes now query with .is_(None) (correctly), they return all duplicate rows and sum them — producing bandwidth totals that grow with each collection interval.

The fix is to use .is_(None) in the lookup as well:

HourlyBandwidth.device_id.is_(device_id)  # or .is_(None) when device_id is None

The same issue appears in the DailyBandwidth upsert. This is a pre-existing bug, but since the PR is explicitly migrating routes to depend on HourlyBandwidth data, it should be fixed here or the migration will silently produce inflated bandwidth figures from day one.


🟡 High — Null guard removed from single-device chart but kept in group chart

File: src/templates/devices.html, ~line 1575

The old code guarded against null fields before accumulating:

if (point.download_mbps !== null && point.upload_mbps !== null) { ... }

The new single-device chart drops this guard:

hourlyData[hour].download_mb += point.download_bytes / 1000000;  // no null check
hourlyData[hour].upload_mb   += point.upload_bytes  / 1000000;

But the group-member chart (line 1790) retains a null check:

if (timestamp >= todayMidnight && point.download_bytes !== null && point.upload_bytes !== null)

HourlyBandwidth.download_bytes / upload_bytes are declared Mapped[float] with an ORM-level default=0.0, but no nullable=False database constraint — so rows written outside the ORM (migrations, direct SQL) could legally store NULL. If any null reaches the single-device chart, null / 1000000 produces NaN, which propagates silently through the reduce totals and corrupts the summary display. Add the same null guard the group chart already has.


🟡 Medium — Tests never seed HourlyBandwidth rows; field shape is untested

File: tests/test_health_routes.py, ~lines 924 and 1023

Both test_bandwidth_history_success and test_network_bandwidth_history_success assert top-level response keys (mac_address, data_points, hours) but never seed any HourlyBandwidth rows in the test fixture. As a result, data["history"] is always [] and no history-item field assertions are ever evaluated. If someone reverts the download_bytes/upload_bytes rename back to download_mbps/upload_mbps, both tests still pass. Consider adding a fixture row and asserting the item shape, e.g.:

assert data["history"][0]["download_bytes"] is not None
assert "download_mbps" not in data["history"][0]

🔵 Minor — from datetime import timedelta inside function is redundant

File: src/api/health/routes.py, inside get_network_bandwidth_history

timedelta is already imported at module scope (line 6). The in-function from datetime import timedelta import is a no-op on every request. It was present in the old code and carried over; removing it keeps things clean.


Summary

Severity Finding
🔴 Critical Collector device_id == None lookup never matches; inserts duplicate rows on every run
🟡 High Single-device hourly chart drops null guard that group chart keeps; NaN propagates silently
🟡 Medium Bandwidth-history tests never seed HourlyBandwidth rows; new field names unverified
🔵 Minor Redundant from datetime import timedelta import inside function body

The signal-quality timezone fix looks correct. The timezone-cutoff logic in both updated endpoints is consistent and matches the established pattern in the file.

Review by Claude Sonnet 4.6

@Yeraze
Yeraze merged commit 64fc657 into main Jun 25, 2026
7 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.

Individual device usage and top bandwidth consumers

1 participant