[enhancement] Move logger calls in subroutine #57 #76
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,41 @@ show_version() { | |
|
|
||
| echoerr() { echo "$@" 1>&2 && exit 1; } | ||
|
|
||
| log() { | ||
| level="$1" | ||
| type="$2" | ||
| shift 2 | ||
|
|
||
| if [ "$type" = "-v" ] && [ "$VERBOSE_MODE" -ne "1" ]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| case "$level" in | ||
| -i) level=daemon.info ;; | ||
| -w) level=daemon.warn ;; | ||
| -e) level=daemon.err ;; | ||
| -*) | ||
| echoerr "Invalid message level : $level" | ||
| ;; | ||
| esac | ||
|
|
||
| logger -s "$@" -p "$level" -t openwisp-monitoring | ||
| } | ||
|
|
||
| time_to_seconds() { | ||
| time=$1 | ||
|
|
||
| { [ "$time" -ge 1 ] 2>/dev/null && seconds="$time"; } \ | ||
| || { [ "${time%s}" -ge 1 ] 2>/dev/null && seconds="${time%s}"; } \ | ||
| || { [ "${time%m}" -ge 1 ] 2>/dev/null && seconds=$((${time%m} * 60)); } \ | ||
| || { [ "${time%h}" -ge 1 ] 2>/dev/null && seconds=$((${time%h} * 3600)); } \ | ||
| || { [ "${time%d}" -ge 1 ] 2>/dev/null && seconds=$((${time%d} * 86400)); } | ||
|
|
||
| echo $seconds | ||
| unset seconds | ||
| unset time | ||
| } | ||
|
|
||
| check_available_memory() { | ||
| while true; do | ||
| total=$(ubus call system info | jsonfilter -e '@.memory.total') | ||
|
|
@@ -50,9 +85,7 @@ check_available_memory() { | |
| if [ -f "$file" ]; then | ||
| rm "$file" | ||
| else | ||
| log -w -v "Not enough memory available, skipping collect data." | ||
| return 1 | ||
| fi | ||
| fi | ||
|
|
@@ -61,14 +94,11 @@ check_available_memory() { | |
|
|
||
| collect_data() { | ||
| n=0 | ||
| log -i -v "Collecting NetJSON Monitoring data." | ||
| until [ "$n" -ge 5 ]; do | ||
| /usr/sbin/netjson-monitoring --dump "$MONITORED_INTERFACES" && break | ||
|
|
||
| [ "$n" -eq 5 ] && log -e -v "Collecting data failed!." | ||
devkapilbansal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| n=$((n + 1)) | ||
| sleep 5 | ||
| done | ||
|
|
@@ -101,8 +131,7 @@ save_data() { | |
| echo "$data" >"$TMP_DIR/$filename" | ||
| # compress data | ||
| gzip "$TMP_DIR/$filename" | ||
| log -i -v "Data saved temporarily." | ||
| fi | ||
| # get process id of the process sending data | ||
| pid=$(pgrep -f "openwisp-monitoring.*--mode send") | ||
|
|
@@ -112,17 +141,15 @@ save_data() { | |
| } | ||
|
|
||
| handle_sigusr1() { | ||
| log -i -v "SIGUSR1 received! Sending data." | ||
| return 0 | ||
| } | ||
|
|
||
| send_data() { | ||
| while true; do | ||
| for file in "$TMP_DIR"/*; do | ||
| if [ ! -f "$file" ]; then | ||
| log -i -v "No data file found to send. Checking after $INTERVAL seconds." | ||
devkapilbansal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| trap handle_sigusr1 USR1 | ||
| # SIGUSR1 signal received, interrupt sleep and continue sending data | ||
| sleep "$INTERVAL" & | ||
|
|
@@ -149,46 +176,36 @@ send_data() { | |
| while true; do | ||
| if [ "$failures" -eq "$MAX_RETRIES" ]; then | ||
| [ -f "$RESPONSE_FILE" ] && error_message="\"$(cat "$RESPONSE_FILE")\"" || error_message='"".' | ||
| log -e -v "Data not sent successfully. Response code is \"$response_code\"." \ | ||
| "Error message is $error_message" | ||
| # check if agent was already passing or not to avoid repeating log messages | ||
| if [ "$FAILING" -eq "0" ]; then | ||
| FAILING=1 | ||
| [ "$VERBOSE_MODE" -ne "1" ] && log -e -n "Data not sent successfully. Response code is \"$response_code\"." \ | ||
| "Run with verbose mode to find more." | ||
|
Member
There was a problem hiding this comment. There's still repetition here which makes the code hard to read. 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: While in verbose mode we would have:
Member
Author
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) |
||
| fi | ||
| break | ||
| fi | ||
| # send data | ||
| response_code=$($CURL_COMMAND -H "Content-Type: application/json" -d "$data" "$url") | ||
| if [ "$response_code" = "200" ]; then | ||
| log -i -v "Data sent successfully." | ||
| # check if agent was already failing or not to avoid repeating log messages | ||
| if [ "$FAILING" -eq "1" ]; then | ||
| FAILING=0 | ||
| rm -f "$RESPONSE_FILE" | ||
| [ "$VERBOSE_MODE" -ne "1" ] && log -i -n "Data sent successfully." | ||
|
Member
There was a problem hiding this comment. same here:
Member
Author
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. |
||
| fi | ||
| # remove saved data | ||
| rm -f "$filename" | ||
| break | ||
| elif [ "$response_code" = "400" ]; then | ||
| log -i -n "Data not sent successfully (HTTP status $response_code), discarding data." | ||
| rm -f "$filename" | ||
| break | ||
| else | ||
| timeout=$(/usr/sbin/openwisp-get-random-number 2 15) | ||
| log -w -v "Data not sent successfully. Retrying in $timeout seconds." | ||
| failures=$((failures + 1)) | ||
| sleep "$timeout" | ||
| fi | ||
|
|
@@ -203,17 +220,13 @@ wait_until_registered() { | |
| if [ -n "$UUID" ] && [ -n "$KEY" ]; then | ||
| return 0 | ||
| fi | ||
| log -i -n "Waiting for device to register." | ||
| UUID=$(uci get openwisp.http.uuid 2>/dev/null) | ||
| KEY=$(uci get openwisp.http.key 2>/dev/null) | ||
| if [ -z "$UUID" ] || [ -z "$KEY" ]; then | ||
| return 1 | ||
| fi | ||
| log -i -n "Setting uuid and key." | ||
| export UUID KEY | ||
| } | ||
|
|
||
|
|
@@ -223,9 +236,7 @@ bootup_delay() { | |
| if [ "$BOOTUP_DELAY" -ne "0" ]; then | ||
| # get a random number between zero and $BOOTUP_DELAY | ||
| DELAY=$(/usr/sbin/openwisp-get-random-number 0 "$BOOTUP_DELAY") | ||
| log -i -n "Delaying initialization of the monitoring agent for $DELAY seconds." | ||
| sleep "$DELAY" | ||
| fi | ||
| # send bootup hotplug event | ||
|
|
@@ -288,8 +299,7 @@ main() { | |
| shift | ||
| ;; | ||
| -*) | ||
| echoerr "Invalid option: $1." | ||
| ;; | ||
| *) break ;; | ||
| esac | ||
|
|
@@ -298,6 +308,8 @@ main() { | |
|
|
||
| INTERVAL=${INTERVAL:-300} | ||
| REGISTRATION_INTERVAL=$((INTERVAL / 10)) | ||
| INTERVAL="$(time_to_seconds "$INTERVAL")" | ||
| [ -z "$INTERVAL" ] && echoerr "Interval is invalid. Use time value(eg: '10', '2m', '3h', '1d')" | ||
| VERBOSE_MODE=${VERBOSE_MODE:-0} | ||
| BOOTUP_DELAY=${BOOTUP_DELAY:-10} | ||
| TMP_DIR="/tmp/openwisp/monitoring" | ||
|
|
@@ -325,7 +337,7 @@ main() { | |
| RESPONSE_FILE="$TMP_DIR"/response.txt | ||
| set_url_and_curl && send_data | ||
| else | ||
| echoerr "The supplied mode is invalid. Only send and collect are allowed." | ||
| fi | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.