search

openwisp / openwrt-openwisp-monitoring

OpenWRT monitoring agent for openwisp-monitoring
https://openwisp.io/docs/dev/openwrt-monitoring-agent/
GNU General Public License v3.0
22 stars 20 forks source link

[enhancement] Move logger call into subroutine #57

Open feckert opened 3 years ago

feckert commented 3 years ago

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 commented 3 years ago

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

feckert commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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? https://github.com/openwisp/openwrt-openwisp-monitoring/blob/bac6d545b0e3f21f2ca6d7e409220024963f111a/openwrt-openwisp-monitoring/files/monitoring.init#L89

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 commented 2 years ago

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.

nemesifier commented 2 years ago

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 https://github.com/openwisp/openwrt-openwisp-monitoring/issues/57#issuecomment-922496499 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 commented 2 years ago

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. https://github.com/openwisp/openwrt-openwisp-monitoring/blob/5460f01df7053b8196edd1b3c092bb11ad557677/openwrt-openwisp-monitoring/files/monitoring.agent#L146-L157

I added a commit to show how I am approaching this: https://github.com/openwisp/openwrt-openwisp-monitoring/commit/d965c42444392dedbd14c7551dde87e52ad7dcf4

nemesifier commented 2 years ago

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 commented 2 years ago

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