Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[enhancement] Move logger calls in subroutine #57 #76

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

devkapilbansal
Copy link
Member

Closes #57

Removed logger calls to simplify the script

exit 1
fi
[ "$interval" -lt 1 ] && log -e "Interval is invalid. Use time value(eg: '10', '2m', '3h', '1d')"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it here and instead send it to stderr because this will come when invalid value is provided in interval

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may do similar kind of refactoring here to as done in openwisp-config in future

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see @feckert's proposals: openwisp/openwisp-config#109

@devkapilbansal devkapilbansal force-pushed the issues/57-move-logger-call-in-subroutine branch 2 times, most recently from be929b2 to 46dad97 Compare October 15, 2021 21:11
@devkapilbansal
Copy link
Member Author

This PR is tested and it works as expected. I will rebase and mark is as Ready for Review once #72 is merged

@devkapilbansal devkapilbansal force-pushed the issues/57-move-logger-call-in-subroutine branch from 46dad97 to 395d66e Compare October 19, 2021 17:38
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just merged #72 🚀

@devkapilbansal devkapilbansal force-pushed the issues/57-move-logger-call-in-subroutine branch from 395d66e to 2f48c70 Compare October 20, 2021 06:26
@devkapilbansal devkapilbansal changed the base branch from issues/46-delete-old-data to master October 20, 2021 06:26
@devkapilbansal devkapilbansal marked this pull request as ready for review October 20, 2021 21:58
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one comment below.

@feckert would be good if you could double check this as well.

@@ -247,8 +252,7 @@ main() {
shift
;;
-*)
echo "Invalid option: $1"
exit 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this exit 1 removed? Does echoerr returns non zero exit code or what?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

echoerr is defined above that echo the message to standard error and returns exit 1 too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exit 1
fi
[ "$interval" -lt 1 ] && log -e "Interval is invalid. Use time value(eg: '10', '2m', '3h', '1d')"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see @feckert's proposals: openwisp/openwisp-config#109

type="$2"
shift 2

if [ "$type" = "-v" ] && [ "$VERBOSE_MODE" -ne "1" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the stupid question. What is the use case of the option -v?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check if we are in verbose mode or not. By using -v option, message will be logged in verbose mode only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feckert BTW in recent OpenWrt versions filtering out log levels doesn't work because the logd implementation doesn't support it.

@devkapilbansal devkapilbansal marked this pull request as draft November 17, 2021 22:07
@devkapilbansal devkapilbansal force-pushed the issues/57-move-logger-call-in-subroutine branch from 9eb15c2 to d54c2a4 Compare November 18, 2021 15:17
@devkapilbansal devkapilbansal marked this pull request as ready for review November 18, 2021 15:19
@devkapilbansal devkapilbansal force-pushed the issues/57-move-logger-call-in-subroutine branch from d54c2a4 to 9e5f570 Compare November 18, 2021 15:20
@github-actions
Copy link

sh-checker report

To get the full details, please check in the job output.

shellcheck errors

'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In monitoring_init line 15:
		local cfg="$1"
                ^-------^ SC3043: In POSIX sh, 'local' is undefined.


In monitoring_init line 16:
		local flag="$2"
                ^--------^ SC3043: In POSIX sh, 'local' is undefined.


In monitoring_init line 17:
		local option="$3"
                ^----------^ SC3043: In POSIX sh, 'local' is undefined.


In monitoring_init line 18:
		local default="$4"
                ^-----------^ SC3043: In POSIX sh, 'local' is undefined.


In monitoring_init line 19:
		local value
                ^---------^ SC3043: In POSIX sh, 'local' is undefined.

For more information:
  https://www.shellcheck.net/wiki/SC3043 -- In POSIX sh, 'local' is undefined.
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.



shfmt errors
'shfmt ' found no issues.

@devkapilbansal devkapilbansal force-pushed the issues/57-move-logger-call-in-subroutine branch from 9e5f570 to 7fec7f7 Compare November 18, 2021 15:42
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @devkapilbansal! I think we should revamp this!

@devkapilbansal devkapilbansal force-pushed the issues/57-move-logger-call-in-subroutine branch from 7fec7f7 to b47e2a5 Compare January 13, 2023 18:44
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

openwisp-monitoring/files/monitoring.agent Outdated Show resolved Hide resolved
openwisp-monitoring/files/monitoring.agent Outdated Show resolved Hide resolved
-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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

FAILING=0
rm -f "$RESPONSE_FILE"
[ "$VERBOSE_MODE" -ne "1" ] && log -i -n "Data sent successfully."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here: Data sent successfully. is repeated twice and there's another unnecessary if here.

Copy link
Member Author

@devkapilbansal devkapilbansal Jan 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for review/testing
Development

Successfully merging this pull request may close these issues.

[enhancement] Move logger call into subroutine
3 participants