Enhancement for vzlogger meter, change logging in throttling.py #220
Conversation
|
Warning Rate limit exceeded@jwatzk has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdded POWER_CALCULATE, POWER_INPUT_UUID, and POWER_OUTPUT_UUID config keys and threaded them from the config loader into VZLogger. VZLogger gained flags/UUIDs to either read a single meter or compute power as input minus output. Throttling prints switched to logger.debug; Docker and dependency handling simplified. Changes
Sequence Diagram(s)sequenceDiagram
participant ConfigLoader
participant VZLogger
participant RemoteAPI
ConfigLoader->>ConfigLoader: Read POWER_CALCULATE, POWER_INPUT_UUID, POWER_OUTPUT_UUID
ConfigLoader->>VZLogger: __init__(ip, port, uuid, power_calculate, input_uuid, output_uuid)
VZLogger->>VZLogger: Store parameters
alt power_calculate == true
VZLogger->>RemoteAPI: GET /json/<POWER_INPUT_UUID>
RemoteAPI-->>VZLogger: input power JSON
VZLogger->>RemoteAPI: GET /json/<POWER_OUTPUT_UUID>
RemoteAPI-->>VZLogger: output power JSON
VZLogger->>VZLogger: compute input − output -> watts
else power_calculate == false
VZLogger->>RemoteAPI: GET /json/<uuid>
RemoteAPI-->>VZLogger: power JSON -> watts
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Fix all issues with AI agents
In @powermeter/vzlogger.py:
- Around line 6-20: In the vzlogger.__init__ constructor, add validation that if
power_calculate is True then power_input_uuid and power_output_uuid are provided
and non-empty (e.g., check they are not None and bool(str.strip()) is True); if
either is missing/empty raise a ValueError with a clear message identifying the
missing UUID(s). This prevents later malformed HTTP requests when power
calculation is enabled.
- Around line 28-35: The current parsing in the method that uses
self.get_json(...) (checking self.power_calculate and returning values using
["data"][0]["tuples"][0][1]) can raise KeyError/IndexError/TypeError if the API
shape changes; wrap the parsing of responses from get_json(self.uuid),
get_json(self.power_input_uuid) and get_json(self.power_output_uuid) in a
try/except that catches KeyError, IndexError and TypeError, validate that the
top-level "data" key exists and is a non-empty list and that "tuples" exists and
has at least one item before indexing, and on failure raise or log a clear error
(e.g., include which UUID and the raw response) or return a safe sentinel (None
or 0) so callers get an explicit, descriptive error instead of an obscure
exception.
🧹 Nitpick comments (2)
powermeter/throttling.py (1)
3-3: Unused import:loggingmodule not referenced.The
loggingmodule is imported but never used directly. Onlyloggerfromconfig.loggeris used.♻️ Remove unused import
-import loggingpowermeter/vzlogger.py (1)
31-32: Remove redundant variable initialization.Lines 31-32 initialize
power_inandpower_outto 0, but these values are immediately overwritten on lines 33-34. The initialization serves no purpose.♻️ Remove redundant initialization
else: - power_in = 0 - power_out = 0 power_in = int(self.get_json(self.power_input_uuid)["data"][0]["tuples"][0][1]) power_out = int(self.get_json(self.power_output_uuid)["data"][0]["tuples"][0][1]) return [power_in - power_out]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config.ini.exampleconfig/config_loader.pypowermeter/throttling.pypowermeter/vzlogger.py
🧰 Additional context used
🧬 Code graph analysis (1)
powermeter/throttling.py (1)
powermeter/base.py (1)
Powermeter(2-8)
🪛 Ruff (0.14.10)
powermeter/throttling.py
74-74: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (3)
powermeter/throttling.py (1)
55-82: Excellent improvement to use debug-level logging.Replacing
print()statements withlogger.debug()aligns with the PR objective to prevent log flooding and control message output via log levels. This will help prevent container memory growth from excessive logging.config.ini.example (1)
92-94: LGTM! Configuration keys properly documented.The new configuration options for power calculation are clearly defined and align with the code changes in
vzlogger.pyandconfig_loader.py.config/config_loader.py (1)
243-245: Configuration reading looks good, but depends on validation in VZLogger.The new configuration parameters are correctly read and passed to the VZLogger constructor. The fallback values are appropriate. However, ensure that the validation suggested in
powermeter/vzlogger.pyis implemented to catch cases wherePOWER_CALCULATEisTruebut the UUID parameters are empty or missing.
| def __init__( | ||
| self, | ||
| ip: str, | ||
| port: str, | ||
| uuid: str, | ||
| power_calculate: bool, | ||
| power_input_uuid: str, | ||
| power_output_uuid: str, | ||
| ): | ||
| self.ip = ip | ||
| self.port = port | ||
| self.uuid = uuid | ||
| self.power_calculate = power_calculate | ||
| self.power_input_uuid = power_input_uuid | ||
| self.power_output_uuid = power_output_uuid |
There was a problem hiding this comment.
Validate required UUIDs when power calculation is enabled.
When power_calculate is True, the power_input_uuid and power_output_uuid parameters are required for proper operation. However, there's no validation to ensure these values are non-empty. If empty strings are passed (which can happen from config defaults), the code will attempt HTTP requests to malformed URLs, resulting in confusing runtime errors.
🛡️ Add validation in constructor
def __init__(
self,
ip: str,
port: str,
uuid: str,
power_calculate: bool,
power_input_uuid: str,
power_output_uuid: str,
):
self.ip = ip
self.port = port
self.uuid = uuid
self.power_calculate = power_calculate
self.power_input_uuid = power_input_uuid
self.power_output_uuid = power_output_uuid
+
+ # Validate required parameters when power calculation is enabled
+ if self.power_calculate:
+ if not self.power_input_uuid or not self.power_output_uuid:
+ raise ValueError(
+ "POWER_INPUT_UUID and POWER_OUTPUT_UUID must be provided when POWER_CALCULATE is True"
+ )
+ elif not self.uuid:
+ raise ValueError("UUID must be provided when POWER_CALCULATE is False")
+
self.session = requests.Session()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @powermeter/vzlogger.py around lines 6 - 20, In the vzlogger.__init__
constructor, add validation that if power_calculate is True then
power_input_uuid and power_output_uuid are provided and non-empty (e.g., check
they are not None and bool(str.strip()) is True); if either is missing/empty
raise a ValueError with a clear message identifying the missing UUID(s). This
prevents later malformed HTTP requests when power calculation is enabled.
There was a problem hiding this comment.
Agree with this issue. Would be good to add validation for a clear error message when users provide an invalid config.
There was a problem hiding this comment.
@tomquist, I'll plan the validation changes for the VZLogger constructor to ensure proper configuration validation.
[plan_code_changes]
Task: Add validation to VZLogger constructor
- Add validation logic after parameter assignment in
__init__ - Check that required UUIDs are non-empty based on power_calculate mode
- Raise ValueError with clear messages for invalid configurations
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @Dockerfile:
- Line 8: Restore the non-root user setup by re-enabling the user creation and
switching to it: uncomment or re-add the RUN useradd -m -u 1003 b2500 line (and
any related chown commands) and add a USER b2500 directive so the container does
not run as root; ensure any application directories and files referenced in the
Dockerfile are chown'd to that UID (or adjust file ownership steps already
present) and if the service needs privileged ports (<1024) either move to
unprivileged ports or grant capabilities explicitly rather than running as root.
- Line 30: Remove the leading indentation before the Dockerfile instruction so
the CMD instruction is left-aligned; change the line containing CMD ["python",
"main.py", "--loglevel", "info"] to have no leading spaces or tabs so the
Dockerfile parser/linter recognizes the instruction correctly.
- Around line 24-27: The HEALTHCHECK using a heredoc plus backslashes is invalid
because Dockerfile line continuations collapse the heredoc; replace it with a
single-line command (e.g., change the HEALTHCHECK to use python -c) so the probe
runs reliably: remove the multiline heredoc and backslashes around "python -
<<'EOF' ..." and use a one-liner like HEALTHCHECK CMD python -c "import
urllib.request; urllib.request.urlopen('http://127.0.0.1:52500/health',
timeout=3)" || exit 1 so the check actually executes and returns proper exit
codes.
In @requirements.txt:
- Around line 1-9: Update the pinned urllib3 version in requirements.txt from
"urllib3==2.2.2" to "urllib3==2.6.3" to address the listed HIGH-severity CVEs;
ensure the change coexists with the existing "requests==2.32.3" (requests allows
urllib3>=1.21.1,<3) and then regenerate lock/assets or run dependency install
and tests to verify no compatibility breakage.
🧹 Nitpick comments (1)
Dockerfile (1)
11-14: Consider collapsing pip installs into one layer.
Fewer layers + simpler caching behavior.Proposed refactor
COPY requirements.txt . -RUN pip install --no-cache-dir --upgrade pip -RUN pip install --no-cache-dir -r requirements.txt +RUN pip install --no-cache-dir --upgrade pip \ + && pip install --no-cache-dir -r requirements.txt
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.dockerignoreDockerfilerequirements.txt
🧰 Additional context used
🪛 OSV Scanner (2.3.1)
requirements.txt
[HIGH] 9-9: urllib3 2.2.2: urllib3 streaming API improperly handles highly compressed data
[HIGH] 9-9: urllib3 2.2.2: Decompression-bomb safeguards bypassed when following HTTP redirects (streaming API)
[HIGH] 9-9: urllib3 2.2.2: urllib3 does not control redirects in browsers and Node.js
[HIGH] 9-9: urllib3 2.2.2: urllib3 allows an unbounded number of links in the decompression chain
[HIGH] 9-9: urllib3 2.2.2: urllib3 redirects are not disabled when retries are disabled on PoolManager instantiation
🔇 Additional comments (1)
.dockerignore (1)
1-1: The ignore patternha_addonis already correct for your repository structure.The
ha_addondirectory exists only at the root level, so the simple pattern excludes it properly. The optional refactors (**/ha_addonorha_addon/) are unnecessary—they would work but add no practical value since the directory doesn't appear nested elsewhere in the repository.
Dockerfile
Outdated
| RUN apt-get update && apt-get install -y \ | ||
| curl \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
| #RUN useradd -m -u 1003 b2500 |
There was a problem hiding this comment.
Don’t run the container as root (security regression).
This container exposes network ports; dropping privileges materially reduces impact of a breakout/RCE.
Proposed fix (restore non-root user)
-#RUN useradd -m -u 1003 b2500
+RUN useradd -m -u 1003 b2500
COPY . /app/
-#RUN chown -R b2500:b2500 /app
-#USER b2500
+RUN chown -R b2500:b2500 /app
+USER b2500Also applies to: 17-18
🤖 Prompt for AI Agents
In @Dockerfile at line 8, Restore the non-root user setup by re-enabling the
user creation and switching to it: uncomment or re-add the RUN useradd -m -u
1003 b2500 line (and any related chown commands) and add a USER b2500 directive
so the container does not run as root; ensure any application directories and
files referenced in the Dockerfile are chown'd to that UID (or adjust file
ownership steps already present) and if the service needs privileged ports
(<1024) either move to unprivileged ports or grant capabilities explicitly
rather than running as root.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
powermeter/vzlogger.py (2)
6-21: LGTM! Constructor follows established pattern.The new parameters for power calculation are consistent with the approach used in
iobroker.py.Consider validating that
power_input_uuidandpower_output_uuidare provided whenpower_calculateisTrueto fail fast with a clear error message rather than at runtime during the HTTP call.
27-45: Good addition of error handling for response parsing.The branching logic for
power_calculatecorrectly mirrors the pattern iniobroker.py, and the error handling for parsing failures (KeyError,IndexError,ValueError) is a valuable improvement for debugging malformed responses.Note: Network errors from
requests(e.g.,ConnectionError,Timeout) will propagate uncaught, which is consistent with other powermeter implementations in this codebase. If you want uniform error handling, you could catchrequests.RequestExceptionas well, but this is optional given the existing patterns.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
powermeter/vzlogger.py
🧰 Additional context used
🧬 Code graph analysis (1)
powermeter/vzlogger.py (4)
powermeter/amisreader.py (2)
get_json(10-12)get_powermeter_watts(14-16)powermeter/iobroker.py (2)
get_json(23-25)get_powermeter_watts(27-44)powermeter/tasmota.py (1)
get_json(29-31)powermeter/base.py (1)
get_powermeter_watts(7-8)
🪛 Ruff (0.14.10)
powermeter/vzlogger.py
34-34: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
powermeter/vzlogger.py (1)
23-25: LGTM!The
get_jsonmethod now accepts a path parameter, which is appropriate for supporting multiple UUID endpoints. The URL construction with the explicit/beforepathworks correctly with UUID-based paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Dockerfile:
- Around line 19-22: The HEALTHCHECK heredoc with backslash-escaped newlines is
collapsing the shell heredoc and causing immediate failures; replace the
heredoc-style block under the HEALTHCHECK directive with a single-line command
(e.g., use python -c "import urllib.request;
urllib.request.urlopen('http://127.0.0.1:52500/health', timeout=3)" or an
equivalent curl/wget one-liner) so the command runs correctly in Docker without
relying on escaped newlines; update the HEALTHCHECK CMD entry in the Dockerfile
accordingly.
🧹 Nitpick comments (1)
Dockerfile (1)
8-10: Preferpython -m pipand collapse the two RUNs into one layer. This avoids any pip/python mismatch edge cases and trims image layers.Proposed change
COPY requirements.txt . -RUN pip install --no-cache-dir --upgrade pip -RUN pip install --no-cache-dir -r requirements.txt +RUN python -m pip install --no-cache-dir --upgrade pip \ + && python -m pip install --no-cache-dir -r requirements.txt
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfilerequirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🔇 Additional comments (2)
Dockerfile (2)
3-4: ENV consolidation looks good. Keeps runtime behavior consistent (PYTHONDONTWRITEBYTECODE,PYTHONUNBUFFERED).
25-25: CMD update is fine and aligns with the “debug-only throttling logs” objective.
There was a problem hiding this comment.
Can we keep the comments? I would prefer to continue using the Pipfile.
There was a problem hiding this comment.
Would prefer if we keep using the Pipfile. I might decide to switch to requirements.txt at some point but this seems unrelated to the change.
| def __init__( | ||
| self, | ||
| ip: str, | ||
| port: str, | ||
| uuid: str, | ||
| power_calculate: bool, | ||
| power_input_uuid: str, | ||
| power_output_uuid: str, | ||
| ): | ||
| self.ip = ip | ||
| self.port = port | ||
| self.uuid = uuid | ||
| self.power_calculate = power_calculate | ||
| self.power_input_uuid = power_input_uuid | ||
| self.power_output_uuid = power_output_uuid |
There was a problem hiding this comment.
Agree with this issue. Would be good to add validation for a clear error message when users provide an invalid config.
| #POWER_CALCULATE = True | ||
| #POWER_INPUT_UUID = grid-import-uuid | ||
| #POWER_OUTPUT_UUID = grid-export-uuid |
There was a problem hiding this comment.
Please add a comment above those lines to document what this does. And also please update the README file accordingly.
2 participants
The NetzOÖ Smartmeter Siemens TD-3511 does not provide a 'saldo' for grid export/import so is has to be calculated.
Messages in throttling.py floods the container logs and causes a constant increase of container memory.
Only print the messages when container starts with '--loglevel debug'.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.