[fix] Fix errors in package and improve debugging #42

devkapilbansal · 2021-07-13T21:00:20Z

Fixes #41

Fix error related to monitoring interfaces when * is passed
Add verbose mode for easy debugging

devkapilbansal · 2021-07-13T21:01:49Z

openwrt-openwisp-monitoring/files/monitoring.init

    procd_set_param respawn "${respawn_threshold:-3600}" "${respawn_timeout:-5}" "${respawn_retry:-5}"
+    procd_set_param stderr 1


Adding this to log all errors in the instance so that it can be accessed using logread

devkapilbansal · 2021-07-13T21:02:47Z

openwrt-openwisp-monitoring/files/monitoring.init

@@ -52,8 +52,9 @@ start_service() {
    monitored_interfaces="--monitored_interfaces $monitored_interfaces"

    procd_open_instance "openwisp_monitoring_monitoring"
-    procd_set_param command $PROG "$base_url" "$uuid" "$key" "$verify_ssl" "$interval" "$monitored_interfaces"
+    procd_set_param command $PROG $base_url $uuid $key $verify_ssl $interval $monitored_interfaces


Reverting quotes as this was causing monitoring.agent to read --url http://www..... as one word

nemesifier

With this patch, the daemon sends data to OpenWISP Monitoring 👍 .

Here's the issues I found during testing.

Non verbose mode

I see these two lines in non-verbose mode:

Fri Jul 16 10:32:36 2021 daemon.info monitoring: Collecting NetJSON Monitoring data
Fri Jul 16 10:32:38 2021 daemon.info monitoring: Data sent successfully.

But this output should not be shown in non-verbose mode, otherwise logs will be filled with irrelevant lines.

Traffic stats seem missing

I don't see traffic stats being collected on the device I am using for testing, once I installed this branch and remove the old monitoring scripts, the traffic charts are flat zero.

Did you test this?

Verbose mode testing

Here's a few problems I see with verbose mode.

Identifeir different

The logread output uses different identifiers: monitoring, openwisp_monitoring, openwisp_monitoring[]. This is bad because log collectors will interpret these as different programs, can you make sure this is always the same? monitoring seems to generic, I think it should be openwisp_monitoring.

Logged response can be improved

Here's an example output: Data not sent successfully. Response code is {"detail":"Authentication credentials were not provided."}403.. The message is readable but it's weird. The response body is {"detail":"Authentication credentials were not provided."} while the response status code is 403. Can you turn this into something like:
The response received was: 403 {"detail":"Authentication credentials were not provided."}?

Run with verbose mode redundant

This is showing up with verbose mode:

Fri Jul 16 10:53:31 2021 daemon.err monitoring: Run with verbose mode to find more.
Fri Jul 16 10:53:31 2021 daemon.err openwisp_monitoring[4023]: monitoring: Run with verbose mode to find more.

we're already in verbose mode so we don't need to send this output.
if we're not in verbose mode, ensure the output is not repeated twice
do you see how one log line is identified by daemon.err monitoring and the otehr one by daemon.err openwisp_monitoring[4023]? This was mentioned in one of the previous review sections but please make sure to avoid it.

nemesifier · 2021-07-17T18:15:31Z

openwrt-openwisp-monitoring/files/monitoring.config

@@ -1,3 +1,4 @@
 config monitoring 'monitoring'
-	option monitored_interfaces '*'
+	option monitored_interfaces '"*"'


requiring the user to remember to wrap the value around double quotes is really bad and weird. I think we can do better than this.

nemesifier · 2021-07-17T18:34:00Z

openwrt-openwisp-monitoring/files/monitoring.init

@@ -50,10 +51,12 @@ start_service() {
    fi
    interval="--interval $interval"
    monitored_interfaces="--monitored_interfaces $monitored_interfaces"


does

monitored_interfaces="--monitored_interfaces \"$monitored_interfaces\""

allow us to not having to wrap the value in quotes in the UCI config file?

Fixes #41

Added default value of version to silent sh: out of range error and replaced == with 0 to follow POSIX notations as it can break on some systems.

devkapilbansal · 2021-07-22T10:11:59Z

Thanks for testing this. I made some changes as follows:

Non Verbose Mode
Unnecessary lines have been removed. Those two lines will be logged in verbose mode only.

Traffic stats
I just ensured that traffic stats are sent in netjson response from the scripts. Will test it to be more sure.

Identifier
All messages tagged with monitoring are changed to openwisp_monitoring. But in verbose_mode, messages are coming as openwisp_monitoring[number] and I am unsure of that number. I think that this is the id of procd instance. 🤔

Redundant Log Messages
Messages were logged twice in verbose mode. This was because messages were logged once by the logger tagged with the identifier we were passing.These messages were catched by the procd instance and logged again with procd instance name as the identifier.
For this, I removed the identifier from the messages I was logging in verbose mode so that we get relevant lines only when grepping log messages.
If we use logread then the messages are redundant in verbose_mode but passing logread | grep openwisp_monitoring will show the message once.

[skip ci]

nemesifier

I did a couple of minor improvements 021307c 0083e00.

Tested and it's working better, I think there's room for improvements but let's merge and I will open new issues for improvements.

devkapilbansal changed the base branch from master to gsoc21 July 13, 2021 21:00

devkapilbansal commented Jul 13, 2021

View reviewed changes

devkapilbansal added this to In progress in [GSoC 2021] OpenWRT OpenWISP Monitoring Package via automation Jul 13, 2021

devkapilbansal force-pushed the issues/41-improve-debugging branch from b294b0b to d860640 Compare July 15, 2021 18:49

devkapilbansal marked this pull request as ready for review July 15, 2021 18:49

devkapilbansal moved this from In progress to Needs Review in [GSoC 2021] OpenWRT OpenWISP Monitoring Package Jul 15, 2021

devkapilbansal force-pushed the issues/41-improve-debugging branch from d860640 to 5cde9f2 Compare July 16, 2021 19:48

devkapilbansal linked an issue Jul 16, 2021 that may be closed by this pull request

[bug] Data not sent successfully. Exit code is 127 #41

Closed

devkapilbansal mentioned this pull request Jul 17, 2021

[test] Lua tests for netjson monitoring #26

Merged

nemesifier requested changes Jul 17, 2021

View reviewed changes

devkapilbansal added 3 commits July 21, 2021 00:50

[fix] Fix package not working minor improvements #41

b2e640d

Fixes #41

[refactor] Use response code instead of status code

bd63705

[refactor] Minor improvements

cdc8584

Added default value of version to silent sh: out of range error and replaced == with 0 to follow POSIX notations as it can break on some systems.

devkapilbansal force-pushed the issues/41-improve-debugging branch from 5cde9f2 to de8ee00 Compare July 20, 2021 19:37

[enhancement] Add verbose_mode for easy debugging

2dd0aff

devkapilbansal force-pushed the issues/41-improve-debugging branch from de8ee00 to 535df1f Compare July 21, 2021 16:54

devkapilbansal added 2 commits July 22, 2021 12:38

[refactor] Minor modifications

4343464

[fix] Fix traffic monitored not included and improved debugging

617b25a

devkapilbansal force-pushed the issues/41-improve-debugging branch from 535df1f to 617b25a Compare July 22, 2021 09:31

[refactor] Minor improvements

6ffcaec

[enhancement] Show error if parameters are missing in agent

ff2d98b

devkapilbansal force-pushed the issues/41-improve-debugging branch from d6ff7a2 to ff2d98b Compare July 23, 2021 11:47

nemesifier added 2 commits July 24, 2021 18:16

[chores] Improved error messages

021307c

[docs] Improved description of verbose_mode

0083e00

[skip ci]

[GSoC 2021] OpenWRT OpenWISP Monitoring Package automation moved this from Needs Review to Reviewer approved Jul 24, 2021

nemesifier approved these changes Jul 24, 2021

View reviewed changes

nemesifier merged commit cded593 into gsoc21 Jul 24, 2021

[GSoC 2021] OpenWRT OpenWISP Monitoring Package automation moved this from Reviewer approved to Done Jul 24, 2021

devkapilbansal deleted the issues/41-improve-debugging branch July 25, 2021 08:17

devkapilbansal mentioned this pull request Aug 10, 2021

[refactor] Improve curl response mesage #50

Closed

devkapilbansal mentioned this pull request Jan 29, 2023

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Fix errors in package and improve debugging #42

[fix] Fix errors in package and improve debugging #42

devkapilbansal commented Jul 13, 2021 •

edited

Loading

devkapilbansal Jul 13, 2021

devkapilbansal Jul 13, 2021

nemesifier left a comment

nemesifier Jul 17, 2021

nemesifier Jul 17, 2021

devkapilbansal commented Jul 22, 2021

nemesifier left a comment

		procd_set_param respawn "${respawn_threshold:-3600}" "${respawn_timeout:-5}" "${respawn_retry:-5}"
		procd_set_param stderr 1

[fix] Fix errors in package and improve debugging #42

[fix] Fix errors in package and improve debugging #42

Conversation

devkapilbansal commented Jul 13, 2021 • edited Loading

devkapilbansal Jul 13, 2021

Choose a reason for hiding this comment

devkapilbansal Jul 13, 2021

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Non verbose mode

Traffic stats seem missing

Verbose mode testing

Identifeir different

Logged response can be improved

Run with verbose mode redundant

nemesifier Jul 17, 2021

Choose a reason for hiding this comment

nemesifier Jul 17, 2021

Choose a reason for hiding this comment

devkapilbansal commented Jul 22, 2021

nemesifier left a comment

Choose a reason for hiding this comment

devkapilbansal commented Jul 13, 2021 •

edited

Loading