feat: Support dynamic mobile signal metrics for multiple interfaces#820
feat: Support dynamic mobile signal metrics for multiple interfaces#820Yash-Chauhan22 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe pull request adds per-interface chart support for mobile signal metrics. Chart title templates for Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.dockerignore:
- Around line 1-8: The .dockerignore file is missing several standard Python
cache and artifact directories that should be excluded from Docker builds to
reduce image size and prevent unnecessary artifacts. Add the following entries
to the .dockerignore file: __pycache__/ for Python bytecode cache directories,
.pytest_cache/ for pytest artifacts, .mypy_cache/ for mypy type checking cache,
*.pyc for compiled Python files, and .coverage for code coverage reports.
Additionally, consider adding environment file patterns like .env and .env.local
to prevent accidental inclusion of secrets or local configuration in the Docker
image.
In `@docker-compose.yml`:
- Around line 9-10: Add a comment block above the volumes section in
docker-compose.yml that documents the purpose of the `.:/opt/openwisp` volume
mount, clarifies that it is intended for development and testing only, and
explains the security implications such as bypassing `.dockerignore` rules and
exposing sensitive local files. Include best practices guidance recommending
that developers use `.env` files (excluded from version control) for secrets,
ensure sensitive local directories are not mounted into the container, and be
aware that files excluded by `.dockerignore` during the image build will still
be accessible at runtime through this volume mount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba0569d5-39e7-4fc8-8653-dc83262acd6d
📒 Files selected for processing (6)
.dockerignoredocker-compose.ymldocs/user/metrics.rstopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.pyopenwisp_monitoring/monitoring/configuration.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rst
📄 CodeRabbit inference engine (AGENTS.md)
Update docs when behavior, settings, public APIs, setup steps, metrics, or supported versions change.
Files:
docs/user/metrics.rst
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Preserve tenant isolation and object-level permissions for organizations, devices, checks, metrics, charts, alerts, and related data.
Be careful with queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, time series database backends, and dashboard/websocket updates.
Watch for cross-tenant data leaks, permission bypasses, unsafe query construction, excessive metric ingestion, insecure credentials, and secrets.
Preserve validation around metric payloads, device data, alert configuration, time series queries, URLs, and backend credentials.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.
Files:
openwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*test*.py: Add or update tests for every behavior change.
For bug fixes, write the regression test first, run it against the unfixed code, confirm it fails for the expected reason, then implement the fix.
Prefer in-process tests so coverage tools can measure changed code.
When changing APIs, include tests for permissions, validation, filtering, pagination, and tenant boundaries.
Files:
openwisp_monitoring/device/tests/test_api.py
🧠 Learnings (7)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-06-17T20:50:49.946Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 816
File: openwisp_monitoring/device/tests/test_api.py:543-544
Timestamp: 2026-06-17T20:50:49.946Z
Learning: In openwisp-monitoring, when an endpoint is routed using Django’s `path()` with the `<uuid:pk>` path converter, malformed UUID inputs are rejected by Django’s URL dispatcher before your view logic runs (resulting in a 404). During code review, don’t recommend adding regression tests that only validate Django’s framework-level malformed-UUID → 404 behavior; instead, focus tests on application-specific behavior (permissions, view/business logic, database effects, and response content) that runs inside the view.
Applied to files:
openwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-06-10T17:25:49.606Z
Learnt from: pandafy
Repo: openwisp/openwisp-monitoring PR: 815
File: openwisp_monitoring/device/tests/test_admin.py:964-964
Timestamp: 2026-06-10T17:25:49.606Z
Learning: In this repo’s OpenWISP monitoring test suite, `_get_org()` is an intentionally-used helper imported/provided via upstream base test mixins (e.g., from openwisp-users/openwisp-utils). During code review, do not flag `_get_org()` as “undefined” or suggest replacing it with `_create_org()`. Treat them as distinct: `_get_org()` retrieves an existing organization, while `_create_org()` creates a new one.
Applied to files:
openwisp_monitoring/device/tests/test_api.py
🔇 Additional comments (4)
openwisp_monitoring/monitoring/configuration.py (1)
575-575: LGTM!Also applies to: 601-601, 626-626
openwisp_monitoring/device/writer.py (1)
80-86: LGTM!Also applies to: 201-203, 221-229, 252-276
openwisp_monitoring/device/tests/test_api.py (1)
1011-1075: LGTM!docs/user/metrics.rst (1)
210-228: LGTM!Also applies to: 239-257, 268-286
| venv/ | ||
| .git/ | ||
| .github/ | ||
| .tox/ | ||
| htmlcov/ | ||
| build/ | ||
| dist/ | ||
| *.egg-info/ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add missing common Python artifact exclusions.
The .dockerignore file is missing several standard Python cache and artifact directories that should be excluded from Docker builds:
__pycache__/(essential—generated during any Python execution).pytest_cache/.mypy_cache/*.pyc.coverage
Additionally, if environment files (.env, .env.local, etc.) may be created or exist in the build context, consider excluding them to prevent accidental inclusion of secrets in the image.
🐳 Proposed additions to .dockerignore
venv/
.git/
.github/
.tox/
htmlcov/
build/
dist/
*.egg-info/
+__pycache__/
+.pytest_cache/
+.mypy_cache/
+*.pyc
+.coverage
+.env
+.env.local📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| venv/ | |
| .git/ | |
| .github/ | |
| .tox/ | |
| htmlcov/ | |
| build/ | |
| dist/ | |
| *.egg-info/ | |
| venv/ | |
| .git/ | |
| .github/ | |
| .tox/ | |
| htmlcov/ | |
| build/ | |
| dist/ | |
| *.egg-info/ | |
| __pycache__/ | |
| .pytest_cache/ | |
| .mypy_cache/ | |
| *.pyc | |
| .coverage | |
| .env | |
| .env.local |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dockerignore around lines 1 - 8, The .dockerignore file is missing several
standard Python cache and artifact directories that should be excluded from
Docker builds to reduce image size and prevent unnecessary artifacts. Add the
following entries to the .dockerignore file: __pycache__/ for Python bytecode
cache directories, .pytest_cache/ for pytest artifacts, .mypy_cache/ for mypy
type checking cache, *.pyc for compiled Python files, and .coverage for code
coverage reports. Additionally, consider adding environment file patterns like
.env and .env.local to prevent accidental inclusion of secrets or local
configuration in the Docker image.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| self.assertIn("Signal Quality (RSRQ): mobile0", chart_titles) | ||
| self.assertIn("Signal Quality (RSRQ): mobile1", chart_titles) | ||
| self.assertIn("Access Technology: mobile0", chart_titles) | ||
| self.assertIn("Access Technology: mobile1", chart_titles) |
There was a problem hiding this comment.
CRITICAL: Per-interface mobile charts display aggregated (incorrect) data.
This test verifies chart titles and counts, but not that each interface's chart returns interface-specific data. The signal_strength, signal_quality, and access_tech InfluxDB query templates in openwisp_monitoring/db/backends/influxdb/queries.py filter only by content_type and object_id — they do not filter by ifname, unlike the traffic and wifi_clients query templates (which include AND ifname = '{ifname}').
With multiple mobile interfaces, each per-interface chart runs the same query, so they aggregate data across all interfaces and return identical values. For this test's data, both the mobile0 and mobile1 signal-strength charts would show the mean of -51 and -55 (-53) instead of -51 and -55 respectively, defeating the purpose of separate per-interface charts.
Consider asserting per-interface summary values (e.g. mobile0 RSSI == -51.0, mobile1 RSSI == -55.0) and adding AND ifname = '{ifname}' to the three mobile query templates so the charts are truly scoped per interface.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The previously flagged CRITICAL issue (per-interface mobile charts returning aggregated data because the InfluxDB query templates lacked an Overview
Files Reviewed (4 files)
Previous Review Summaries (3 snapshots, latest commit 0c1af09)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0c1af09)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (7 files)
Note: The CRITICAL finding lives in Fix these issues in Kilo Cloud Previous review (commit f4d870b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (6 files)
Note: The CRITICAL finding lives in Fix these issues in Kilo Cloud Previous review (commit c1b0499)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (6 files)
Note: The CRITICAL finding lives in Reviewed by glm-5.2-20260616 · Input: 299.8K · Output: 5.9K · Cached: 156.6K |
ReStructuredText Formatting FailureHello @Yash-Chauhan22, The CI is failing due to a ReStructuredText formatting issue in the documentation. Failure:
Fix: Please reformat the If you have |
Yash-Chauhan22
force-pushed
the
feature/dynamic-mobile-metrics
branch
from
c1b0499 to
f4d870b
Compare
ReStructuredText Formatting ErrorsHello @Yash-Chauhan22, The CI failed because of ReStructuredText formatting errors in the documentation. To fix this, please run the following command in your local environment: openwisp-qa-format --fix-rstThis command will automatically reformat the ReStructuredText files in your project, including the |
Yash-Chauhan22
force-pushed
the
feature/dynamic-mobile-metrics
branch
from
f4d870b to
0c1af09
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.dockerignore (1)
1-8: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd missing common Python artifact exclusions to reduce image size and security surface.
The
.dockerignorefile is missing several standard Python cache and artifact directories that are generated during Python execution and should not be included in the Docker image. These add unnecessary size and, in the case of environment files, pose a security risk.📦 Proposed additions to .dockerignore
venv/ .git/ .github/ .tox/ htmlcov/ build/ dist/ *.egg-info/ +__pycache__/ +.pytest_cache/ +.mypy_cache/ +*.pyc +.coverage +.env +.env.localThe additions exclude:
__pycache__/and*.pyc: Python bytecode caches (regenerated at runtime).pytest_cache/and.mypy_cache/: Test and type-checking artifacts.coverage: Local code coverage reports.envand.env.local: Environment files that may contain secrets or local configuration🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.dockerignore around lines 1 - 8, The .dockerignore file is missing several standard Python cache and artifact directories that should be excluded to reduce image size and security risks. Add the following entries to the .dockerignore file: __pycache__/, *.pyc, .pytest_cache/, .mypy_cache/, .coverage, .env, and .env.local. These entries will prevent Python bytecode caches, test artifacts, type-checking caches, coverage reports, and potentially sensitive environment files from being included in the Docker image.docker-compose.yml (1)
9-10: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocument the purpose and security implications of the volume mount.
The volume mount
.:/opt/openwispenables live code reloading but bypasses the.dockerignorerules applied during image build. This means files excluded by.dockerignore(e.g.,venv/,.git/,.env) are still accessible in the container if they exist locally. Add a comment documenting the purpose, that this is for development/testing, and clarifying best practices for handling secrets in this setup.📝 Proposed documentation comment
ports: - "8000:8000" + # Volume mount for development and testing: + # - Enables live code reloading without rebuilding the image. + # - Bypasses .dockerignore rules—files excluded during build are still accessible if they exist locally. + # - WARNING: Local .env files and other sensitive artifacts will be mounted into the container. + # - Use .env.local (excluded from .dockerignore) for local secrets, never commit secrets to version control. volumes: - .:/opt/openwisp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` around lines 9 - 10, Add a comment block above the volumes section in the docker-compose.yml file that documents the purpose and security implications of the `.:/opt/openwisp` volume mount. The comment should explain that this volume enables live code reloading for development, clarify that it bypasses .dockerignore filtering and makes locally excluded files (such as venv/, .git/, .env) accessible within the container, note that this setup is intended only for development and testing environments, and include a warning about best practices for handling sensitive credentials and secrets to avoid exposing them through this volume mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.dockerignore:
- Around line 1-8: The .dockerignore file is missing several standard Python
cache and artifact directories that should be excluded to reduce image size and
security risks. Add the following entries to the .dockerignore file:
__pycache__/, *.pyc, .pytest_cache/, .mypy_cache/, .coverage, .env, and
.env.local. These entries will prevent Python bytecode caches, test artifacts,
type-checking caches, coverage reports, and potentially sensitive environment
files from being included in the Docker image.
In `@docker-compose.yml`:
- Around line 9-10: Add a comment block above the volumes section in the
docker-compose.yml file that documents the purpose and security implications of
the `.:/opt/openwisp` volume mount. The comment should explain that this volume
enables live code reloading for development, clarify that it bypasses
.dockerignore filtering and makes locally excluded files (such as venv/, .git/,
.env) accessible within the container, note that this setup is intended only for
development and testing environments, and include a warning about best practices
for handling sensitive credentials and secrets to avoid exposing them through
this volume mount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34a2cde0-2f68-4ff1-a172-00278bb76d41
📒 Files selected for processing (7)
.dockerignoredocker-compose.ymldocs/user/metrics.rstopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.pyopenwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/monitoring/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Preserve tenant isolation and object-level permissions for organizations, devices, checks, metrics, charts, alerts, and related data.
Be careful with queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, time series database backends, and dashboard/websocket updates.
Watch for cross-tenant data leaks, permission bypasses, unsafe query construction, excessive metric ingestion, insecure credentials, and secrets.
Preserve validation around metric payloads, device data, alert configuration, time series queries, URLs, and backend credentials.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.
Files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*test*.py: Add or update tests for every behavior change.
For bug fixes, write the regression test first, run it against the unfixed code, confirm it fails for the expected reason, then implement the fix.
Prefer in-process tests so coverage tools can measure changed code.
When changing APIs, include tests for permissions, validation, filtering, pagination, and tenant boundaries.
Files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/device/tests/test_api.py
**/*.rst
📄 CodeRabbit inference engine (AGENTS.md)
Update docs when behavior, settings, public APIs, setup steps, metrics, or supported versions change.
Files:
docs/user/metrics.rst
🧠 Learnings (7)
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-06-17T20:50:49.946Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 816
File: openwisp_monitoring/device/tests/test_api.py:543-544
Timestamp: 2026-06-17T20:50:49.946Z
Learning: In openwisp-monitoring, when an endpoint is routed using Django’s `path()` with the `<uuid:pk>` path converter, malformed UUID inputs are rejected by Django’s URL dispatcher before your view logic runs (resulting in a 404). During code review, don’t recommend adding regression tests that only validate Django’s framework-level malformed-UUID → 404 behavior; instead, focus tests on application-specific behavior (permissions, view/business logic, database effects, and response content) that runs inside the view.
Applied to files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/monitoring/configuration.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/writer.py
📚 Learning: 2026-06-10T17:25:49.606Z
Learnt from: pandafy
Repo: openwisp/openwisp-monitoring PR: 815
File: openwisp_monitoring/device/tests/test_admin.py:964-964
Timestamp: 2026-06-10T17:25:49.606Z
Learning: In this repo’s OpenWISP monitoring test suite, `_get_org()` is an intentionally-used helper imported/provided via upstream base test mixins (e.g., from openwisp-users/openwisp-utils). During code review, do not flag `_get_org()` as “undefined” or suggest replacing it with `_create_org()`. Treat them as distinct: `_get_org()` retrieves an existing organization, while `_create_org()` creates a new one.
Applied to files:
openwisp_monitoring/monitoring/tests/test_models.pyopenwisp_monitoring/device/tests/test_api.py
🔇 Additional comments (5)
openwisp_monitoring/monitoring/configuration.py (1)
575-575: LGTM!Also applies to: 601-601, 626-626
openwisp_monitoring/device/writer.py (1)
80-86: LGTM!Also applies to: 201-203, 221-229, 252-276
openwisp_monitoring/device/tests/test_api.py (1)
1011-1075: LGTM!docs/user/metrics.rst (1)
210-228: LGTM!Also applies to: 239-257, 268-286
openwisp_monitoring/monitoring/tests/test_models.py (1)
340-348: LGTM!
ReStructuredText Formatting FailureHello @Yash-Chauhan22, The CI failed because of a ReStructuredText formatting issue in the documentation. Fix: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.py`:
- Around line 27-33: The migration uses Metric.objects directly for queries and
bulk_update operations without specifying the correct database connection. In
multi-database setups, this can cause operations to hit the wrong database. Fix
this by chaining .using(schema_editor.connection.alias) to all Metric.objects
method calls throughout the migration (including filter operations at the lines
you mentioned and the bulk_update operation), ensuring all database operations
use the correct connection alias provided by the schema_editor.
In
`@tests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py`:
- Around line 21-27: The migration functions do not route database operations to
the correct database in multi-database setups. At the beginning of both
migration functions (forwards and backwards), extract the database alias by
adding `db_alias = schema_editor.connection.alias`. Then, modify all queryset
operations on the Metric model to route through the correct database by
appending `.using(db_alias)` to every `Metric.objects` query (both the filter
call shown in the diff and any subsequent update or other database operations).
This ensures all read and write operations in the migration respect the
schema_editor connection alias.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b702329d-a5fd-4bca-b5d6-10eb708452b5
📒 Files selected for processing (3)
openwisp_monitoring/db/backends/influxdb/queries.pyopenwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pytests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pyopenwisp_monitoring/db/backends/influxdb/queries.pytests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Preserve tenant isolation and object-level permissions for organizations, devices, checks, metrics, charts, alerts, and related data.
Be careful with queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, time series database backends, and dashboard/websocket updates.
Watch for cross-tenant data leaks, permission bypasses, unsafe query construction, excessive metric ingestion, insecure credentials, and secrets.
Preserve validation around metric payloads, device data, alert configuration, time series queries, URLs, and backend credentials.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.
Files:
openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pyopenwisp_monitoring/db/backends/influxdb/queries.pytests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py
🧠 Learnings (5)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pyopenwisp_monitoring/db/backends/influxdb/queries.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pyopenwisp_monitoring/db/backends/influxdb/queries.pytests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pyopenwisp_monitoring/db/backends/influxdb/queries.pytests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
tests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py
📚 Learning: 2026-06-17T20:50:49.946Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 816
File: openwisp_monitoring/device/tests/test_api.py:543-544
Timestamp: 2026-06-17T20:50:49.946Z
Learning: In openwisp-monitoring, when an endpoint is routed using Django’s `path()` with the `<uuid:pk>` path converter, malformed UUID inputs are rejected by Django’s URL dispatcher before your view logic runs (resulting in a 404). During code review, don’t recommend adding regression tests that only validate Django’s framework-level malformed-UUID → 404 behavior; instead, focus tests on application-specific behavior (permissions, view/business logic, database effects, and response content) that runs inside the view.
Applied to files:
tests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py
🔇 Additional comments (1)
openwisp_monitoring/db/backends/influxdb/queries.py (1)
83-85: LGTM!Also applies to: 87-93, 94-98
| Metric = apps.get_model("monitoring", "Metric") | ||
| updated_metrics = [] | ||
| for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items(): | ||
| metric_qs = Metric.objects.filter( | ||
| configuration=configuration, | ||
| name=old_suffix, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pyRepository: openwisp/openwisp-monitoring
Length of output: 3942
🏁 Script executed:
# Check if project has multi-database configuration
find . -name "settings*.py" -o -name "settings" -type d | head -5Repository: openwisp/openwisp-monitoring
Length of output: 356
🏁 Script executed:
# Check main settings for database configuration
grep -A 20 "DATABASES" openwisp_monitoring/settings.py | head -30Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check Metric model for tenant isolation and data integrity concerns
grep -n "class Metric" openwisp_monitoring/monitoring/models/*.pyRepository: openwisp/openwisp-monitoring
Length of output: 242
🏁 Script executed:
# Find the Metric model
find . -name "*.py" -path "*/monitoring/*" | xargs grep -l "class Metric" | head -5Repository: openwisp/openwisp-monitoring
Length of output: 251
🏁 Script executed:
# Check for database configuration in test settings
cat openwisp_monitoring/settings.py | head -50Repository: openwisp/openwisp-monitoring
Length of output: 956
🏁 Script executed:
# Check the Metric model for structure and tenant isolation
ast-grep outline openwisp_monitoring/monitoring/models.py | head -100Repository: openwisp/openwisp-monitoring
Length of output: 375
🏁 Script executed:
# Check test settings for database configuration
grep -A 20 "DATABASES" tests/openwisp2/settings.pyRepository: openwisp/openwisp-monitoring
Length of output: 1205
🏁 Script executed:
# Check if other migrations in the same app use db_alias pattern
find openwisp_monitoring/monitoring/migrations -name "*.py" | xargs grep -l "schema_editor.connection.alias" | head -5Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check if other RunPython migrations in the project follow the db_alias pattern
find . -path "*/migrations/*.py" -name "*.py" | xargs grep -B5 -A10 "def.*migration.*apps.*schema_editor" | grep -A10 "schema_editor.connection.alias" | head -40Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check if project documentation or comments mention multi-database support
rg -i "multi.?db|database.*alias|schema_editor" --type md -A 3 -B 3 | head -50Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check other files in the monitoring app for any existing migrations patterns
ls -la openwisp_monitoring/monitoring/migrations/ | head -20Repository: openwisp/openwisp-monitoring
Length of output: 1607
🏁 Script executed:
# Check migration 0013 for comparison - does it use db_alias?
sed -n '1,100p' openwisp_monitoring/monitoring/migrations/0013_replace_jsonfield_with_django_builtin.pyRepository: openwisp/openwisp-monitoring
Length of output: 1974
🏁 Script executed:
# Check migration 0012 for RunPython usage
cat openwisp_monitoring/monitoring/migrations/0012_migrate_signal_metrics.pyRepository: openwisp/openwisp-monitoring
Length of output: 1094
🏁 Script executed:
# Check the influxdb migration helper
cat openwisp_monitoring/monitoring/migrations/influxdb/influxdb_alter_structure_0006.py | head -80Repository: openwisp/openwisp-monitoring
Length of output: 2606
🏁 Script executed:
# Check Django documentation pattern - look for any comments in the codebase about migration best practices
rg -i "runpython|schema_editor" --type py openwisp_monitoring/monitoring/migrations/ -A 2 -B 2 | head -80Repository: openwisp/openwisp-monitoring
Length of output: 6814
🏁 Script executed:
# Verify the exact lines mentioned in the review comment one more time
sed -n '22,72p' openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.py | cat -nRepository: openwisp/openwisp-monitoring
Length of output: 2852
Use the migration connection alias for all ORM reads/writes.
Lines 30, 40, 46, 59, 67, and 70 query and update through Metric.objects directly. In RunPython, this can hit the wrong database in multi-DB setups. Use schema_editor.connection.alias consistently for both queryset iteration and bulk_update.
Suggested patch
def forward_migration(apps, schema_editor):
+ db_alias = schema_editor.connection.alias
Metric = apps.get_model("monitoring", "Metric")
updated_metrics = []
for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items():
- metric_qs = Metric.objects.filter(
+ metric_qs = Metric.objects.using(db_alias).filter(
configuration=configuration,
name=old_suffix,
)
for metric in metric_qs.iterator(chunk_size=CHUNK_SIZE):
@@
updated_metrics.append(metric)
if len(updated_metrics) >= CHUNK_SIZE:
- Metric.objects.bulk_update(updated_metrics, fields=["name"])
+ Metric.objects.using(db_alias).bulk_update(
+ updated_metrics, fields=["name"]
+ )
@@
if updated_metrics:
- Metric.objects.bulk_update(updated_metrics, fields=["name"])
+ Metric.objects.using(db_alias).bulk_update(updated_metrics, fields=["name"])
@@
def reverse_migration(apps, schema_editor):
+ db_alias = schema_editor.connection.alias
Metric = apps.get_model("monitoring", "Metric")
updated_metrics = []
for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items():
- metric_qs = Metric.objects.filter(configuration=configuration)
+ metric_qs = Metric.objects.using(db_alias).filter(configuration=configuration)
for metric in metric_qs.iterator(chunk_size=CHUNK_SIZE):
@@
updated_metrics.append(metric)
if len(updated_metrics) >= CHUNK_SIZE:
- Metric.objects.bulk_update(updated_metrics, fields=["name"])
+ Metric.objects.using(db_alias).bulk_update(
+ updated_metrics, fields=["name"]
+ )
updated_metrics = []
if updated_metrics:
- Metric.objects.bulk_update(updated_metrics, fields=["name"])
+ Metric.objects.using(db_alias).bulk_update(updated_metrics, fields=["name"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Metric = apps.get_model("monitoring", "Metric") | |
| updated_metrics = [] | |
| for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items(): | |
| metric_qs = Metric.objects.filter( | |
| configuration=configuration, | |
| name=old_suffix, | |
| ) | |
| Metric = apps.get_model("monitoring", "Metric") | |
| updated_metrics = [] | |
| for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items(): | |
| metric_qs = Metric.objects.using(db_alias).filter( | |
| configuration=configuration, | |
| name=old_suffix, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.py`
around lines 27 - 33, The migration uses Metric.objects directly for queries and
bulk_update operations without specifying the correct database connection. In
multi-database setups, this can cause operations to hit the wrong database. Fix
this by chaining .using(schema_editor.connection.alias) to all Metric.objects
method calls throughout the migration (including filter operations at the lines
you mentioned and the bulk_update operation), ensuring all database operations
use the correct connection alias provided by the schema_editor.
| Metric = apps.get_model("sample_monitoring", "Metric") | ||
| updated_metrics = [] | ||
| for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items(): | ||
| metric_qs = Metric.objects.filter( | ||
| configuration=configuration, | ||
| name=old_suffix, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
fd -t f "0006_update_signal_metric_names.py" --exec wc -l {} \;Repository: openwisp/openwisp-monitoring
Length of output: 251
🏁 Script executed:
cat -n tests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.pyRepository: openwisp/openwisp-monitoring
Length of output: 3296
🏁 Script executed:
# Search for other migrations using .using() or db_alias pattern
find tests/openwisp2 -name "*.py" -path "*/migrations/*" | head -20 | xargs rg -l "db_alias|\.using\(" 2>/dev/nullRepository: openwisp/openwisp-monitoring
Length of output: 170
🏁 Script executed:
# Check if other migrations in openwisp use db_alias pattern
rg "schema_editor\.connection\.alias" --type py -A 2 -B 2Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Look for Django migration patterns in the codebase
rg "def forward|RunPython" tests/openwisp2 -l --type py | head -10Repository: openwisp/openwisp-monitoring
Length of output: 570
🏁 Script executed:
# Check main project settings for multi-database configuration
find . -name "settings*.py" | head -5 | xargs grep -l "DATABASES" 2>/dev/nullRepository: openwisp/openwisp-monitoring
Length of output: 196
🏁 Script executed:
# Check settings for DATABASES configuration
grep -A 20 "DATABASES" tests/openwisp2/settings.py | head -30Repository: openwisp/openwisp-monitoring
Length of output: 1186
🏁 Script executed:
# Check another migration to see if they use db_alias pattern
cat -n tests/openwisp2/sample_monitoring/migrations/0002_create_general_metric.pyRepository: openwisp/openwisp-monitoring
Length of output: 664
🏁 Script executed:
# Search more broadly for .using() in migrations
rg "\.using\(" tests/openwisp2/sample_monitoring/migrations/ -nRepository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Find migrations in the main monitoring app
find . -path "*/monitoring/migrations/*.py" -name "*.py" | head -20Repository: openwisp/openwisp-monitoring
Length of output: 1512
🏁 Script executed:
# Check the create_general_metrics function
rg "def create_general_metrics|def delete_general_metrics" -A 15 --type pyRepository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Search for db_alias usage in the entire codebase
rg "db_alias|schema_editor\.connection\.alias" . --type py | head -20Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check the main migration that the test migration mirrors
cat -n ./openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pyRepository: openwisp/openwisp-monitoring
Length of output: 3942
🏁 Script executed:
# Search for the create_general_metrics function definition
rg "def create_general_metrics" . -A 20 --type pyRepository: openwisp/openwisp-monitoring
Length of output: 2009
🏁 Script executed:
# Check if there are any migrations in this project that properly use db_alias
rg "connection\.alias" . --type py -B 3 -A 3Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check Django best practices documentation by looking at what patterns exist
rg "using\(" openwisp_monitoring/ --type py | head -15Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Verify the main migration doesn't have db_alias either
grep -n "\.using\|db_alias" ./openwisp_monitoring/monitoring/migrations/0014_update_signal_metric_names.pyRepository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check if the project/library is meant to support multi-DB
rg "DATABASES" . --type py | grep -v test | grep -v ".pyc"Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check README or docs for multi-database mention
find . -maxdepth 2 -name "README*" -o -name "*.md" | xargs grep -l "database\|multi" 2>/dev/null | head -5Repository: openwisp/openwisp-monitoring
Length of output: 178
🏁 Script executed:
# Check if this is a reusable app/library by looking at setup.py or pyproject.toml
cat setup.py 2>/dev/null || cat pyproject.toml 2>/dev/null | head -50Repository: openwisp/openwisp-monitoring
Length of output: 1893
🏁 Script executed:
# Check if there's any discussion or related issues about multi-DB in the project
rg "multi.*database|multiple.*database" . --type py --type md | head -10Repository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Look for any Django router configuration
rg "DATABASE_ROUTERS" . --type pyRepository: openwisp/openwisp-monitoring
Length of output: 166
🏁 Script executed:
# Check the openwisp codebase structure - does it support swappable models (common in reusable apps)?
rg "swapper" openwisp_monitoring/ --type py | head -10Repository: openwisp/openwisp-monitoring
Length of output: 981
Add database alias routing to ensure migrations run on correct database in multi-database setups.
The migration uses Metric.objects for both reads and writes without routing through schema_editor.connection.alias. In multi-database environments, this can cause operations to target the wrong database. Add db_alias = schema_editor.connection.alias at the start of both migration functions and use .using(db_alias) on all queryset operations.
Suggested patch
def forward_migration(apps, schema_editor):
+ db_alias = schema_editor.connection.alias
Metric = apps.get_model("sample_monitoring", "Metric")
updated_metrics = []
for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items():
- metric_qs = Metric.objects.filter(
+ metric_qs = Metric.objects.using(db_alias).filter(
configuration=configuration,
name=old_suffix,
)
@@
updated_metrics.append(metric)
if len(updated_metrics) >= CHUNK_SIZE:
- Metric.objects.bulk_update(updated_metrics, fields=["name"])
+ Metric.objects.using(db_alias).bulk_update(
+ updated_metrics, fields=["name"]
+ )
updated_metrics = []
if updated_metrics:
- Metric.objects.bulk_update(updated_metrics, fields=["name"])
+ Metric.objects.using(db_alias).bulk_update(updated_metrics, fields=["name"])
@@
def reverse_migration(apps, schema_editor):
+ db_alias = schema_editor.connection.alias
Metric = apps.get_model("sample_monitoring", "Metric")
updated_metrics = []
for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items():
- metric_qs = Metric.objects.filter(configuration=configuration)
+ metric_qs = Metric.objects.using(db_alias).filter(configuration=configuration)
@@
updated_metrics.append(metric)
if len(updated_metrics) >= CHUNK_SIZE:
- Metric.objects.bulk_update(updated_metrics, fields=["name"])
+ Metric.objects.using(db_alias).bulk_update(
+ updated_metrics, fields=["name"]
+ )
updated_metrics = []
if updated_metrics:
- Metric.objects.bulk_update(updated_metrics, fields=["name"])
+ Metric.objects.using(db_alias).bulk_update(updated_metrics, fields=["name"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Metric = apps.get_model("sample_monitoring", "Metric") | |
| updated_metrics = [] | |
| for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items(): | |
| metric_qs = Metric.objects.filter( | |
| configuration=configuration, | |
| name=old_suffix, | |
| ) | |
| def forward_migration(apps, schema_editor): | |
| db_alias = schema_editor.connection.alias | |
| Metric = apps.get_model("sample_monitoring", "Metric") | |
| updated_metrics = [] | |
| for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items(): | |
| metric_qs = Metric.objects.using(db_alias).filter( | |
| configuration=configuration, | |
| name=old_suffix, | |
| ) | |
| for metric in metric_qs: | |
| metric.name = metric.name.replace(old_suffix, new_suffix) | |
| updated_metrics.append(metric) | |
| if len(updated_metrics) >= CHUNK_SIZE: | |
| Metric.objects.using(db_alias).bulk_update( | |
| updated_metrics, fields=["name"] | |
| ) | |
| updated_metrics = [] | |
| if updated_metrics: | |
| Metric.objects.using(db_alias).bulk_update(updated_metrics, fields=["name"]) | |
| def reverse_migration(apps, schema_editor): | |
| db_alias = schema_editor.connection.alias | |
| Metric = apps.get_model("sample_monitoring", "Metric") | |
| updated_metrics = [] | |
| for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items(): | |
| metric_qs = Metric.objects.using(db_alias).filter(configuration=configuration) | |
| for metric in metric_qs: | |
| metric.name = metric.name.replace(new_suffix, old_suffix) | |
| updated_metrics.append(metric) | |
| if len(updated_metrics) >= CHUNK_SIZE: | |
| Metric.objects.using(db_alias).bulk_update( | |
| updated_metrics, fields=["name"] | |
| ) | |
| updated_metrics = [] | |
| if updated_metrics: | |
| Metric.objects.using(db_alias).bulk_update(updated_metrics, fields=["name"]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/openwisp2/sample_monitoring/migrations/0006_update_signal_metric_names.py`
around lines 21 - 27, The migration functions do not route database operations
to the correct database in multi-database setups. At the beginning of both
migration functions (forwards and backwards), extract the database alias by
adding `db_alias = schema_editor.connection.alias`. Then, modify all queryset
operations on the Metric model to route through the correct database by
appending `.using(db_alias)` to every `Metric.objects` query (both the filter
call shown in the diff and any subsequent update or other database operations).
This ensures all read and write operations in the migration respect the
schema_editor connection alias.
ReStructuredText Formatting FailureHello @Yash-Chauhan22, The CI failed because of a ReStructuredText formatting issue in the documentation. Fix: |
1 participant
Checklist
Reference to Existing Issue
Closes #781
(Note: Please replace
<issue-number>with your actual issue number)Description of Changes
This pull request introduces support for tracking mobile signal metrics dynamically across multiple mobile interfaces.
Specifically, these changes:
openwisp_monitoring/device/writer.pyto generate unique mobile metrics per interface by adding the interface name (ifname) to theextra_tags(e.g. trackingmobile_rx_bytes,mobile_tx_bytes,mobile_signaluniquely for each).openwisp_monitoring/monitoring/configuration.pyto support dynamic chart titles utilizing the{ifname}placeholder, ensuring separate and clearly identifiable charts are produced for each mobile interface.test_multiple_mobile_charts) intest_api.pyto verify the creation and correct naming of multi-interface charts.docs/user/metrics.rstto document the newextra_tags(likeifnameandoperator) and explain the multi-interface mobile charting behavior.