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 call into subroutine #57

Open
feckert opened this issue Aug 12, 2021 · 11 comments · May be fixed by #76
Open

[enhancement] Move logger call into subroutine #57

feckert opened this issue Aug 12, 2021 · 11 comments · May be fixed by #76
Labels
enhancement New feature or request hacktoberfest Easy issues for attracting Hacktoberfest participants.

Comments

@feckert
Copy link
Contributor

feckert commented Aug 12, 2021

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

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

@feckert
Copy link
Contributor Author

feckert commented Aug 17, 2021

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

@devkapilbansal devkapilbansal added the enhancement New feature or request label Sep 14, 2021
@nemesifier
Copy link
Member

@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
Copy link
Member

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

nemesifier commented Sep 19, 2021

@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
Copy link
Member

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?

[ "$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.

@nemesifier nemesifier added the hacktoberfest Easy issues for attracting Hacktoberfest participants. label Oct 12, 2021
@devkapilbansal
Copy link
Member

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

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

devkapilbansal commented Oct 13, 2021

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.

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

nemesifier commented Oct 14, 2021

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest Easy issues for attracting Hacktoberfest participants.
Projects
Status: To do (OpenWRT)
Development

Successfully merging a pull request may close this issue.

3 participants