[chores] Move logger calls in subroutine #57 #76
Conversation
| exit 1 | ||
| fi | ||
| [ "$interval" -lt 1 ] && log -e "Interval is invalid. Use time value(eg: '10', '2m', '3h', '1d')" | ||
|
|
There was a problem hiding this comment.
can we move all these checks to the agent as @feckert did recently in openwisp-config?
So we avoid to define the log function twice which is really bad.
There was a problem hiding this comment.
I left it here and instead send it to stderr because this will come when invalid value is provided in interval
There was a problem hiding this comment.
We may do similar kind of refactoring here to as done in openwisp-config in future
There was a problem hiding this comment.
Yes, see @feckert's proposals: openwisp/openwisp-config#109
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
2 times, most recently
from
be929b2 to
46dad97
Compare
|
This PR is tested and it works as expected. I will rebase and mark is as |
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
46dad97 to
395d66e
Compare
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
395d66e to
2f48c70
Compare
devkapilbansal
changed the base branch from
issues/46-delete-old-data
to
master
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I left one comment below.
@feckert would be good if you could double check this as well.
| ;; | ||
| -*) | ||
| echo "Invalid option: $1" | ||
| exit 1 |
There was a problem hiding this comment.
why is this exit 1 removed? Does echoerr returns non zero exit code or what?
There was a problem hiding this comment.
echoerr is defined above that echo the message to standard error and returns exit 1 too
There was a problem hiding this comment.
| exit 1 | ||
| fi | ||
| [ "$interval" -lt 1 ] && log -e "Interval is invalid. Use time value(eg: '10', '2m', '3h', '1d')" | ||
|
|
There was a problem hiding this comment.
Yes, see @feckert's proposals: openwisp/openwisp-config#109
| type="$2" | ||
| shift 2 | ||
|
|
||
| if [ "$type" = "-v" ] && [ "$VERBOSE_MODE" -ne "1" ]; then |
There was a problem hiding this comment.
Sorry for the stupid question. What is the use case of the option -v?
There was a problem hiding this comment.
To check if we are in verbose mode or not. By using -v option, message will be logged in verbose mode only
There was a problem hiding this comment.
I wouldn't do it like that.
That's what the different log levels are for.
I would add an additional debug log-level here.
If the log-level of the software (your script) is higher than the log-level of the log message, then this message is not logged and so not displayed.
The option for the shell script is extended by the option --log-level.
This are the following options that could be used:
- error
- warn
- info
- debug
Each message has one of this log-level.
I think this is more common.
log warn "Not enough memory available, skipping collect data."
or
log info "SIGUSR1 received! Sending data"
This is the loglevel handling in the kernel but this does also work for the userland.
https://www.linuxtopia.org/online_books/linux_kernel/kernel_configuration/re06.html
There was a problem hiding this comment.
@feckert some programs do have their own verbosity level, think of openvpn. Here it's a similar case but simpler. In verbose mode we make debugging easier.
There was a problem hiding this comment.
@feckert BTW in recent OpenWrt versions filtering out log levels doesn't work because the logd implementation doesn't support it.
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
9eb15c2 to
d54c2a4
Compare
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
d54c2a4 to
9e5f570
Compare
|
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
9e5f570 to
7fec7f7
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Hey @devkapilbansal! I think we should revamp this!
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
7fec7f7 to
b47e2a5
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Great work @devkapilbansal, I spotted some minor typos and I think there's room for improvement to avoid repetition and simplifying the code further, please see below.
| [ "$VERBOSE_MODE" -eq "1" ] \ | ||
| && logger -s "Not enough memory available, skipping collect data." \ | ||
| -p daemon.warn | ||
| log -w -v "Not enough memory available, skipping collect data." |
There was a problem hiding this comment.
After reading this code now.. it seems to me that this message shouldn't be logged only in verbose mode because it is not a condition which should happen often and when it happens we most likely want to know. What do you think?
I would change this in a separate commit/PR though, not now. Let's maintain the behavior unaltered in this PR.
There was a problem hiding this comment.
Hi, let's say that the memory is full. Then after every 5 mins this message will be logged unless the memory is freed once again. I thought that it will add unnecessary noise.
There was a problem hiding this comment.
@devkapilbansal I think the openwrt logger allocates a certain amount of memory for its logging, it automatically deletes old log lines from memory to make room for new lines, therefore we should be fine, the logger will fille up with this message but it's fine, we definitely want to know if this happens. Without doing this, in most cases when memory runs out, we won't understand what's going on without switching to verbose mode.
| -t openwisp-monitoring \ | ||
| -p daemon.err | ||
| [ "$VERBOSE_MODE" -ne "1" ] && log -e -n "Data not sent successfully. Response code is \"$response_code\"." \ | ||
| "Run with verbose mode to find more." |
There was a problem hiding this comment.
There's still repetition here which makes the code hard to read.
I think we need to edit the log function to avoid this.
Ideally we could do something like:
log -e -n "Data not sent successfully. Response code is \"$response_code\"." \
-v "Error message is $error_message"The non verbose output should be printed also in verbose mode, while the verbose output is appended to the non verbose output but only when in verbose mode, so in non verbose mode we would have:
Data not sent successfully. Response code is "400".`
While in verbose mode we would have:
Data not sent successfully. Response code is "400". Error message is ERROR_MESSAGE_HERE.
There was a problem hiding this comment.
The condition is here to ensure that the logs are not added unnecessarily when the connection is interrupted due to some issue for a long time.
Discussed here in "Non-verbose mode" #42 (review)
| if [ "$FAILING" -eq "1" ]; then | ||
| FAILING=0 | ||
| rm -f "$RESPONSE_FILE" | ||
| [ "$VERBOSE_MODE" -ne "1" ] && log -i -n "Data sent successfully." |
There was a problem hiding this comment.
same here: Data sent successfully. is repeated twice and there's another unnecessary if here.
There was a problem hiding this comment.
I added this so that we don't fill logs uncessarily when data is transferred normally. In non-verbose mode, it will add this log "Data sent successfully" only once and the line will appear again if the connection is interrupted.
If the variable FAILING is set to 1, then only this line will be logged.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds centralized logging and interval parsing to the agent and consolidates option assembly in the init script. monitoring.agent: introduces public log() and time_to_seconds(), replaces many direct or verbose-guarded logger calls with log(), validates INTERVAL via time_to_seconds(), adjusts messaging, tracks consecutive successful sends and inserts a randomized pause every 10 successes, and refines HTTP 404 handling. monitoring.init: adds add_option() to build command-line parameters from config, removes inline interval parsing, and creates separate procd instances for send_data and collect_data; minor lifecycle message punctuation updates. Sequence Diagram(s)sequenceDiagram
participant Init as Init (monitoring.init)
participant Agent as Agent (monitoring.agent)
participant Collector as Collector (collect process)
participant Server as HTTP API
Init->>Agent: start send procd instance (PROG + add_option args)
Init->>Collector: start collect procd instance (PROG + add_option args)
Collector->>Agent: invoke collect_data -> produce payload
Agent->>Server: POST /data (send_data)
alt 200 OK
Server-->>Agent: 200 Success
Agent->>Agent: increment success_count
alt success_count % 10 == 0
Agent->>Agent: sleep(random pause)
end
Agent->>Init: log("data sent") via log()
else 404 Not Found
Server-->>Agent: 404
Agent->>Agent: log("device may be deleted") via log()
Agent->>Collector: continue (no forced exit)
else other error
Server-->>Agent: error
Agent->>Agent: log("send error") and handle retries
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
nemesifier
changed the title
![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 `@openwisp-monitoring/files/monitoring.agent`:
- Around line 98-104: The failure message is never logged because the loop
checks [ "$n" -eq 5 ] before incrementing n; update the retry logic in the until
loop that runs /usr/sbin/netjson-monitoring so the log -e -v "Collecting data
failed!" is executed when the final retry fails: either move the logging check
to after n=$((n + 1)) or change the condition to test for the last attempt
(e.g., [ "$n" -eq 4 ] or [ "$n" -ge 5 ]) so that when netjson-monitoring
(invoked with "$MONITORED_INTERFACES") has exhausted retries the log call inside
the loop runs; keep the loop structure with the variable n and the existing log
-e -v invocation.
- Around line 309-312: REGISTRATION_INTERVAL is computed before INTERVAL is
converted by time_to_seconds, causing values like "5m" to yield incorrect
arithmetic; move the REGISTRATION_INTERVAL=$((INTERVAL / 10)) line to after
INTERVAL="$(time_to_seconds "$INTERVAL")" (and after the [ -z "$INTERVAL" ]
invalid check) so REGISTRATION_INTERVAL is calculated from the numeric seconds
value returned by time_to_seconds; update any related comments or dependent
logic referencing REGISTRATION_INTERVAL to reflect the new position.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp-monitoring/files/monitoring.agentopenwisp-monitoring/files/monitoring.init
🔇 Additional comments (15)
openwisp-monitoring/files/monitoring.agent (12)
37-56: Well-structured logging wrapper.The centralized
log()function cleanly consolidates logger calls with level mapping and verbose-mode gating. The implementation correctly handles the-vtype for verbose-only messages.Minor observation: The
typeparameter accepts-vfor verbose and implicitly-nfor normal, but there's no validation if an invalid type is passed. Consider adding a case check fortypesimilar tolevelif stricter validation is desired.
58-70: Time parsing function handles common formats.The cascading conditional correctly parses numeric intervals and time suffixes (s, m, h, d). The
unsetcleanup is good shell hygiene.Note: Values like
0,0s, or negative numbers will return empty (since-ge 1fails), which the caller handles at line 312. If zero intervals should be explicitly rejected with a clearer message, consider adding a dedicated check.
88-88: Appropriate logging level for memory warnings.Using
-n(non-verbose) ensures memory availability warnings are always logged, which aligns with the earlier review discussion that these rare but significant events should be visible without verbose mode.
134-134: LGTM!Correctly converted to use the centralized
log()wrapper with appropriate verbose-only level.
143-146: LGTM!Signal handler logging appropriately converted to verbose-only mode.
179-197: Logging logic preserved from original implementation.The
FAILINGstate-based conditional logging correctly maintains the existing behavior where non-verbose mode only logs state transitions (first failure and first success after recovery). This addresses the earlier review discussion about avoiding log noise while still capturing important state changes.
152-152: LGTM!Appropriate verbose-only logging for routine "no data" checks.
203-203: Good decision to always log HTTP 400 errors.Since data is being discarded on HTTP 400, this warrants non-verbose (
-n) logging so operators are aware of data loss.
219-231: LGTM!Registration status messages appropriately use non-verbose logging since device registration is an important operational event.
239-239: LGTM!Bootup delay notification appropriately uses non-verbose logging.
301-303: LGTM!Punctuation standardization is consistent with other error messages.
340-340: LGTM!Clear, well-punctuated error message for invalid mode.
openwisp-monitoring/files/monitoring.init (3)
12-24: Clean helper function for option construction.The
add_option()function effectively reduces code duplication by encapsulating theconfig_get+procd_append_parampattern. The shellcheck disable for SC3043 is appropriate since OpenWrt's ash shell supportslocal.
64-68: LGTM!Consistent punctuation in start/stop log messages.
52-62: Missing--bootup_delayoption for collect instance.The collect instance doesn't pass
--bootup_delay, so the agent always uses its hardcoded default of 10 seconds (line 314 in monitoring.agent). If this configuration should be user-configurable, consider adding:add_option "monitoring" "--bootup_delay" bootup_delay "10"If the hardcoded default is intentional, this can be ignored.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-project-automation
bot
moved this from Ready for review/testing
to In progress
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this from Ready for review/testing
to In progress
in 26.05 Release
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp-monitoring/files/monitoring.agent`:
- Line 204: The line setting the success counter ("success=$((success + 1))") is
indented with spaces while the rest of monitoring.agent uses tabs; replace the
leading spaces with a tab so the assignment aligns with surrounding tab-indented
blocks (ensure other similar lines use tabs for consistency) and save the file
with consistent indentation for the success variable and related statements.
♻️ Duplicate comments (1)
openwisp-monitoring/files/monitoring.agent (1)
100-106: Bug: Failure message is never logged.The condition
[ "$n" -eq 5 ]on line 103 is checked beforenis incremented. The loop iterates withnvalues 0, 1, 2, 3, 4. On the final iteration whenn=4, the checkn -eq 5is false, thennbecomes 5 and the loop exits. The error message is never executed.Proposed fix
until [ "$n" -ge 5 ]; do /usr/sbin/netjson-monitoring --dump "$MONITORED_INTERFACES" && break - [ "$n" -eq 5 ] && log -e -v "Collecting data failed!" + [ "$n" -eq 4 ] && log -e -v "Collecting data failed!" n=$((n + 1)) sleep 5 done
🧹 Nitpick comments (2)
openwisp-monitoring/files/monitoring.agent (2)
184-186: Inconsistency: Direct logger call should uselog()wrapper.This block still uses the direct
loggercommand instead of the newlog()wrapper, which contradicts the PR's goal of consolidating logger calls.Proposed fix
- [ "$VERBOSE_MODE" -eq "1" ] && logger -s "data file $filename not found." \ - -p daemon.info + log -i -v "data file $filename not found." continue
228-230: Inconsistency: Direct logger call should uselog()wrapper.This critical error message still uses the direct
loggercommand instead of thelog()wrapper. For consistency and maintainability, consider using the wrapper here as well.Proposed fix
- logger -s "Giving up and shutting down: the device may have been deleted from OpenWISP Controller" \ - -t openwisp-monitoring \ - -p daemon.err + log -e -n "Giving up and shutting down: the device may have been deleted from OpenWISP Controller"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp-monitoring/files/monitoring.agentopenwisp-monitoring/files/monitoring.init
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: QA-Checks and Tests
- GitHub Check: Build and upload package as artifacts
🔇 Additional comments (9)
openwisp-monitoring/files/monitoring.init (4)
12-24: LGTM! Good abstraction for config-to-command mapping.The
add_optionfunction cleanly centralizes configuration retrieval and command building. Minor style note: the brace grouping around thelocaldeclarations (lines 14-20) is unnecessary sincelocaloperates at function scope, not block scope. The shellcheck disable comment applies to the following command regardless of braces.
26-52: Correct config loading sequence for first instance.The flow correctly loads
openwispconfig first, captures respawn values (lines 30-32), adds http options (lines 37-42), then loadsopenwisp-monitoringbefore adding monitoring options (lines 46-48). This ordering is important sinceconfig_loadreplaces the previously loaded config.
54-64: Collect instance correctly configured without http options.The second instance appropriately omits http config options since collect mode only gathers data locally. The respawn parameters (captured from http config at lines 30-32) are shared between both instances, which is sensible for consistent process management.
69-75: LGTM!Standard procd pattern - the
stop_servicehook handles logging while procd manages actual process termination. Theservice_triggerscorrectly monitors both config files.openwisp-monitoring/files/monitoring.agent (5)
39-58: LGTM!The
log()wrapper cleanly encapsulates level mapping, verbose-mode gating, and tagged logging. The early return for non-verbose mode prevents unnecessary logging calls.
60-72: LGTM!The
time_to_seconds()function correctly handles numeric values and common time suffixes. The fallback to empty output for invalid values is properly validated downstream inmain().
129-150: LGTM!The
save_data()andhandle_sigusr1()functions correctly use the newlog()wrapper, maintaining consistent logging throughout the codebase.
252-278: LGTM!The
wait_until_registered()andbootup_delay()functions correctly use the newlog()wrapper with appropriate log levels.
350-354: LGTM!The
INTERVALconversion andREGISTRATION_INTERVALcomputation are now correctly ordered —REGISTRATION_INTERVALis computed aftertime_to_seconds()converts the interval to numeric seconds.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| logger -s "Data sent successfully." \ | ||
| -t openwisp-monitoring \ | ||
| -p daemon.info | ||
| success=$((success + 1)) |
There was a problem hiding this comment.
Indentation inconsistency.
Line 204 uses spaces for indentation while the rest of the file uses tabs. This may cause issues with shell scripts that rely on consistent whitespace.
Proposed fix
- success=$((success + 1))
+ success=$((success + 1))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| success=$((success + 1)) | |
| success=$((success + 1)) |
🤖 Prompt for AI Agents
In `@openwisp-monitoring/files/monitoring.agent` at line 204, The line setting the
success counter ("success=$((success + 1))") is indented with spaces while the
rest of monitoring.agent uses tabs; replace the leading spaces with a tab so the
assignment aligns with surrounding tab-indented blocks (ensure other similar
lines use tabs for consistency) and save the file with consistent indentation
for the success variable and related statements.
d80d4df to
7103595
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 (1)
openwisp-monitoring/files/monitoring.agent (1)
242-246: Fix pause condition to avoid throttling with zero successes.As written, the pause block triggers when
success=0(since0 % 10 == 0), which contradicts the intent of “every 10 successful requests.” This adds avoidable delay on failed sends.✅ Proposed fix
- if [ $((success % 10)) -eq 0 ]; then + if [ "$success" -gt 0 ] && [ $((success % 10)) -eq 0 ]; then pause_duration=$(/usr/sbin/openwisp-get-random-number 1 5) sleep "$pause_duration" fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp-monitoring/files/monitoring.agent
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and upload package as artifacts
- GitHub Check: QA-Checks and Tests
🔇 Additional comments (2)
openwisp-monitoring/files/monitoring.agent (2)
39-72: LGTM: centralized logging + time parsing.Clean consolidation of logging and interval parsing; the helper usage keeps behavior consistent and validation intact.
Also applies to: 351-352
90-90: LGTM on log() adoption and message cleanups.The routing of messages through
log()reads clearer and keeps verbose gating consistent.Also applies to: 99-104, 138-148, 157-157, 191-198, 204-211, 216-216, 221-221, 256-262, 272-272, 343-343, 381-381
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
7103595 to
7062db2
Compare
Pull Request Test Coverage Report for Build 21194873342Details
💛 - Coveralls |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@devkapilbansal the PR history got completely messed up.
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
7062db2 to
bb5a19f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp-monitoring/files/monitoring.init`:
- Around line 40-41: The two lines using add_option ("add_option \"http\"
\"--cacert\" cacert" and "add_option \"http\" \"--capath\" capath") are indented
with spaces; replace the leading spaces with a single tab (to match the file's
existing tab-based indentation) so the indentation is consistent with the rest
of the file.
🧹 Nitpick comments (2)
openwisp-monitoring/files/monitoring.agent (2)
184-186: Inconsistent: Direct logger call instead of log() wrapper.This verbose-mode logger call should use the new
log()wrapper for consistency with the rest of the refactoring.Proposed fix
- [ "$VERBOSE_MODE" -eq "1" ] && logger -s "data file $filename not found." \ - -p daemon.info + log -i -v "Data file $filename not found."
228-230: Consider using log() wrapper for consistency.While this error message is critical and should always be logged (correctly using
-p daemon.err), using thelog()wrapper would maintain consistency. However, since this is followed by anexit, the direct call is acceptable.Optional: Use log() wrapper
- logger -s "Giving up and shutting down: the device may have been deleted from OpenWISP Controller" \ - -t openwisp-monitoring \ - -p daemon.err + log -e -n "Giving up and shutting down: the device may have been deleted from OpenWISP Controller"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp-monitoring/files/monitoring.agentopenwisp-monitoring/files/monitoring.init
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and upload package as artifacts
- GitHub Check: QA-Checks and Tests
🔇 Additional comments (14)
openwisp-monitoring/files/monitoring.init (4)
12-24: LGTM! Clean helper function for config-to-command mapping.The
add_option()function correctly encapsulates the pattern of reading a config value and conditionally appending it to the procd command. The shellcheck disable forlocalin POSIX sh is appropriately documented.
34-52: First procd instance setup looks correct.The
openwisp-monitoring_send_datainstance is properly configured with HTTP options, monitoring options, and the--mode sendflag. The respawn parameters are correctly applied.
54-64: Second procd instance setup looks correct.The
openwisp-monitoring_collect_datainstance is properly configured with monitoring-specific options (monitored_interfaces,required_memory) and the--mode collectflag.
66-71: LGTM! Lifecycle logging with consistent punctuation.The punctuation changes ("started." and "stopping.") are consistent and improve message clarity.
openwisp-monitoring/files/monitoring.agent (10)
39-58: LGTM! Well-structured logging wrapper.The
log()function cleanly centralizes logging, respects verbose mode, and maps level flags to syslog priorities. This addresses the PR objective of moving logger calls into a subroutine.
60-72: Time parsing function is functional, validation handled downstream.The function correctly parses numeric and suffixed time values. Invalid inputs result in an empty output, which
main()properly validates with the[ -z "$INTERVAL" ]check on line 352.One edge case to note: values like
"0"or"0s"will fail the-ge 1checks and produce empty output, but a zero interval is likely invalid anyway, so this behavior is acceptable.
90-91: Good: Memory warning now logs in non-verbose mode.This aligns with the discussion in past reviews—memory issues should be logged regardless of verbose mode since they indicate important system state.
97-107: LGTM! Retry logic bug fixed.The condition on line 103 is now correctly
[ "$n" -eq 4 ], ensuring the failure message is logged on the final retry attempt before the loop exits.
129-145: LGTM! Clean logging wrapper usage.
191-211: State-tracking logic for FAILING flag is clear and functional.The logic correctly avoids spamming logs during prolonged failures and logs status transitions appropriately.
242-247: Good addition: Throttling to prevent server overload.The random pause every 10 successful requests is a sensible addition to prevent overwhelming the server when there's a backlog of data to send.
252-264: LGTM! Clean logging wrapper usage for registration flow.
350-354: LGTM! Interval conversion order fixed.
REGISTRATION_INTERVALis now correctly computed aftertime_to_seconds()convertsINTERVALto numeric seconds, fixing the previously identified bug.
367-382: Mode handling and final flow looks correct.Both
collectandsendmodes are properly initialized with their respective configuration and entry points.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
devkapilbansal
force-pushed
the
issues/57-move-logger-call-in-subroutine
branch
from
2c66e47 to
9cc4b5e
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp-monitoring/files/monitoring.init`:
- Around line 30-32: The three variables are being set with command substitution
which yields nothing because config_get assigns variables rather than printing
them; replace the "$(config_get ...)" usages with direct calls to config_get so
that config_get assigns respawn_threshold, respawn_timeout, and respawn_retry
(i.e., call config_get http respawn_threshold, config_get http respawn_timeout,
config_get http respawn_retry) and keep the existing default fallbacks
("${respawn_threshold:-3600}", etc.) intact.
♻️ Duplicate comments (1)
openwisp-monitoring/files/monitoring.agent (1)
86-91: Confirm low-memory logging is now non-verbose.
Line 90 uses-n, so this logs even when verbose mode is off. If the prior behavior was verbose-only, please confirm this change is intended.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp-monitoring/files/monitoring.agentopenwisp-monitoring/files/monitoring.init
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: QA-Checks and Tests
- GitHub Check: Build and upload package as artifacts
🔇 Additional comments (9)
openwisp-monitoring/files/monitoring.agent (6)
39-58: Centralizedlog()wrapper reads clean and keeps severity mapping explicit.
60-72:time_to_seconds()parsing looks solid for s/m/h/d suffixes.
97-149: Log wrapper usage in collect/save/signal flows is consistent.
152-234: Send-path log consolidation looks good.
249-270: Registration/bootup log updates are clear.
339-349: Interval validation and clearer error messages look good.Also applies to: 377-378
openwisp-monitoring/files/monitoring.init (3)
12-23:add_optionhelper nicely encapsulates option assembly.
34-62: Splitting send/collect procd instances withadd_optionreads clean.
66-70: Lifecycle log punctuation update is fine.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| respawn_threshold=$(config_get http respawn_threshold) | ||
| respawn_timeout=$(config_get http respawn_timeout) | ||
| respawn_retry=$(config_get http respawn_retry) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "monitoring.init" | head -5Repository: openwisp/openwrt-openwisp-monitoring
Length of output: 123
🏁 Script executed:
# Search for config_get function definition
rg "config_get" -B 3 -A 5 | head -100Repository: openwisp/openwrt-openwisp-monitoring
Length of output: 1345
🏁 Script executed:
cat -n ./openwisp-monitoring/files/monitoring.initRepository: openwisp/openwrt-openwisp-monitoring
Length of output: 2700
🌐 Web query:
OpenWrt config_get function behavior libuci.sh
💡 Result:
Summary — config_get (libuci / functions.sh)
-
What it does: config_get
[default] reads the option value for from the in-memory UCI config loaded by config_load and stores it in the shell variable named by . It does not call the uci binary each time — it reads the data exported to the environment by config_load. [1][2] -
Lists: If the option is a UCI list, config_get returns the list items joined by spaces (e.g. "item1 item2"). This can break if list items contain spaces; use config_list_foreach to iterate list elements safely. [3]
-
Boolean helper: config_get_bool parses common truthy/falsey strings ("on","off","yes","no","true","false","enabled","disabled") and returns 1 or 0. [1]
-
Key difference vs uci_get: config_get operates on the config already loaded by config_load (faster, no disk I/O); uci_get queries the config files each call. Use config_get when you call config_load once and access multiple options. [2]
References:
- OpenWrt scripting docs (config/list behavior). [1]
- Explanation of config_/uci_ differences and implementation details. [2]
- Notes about list handling and config_list_foreach. [3]
If you want, I can show the exact function implementation lines from your OpenWrt version (need target release or a copy of /lib/functions.sh).
config_get in command substitution drops respawn values.
config_get assigns to a variable and does not output; $(config_get …) leaves respawn_threshold, respawn_timeout, and respawn_retry empty. The values are then always replaced by defaults in lines 51 and 63: "${respawn_threshold:-3600}".
🛠️ Proposed fix
-respawn_threshold=$(config_get http respawn_threshold)
-respawn_timeout=$(config_get http respawn_timeout)
-respawn_retry=$(config_get http respawn_retry)
+config_get respawn_threshold http respawn_threshold
+config_get respawn_timeout http respawn_timeout
+config_get respawn_retry http respawn_retry🤖 Prompt for AI Agents
In `@openwisp-monitoring/files/monitoring.init` around lines 30 - 32, The three
variables are being set with command substitution which yields nothing because
config_get assigns variables rather than printing them; replace the
"$(config_get ...)" usages with direct calls to config_get so that config_get
assigns respawn_threshold, respawn_timeout, and respawn_retry (i.e., call
config_get http respawn_threshold, config_get http respawn_timeout, config_get
http respawn_retry) and keep the existing default fallbacks
("${respawn_threshold:-3600}", etc.) intact.
Closes #57
Removed logger calls to simplify the script