[enhancement] Move logger call into subroutine

[feckert]

The logger shell call is used over and over again.
Which makes the code confusing.
I would suggest that you move the call to a subroutine.

https://github.com/openwisp/openwrt-openwisp-monitoring/blob/master/openwrt-openwisp-monitoring/files/monitoring.init
https://github.com/openwisp/openwrt-openwisp-monitoring/blob/master/openwrt-openwisp-monitoring/files/monitoring.agent

[devkapilbansal]

@feckert Here subroutine refers to creating a specific function for logger calls?

[feckert]

@devkapilbansal In the described scripts in my description there is no function for the logger call.
Following the pattern don't repeat your self I would suggest that we add the following subroutine.
log "<level>" "<message>"
This makes the code clearer because it is one liner.

[nemesifier]

@devkapilbansal I have noticed that there are quite a few instances of the calls to logger in which the -t (tag) flag is not used. Is that on purpose?
I think all logger calls should have this.
I also second the proposal of @feckert, I think we can move the -t openwisp_monitoring to our function which we can then reuse. We should also write it so we don't have to pass "daemon.err" but only "err", moreover, we could default the log level to info so that in many cases the call to the logging facility will be very short.

I think this would make the code more readable since we have many calls to the logger.

[devkapilbansal]

Yes, I remove -t flag when logging in verbose mode. In verbose_mode, it is automatically tagging the logged message by the procd service name and if I pass a tag, then the message is logged twice

[nemesifier]

@devkapilbansal one more thing, regarding verbose mode, I think we could handle the verbose output using this function we're imagining here.

We have a lot of if/else in the code for verbose messages which are harming the readability of the agent code, what about solving this with our function?

Eg:

log "<message>" "<level>" "<verbose_message>" "<non_verbose_message_only>"

Practical examples below.

Message shown regardless of mode

log "Something happened." "warn"

Result is shown both in verbose mode and non verbose:

Sep 18 18:18:20 Router daemon.warn openwisp-monitoring: Something happened.

Message + verbose appended

log "Something happened." "warn" "Verbose output containing more detailed info here."

Result seen in non-verbose mode:

Sep 18 18:18:20 Router daemon.warn openwisp-monitoring: Something happened.

Result seen in verbose mode:

Sep 18 18:18:20 Router daemon.warn openwisp-monitoring: Something happened. Verbose output containing more detailed info here.

Verbose message only

log "" "warn" "Message shown only in verbose mode."

Result seen only in verbose mode (in non-verbose mode the log line is not executed at all):

Sep 18 18:18:20 Router daemon.warn openwisp-monitoring: Message shown only in verbose mode.

Non verbose message

log "Something happened." "warn" "More details here..." "Run in verbose mode to find out more."

Non verbose result:

Sep 18 18:18:20 Router daemon.warn openwisp-monitoring: Something happened. Run in verbose mode to find out more.

Verbose result:

Sep 18 18:18:20 Router daemon.warn openwisp-monitoring: Something happened. More details here...

[nemesifier]

Yes, I remove -t flag when logging in verbose mode. In verbose_mode, it is automatically tagging the logged message by the procd service name and if I pass a tag, then the message is logged twice

What do you think about adding a new "debug mode" to turn this on?

openwrt-openwisp-monitoring/openwrt-openwisp-monitoring/files/monitoring.init

Line 89 in bac6d54

[ "$verbose_mode" -eq "1" ] && procd_set_param stdout 1 && procd_set_param stderr 1

In verbose mode we could avoid using it. In debug mode we would know (and document) that any output is sent to the logging facility, causing some output to be repeated, Or alternatively, we could even modify our log function to use echo instead of logger.

[devkapilbansal]

Can we do something like this 👇

log -v -w "message"

-v --> Verbose mode
-w --> Warning
-e --> Error
-i --> Info
-n --> Non Verbose mode

When -n is passed, the message will be shown in both verbose_mode and non-verbose mode.
In the case of -v, the message will be shown in verbose_mode only.

[nemesifier]

Can we do something like this point_down

log -v -w "message"

-v --> Verbose mode
-w --> Warning
-e --> Error
-i --> Info
-n --> Non Verbose mode

When -n is passed, the message will be shown in both verbose_mode and non-verbose mode. In the case of -v, the message will be shown in verbose_mode only.

Sounds good, but how can all the cases mentioned in #57 (comment) be covered by this?

Can you please provide some examples of how it's going to look like?
The cases to cover are quite a bit and that draft idea doesn't seem to me it can cover them all.

[devkapilbansal]

Here are some examples on how it will work:

Message shown regardless of mode

log -n -w "This is a warning"

The result will be shown in both verbose and non-verbose mode

Oct 14 03:46:15 Router daemon.warn openwisp-monitoring: This is a warning

Verbose mode only

log -v -e "This is an error"

Result:

Oct 14 03:46:15 Router daemon.err openwisp-monitoring: This is an error

I don't take the other two cases into consideration as we are handling them in if-else to stop redundant messages in non-verbose mode as found here.

openwrt-openwisp-monitoring/openwrt-openwisp-monitoring/files/monitoring.agent

Lines 146 to 157 in 5460f01

	if [ "$VERBOSE_MODE" -eq "1" ]; then
	logger -s "Data not sent successfully. Response code is \"$response_code\"." \
	"Error message is $error_message" \
	-p daemon.err
	elif [ "$FAILING" -eq "0" ]; then
	FAILING=1
	logger -s "Data not sent successfully. Response code is \"$response_code\"." \
	"Error message is $error_message" \
	"Run with verbose mode to find more." \
	-t openwisp-monitoring \
	-p daemon.err
	fi

I added a commit to show how I am approaching this: d965c42

[nemesifier]

Ok but what about all the other cases?
There are cases in which in verbose mode we display additional info.

Here are some examples on how it will work:

Message shown regardless of mode

log -n -w "This is a warning"

I think -n should precede the message, eg: -n "This is a warning"

Verbose mode only

log -v -e "This is an error"

Same here, I think -v should precede the message, eg: -v "This is a warning".

So if we need to display a message that in verbose mode should contain more info we may be able to do:

log -e -n "Error detected" -v "More information here"

These combinations should cover all the most critical cases, if there are other cases to handle we could just simplify. What do you think?

[nemesifier]

@jedboywonder these messages are out of topic here, I am going to delete them.

Django can be configure to send logs to a log collector. Use the general chat to ask questions.