Hybrid Speedtest implementation (Python-First with Native Opt-in) #1370
Conversation
📝 WalkthroughWalkthroughAdds optional native Ookla speedtest binary execution (configurable path) with timeout-aware subprocess and JSON parsing; falls back to the existing Python CLI on failures/timeouts; plugin results now include a propagated Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as "API Server\n(nettools_endpoint)"
participant Native as "Native Binary\n(NATIVE_SPEEDTEST_PATH)"
participant CLI as "speedtest-cli\n(baseline)"
Client->>API: GET /speedtest
Note right of API `#f2f4f8`: Attempt native binary first (60s timeout)
API->>Native: exec --format=json --accept-license --accept-gdpr (60s)
alt Native returns valid JSON (exit 0)
Native-->>API: JSON output
API->>API: parse → download/upload/latency/ISP/server, full_json
API-->>Client: 200 OK + structured output (includes full_json)
else Native missing / timeout / non-zero / parse error
Native--xAPI: Error / Timeout / Invalid JSON
Note right of API `#fff4e6`: Log error and fallback to CLI (60s)
API->>CLI: exec --secure --simple (60s)
alt CLI returns success
CLI-->>API: CLI output lines
API->>API: build result (download/upload, full_json if available)
API-->>Client: 200 OK + output_lines (full_json may be baseline JSON or error string)
else CLI timeout
CLI--xAPI: TimeoutExpired
API-->>Client: 504 Gateway Timeout
end
end
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
🧹 Nitpick comments (3)
front/plugins/internet_speedtest/script.py (3)
53-54: Consider making the binary path configurable.The binary path is hardcoded to
/usr/bin/speedtest. While acceptable for the default installation, consider usingget_setting_value()to allow users to override the path if they've installed the binary elsewhere.🔎 Proposed change to support configurable path
def run_speedtest(): - native_bin = '/usr/bin/speedtest' + native_bin = get_setting_value('INTRSPD_BINARY_PATH') or '/usr/bin/speedtest' cmd = [native_bin, "--format=json", "--accept-license", "--accept-gdpr"]Then add to the plugin's configuration manifest or
server/initialise.py:ccd( 'INTRSPD_BINARY_PATH', '/usr/bin/speedtest', 'str', 'Path to native Ookla speedtest binary' )
57-62: Adjust log levels for important messages.Per coding guidelines, use
'none'level for the most important messages that should always appear, such as test results and exceptions. Reserve'verbose'for extra context.Current usage logs critical information (results, errors) at
'verbose', which may be missed in default configurations.🔎 Proposed log level adjustments
try: - mylog('verbose', [f"[INTRSPD] Executing native binary: {native_bin}"]) + mylog('debug', [f"[INTRSPD] Executing native binary: {native_bin}"]) result = subprocess.run(cmd, capture_output=True, text=True) if result.returncode != 0: - mylog('verbose', [f"[INTRSPD] Native binary failed: {result.stderr}"]) + mylog('none', [f"[INTRSPD] Native binary failed (exit {result.returncode}): {result.stderr}"]) return {'down': 0.0, 'up': 0.0, 'json': '{}'} o = json.loads(result.stdout) down_mbps = round((o['download']['bandwidth'] * 8) / 10**6, 2) up_mbps = round((o['upload']['bandwidth'] * 8) / 10**6, 2) # Payload optimized for n8n/webhooks payload = { "download": int(o['download']['bandwidth'] * 8), "upload": int(o['upload']['bandwidth'] * 8), "ping": o['ping']['latency'], "server": {"name": o['server']['name']}, "timestamp": o['timestamp'] } - mylog('verbose', [f"[INTRSPD] Result (down|up): {down_mbps} Mbps | {up_mbps} Mbps"]) + mylog('none', [f"[INTRSPD] Result: ↓ {down_mbps} Mbps | ↑ {up_mbps} Mbps | Ping: {payload['ping']} ms"]) return { 'down': down_mbps, 'up': up_mbps, 'json': json.dumps(payload) } except Exception as e: - mylog('verbose', [f"[INTRSPD] Error: {str(e)}"]) + mylog('none', [f"[INTRSPD] Error: {str(e)}"]) return {'down': 0.0, 'up': 0.0, 'json': json.dumps({"error": str(e)})}Also applies to: 78-78, 87-87
86-88: Refine exception handling to catch specific exceptions.The broad
except Exceptioncatches all exceptions, includingKeyboardInterruptandSystemExit. Catching specific exceptions improves debuggability and allows unexpected errors to surface properly.🔎 Proposed refinement with specific exception types
- except Exception as e: - mylog('none', [f"[INTRSPD] Error: {str(e)}"]) + except subprocess.TimeoutExpired: + mylog('none', [f"[INTRSPD] Speedtest timed out after {timeout_sec}s"]) + return {'down': 0.0, 'up': 0.0, 'json': json.dumps({"error": "timeout"})} + except subprocess.SubprocessError as e: + mylog('none', [f"[INTRSPD] Subprocess error: {e}"]) + return {'down': 0.0, 'up': 0.0, 'json': json.dumps({"error": f"subprocess: {e}"})} + except (json.JSONDecodeError, KeyError) as e: + mylog('none', [f"[INTRSPD] Failed to parse speedtest output: {e}"]) + return {'down': 0.0, 'up': 0.0, 'json': json.dumps({"error": f"parse: {e}"})} + except Exception as e: + mylog('none', [f"[INTRSPD] Unexpected error: {e}"]) return {'down': 0.0, 'up': 0.0, 'json': json.dumps({"error": str(e)})}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfilefront/plugins/internet_speedtest/script.pyinstall/proxmox/requirements.txtinstall/ubuntu24/requirements.txtrequirements.txt
💤 Files with no reviewable changes (3)
- install/proxmox/requirements.txt
- install/ubuntu24/requirements.txt
- requirements.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
front/plugins/internet_speedtest/script.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
🧠 Learnings (6)
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.
Applied to files:
Dockerfile
📚 Learning: 2025-09-20T14:13:33.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:11-16
Timestamp: 2025-09-20T14:13:33.635Z
Learning: In the NetAlertX devcontainer setup, the `python -m venv /opt/venv` command works successfully on Alpine 3.22 despite the typical Alpine behavior of not providing a /usr/bin/python symlink by default. The build completes successfully and pytest runs without issues.
Applied to files:
Dockerfile
📚 Learning: 2025-09-20T14:13:33.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:11-16
Timestamp: 2025-09-20T14:13:33.635Z
Learning: In the NetAlertX repository with Alpine 3.22 base image, the `python -m venv` command works correctly in the devcontainer setup, likely due to symlink creation in the root Dockerfile that makes `python` available as an alias to `python3`.
Applied to files:
Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding paths like `/data/db` or relative paths.
Applied to files:
Dockerfile
📚 Learning: 2025-09-20T14:09:29.159Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Applied to files:
Dockerfile
📚 Learning: 2025-10-26T17:09:18.621Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: .devcontainer/scripts/setup.sh:146-148
Timestamp: 2025-10-26T17:09:18.621Z
Learning: In `.devcontainer/scripts/setup.sh` and other devcontainer setup scripts for NetAlertX, chmod 666 on /var/run/docker.sock is acceptable because devcontainer environments are single-user development contexts where convenience can take priority over strict permission hardening.
Applied to files:
Dockerfile
🧬 Code graph analysis (1)
front/plugins/internet_speedtest/script.py (3)
server/logger.py (1)
mylog(79-84)front/plugins/plugin_helper.py (2)
Plugin_Objects(251-310)add_object(262-292)server/utils/datetime_utils.py (1)
timeNowDB(41-61)
🪛 Ruff (0.14.10)
front/plugins/internet_speedtest/script.py
58-58: subprocess call: check for execution of untrusted input
(S603)
86-86: Do not catch blind exception: Exception
(BLE001)
87-87: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (3)
Dockerfile (1)
135-135: LGTM! Appropriate runtime dependency for glibc compatibility.Adding
gcompatenables running the glibc-linked Ookla binary on Alpine's musl libc without switching to a heavier base image.front/plugins/internet_speedtest/script.py (2)
5-6: LGTM! Required imports for native binary invocation.The subprocess and json imports are necessary for calling the native speedtest binary and parsing its JSON output.
32-49: LGTM! Clean integration with Plugin_Objects.The main function correctly consumes the new speed_data structure and maps download/upload speeds and webhook payload to the appropriate watched fields.
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
85f0afb to
1d8f03c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
front/plugins/internet_speedtest/script.py (2)
67-69: Use 'none' log level for error conditions.Per coding guidelines,
mylog('none', ...)should be used for important messages that should always appear, such as exceptions and errors. The binary failure is an error condition worth logging at the highest priority.🔎 Proposed fix
if result.returncode != 0: - mylog('verbose', [f"[INTRSPD] Native binary failed: {result.stderr}"]) + mylog('none', [f"[INTRSPD] Native binary failed: {result.stderr}"]) return {'down': 0.0, 'up': 0.0, 'json': '{}'}
96-104: Consider using f-string conversion flags for consistency.The static analysis suggests using explicit conversion flags (
{e!s}) instead ofstr(e)for cleaner f-string syntax. This is a minor style improvement.🔎 Proposed fix
except json.JSONDecodeError as e: - mylog('none', [f"[INTRSPD] Failed to parse JSON output: {str(e)}"]) + mylog('none', [f"[INTRSPD] Failed to parse JSON output: {e!s}"]) return {'down': 0.0, 'up': 0.0, 'json': json.dumps({"error": "json_parse_error"})} except subprocess.SubprocessError as e: - mylog('none', [f"[INTRSPD] Subprocess error: {str(e)}"]) + mylog('none', [f"[INTRSPD] Subprocess error: {e!s}"]) return {'down': 0.0, 'up': 0.0, 'json': json.dumps({"error": str(e)})} except Exception as e: - mylog('none', [f"[INTRSPD] Unexpected error: {str(e)}"]) + mylog('none', [f"[INTRSPD] Unexpected error: {e!s}"]) return {'down': 0.0, 'up': 0.0, 'json': json.dumps({"error": str(e)})}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Dockerfilefront/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyinstall/proxmox/requirements.txtinstall/ubuntu24/requirements.txtrequirements.txt
💤 Files with no reviewable changes (3)
- requirements.txt
- install/proxmox/requirements.txt
- install/ubuntu24/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
🧰 Additional context used
📓 Path-based instructions (3)
front/plugins/*/config.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/config.json: Plugin manifest lives atfront/plugins/<code_name>/config.json;code_namemust equal folder name, andunique_prefixdrives settings and filenames (e.g.,ARPSCAN).
Control plugins via settings:<PREF>_RUN(phase),<PREF>_RUN_SCHD(cron-like),<PREF>_CMD(script path),<PREF>_RUN_TIMEOUT,<PREF>_WATCH(diff columns).
Files:
front/plugins/internet_speedtest/config.json
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
front/plugins/internet_speedtest/script.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
🧠 Learnings (2)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to front/plugins/*/config.json : Control plugins via settings: `<PREF>_RUN` (phase), `<PREF>_RUN_SCHD` (cron-like), `<PREF>_CMD` (script path), `<PREF>_RUN_TIMEOUT`, `<PREF>_WATCH` (diff columns).
Applied to files:
front/plugins/internet_speedtest/config.json
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
front/plugins/internet_speedtest/script.py
🧬 Code graph analysis (1)
front/plugins/internet_speedtest/script.py (4)
server/logger.py (1)
mylog(79-84)front/plugins/plugin_helper.py (2)
Plugin_Objects(251-310)add_object(262-292)server/utils/datetime_utils.py (1)
timeNowDB(41-61)server/helper.py (1)
get_setting_value(200-257)
🪛 Ruff (0.14.10)
front/plugins/internet_speedtest/script.py
65-65: subprocess call: check for execution of untrusted input
(S603)
97-97: Use explicit conversion flag
Replace with conversion flag
(RUF010)
100-100: Use explicit conversion flag
Replace with conversion flag
(RUF010)
102-102: Do not catch blind exception: Exception
(BLE001)
103-103: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (4)
front/plugins/internet_speedtest/config.json (1)
557-593: LGTM - New TIMEOUT setting is well-structured.The setting correctly defines the configurable timeout for the native speedtest binary, with appropriate localization and a sensible default of 60 seconds. The minimum enforcement is handled in the script code.
Minor observation: Line 592 contains an extra blank line before the closing brace, which is atypical JSON formatting but syntactically valid.
front/plugins/internet_speedtest/script.py (3)
5-6: LGTM - Clean import updates for native binary approach.The replacement of the
speedtest-clilibrary withsubprocessandjsonimports aligns with the PR objective to use the native Ookla binary.
32-49: LGTM - main() follows plugin guidelines.The function correctly uses
Plugin_Objects, maps the speedtest results to the appropriate watched values as documented in the PR objectives, and callswrite_result_file()exactly once at the end per the coding guidelines.
52-65: Timeout handling and subprocess call are well-implemented.The timeout configuration with minimum 60-second enforcement addresses the previous review concern. The explicit timeout parameter in
subprocess.run()follows the coding guidelines requiring explicit timeouts on subprocess calls.Note: The static analysis warning S603 about "untrusted input" is a false positive since the command is entirely hardcoded with no user-controlled components.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/plugins/internet_speedtest/config.json (1)
627-636: Update outdated description for Watched_Value3.The description states
Watched_Value3is "unused", but according to the PR objectives and script implementation, it now contains the optimized JSON payload for webhooks and n8n consumers (download/upload bandwidth, ping, server, timestamp).🔎 Proposed fix to update the description
"description": [ { "language_code": "en_us", - "string": "Send a notification if selected values change. Use <code>CTRL + Click</code> to select/deselect. <ul> <li><code>Watched_Value1</code> is Download speed (not recommended)</li><li><code>Watched_Value2</code> is Upload speed (not recommended)</li><li><code>Watched_Value3</code> unused </li><li><code>Watched_Value4</code> unused </li></ul>" + "string": "Send a notification if selected values change. Use <code>CTRL + Click</code> to select/deselect. <ul> <li><code>Watched_Value1</code> is Download speed (not recommended)</li><li><code>Watched_Value2</code> is Upload speed (not recommended)</li><li><code>Watched_Value3</code> is JSON payload for webhooks </li><li><code>Watched_Value4</code> unused </li></ul>" }, { "language_code": "de_de", - "string": "Sende eine Benachrichtigung, wenn ein ausgwählter Wert sich ändert. <code>STRG + klicken</code> zum aus-/abwählen. <ul> <li><code>Watched_Value1</code> ist die Download-Geschwindigkeit (nicht empfohlen)</li><li><code>Watched_Value2</code> ist die Upload-Geschwindigkeit (nicht empfohlen)</li><li><code>Watched_Value3</code> ist nicht in Verwendung </li><li><code>Watched_Value4</code> ist nicht in Verwendung </li></ul>" + "string": "Sende eine Benachrichtigung, wenn ein ausgwählter Wert sich ändert. <code>STRG + klicken</code> zum aus-/abwählen. <ul> <li><code>Watched_Value1</code> ist die Download-Geschwindigkeit (nicht empfohlen)</li><li><code>Watched_Value2</code> ist die Upload-Geschwindigkeit (nicht empfohlen)</li><li><code>Watched_Value3</code> ist JSON-Payload für Webhooks </li><li><code>Watched_Value4</code> ist nicht in Verwendung </li></ul>" } ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
front/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.py
🧰 Additional context used
📓 Path-based instructions (3)
front/plugins/*/config.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/config.json: Plugin manifest lives atfront/plugins/<code_name>/config.json;code_namemust equal folder name, andunique_prefixdrives settings and filenames (e.g.,ARPSCAN).
Control plugins via settings:<PREF>_RUN(phase),<PREF>_RUN_SCHD(cron-like),<PREF>_CMD(script path),<PREF>_RUN_TIMEOUT,<PREF>_WATCH(diff columns).
Files:
front/plugins/internet_speedtest/config.json
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
front/plugins/internet_speedtest/script.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
🧠 Learnings (2)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to front/plugins/*/config.json : Control plugins via settings: `<PREF>_RUN` (phase), `<PREF>_RUN_SCHD` (cron-like), `<PREF>_CMD` (script path), `<PREF>_RUN_TIMEOUT`, `<PREF>_WATCH` (diff columns).
Applied to files:
front/plugins/internet_speedtest/config.json
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
front/plugins/internet_speedtest/script.py
🧬 Code graph analysis (1)
front/plugins/internet_speedtest/script.py (4)
server/logger.py (1)
mylog(79-84)front/plugins/plugin_helper.py (2)
Plugin_Objects(251-310)add_object(262-292)server/utils/datetime_utils.py (1)
timeNowDB(41-61)server/helper.py (1)
get_setting_value(200-257)
🪛 Ruff (0.14.10)
front/plugins/internet_speedtest/script.py
65-65: subprocess call: check for execution of untrusted input
(S603)
106-106: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
front/plugins/internet_speedtest/script.py (4)
32-50: LGTM! Clean integration with new data structure.The
main()function correctly consumes the new structured return format fromrun_speedtest(), mapping thedown,up, andjsonkeys to the appropriate watched values. The log message clearly indicates the native binary optimization, and the Plugin_Objects pattern is used correctly per coding guidelines.
52-62: Excellent timeout implementation per coding guidelines.The timeout configuration correctly:
- Uses
get_setting_value()to retrieve the configurable timeout- Enforces the minimum 60-second requirement as mandated by coding guidelines
- Handles type conversion errors gracefully with appropriate fallback
- Addresses the previous review feedback about explicit timeout handling
The native binary path and command flags are appropriate for the controlled container environment.
As per coding guidelines:
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise.
63-95: Robust implementation with comprehensive data processing.The success path implementation is excellent:
Subprocess execution (line 65): Correctly includes
timeout,capture_output, andtextparameters. The static analysis warning about untrusted input (S603) is a false positive—the binary path and flags are hardcoded and safe.Error detection (lines 67-69): Non-zero return codes are properly logged at the
nonelevel (most important messages) and return safe fallback values.JSON parsing (lines 71-87): The nested try-except structure correctly addresses the previous review feedback about KeyError handling. Catches
KeyError,ValueError, andTypeErrorto handle unexpected JSON structures gracefully.Data transformations (lines 74-75, 78-84): Bandwidth conversions are mathematically correct (bytes/sec × 8 ÷ 10⁶ = Mbps), and the webhook payload includes all relevant fields (bandwidth in bits/sec as integers, ping, server, timestamp).
Error diagnostics (line 86): Including raw output in error payloads aids debugging without security concerns (speedtest output is benign).
97-108: Comprehensive error handling with appropriate fail-safe.The error handling is well-structured with specific exceptions for different failure modes:
- TimeoutExpired (lines 97-99): Handles subprocess timeouts with clear messaging
- JSONDecodeError (lines 100-102): Specific to JSON parsing failures
- SubprocessError (lines 103-105): Catches subprocess-specific issues
The final generic
Exceptioncatch (lines 106-108) is flagged by the linter (BLE001) but is appropriate in this context because:
- It serves as a terminal fail-safe to prevent plugin crashes
- The exception is fully logged with
{e!s}conversion for diagnostics- Returns a safe, consistent structure that won't break downstream processing
- All error cases use the
nonelog level as mandated by coding guidelines for exceptionsAll error paths maintain consistent return structure with descriptive error information preserved in the
jsonfield for webhook consumers.As per coding guidelines:
Use
nonefor most important messages that should always appear, such as exceptions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
front/plugins/internet_speedtest/config.json
🧰 Additional context used
📓 Path-based instructions (1)
front/plugins/*/config.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/config.json: Plugin manifest lives atfront/plugins/<code_name>/config.json;code_namemust equal folder name, andunique_prefixdrives settings and filenames (e.g.,ARPSCAN).
Control plugins via settings:<PREF>_RUN(phase),<PREF>_RUN_SCHD(cron-like),<PREF>_CMD(script path),<PREF>_RUN_TIMEOUT,<PREF>_WATCH(diff columns).
Files:
front/plugins/internet_speedtest/config.json
🧠 Learnings (1)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to front/plugins/*/config.json : Control plugins via settings: `<PREF>_RUN` (phase), `<PREF>_RUN_SCHD` (cron-like), `<PREF>_CMD` (script path), `<PREF>_RUN_TIMEOUT`, `<PREF>_WATCH` (diff columns).
Applied to files:
front/plugins/internet_speedtest/config.json
🔇 Additional comments (1)
front/plugins/internet_speedtest/config.json (1)
557-600: Spanish localization has been added.The past review comment flagged missing Spanish localization for this TIMEOUT setting. The current code shows that Spanish (
es_es) localization has been added for both the name (lines 578-579) and description (lines 592-593), resolving the previous concern.
|
Hi @amir0ff , Thanks a lot for the PR. I'm on the fence of accepting it as it adds unnecessary maintenance overhead, complicates teh isntallation scripts further, it breaks the /speedtest API endpoint (See failed unit tests) and adds only small improvements, which I didn't see any issues reported about. The preference is to use python packages which are easier to include and maintain in the long term than binaries. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api_server/nettools_endpoint.py (1)
119-182: Major: Missing logging for debugging and monitoring.The speedtest() function lacks logging via
mylog(), making it difficult to debug failures or track usage. Per coding guidelines: "Usemylog(level, [message])for logging" and "Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging."Consider adding logging for:
- Binary detection result (native vs. python)
- Speedtest execution start/completion
- JSON parsing failures (currently silent fallback)
- Any errors before returning 503/500 responses
🔎 Example logging additions
# After line 135 (detection) mylog('verbose', f'[Speedtest] Using {"native Ookla" if is_native else "python-based"} binary at {SPEEDTEST_CLI_PATH}') # After line 141 (before execution) mylog('debug', f'[Speedtest] Executing: {" ".join(cmd)}') # After line 147 (success) mylog('verbose', f'[Speedtest] Completed successfully') # In except block at line 158 (JSON parse failure) mylog('minimal', f'[Speedtest] JSON parse failed, using raw output: {str(e)}') # In error handlers (lines 165, 174) mylog('none', f'[Speedtest] Failed: {str(e)}')
🧹 Nitpick comments (1)
server/api_server/nettools_endpoint.py (1)
135-135: Fragile binary detection heuristic.Using
os.path.basename(SPEEDTEST_CLI_PATH) == "speedtest"to detect the native binary is brittle. A renamed or symlinked python speedtest-cli binary named "speedtest" would be misidentified, causing incorrect argument usage and JSON parsing.🔎 More robust detection approach
Consider detecting the binary type by inspecting its behavior or metadata:
# Option 1: Check version output format def _is_native_ookla(path): """Detect if the binary is native Ookla by checking version output.""" try: result = subprocess.run( [path, "--version"], capture_output=True, text=True, timeout=5, ) # Native Ookla returns "Speedtest by Ookla" return "Ookla" in result.stdout except Exception: return False # Then use it: is_native = _is_native_ookla(SPEEDTEST_CLI_PATH)Alternatively, check for the presence of
--formatflag in help output, or cache the detection result during module initialization rather than relying on filename alone.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api_server/nettools_endpoint.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
server/api_server/nettools_endpoint.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/api_server/nettools_endpoint.py
🪛 Ruff (0.14.10)
server/api_server/nettools_endpoint.py
25-28: Avoid specifying long messages outside the exception class
(TRY003)
142-142: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (1)
server/api_server/nettools_endpoint.py (1)
17-33: LGTM! Binary resolution logic is sound.The preference for the native Ookla binary with fallback to speedtest-cli is correctly implemented using
shutil.which(). Error messages are clear and the fail-fast approach at module load is appropriate.
Hi @jokob-sk, I appreciate the candid feedback. I completely understand your concerns. The primary motivation for this change is that the current Python package (speedtest-cli) is essentially 'legacy'—it's single-threaded and consistently bottlenecks on connections over 200-300Mbps. For users with Gigabit fiber (like me), the current implementation reports significantly inaccurate data. To address your concerns, I’d like to propose a refactored approach:
Would you be open to reviewing a revised version?" P.S. I recently moved away from my proprietary Fingbox for this project! I’m really invested in making NetAlertX the best open-source alternative for high-speed home networks 💪🏼 |
|
Hi @amir0ff, Thank you for understanding. The proposed solution sounds much more acceptable, if there is no better suited python library. You could theoretically write one (e. G. I created a lightweight unifi-sm-api wrapper, it's pretty straightforward https://github.com/jokob-sk/unifi-sm-api ) if that's possible, but unsure since we are dealing with proprietary binaries. Still, your outlined approach sounds like a good compromise. Glad to hear that you find the project useful and looking forward to your contributions 🙏 |
|
Ideally the plugin should call the API endpoint - I'm in the middle of refactoring the API layer and doing a clean up of the messy parts of the code. |
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
523d595 to
ba8e3c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
front/plugins/internet_speedtest/script.py (1)
5-6: Remove unusedsubprocessimport.The
subprocessmodule is no longer used directly in this file since speedtest execution is now delegated torun_speedtest_core(). Thejsonimport is still needed.🔎 Proposed fix
import os import sys -import subprocess import json
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfilefront/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyinstall/proxmox/requirements.txtinstall/ubuntu24/requirements.txtrequirements.txtserver/api_server/nettools_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (4)
- front/plugins/internet_speedtest/config.json
- install/ubuntu24/requirements.txt
- install/proxmox/requirements.txt
- Dockerfile
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
front/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/api_server/nettools_endpoint.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
front/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
🪛 Ruff (0.14.10)
front/plugins/internet_speedtest/script.py
20-20: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
server/api_server/nettools_endpoint.py
25-28: Avoid specifying long messages outside the exception class
(TRY003)
140-140: subprocess call: check for execution of untrusted input
(S603)
157-157: Consider moving this statement to an else block
(TRY300)
173-173: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (6)
requirements.txt (1)
19-20: LGTM — cosmetic reordering only.The
speedtest-clidependency is retained for fallback support when the native Ookla binary is unavailable, which aligns with the PR discussion about maintaining backward compatibility.server/api_server/nettools_endpoint.py (3)
16-33: Well-structured binary resolution with proper fallback.The preference for native Ookla binary with fallback to
speedtest-cliis cleanly implemented. The executability check prevents cryptic errors at runtime.
119-179: Core speedtest logic is well-structured with proper timeout and error handling.Good improvements:
- Timeout is now properly set (addressing previous review feedback)
- Structured error responses with appropriate HTTP status codes
- Native binary detection and backward-compatible output formatting
- KeyError/JSONDecodeError handled for malformed native output
The catch-all
Exceptionat line 173 is acceptable as a safety net, though it could be narrowed toOSErrorif you want to be more specific.
182-189: Clean API endpoint wrapper.The delegation to
run_speedtest_core()and status code extraction pattern is clean. Poppingstatus_codebefore returning prevents internal fields from leaking to API consumers.front/plugins/internet_speedtest/script.py (2)
34-51: LGTM — clean plugin main function.The plugin correctly follows the pattern: creates
Plugin_Objects, collects results viarun_speedtest(), maps to watched fields, and callswrite_result_file()exactly once at the end.
54-117: Well-implemented speedtest wrapper with proper error handling.Good implementation:
- Timeout enforcement with 60s minimum (lines 57-60) per coding guidelines
- Defensive parsing with try/except for speed extraction (lines 74-82)
- KeyError/ValueError/TypeError handling for native payload construction (line 98)
- Consistent return structure with fallback values
The parsing of output lines (e.g.,
"Download: 100.5 Mbit/s") relies on specific formatting fromrun_speedtest_core. This coupling is acceptable since both are under the same codebase control.
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
ba8e3c6 to
1d73075
Compare
amir0ff
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
front/plugins/internet_speedtest/script.py (1)
19-19: Remove unusednoqadirective.Static analysis indicates the
noqa: E402directive on this line is not needed as the linting rule isn't triggered. Keeping unused directives can mask future issues.🔎 Proposed fix
-from api_server.nettools_endpoint import run_speedtest_core # noqa: E402 [flake8 lint suppression] +from api_server.nettools_endpoint import run_speedtest_coreserver/api_server/nettools_endpoint.py (1)
50-51: Pre-existing: Other subprocess calls in this file lack timeouts.While not introduced by this PR, several other subprocess calls in this file (
wakeonlan,traceroute,nslookup,nmap_scan,internet_info,network_interfaces) are missing explicit timeouts. Per coding guidelines, all subprocess calls should have a minimum 60-second timeout to prevent indefinite hangs.Consider addressing these in a follow-up commit to align the entire file with the timeout requirements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
front/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyinstall/proxmox/requirements.txtinstall/ubuntu24/requirements.txtrequirements.txtserver/api_server/nettools_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (3)
- install/proxmox/requirements.txt
- front/plugins/internet_speedtest/config.json
- install/ubuntu24/requirements.txt
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
server/api_server/nettools_endpoint.pyfront/plugins/internet_speedtest/script.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/api_server/nettools_endpoint.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
server/api_server/nettools_endpoint.pyfront/plugins/internet_speedtest/script.py
🧬 Code graph analysis (1)
front/plugins/internet_speedtest/script.py (4)
server/api_server/nettools_endpoint.py (1)
run_speedtest_core(119-179)front/plugins/plugin_helper.py (2)
Plugin_Objects(251-310)add_object(262-292)server/utils/datetime_utils.py (1)
timeNowDB(41-61)server/helper.py (1)
get_setting_value(200-257)
🪛 Ruff (0.14.10)
server/api_server/nettools_endpoint.py
25-28: Avoid specifying long messages outside the exception class
(TRY003)
140-140: subprocess call: check for execution of untrusted input
(S603)
157-157: Consider moving this statement to an else block
(TRY300)
173-173: Do not catch blind exception: Exception
(BLE001)
front/plugins/internet_speedtest/script.py
19-19: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (6)
requirements.txt (1)
19-20: LGTM!The dependency reordering has no functional impact. Note that
speedtest-cliis retained for the Python fallback path, which aligns with the PR's hybrid approach discussed in the comments.front/plugins/internet_speedtest/script.py (2)
33-50: LGTM!The
main()function correctly follows plugin patterns: it usesPlugin_Objects, maps the speedtest results to the appropriate watched fields, callstimeNowDB()for timestamps, and writes the result file exactly once at the end.
53-116: Well-structured speedtest logic with proper error handling.The function correctly retrieves the timeout setting and enforces a minimum of 60 seconds, delegating to
run_speedtest_core(timeout)which handles the subprocess execution with proper timeout.Key strengths:
- Timeout enforcement at min 60s aligns with coding guidelines.
- KeyError/ValueError/TypeError handling for native payload construction (lines 97-98) addresses prior review feedback.
- Error logging uses
'none'level (line 109) ensuring failures are always visible.- Graceful degradation with fallback payload structure for Python CLI mode.
server/api_server/nettools_endpoint.py (3)
16-33: LGTM!The path resolution correctly prioritizes the native Ookla
speedtestbinary and falls back tospeedtest-cli. The executable permission check is a good defensive measure. The descriptive error message aids troubleshooting when neither binary is available.
182-189: LGTM!Clean endpoint wrapper that delegates to the core function and properly extracts the status code for the HTTP response. The response structure with
successkey aligns with the API guidelines.
119-179: Well-implemented core speedtest logic with proper timeout handling and authorization enforcement.The function correctly:
- Accepts a configurable timeout (default 120s, exceeds the 60s minimum guideline) and passes it to
subprocess.run- Detects native vs Python CLI and builds appropriate commands
- Handles JSON parsing failures gracefully with fallback to raw stdout
- Returns structured error responses with appropriate status codes
The API route wrapper (
api_speedtestinapi_server_start.py) properly enforces authorization viais_authorized()before calling the endpoint.One minor consideration: The native binary detection (
os.path.basename(SPEEDTEST_CLI_PATH) == "speedtest") assumes the binary is named exactly "speedtest". If the binary is installed or symlinked with a different name (e.g., "my-speedtest"), it would be misdetected as the Python CLI, causing incorrect command-line arguments to be used. While the function would still execute and likely return output (with fallback handling), consider validating the binary type via output inspection if this becomes a concern in deployment.
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
1d73075 to
89ee2ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
front/plugins/internet_speedtest/script.py (1)
83-105: Payload structure differs between native and Python engines.The native engine payload includes
ping,server, andtimestampfields while the Python fallback only includesdownload,upload, andengine. This asymmetry is acceptable since the Python CLI's--simplemode doesn't provide this metadata, but consumers ofWatched_Value3should be aware that the payload schema varies by engine.Consider documenting this in the WATCH description or adding a comment here for maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
front/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyinstall/proxmox/requirements.txtinstall/ubuntu24/requirements.txtrequirements.txtserver/api_server/nettools_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (3)
- install/ubuntu24/requirements.txt
- requirements.txt
- install/proxmox/requirements.txt
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
server/api_server/nettools_endpoint.pyfront/plugins/internet_speedtest/script.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/api_server/nettools_endpoint.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
front/plugins/*/config.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/config.json: Plugin manifest lives atfront/plugins/<code_name>/config.json;code_namemust equal folder name, andunique_prefixdrives settings and filenames (e.g.,ARPSCAN).
Control plugins via settings:<PREF>_RUN(phase),<PREF>_RUN_SCHD(cron-like),<PREF>_CMD(script path),<PREF>_RUN_TIMEOUT,<PREF>_WATCH(diff columns).
Files:
front/plugins/internet_speedtest/config.json
🧠 Learnings (2)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
server/api_server/nettools_endpoint.pyfront/plugins/internet_speedtest/script.py
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to front/plugins/*/config.json : Control plugins via settings: `<PREF>_RUN` (phase), `<PREF>_RUN_SCHD` (cron-like), `<PREF>_CMD` (script path), `<PREF>_RUN_TIMEOUT`, `<PREF>_WATCH` (diff columns).
Applied to files:
front/plugins/internet_speedtest/config.json
🧬 Code graph analysis (1)
front/plugins/internet_speedtest/script.py (3)
server/api_server/nettools_endpoint.py (1)
run_speedtest_core(120-180)front/plugins/plugin_helper.py (1)
add_object(262-292)server/helper.py (1)
get_setting_value(200-257)
🪛 Ruff (0.14.10)
server/api_server/nettools_endpoint.py
25-28: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Starting a process with a partial executable path
(S607)
141-141: subprocess call: check for execution of untrusted input
(S603)
158-158: Consider moving this statement to an else block
(TRY300)
174-174: Do not catch blind exception: Exception
(BLE001)
207-207: Starting a process with a partial executable path
(S607)
🔇 Additional comments (12)
front/plugins/internet_speedtest/config.json (2)
557-600: LGTM! New TIMEOUT setting properly configured.The new
INTRSPD_TIMEOUTsetting is well-structured with:
- Correct integer dataType and numeric input element
- Sensible default of 60 seconds matching the minimum enforcement in the script
- Complete localizations for en_us, es_es, and de_de
- Clear description explaining the 60-second minimum and behavior on timeout
635-648: WATCH description updates look complete.The updated descriptions correctly document that
Watched_Value3now represents a JSON payload for webhooks, with all three language localizations (en_us, es_es, de_de) properly included.front/plugins/internet_speedtest/script.py (3)
53-59: Timeout handling is well implemented.The timeout retrieval correctly:
- Uses
INTRSPD_TIMEOUTwhich matches the config.json setting (prefix + function name)- Enforces a minimum of 60 seconds as documented
- Handles
ValueError/TypeErrorgracefully with fallback to 60s
33-50: LGTM! Main function follows plugin guidelines.The implementation correctly:
- Uses
Plugin_Objectsfor result collection- Maps speedtest results to watched values with clear comments
- Calls
write_result_file()exactly once at the end- Includes informative logging
19-19: Import path is properly configured and will work correctly.The import
from api_server.nettools_endpoint import run_speedtest_coreresolves correctly. Line 8 properly adds/app/servertosys.path, enabling Python to locate theapi_server.nettools_endpointmodule. The function exists at line 120 in./server/api_server/nettools_endpoint.pyand module resolution is consistent across execution contexts.server/api_server/nettools_endpoint.py (7)
16-33: Path resolution updated to prefer native binary.The function now correctly:
- Prioritizes the native Ookla
speedtestbinary for higher accuracy- Falls back to
speedtest-cliPython package- Validates executability before returning
This aligns with the PR's hybrid approach design.
120-130: Core function signature and error handling look good.The
run_speedtest_core(timeout=120)function:
- Accepts a configurable timeout (used by the plugin with its own setting)
- Returns a properly structured dict with
success,output, optionalraw, andstatus_code- Handles the case when no speedtest executable is found with a 503 status
149-163: Backward-compatible output format with enhanced data.The implementation elegantly:
- Parses native JSON and formats it to match
--simpleoutput for backward compatibility- Preserves raw data in the
rawkey for enhanced webhook payloads- Falls back gracefully if JSON parsing fails (returns stdout as-is)
The static analysis hint about moving the return to an
elseblock (TRY300) is a false positive here—the return on line 158 is the success path after valid JSON parsing.
165-180: Comprehensive exception handling with structured responses.The error handling covers:
TimeoutExpired→ 504 Gateway Timeout (appropriate for slow external service)CalledProcessError→ 500 with stderr details (command execution failure)- Generic
Exception→ 500 with error message (catch-all for unexpected issues)The static analysis warning about catching bare
Exception(BLE001) is acceptable here—this is a top-level handler that needs to return a structured response rather than crash the API. The structured error responses follow the API guidelines requirement for"success": Falsewith an"error"message.
50-51: Excellent: All subprocess calls now have explicit timeouts.The addition of
timeout=60to all subprocess calls in the module (wakeonlan,traceroute,nslookup,nmap_scan,internet_info,network_interfaces) follows the coding guidelines requirement: "Always set explicit timeouts on subprocess calls. Default to 60s minimum."This prevents any endpoint from hanging indefinitely due to a stalled external process.
Also applies to: 100-101, 206-207, 259-260, 288-289, 323-324
183-190: API endpoint correctly delegates to core function.The
speedtest()endpoint:
- Uses
run_speedtest_core()to avoid code duplication- Properly extracts and uses
status_codefor the HTTP response- Returns a JSON response with
successkey per API guidelines- Authorization is properly handled at the route registration level via
is_authorized()check in theapi_speedtest()wrapper
133-139: Native binary detection via filename is acceptable for standard installations.The detection
os.path.basename(SPEEDTEST_CLI_PATH) == "speedtest"is appropriate for typical setups where the official Ookla binary is mounted to/usr/bin/speedtest. While filename-based detection could theoretically break with custom renames or symlinks, this approach is practical and aligns with documented installation instructions.
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
89ee2ab to
01084a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
front/plugins/internet_speedtest/script.py (1)
68-81: Parsing logic is fragile but acceptable given the core's output format.The string-splitting logic to extract download/upload speeds depends on the specific format returned by
run_speedtest_core()(e.g., "Download: 123.45 Mbit/s"). While this is fragile and could break silently if the core's output format changes, the exception handling provides a safe fallback to 0.0.Consider adding a brief comment referencing the expected format from
run_speedtest_core()to make this dependency explicit, but this is optional given the tight coupling between the two functions.server/api_server/nettools_endpoint.py (2)
149-160: Consider logging the JSON parsing failure for debugging.When native binary JSON parsing fails (line 159-160), the function returns
success=Truewith raw stdout, which may not contain the expected "Download: X Mbit/s" format. While the calling code handles this gracefully by defaulting to 0.0 values, this silent fallback could make troubleshooting difficult.🔎 Suggested enhancement for debugging
if is_native: try: data = json.loads(result.stdout) # Format to match --simple output for backward compatibility output_lines = [ f"Ping: {data['ping']['latency']} ms", f"Download: {round((data['download']['bandwidth'] * 8) / 10**6, 2)} Mbit/s", f"Upload: {round((data['upload']['bandwidth'] * 8) / 10**6, 2)} Mbit/s" ] return {"success": True, "output": output_lines, "raw": data} - except (json.JSONDecodeError, KeyError): + except (json.JSONDecodeError, KeyError) as e: + print(f"Warning: Failed to parse native speedtest JSON: {e}. Raw output: {result.stdout[:200]}", file=sys.stderr) return {"success": True, "output": [result.stdout.strip()]}
44-67: Consider adding explicit TimeoutExpired handling for clearer error messages.While the current exception handlers will catch
subprocess.TimeoutExpired, adding explicit handlers (similar to the one inrun_speedtest_coreat line 165-166) would provide more specific error messages and make timeout behavior more obvious to future maintainers.Example for
wakeonlan:except subprocess.TimeoutExpired: return jsonify({"success": False, "error": "WOL command timed out"}), 504 except subprocess.CalledProcessError as e: # existing handlerThis is optional since the current handlers will catch timeouts, but explicit handling improves clarity and allows for timeout-specific status codes (504 Gateway Timeout).
Also applies to: 70-117, 193-220, 223-275, 278-309, 312-397
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
front/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyinstall/proxmox/requirements.txtinstall/ubuntu24/requirements.txtrequirements.txtserver/api_server/nettools_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (2)
- install/ubuntu24/requirements.txt
- install/proxmox/requirements.txt
🧰 Additional context used
📓 Path-based instructions (5)
front/plugins/*/config.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/config.json: Plugin manifest lives atfront/plugins/<code_name>/config.json;code_namemust equal folder name, andunique_prefixdrives settings and filenames (e.g.,ARPSCAN).
Control plugins via settings:<PREF>_RUN(phase),<PREF>_RUN_SCHD(cron-like),<PREF>_CMD(script path),<PREF>_RUN_TIMEOUT,<PREF>_WATCH(diff columns).
Files:
front/plugins/internet_speedtest/config.json
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
front/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/api_server/nettools_endpoint.py
🧠 Learnings (2)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to front/plugins/*/config.json : Control plugins via settings: `<PREF>_RUN` (phase), `<PREF>_RUN_SCHD` (cron-like), `<PREF>_CMD` (script path), `<PREF>_RUN_TIMEOUT`, `<PREF>_WATCH` (diff columns).
Applied to files:
front/plugins/internet_speedtest/config.json
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
front/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
🧬 Code graph analysis (1)
front/plugins/internet_speedtest/script.py (3)
server/api_server/nettools_endpoint.py (1)
run_speedtest_core(120-180)front/plugins/plugin_helper.py (2)
Plugin_Objects(251-310)add_object(262-292)server/helper.py (1)
get_setting_value(200-257)
🪛 Ruff (0.14.10)
server/api_server/nettools_endpoint.py
25-28: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Starting a process with a partial executable path
(S607)
141-141: subprocess call: check for execution of untrusted input
(S603)
158-158: Consider moving this statement to an else block
(TRY300)
174-174: Do not catch blind exception: Exception
(BLE001)
207-207: Starting a process with a partial executable path
(S607)
🔇 Additional comments (11)
front/plugins/internet_speedtest/config.json (2)
557-600: LGTM! Configuration follows plugin conventions.The new
TIMEOUTsetting is well-structured with complete localizations for all three languages (en_us, es_es, de_de), a sensible default of 60 seconds, and clear descriptions of the minimum value and timeout behavior.
635-648: LGTM! WATCH descriptions updated correctly.The updated descriptions properly reflect that
Watched_Value3now contains a JSON payload for webhooks, with all three language localizations (en_us, es_es, de_de) consistently updated. The note about schema variations by engine is helpful context for users.front/plugins/internet_speedtest/script.py (5)
5-5: LGTM! Appropriate imports for the refactored implementation.The
jsonimport andrun_speedtest_coreimport are correctly added to support the new hybrid speedtest implementation that delegates to the shared core logic.Also applies to: 19-19
35-50: LGTM! Main function correctly integrates the refactored speedtest logic.The updated
main()function properly callsrun_speedtest()and maps the returned dictionary (down,up,json) to the appropriate watched values, aligning with the configuration changes that designateWatched_Value3as the JSON payload for webhooks.
54-59: LGTM! Timeout handling follows coding guidelines.The timeout configuration properly enforces a minimum of 60 seconds as required by the coding guidelines and handles type conversion errors gracefully with an appropriate fallback.
As per coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
83-110: LGTM! Robust JSON payload construction with comprehensive error handling.The payload construction logic properly handles both native and Python CLI data paths, with appropriate exception handling for missing keys or invalid data. The error logging at the 'none' level ensures visibility of failures, and fallback payloads maintain a consistent JSON structure for webhook consumers.
The bandwidth conversion (multiplying by 8 to convert bytes/s to bits/s) aligns with the native binary's output format.
112-116: LGTM! Return structure matches expected interface.The function returns a well-structured dictionary with
down,up, andjsonkeys that align with the calling code inmain()and the plugin's watched value mapping.server/api_server/nettools_endpoint.py (4)
16-33: LGTM! Clear binary detection logic with appropriate fallback.The updated
_get_speedtest_cli_path()properly implements the "Python-First Hybrid" approach by preferring the native Ookla binary while falling back tospeedtest-cli. The executability check and clear error messages enhance robustness.
120-163: LGTM! Core function properly implements dual-engine support with timeout.The
run_speedtest_core()function correctly:
- Enforces configurable timeout on subprocess calls (line 146)
- Detects and handles both native Ookla and Python CLI binaries
- Parses native JSON output and formats it for backward compatibility
- Returns a consistent structure with
success,output, and optionallyrawkeysThe timeout handling and comprehensive exception catching ensure the function won't hang or crash unexpectedly.
183-190: LGTM! Endpoint properly delegates to core with clean response structure.The
speedtest()endpoint correctly uses the new core function and properly separates the HTTP status code from the JSON response body by popping it before returning. The default 120-second timeout provides a reasonable margin for slower connections.
51-51: LGTM! Timeout enforcement improves resilience across all network diagnostic functions.The addition of explicit 60-second timeouts to
wakeonlan,traceroute,nslookup,nmap_scan,internet_info, andnetwork_interfacessubprocess calls follows the coding guidelines and prevents these endpoints from hanging indefinitely on network issues.As per coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
Also applies to: 101-101, 207-207, 260-260, 289-289, 324-324
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
01084a9 to
4654a44
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Hi @jokob-sk, Thank you for the guidance! I’ve updated the PR to fully align with the "Python-first hybrid" approach and your suggestions regarding the API layer. Here is a summary of the finalized implementation:
I believe this strikes the right balance between maintaining a lightweight baseline and offering a high-performance path for power users with high-speed fiber. Looking forward to your review!" |
|
|
||
| def run_speedtest(): | ||
| # Get configurable timeout with minimum 60s enforcement | ||
| try: |
There was a problem hiding this comment.
this section is only getting settings - unnecessary overhead
|
Hi @amir0ff , Thanks a lot for incorporating teh changes. looks much better. A couple of questions:
Once above is solved looks good to merge. If possible don't change the formatting on existing code blocks - makes the review more time consuming 🙏 |
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
4654a44 to
b0e7e0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api_server/nettools_endpoint.py (1)
167-172: Add timeout to speedtest-cli subprocess call.The fallback CLI path lacks an explicit timeout parameter, which can cause the endpoint to hang if speedtest-cli stalls. Per coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
result = subprocess.run( [SPEEDTEST_CLI_PATH, "--secure", "--simple"], capture_output=True, text=True, check=True, + timeout=60, )Also add handling for
subprocess.TimeoutExpired:except subprocess.TimeoutExpired: return jsonify( { "success": False, "error": "Speedtest timed out after 60 seconds", } ), 504Based on coding guidelines and retrieved learnings.
♻️ Duplicate comments (1)
front/plugins/internet_speedtest/script.py (1)
53-53: Duplicate constant: Extract native path to shared configuration.This hardcoded path
"/usr/bin/speedtest"duplicates the constant defined inserver/api_server/nettools_endpoint.py(line 14). Per coding guidelines: "Define config once and read it via helpers everywhere."🔎 Recommended approach
Define the path once in a central location (e.g., via
ccd()inserver/initialise.pyor inconf.py) and import it in both files to maintain a single source of truth.Based on coding guidelines.
🧹 Nitpick comments (3)
front/plugins/internet_speedtest/README.md (1)
22-26: Consider rewording to improve readability.Three consecutive list items begin with "Watched_Value," which slightly impacts readability. While this is consistent with the field naming, consider using a table format or alternative phrasing for better flow.
🔎 Optional alternative format
-### Data Mapping - -- **Watched_Value1**: Download Speed (Mbps). -- **Watched_Value2**: Upload Speed (Mbps). -- **Watched_Value3**: Full JSON payload (useful for n8n or detailed webhooks). +### Data Mapping + +| Field | Description | +|-------|-------------| +| **Watched_Value1** | Download Speed (Mbps) | +| **Watched_Value2** | Upload Speed (Mbps) | +| **Watched_Value3** | Full JSON payload (useful for n8n or detailed webhooks) |server/api_server/nettools_endpoint.py (1)
14-14: Verify consistency with plugin script's native path.The constant
NATIVE_SPEEDTEST_PATHis defined here as"/usr/bin/speedtest", and the same path is hardcoded infront/plugins/internet_speedtest/script.py(line 53). Consider extracting this to a shared configuration constant to maintain a single source of truth.🔎 Suggested approach
Define the path once in a shared location (e.g.,
conf.pyor viaccd()inserver/initialise.py) and import it in both files:# In server/initialise.py or conf.py NATIVE_SPEEDTEST_PATH = "/usr/bin/speedtest"Then import in both files:
from conf import NATIVE_SPEEDTEST_PATHBased on coding guidelines: "Define config once and read it via helpers everywhere."
front/plugins/internet_speedtest/script.py (1)
64-64: Remove redundant str() conversion in f-string.F-strings automatically convert expressions to strings, so
str(download_speed)is redundant. This also applies to lines 73, 81, and 88.🔎 Proposed fix
- mylog('verbose', [f"[INTRSPD] Native Result (down|up): {str(download_speed)} Mbps|{upload_speed} Mbps"]) + mylog('verbose', [f"[INTRSPD] Native Result (down|up): {download_speed} Mbps|{upload_speed} Mbps"])Apply similar changes to lines 73, 81, and 88.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
front/plugins/internet_speedtest/README.mdfront/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
server/api_server/nettools_endpoint.pyfront/plugins/internet_speedtest/script.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/api_server/nettools_endpoint.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
front/plugins/*/config.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/config.json: Plugin manifest lives atfront/plugins/<code_name>/config.json;code_namemust equal folder name, andunique_prefixdrives settings and filenames (e.g.,ARPSCAN).
Control plugins via settings:<PREF>_RUN(phase),<PREF>_RUN_SCHD(cron-like),<PREF>_CMD(script path),<PREF>_RUN_TIMEOUT,<PREF>_WATCH(diff columns).
Files:
front/plugins/internet_speedtest/config.json
🧠 Learnings (1)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
server/api_server/nettools_endpoint.pyfront/plugins/internet_speedtest/script.py
🪛 LanguageTool
front/plugins/internet_speedtest/README.md
[style] ~26-~26: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ched_Value2**: Upload Speed (Mbps). - Watched_Value3: Full JSON payload (useful for...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.14.10)
server/api_server/nettools_endpoint.py
123-123: subprocess call: check for execution of untrusted input
(S603)
151-153: try-except-pass detected, consider logging the exception
(S110)
151-151: Do not catch blind exception: Exception
(BLE001)
151-151: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
front/plugins/internet_speedtest/script.py
59-59: subprocess call: check for execution of untrusted input
(S603)
64-64: Use explicit conversion flag
Replace with conversion flag
(RUF010)
72-72: Do not catch blind exception: Exception
(BLE001)
73-73: Use explicit conversion flag
Replace with conversion flag
(RUF010)
81-81: Use explicit conversion flag
Replace with conversion flag
(RUF010)
87-87: Do not catch blind exception: Exception
(BLE001)
88-88: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (3)
front/plugins/internet_speedtest/config.json (2)
243-256: LGTM! Localization updates align with feature changes.The Watched_Value3 name has been properly updated across all three languages to reflect that it now contains the full JSON payload. This aligns well with the documented behavior in the README and script changes.
591-604: LGTM! Comprehensive documentation for webhook payload.The WATCH description accurately documents the new Watched_Value3 behavior, including the note that the schema varies by engine. All three localizations are present and consistent.
front/plugins/internet_speedtest/script.py (1)
87-93: The string'null'for the error case is intentional and consistent with the codebase. Result files use pipe-delimited format (not JSON), and watched fields are treated as plain strings. Multiple other plugins use the same'null'string sentinel for empty/error cases (website_monitor, _publisher_telegram, _publisher_email, etc.). This implementation is correct.Likely an incorrect or invalid review comment.
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
b0e7e0a to
c6dbc42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
server/api_server/nettools_endpoint.py (1)
120-159: Add logging for non-zero return code from native binary.When
result.returncode != 0(line 129), the code silently falls through to the CLI fallback without logging. This makes it difficult to diagnose why the native binary failed. Per coding guidelines, add more logging when debugging might be difficult.🔎 Proposed fix
if result.returncode == 0: try: data = json.loads(result.stdout) # ... parsing logic ... except (json.JSONDecodeError, KeyError, TypeError) as parse_error: print(f"Failed to parse native speedtest output: {parse_error}", file=sys.stderr) # Fall through to CLI fallback + else: + print(f"Native speedtest exited with code {result.returncode}: {result.stderr}", file=sys.stderr) + # Fall through to CLI fallbackfront/plugins/internet_speedtest/script.py (3)
60-69: Add specific handling for JSON parsing errors.While the generic
Exceptionhandler on line 72 will catchJSONDecodeErrorandKeyError, the error message "Error running native speedtest" is misleading when the actual issue is malformed JSON output. This complicates debugging.🔎 Proposed fix to distinguish parsing errors
if result.returncode == 0: + try: data = json.loads(result.stdout) download_speed = round(data['download']['bandwidth'] * 8 / 10**6, 2) upload_speed = round(data['upload']['bandwidth'] * 8 / 10**6, 2) - mylog('verbose', [f"[INTRSPD] Native Result (down|up): {str(download_speed)} Mbps|{upload_speed} Mbps"]) + except (json.JSONDecodeError, KeyError, TypeError) as parse_error: + mylog('none', [f"[INTRSPD] Failed to parse native JSON: {parse_error}"]) + # Fall through to baseline + else: + mylog('verbose', [f"[INTRSPD] Native Result (down|up): {download_speed} Mbps|{upload_speed} Mbps"]) return { 'download_speed': download_speed, 'upload_speed': upload_speed, 'full_json': result.stdout.strip() }
70-73: Use 'none' log level for exceptions.Per coding guidelines: "Use
nonefor most important messages that should always appear, such as exceptions." The timeout and error conditions should be logged at 'none' level to ensure visibility when debugging.🔎 Proposed fix
except subprocess.TimeoutExpired: - mylog('verbose', ["[INTRSPD] Native speedtest timed out."]) + mylog('none', ["[INTRSPD] Native speedtest timed out, falling back to baseline."]) except Exception as e: - mylog('verbose', [f"[INTRSPD] Error running native speedtest: {str(e)}"]) + mylog('none', [f"[INTRSPD] Error running native speedtest: {e!s}, falling back to baseline."])Based on coding guidelines.
87-88: Use 'none' log level for baseline exception.Same as above—exceptions in the baseline fallback should use 'none' level for visibility.
🔎 Proposed fix
except Exception as e: - mylog('verbose', [f"[INTRSPD] Error running baseline speedtest: {str(e)}"]) + mylog('none', [f"[INTRSPD] Error running baseline speedtest: {e!s}"])Based on coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
front/plugins/internet_speedtest/README.mdfront/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- front/plugins/internet_speedtest/config.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
server/api_server/nettools_endpoint.pyfront/plugins/internet_speedtest/script.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/api_server/nettools_endpoint.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
server/api_server/nettools_endpoint.pyfront/plugins/internet_speedtest/script.py
🪛 LanguageTool
front/plugins/internet_speedtest/README.md
[style] ~26-~26: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ched_Value2**: Upload Speed (Mbps). - Watched_Value3: Full JSON payload (useful for...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.14.10)
server/api_server/nettools_endpoint.py
123-123: subprocess call: check for execution of untrusted input
(S603)
157-157: Do not catch blind exception: Exception
(BLE001)
front/plugins/internet_speedtest/script.py
59-59: subprocess call: check for execution of untrusted input
(S603)
64-64: Use explicit conversion flag
Replace with conversion flag
(RUF010)
72-72: Do not catch blind exception: Exception
(BLE001)
73-73: Use explicit conversion flag
Replace with conversion flag
(RUF010)
81-81: Use explicit conversion flag
Replace with conversion flag
(RUF010)
87-87: Do not catch blind exception: Exception
(BLE001)
88-88: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (5)
server/api_server/nettools_endpoint.py (2)
14-14: LGTM on constant definition.The native speedtest path constant is clearly defined and aligns with the documented opt-in flow for users mounting the binary.
173-191: LGTM on CLI timeout handling.The CLI subprocess call now includes the required 60-second timeout, and the TimeoutExpired exception is properly handled with an appropriate 504 response.
front/plugins/internet_speedtest/script.py (1)
75-86: LGTM on baseline fallback.The baseline path using the Python
speedtestlibrary correctly captures results and serializesfull_jsonfor webhook consumption.front/plugins/internet_speedtest/README.md (2)
7-19: Clear documentation for opt-in native engine.The usage instructions are well-structured with a concrete docker-compose example. The link to the Speedtest CLI homepage gives users a clear path to download the binary.
22-26: LGTM on data mapping documentation.The data mapping section clearly describes the three watched values, which aligns with the script changes and will help users configure webhooks.
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
c6dbc42 to
9a8f071
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
front/plugins/internet_speedtest/README.md (1)
24-26: Minor style: Consider varying the sentence structure in the Data Mapping list.The three bullet points all begin with the dash-prefixed identifier pattern (
**Watched_Value…**), creating repetitive sentence openings. While clear, a small rewording can improve readability.🔎 Proposed style improvement
- **Watched_Value1**: Download Speed (Mbps). - **Watched_Value2**: Upload Speed (Mbps). - **Watched_Value3**: Full JSON payload (useful for n8n or detailed webhooks). + **Watched_Value1** — Download Speed (Mbps). + **Watched_Value2** — Upload Speed (Mbps). + **Watched_Value3** — Full JSON payload (useful for n8n or detailed webhooks).This uses an em-dash instead of a colon, subtly breaking the repetition while maintaining list structure.
front/plugins/internet_speedtest/script.py (1)
53-53: Consider extracting the native binary path to a shared constant.The path
/usr/bin/speedtestis duplicated between this plugin script andserver/api_server/nettools_endpoint.py(line 14). Consider extracting this to a shared location (e.g., a constants module) to ensure consistency and single point of maintenance.🔎 Example approach
Create a shared constant in a common location:
# In a shared constants module (e.g., server/const.py or similar) NATIVE_SPEEDTEST_PATH = "/usr/bin/speedtest"Then import from both files:
-native_path = "/usr/bin/speedtest" +from const import NATIVE_SPEEDTEST_PATH +native_path = NATIVE_SPEEDTEST_PATH
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
front/plugins/internet_speedtest/README.mdfront/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- front/plugins/internet_speedtest/config.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
front/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/api_server/nettools_endpoint.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
front/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
🧬 Code graph analysis (1)
front/plugins/internet_speedtest/script.py (2)
server/helper.py (1)
get_setting_value(200-257)server/api_server/nettools_endpoint.py (1)
speedtest(115-211)
🪛 LanguageTool
front/plugins/internet_speedtest/README.md
[style] ~26-~26: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ched_Value2**: Upload Speed (Mbps). - Watched_Value3: Full JSON payload (useful for...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.14.10)
front/plugins/internet_speedtest/script.py
68-68: subprocess call: check for execution of untrusted input
(S603)
88-88: Do not catch blind exception: Exception
(BLE001)
103-103: Do not catch blind exception: Exception
(BLE001)
server/api_server/nettools_endpoint.py
123-123: subprocess call: check for execution of untrusted input
(S603)
159-159: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
front/plugins/internet_speedtest/README.md (1)
3-31: Excellent documentation addressing PR objectives.The README clearly documents the dual-engine model, opt-in flow for the native binary, automatic fallback, and data mapping. The docker-compose volume example is practical and helpful. The content aligns well with the underlying implementation and directly addresses the maintainer's request for installation/use instructions.
server/api_server/nettools_endpoint.py (3)
14-14: LGTM: Native binary path constant.The hardcoded path
/usr/bin/speedtestaligns with the PR design for optional native binary detection. Users mount the Ookla binary to this location as documented.
120-161: LGTM: Native speedtest implementation with proper safeguards.The implementation correctly:
- Uses a 60-second timeout per coding guidelines
- Catches JSON parsing errors (
JSONDecodeError,KeyError,TypeError) with logging- Logs non-zero exit codes and timeout events
- Falls back gracefully to CLI on any failure
The broad
Exceptioncatch on line 159 is appropriate here as the goal is graceful degradation to the CLI fallback.
173-193: LGTM: CLI path with timeout handling.The CLI fallback now properly includes:
- 60-second timeout per coding guidelines
- Appropriate 504 Gateway Timeout response for timeout scenarios
- Consistent error response format with
success: Falseanderrormessagefront/plugins/internet_speedtest/script.py (4)
6-7: LGTM: Added necessary imports.The
subprocessandjsonimports are required for the native speedtest binary execution and JSON parsing.
38-49: LGTM: Plugin result includes full JSON payload.The
watched3field now properly exposes the full JSON payload for webhook consumers (n8n, etc.) as documented in the PR objectives. Results are correctly written viaplugin_objects.write_result_file()per coding guidelines.
58-65: LGTM: Robust timeout parsing with minimum enforcement.The timeout parsing is properly defensive:
- Handles missing settings (
raw_timeoutbeingNone)- Catches
ValueError/TypeErrorfor invalid values- Enforces 60-second minimum per coding guidelines
91-109: LGTM: Baseline fallback with consistent result structure.The baseline path using the Python
speedtestlibrary:
- Properly serializes results to JSON via
st.results.dict()- Returns consistent result structure with
full_jsonfield- Uses appropriate error logging at
nonelevel for visibility- Returns sentinel values (
-1) on failure for downstream handling
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
9a8f071 to
3788c75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
server/api_server/nettools_endpoint.py (6)
46-47: Missing timeout onwakeonlansubprocess call.Per coding guidelines, all subprocess calls require explicit timeouts (minimum 60s). The
wakeonlancommand could hang indefinitely without one.🔎 Proposed fix
result = subprocess.run( - ["wakeonlan", mac], capture_output=True, text=True, check=True + ["wakeonlan", mac], capture_output=True, text=True, check=True, timeout=60 )Based on coding guidelines.
92-97: Missing timeout ontraceroutesubprocess call.Traceroute can take a long time or hang on unreachable hosts. Add a timeout per coding guidelines.
🔎 Proposed fix
result = subprocess.run( ["traceroute", ip], # Command and argument capture_output=True, # Capture stdout/stderr text=True, # Return output as string check=True, # Raise CalledProcessError on non-zero exit + timeout=60, # Minimum timeout per coding guidelines )Based on coding guidelines.
227-228: Missing timeout onnslookupsubprocess call.DNS lookups can hang on network issues. Add a timeout for consistency with other subprocess calls.
🔎 Proposed fix
result = subprocess.run( - ["nslookup", ip], capture_output=True, text=True, check=True + ["nslookup", ip], capture_output=True, text=True, check=True, timeout=60 )Based on coding guidelines.
276-281: Missing timeout onnmapsubprocess call.Nmap scans, especially detail mode (
-A), can take several minutes. Consider a longer timeout or making it configurable.🔎 Proposed fix
cmd = ["nmap"] + mode_args[mode] + [ip] result = subprocess.run( cmd, capture_output=True, text=True, check=True, + timeout=300, # Nmap scans can take longer, 5 min default )Based on coding guidelines.
304-309: Missing timeout oncurlsubprocess call.External HTTP requests can hang on network issues.
🔎 Proposed fix
result = subprocess.run( - ["curl", "-s", "https://ipinfo.io/json"], + ["curl", "-s", "--max-time", "30", "https://ipinfo.io/json"], capture_output=True, text=True, check=True, + timeout=60, )Based on coding guidelines.
338-343: Missing timeout onnmap --iflistsubprocess call.While this is a local operation, adding a timeout ensures consistent behavior.
🔎 Proposed fix
nmap_output = subprocess.run( ["nmap", "--iflist"], capture_output=True, text=True, check=True, + timeout=60, ).stdout.strip()Based on coding guidelines.
🧹 Nitpick comments (3)
front/plugins/internet_speedtest/script.py (2)
19-19: Clean up unused noqa directive.The
noqa: E402directive on this line is flagged as unused by Ruff. Consider removing it to keep the codebase clean.🔎 Proposed fix
-from const import logPath, NATIVE_SPEEDTEST_PATH # noqa: E402 [flake8 lint suppression] +from const import logPath, NATIVE_SPEEDTEST_PATH
105-108: Consider usingNoneor empty JSON object instead of string'null'for error cases.The error case returns
'null'as a string literal forfull_json, while successful cases return actual JSON strings. This inconsistency could cause issues for downstream consumers expecting valid JSON parsing.🔎 Proposed fix
return { 'download_speed': -1, 'upload_speed': -1, - 'full_json': 'null' + 'full_json': json.dumps({"error": str(e)}) }Alternatively, if
'null'is intentional for plugin result file compatibility, consider documenting this behavior.front/plugins/internet_speedtest/README.md (1)
15-18: Consider adding a note about host binary requirements.The volume mapping assumes the native speedtest binary is installed on the host at
/usr/bin/speedtest. Adding a note clarifying this could help users troubleshoot.🔎 Suggested addition
```yaml volumes: - /usr/bin/speedtest:/usr/bin/speedtest:ro+- Ensure the native
speedtestbinary is installed on the host at the source path.</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9a8f071cc8be3f24ffd40240506ff843e9424d4a and 3788c75fa8e6c524d208a127c71459dae2b15b14. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `front/plugins/internet_speedtest/README.md` * `front/plugins/internet_speedtest/config.json` * `front/plugins/internet_speedtest/script.py` * `server/api_server/nettools_endpoint.py` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * front/plugins/internet_speedtest/config.json </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (4)</summary> <details> <summary>**/*.py</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > `**/*.py`: Use `mylog(level, [message])` for logging; levels are: none/minimal/verbose/debug/trace. Use `none` for most important messages that should always appear, such as exceptions. > Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout. > Never hardcode ports or secrets; always use `get_setting_value()` to retrieve configuration values. > Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding paths like `/data/db` or relative paths. > Use `helper.py` functions (`timeNowDB`, `normalize_mac`, sanitizers) for time/MAC/string operations. Validate MACs before DB writes. > Add/modify settings via `ccd()` in `server/initialise.py` or per-plugin manifest. Define config once and read it via helpers everywhere. > Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging. Files: - `server/api_server/nettools_endpoint.py` - `front/plugins/internet_speedtest/script.py` </details> <details> <summary>server/api_server/*.py</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > `server/api_server/*.py`: All API responses must return `"success":<False|True>` and if `False` must include an `"error"` message, e.g. `{"success": False, "error": "No stored open ports for Device"}` > All API routes must expect header `Authorization: Bearer <API_TOKEN>` retrieved via `get_setting_value('API_TOKEN')` for authorization. Files: - `server/api_server/nettools_endpoint.py` </details> <details> <summary>server/**/*.py</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > Prefer `server/db/db_helper.py` functions (e.g., `get_table_json`, device condition helpers) over raw SQL in new code paths. Files: - `server/api_server/nettools_endpoint.py` </details> <details> <summary>front/plugins/*/script.py</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > `front/plugins/*/script.py`: For plugins: define `database_column_definitions` when creating/updating devices; watched fields trigger notifications. > Plugin scripts must write results to `/tmp/log/plugins/last_result.<PREF>.log` with pipe-delimited format (9 required cols + optional 4). Use `Plugin_Objects` to sanitize text and normalize MACs, then call `write_result_file()` exactly once at the end. > Collect plugin results with `Plugin_Objects.add_object(...)` during processing and call `plugin_objects.write_result_file()` exactly once at the end of the script. Do not write ad-hoc files for results. > Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise at `info` level and use `verbose` or `debug` for extra context. Files: - `front/plugins/internet_speedtest/script.py` </details> </details><details> <summary>🧠 Learnings (1)</summary> <details> <summary>📚 Learning: 2025-12-08T01:59:00.903Z</summary>Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.**Applied to files:** - `server/api_server/nettools_endpoint.py` - `front/plugins/internet_speedtest/script.py` </details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>front/plugins/internet_speedtest/README.md</summary> [style] ~26-~26: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. Context: ...hed_Value2** — Upload Speed (Mbps). - **Watched_Value3** — Full JSON payload (useful fo... (ENGLISH_WORD_REPEAT_BEGINNING_RULE) </details> </details> <details> <summary>🪛 Ruff (0.14.10)</summary> <details> <summary>server/api_server/nettools_endpoint.py</summary> 123-123: `subprocess` call: check for execution of untrusted input (S603) --- 159-159: Do not catch blind exception: `Exception` (BLE001) </details> <details> <summary>front/plugins/internet_speedtest/script.py</summary> 19-19: Unused `noqa` directive (non-enabled: `E402`) Remove unused `noqa` directive (RUF100) --- 68-68: `subprocess` call: check for execution of untrusted input (S603) --- 88-88: Do not catch blind exception: `Exception` (BLE001) --- 103-103: Do not catch blind exception: `Exception` (BLE001) </details> </details> </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>server/api_server/nettools_endpoint.py (2)</summary><blockquote> `14-14`: **LGTM — clear constant for native binary path.** Using a module-level constant for the native speedtest path is clean and allows easy modification if needed. --- `120-161`: **Well-implemented dual-engine speedtest with proper fallback.** The native binary detection and execution logic is well-structured: - Timeout of 60s correctly applied per coding guidelines - JSON parsing errors handled gracefully with fallback to CLI - Logging captures failure reasons for debugging - Clean fallthrough to CLI on any native path failure </blockquote></details> <details> <summary>front/plugins/internet_speedtest/script.py (3)</summary><blockquote> `6-7`: **LGTM — necessary imports for native binary support.** The subprocess and json imports enable the native speedtest execution path. --- `52-90`: **Well-structured native speedtest path with robust error handling.** The implementation addresses all previously raised concerns: - Safe parsing of timeout with proper fallback to 60s minimum - JSON parsing wrapped in try/except catching JSONDecodeError, KeyError, TypeError - Timeout handling with clear fallback messaging - Generic exception catch for unexpected failures with informative logging The try/except/else pattern on lines 71-84 is clean and ensures the return only happens when parsing succeeds. --- `91-109`: **Baseline fallback correctly implemented.** The baseline path using the speedtest library: - Properly wraps results in the expected format with `full_json` - Handles errors gracefully returning -1 for speeds - Uses `st.results.dict()` serialized to JSON for consistency with native path </blockquote></details> <details> <summary>front/plugins/internet_speedtest/README.md (3)</summary><blockquote> `7-19`: **Clear and actionable documentation for the dual-engine approach.** The instructions for opting into the native engine are well-structured with the docker-compose volume mapping example. This aligns with the implementation in `script.py` that checks for the binary at `NATIVE_SPEEDTEST_PATH`. --- `22-26`: **Data mapping documentation matches implementation.** The Watched_Value fields correctly reflect what `script.py` populates: - `watched1` → download_speed - `watched2` → upload_speed - `watched3` → full_json --- `30-31`: **Consider adding fallback behavior detail.** The note mentions fallback to baseline library but doesn't mention timeout or parse error fallbacks that are implemented in `script.py`. This is optional since the current wording covers the main case. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
3788c75 to
b2678e9
Compare
Hi @jokob-sk, Thanks for the feedback! I’ve updated the PR to address your points. Let me know if there’s anything else! |
- Introduce native Ookla Speedtest binary support for Gigabit connections - Add intelligent engine detection with automatic fallback to python-cli version - Map full JSON payload to Watched_Value3 for n8n integration - Add Spanish (es_es) localizations and update README instructions
amir0ff
force-pushed
the
optimize-speedtest-native
branch
from
b2678e9 to
3b1b853
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
front/plugins/internet_speedtest/README.md (1)
24-33: Data mapping documentation is clear.The mapping of watched values is well-documented. The static analysis hint about repetitive sentence structure (lines 26-28) is a minor stylistic nitpick that doesn't affect clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
front/plugins/internet_speedtest/README.mdfront/plugins/internet_speedtest/config.jsonfront/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.pyserver/const.py
🚧 Files skipped from review as they are similar to previous changes (1)
- front/plugins/internet_speedtest/config.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
server/const.pyfront/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer
server/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths.
Files:
server/const.pyserver/api_server/nettools_endpoint.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/*.py: All API responses must return"success":<False|True>and ifFalsemust include an"error"message, e.g.{"success": False, "error": "No stored open ports for Device"}
All API routes must expect headerAuthorization: Bearer <API_TOKEN>retrieved viaget_setting_value('API_TOKEN')for authorization.
Files:
server/api_server/nettools_endpoint.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
front/plugins/internet_speedtest/script.pyserver/api_server/nettools_endpoint.py
🧬 Code graph analysis (1)
front/plugins/internet_speedtest/script.py (2)
server/helper.py (1)
get_setting_value(200-257)server/api_server/nettools_endpoint.py (1)
speedtest(116-212)
🪛 LanguageTool
front/plugins/internet_speedtest/README.md
[style] ~28-~28: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hed_Value2** — Upload Speed (Mbps). - Watched_Value3 — Full JSON payload (useful fo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.14.10)
front/plugins/internet_speedtest/script.py
67-67: subprocess call: check for execution of untrusted input
(S603)
87-87: Do not catch blind exception: Exception
(BLE001)
102-102: Do not catch blind exception: Exception
(BLE001)
103-103: Use explicit conversion flag
Replace with conversion flag
(RUF010)
server/api_server/nettools_endpoint.py
124-124: subprocess call: check for execution of untrusted input
(S603)
160-160: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (12)
server/const.py (1)
48-49: LGTM: Environment-based configuration.The constant follows coding guidelines by using an environment variable with a sensible default path.
front/plugins/internet_speedtest/README.md (1)
3-22: LGTM: Clear documentation for hybrid approach.The README effectively documents both engines and provides clear opt-in instructions with a YAML example for mounting the native binary.
server/api_server/nettools_endpoint.py (4)
9-9: LGTM: Necessary import for native speedtest path.
122-163: Native speedtest implementation is robust.The native execution path correctly implements:
- 60s timeout (meets coding guidelines)
- Comprehensive JSON parsing error handling (JSONDecodeError, KeyError, TypeError)
- Appropriate logging for all error paths
- Graceful fallback to speedtest-cli
Past review concerns have been addressed.
181-181: LGTM: Timeout added to speedtest-cli call.The 60-second timeout meets coding guidelines and addresses the previous review concern.
188-194: LGTM: Appropriate timeout error handling.The TimeoutExpired handler returns a proper 504 status with a descriptive error message.
front/plugins/internet_speedtest/script.py (6)
6-7: LGTM: Necessary imports for native speedtest support.
19-19: LGTM: Import of NATIVE_SPEEDTEST_PATH constant.
43-43: LGTM: Proper mapping of full JSON payload.The watched3 field now correctly uses the full_json result, enabling detailed webhook integrations as documented in the README.
55-88: Native speedtest implementation handles errors correctly.The implementation properly:
- Checks for native binary existence
- Parses timeout with appropriate defaults and minimum enforcement
- Executes subprocess with timeout
- Handles JSON parsing errors with try/except/else pattern
- Logs all error conditions appropriately
- Falls back to baseline on any failure
The timeout parsing (lines 57-61) provides defensive handling for invalid configuration values, which aligns with coding guidelines.
Based on coding guidelines.
91-101: LGTM: Baseline fallback correctly implements full_json.The baseline path now properly serializes the complete speedtest results as JSON, maintaining consistency with the native path.
102-108: LGTM: Appropriate error handling for baseline fallback.The broad Exception catch is intentional as the final fallback, ensuring the plugin always returns a valid result structure. The error is logged and encoded in the full_json field for downstream analysis.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
front/plugins/internet_speedtest/README.md (1)
34-38: Vary the structure of data mapping bullets to improve readability.The three bullet points in the Data Mapping section all begin with "Watched_Value", which creates a repetitive pattern. Consider restructuring one or two bullets to vary the format while maintaining clarity.
🔎 Proposed improvement
### Data Mapping - **Watched_Value1** — Download Speed (Mbps). - **Watched_Value2** — Upload Speed (Mbps). - **Watched_Value3** — Full JSON payload (useful for n8n or detailed webhooks). + +Alternatively: + +- **Download Speed (Watched_Value1)** — Measured in Mbps. +- **Upload Speed (Watched_Value2)** — Measured in Mbps. +- **Full JSON Payload (Watched_Value3)** — Useful for n8n or detailed webhooks.Pick whichever format feels more natural to you; the key is breaking up the repetition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
front/plugins/internet_speedtest/README.md
🧰 Additional context used
🪛 LanguageTool
front/plugins/internet_speedtest/README.md
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hed_Value2** — Upload Speed (Mbps). - Watched_Value3 — Full JSON payload (useful fo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (1)
front/plugins/internet_speedtest/README.md (1)
1-43: Documentation is clear, well-structured, and aligns with the hybrid implementation.The README effectively documents the Python-First dual-engine design:
- Setup instructions for the optional native binary are detailed and accurate.
- The Docker Compose mapping rationale and example are helpful.
- Fallback behavior and performance recommendations are clearly stated.
- Data mapping descriptions correctly reflect the plugin's output fields.
Excellent work bridging the gap between the optional native binary and baseline Python engine for users.
|
Thanks a lot for incorporating the changes @amir0ff - it's almost perfect. Appreciate the code clean up and endpoint call instead of the old approach - I should have done that myself after creating the endpoint. I noticed you are using the Happy to merge after the above is addressed. |
- Added details for NATIVE_SPEEDTEST_PATH to the README under 'Usage'. - Explained default behavior and included examples for overriding the binary location. - Added a verbose log to print the binary path when the plugin starts up.
Thanks for pointing that out! The Additionally, I added a one-line verbose log to the plugin startup, which prints the resolved binary path for easier debugging. This ensures users have transparency and helps with troubleshooting. Let me know if anything else needs adjustment! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
front/plugins/internet_speedtest/script.py (2)
70-84: Add logging for native binary non-zero exit codes.When
result.returncode != 0, the code silently falls through to baseline without logging the failure or stderr output. Per coding guidelines, logging should aid troubleshooting.🔎 Proposed fix
if result.returncode == 0: try: data = json.loads(result.stdout) download_speed = round(data['download']['bandwidth'] * 8 / 10**6, 2) upload_speed = round(data['upload']['bandwidth'] * 8 / 10**6, 2) except (json.JSONDecodeError, KeyError, TypeError) as parse_error: mylog('none', [f"[INTRSPD] Failed to parse native JSON: {parse_error}"]) # Fall through to baseline fallback else: mylog('verbose', [f"[INTRSPD] Native Result (down|up): {download_speed} Mbps|{upload_speed} Mbps"]) return { 'download_speed': download_speed, 'upload_speed': upload_speed, 'full_json': result.stdout.strip() } + else: + mylog('none', [f"[INTRSPD] Native binary failed (exit {result.returncode}): {result.stderr.strip()}"])
103-108: Minor: Inconsistent string conversion style.Line 104 uses
str(e)while line 89 uses{e!s}. For consistency, prefer the f-string conversion flag.🔎 Proposed fix
except Exception as e: - mylog('verbose', [f"[INTRSPD] Error running speedtest: {str(e)}"]) + mylog('verbose', [f"[INTRSPD] Error running speedtest: {e!s}"]) return {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
front/plugins/internet_speedtest/README.mdfront/plugins/internet_speedtest/script.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
front/plugins/internet_speedtest/script.py
front/plugins/*/script.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
front/plugins/*/script.py: For plugins: definedatabase_column_definitionswhen creating/updating devices; watched fields trigger notifications.
Plugin scripts must write results to/tmp/log/plugins/last_result.<PREF>.logwith pipe-delimited format (9 required cols + optional 4). UsePlugin_Objectsto sanitize text and normalize MACs, then callwrite_result_file()exactly once at the end.
Collect plugin results withPlugin_Objects.add_object(...)during processing and callplugin_objects.write_result_file()exactly once at the end of the script. Do not write ad-hoc files for results.
Always check relevant logs first. Use logging as shown in other plugins. Prefer to log a brief summary before writing results to aid troubleshooting; keep logs concise atinfolevel and useverboseordebugfor extra context.
Files:
front/plugins/internet_speedtest/script.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
front/plugins/internet_speedtest/script.py
🪛 LanguageTool
front/plugins/internet_speedtest/README.md
[style] ~48-~48: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hed_Value2** — Upload Speed (Mbps). - Watched_Value3 — Full JSON payload (useful fo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.14.10)
front/plugins/internet_speedtest/script.py
68-68: subprocess call: check for execution of untrusted input
(S603)
88-88: Do not catch blind exception: Exception
(BLE001)
103-103: Do not catch blind exception: Exception
(BLE001)
104-104: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (6)
front/plugins/internet_speedtest/script.py (2)
38-48: LGTM!The
watched3field now correctly propagates thefull_jsonpayload, enabling detailed webhook data. The plugin correctly usesadd_object()andwrite_result_file()per coding guidelines.
56-68: Good defensive timeout handling.The safe parsing of
INTRSPD_RUN_TIMEOUTwith fallback to 60s and minimum enforcement aligns with coding guidelines requiring explicit timeouts (≥60s) on subprocess calls.front/plugins/internet_speedtest/README.md (4)
7-19: Clear dual-engine documentation.The documentation accurately describes the baseline and native engine options with proper configuration guidance for
NATIVE_SPEEDTEST_PATH.
21-42: LGTM!The setup instructions are clear with actionable steps, and the rationale for the
/usr/bin/speedtestmapping is well-explained.
44-48: LGTM!The data mapping accurately documents the watched values that align with the script implementation.
50-53: LGTM!The notes accurately describe the performance recommendation threshold and fallback behavior.
|
Thanks @amir0ff - looks great! Appreciate the help and contribution. |
2 participants
Internet Speed Plugin (INTRSPD)
Description
This PR optimizes the Internet Speed Plugin (
INTRSPD) by introducing a Python-First Hybrid execution model. It addresses CPU-bottleneck issues associated with single-threaded Python on high-speed connections while satisfying requirements for long-term maintainability and system compatibility.Engine Architecture
speedtest-cliPython library. This ensures 100% universal compatibility across all platforms for a stable "out-of-the-box" experience.Usage & Installation
To enable the high-performance native engine (recommended for connections > 100 Mbps):
/opt/netalertx/) and ensure it has executable permissions:/usr/bin/speedtestinside the container via yourdocker-compose.yml:Why this mapping?
Inside the container, a Python version of speedtest often exists in the virtual environment (
/opt/venv/bin/speedtest). Mapping your native binary specifically to/usr/bin/speedtestallows the plugin to prioritize the high-performance native engine over the baseline library.Verified Performance (Native Opt-in Mode)
Testing performed on a Raspberry Pi 4 (High-speed Fiber)
List of speed valeus during testing and debugging:
Data Mapping
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.