feat: Support dynamic mobile signal metrics for multiple interfaces #820
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| venv/ | ||
| .git/ | ||
| .github/ | ||
| .tox/ | ||
| htmlcov/ | ||
| build/ | ||
| dist/ | ||
| *.egg-info/ | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,82 @@ | ||||||||||||||||||||||||||||||
| # Manually created | ||||||||||||||||||||||||||||||
| # Updates signal metric names to include the interface name (ifname) prefix, | ||||||||||||||||||||||||||||||
| # matching the new naming convention introduced by writer.py which now uses | ||||||||||||||||||||||||||||||
| # "{ifname} signal strength", "{ifname} signal quality", "{ifname} access technology" | ||||||||||||||||||||||||||||||
| # instead of the old static names "signal strength", "signal quality", "access technology". | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| from django.db import migrations | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| CHUNK_SIZE = 1000 | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| SIGNAL_CONFIGURATIONS = { | ||||||||||||||||||||||||||||||
| "signal_strength": "signal strength", | ||||||||||||||||||||||||||||||
| "signal_quality": "signal quality", | ||||||||||||||||||||||||||||||
| "access_tech": "access technology", | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def forward_migration(apps, schema_editor): | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| Update old signal metric names to include the interface name. | ||||||||||||||||||||||||||||||
| e.g. "signal strength" → "mobile0 signal strength" | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+33
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 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| for metric in metric_qs.iterator(chunk_size=CHUNK_SIZE): | ||||||||||||||||||||||||||||||
| ifname = metric.main_tags.get("ifname", "") | ||||||||||||||||||||||||||||||
| if ifname: | ||||||||||||||||||||||||||||||
| metric.name = f"{ifname} {old_suffix}" | ||||||||||||||||||||||||||||||
| updated_metrics.append(metric) | ||||||||||||||||||||||||||||||
| if len(updated_metrics) >= CHUNK_SIZE: | ||||||||||||||||||||||||||||||
| Metric.objects.bulk_update(updated_metrics, fields=["name"]) | ||||||||||||||||||||||||||||||
| logger.info( | ||||||||||||||||||||||||||||||
| f"Bulk updated {len(updated_metrics)} signal metric names." | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| updated_metrics = [] | ||||||||||||||||||||||||||||||
| if updated_metrics: | ||||||||||||||||||||||||||||||
| Metric.objects.bulk_update(updated_metrics, fields=["name"]) | ||||||||||||||||||||||||||||||
| logger.info(f"Bulk updated {len(updated_metrics)} signal metric names.") | ||||||||||||||||||||||||||||||
| logger.info("Signal metric name migration (forward) completed.") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def reverse_migration(apps, schema_editor): | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| Revert signal metric names back to the old static names. | ||||||||||||||||||||||||||||||
| e.g. "mobile0 signal strength" → "signal strength" | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| Metric = apps.get_model("monitoring", "Metric") | ||||||||||||||||||||||||||||||
| updated_metrics = [] | ||||||||||||||||||||||||||||||
| for configuration, old_suffix in SIGNAL_CONFIGURATIONS.items(): | ||||||||||||||||||||||||||||||
| metric_qs = Metric.objects.filter(configuration=configuration) | ||||||||||||||||||||||||||||||
| for metric in metric_qs.iterator(chunk_size=CHUNK_SIZE): | ||||||||||||||||||||||||||||||
| ifname = metric.main_tags.get("ifname", "") | ||||||||||||||||||||||||||||||
| expected_new_name = f"{ifname} {old_suffix}" if ifname else old_suffix | ||||||||||||||||||||||||||||||
| if metric.name == expected_new_name: | ||||||||||||||||||||||||||||||
| metric.name = old_suffix | ||||||||||||||||||||||||||||||
| updated_metrics.append(metric) | ||||||||||||||||||||||||||||||
| if len(updated_metrics) >= CHUNK_SIZE: | ||||||||||||||||||||||||||||||
| Metric.objects.bulk_update(updated_metrics, fields=["name"]) | ||||||||||||||||||||||||||||||
| updated_metrics = [] | ||||||||||||||||||||||||||||||
| if updated_metrics: | ||||||||||||||||||||||||||||||
| Metric.objects.bulk_update(updated_metrics, fields=["name"]) | ||||||||||||||||||||||||||||||
| logger.info("Signal metric name migration (reverse) completed.") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| class Migration(migrations.Migration): | ||||||||||||||||||||||||||||||
| dependencies = [("monitoring", "0013_replace_jsonfield_with_django_builtin")] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| operations = [ | ||||||||||||||||||||||||||||||
| migrations.RunPython( | ||||||||||||||||||||||||||||||
| forward_migration, | ||||||||||||||||||||||||||||||
| reverse_code=reverse_migration, | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add missing common Python artifact exclusions.
The
.dockerignorefile 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.coverageAdditionally, 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
📝 Committable suggestion
🤖 Prompt for AI Agents