Re: Re: Working Version of Net_Monitor - Please Review :)

Hi Bertrand, bertrand Gugger wrote:

Hi Robert Peake who wrote:

Thanks for the constructive feedback so far. I am reluctant to move this to "proposal" yet as my understanding is that I can not edit the main document after it reaches this stage.

You're wrong, in "proposal" state you can still edit it, even change the package's name if you want. Hmmm, in the mean time you are proposing...

Yes -- I realized this. :) Bertrand M. suggested moving to proposal so that developers can post comments on the proposal page.

So now, the smart message is in. But so long you store it in some internal opaque format, then should the API furnish finer handling of it.

No, I don't think so. I thought of using the Config package, but since the state file is completely internal to the program and should never be modified by an end user, a simple tab-delimited file should suffice.

The resetState() method is samewhat too violent, as it will provoke the resending of all alerts.

resetState() will reset the previous state information. The purpose of this feature is to allow end users to reset the previous state, add new services and alerts, and run a completely different monitoring session if and when the previous monitoring session's state information is no longer relevant. Once the state has been cleared, alerts will only be sent if there is a problem. On a side note, an end user can run multiple monitoring sessions and keep track of multiple states by using Net_Monitor::setOptions(array('state_file' => 'SomeNewFileName')) for each new session. Clearing state altogether is just an optional feature that might be useful in some circumstances.

It should be possible to reset only one service for example.

This is something I can look into. I suppose you might want to forget that a single host/service combination in a monitoring session has returned a non-OK result. However, this seems more rare than clearing the session state altogether to me.

BTW, it's to be hoping, nobody puts a "\t" in message,

AFAIK, messages returned from Net_* packges don't have tabs in them. I will look at possibly quoting the return message just in case.

I think this save format is quite dangerous.

What format do you think would be more safe?

It's not directly concerning your package, but to get it work some user interface would be usefull to handle the services and recipients lists.

See the 'examples' directory for an example of using an INI file to set up a monitoring session. Using this approach INI files could be passed as options on the command line, allowing for easy scheduling of monitoring sessions from inside a scheduler like crontab.

so even have the user (un)subscribe from itself.

This would be a simple matter of a Net_Monitor->setAlerts(). I leave this up to the end user who integrates this into his/her application, since the purpose of Net_Monitor is monitor network services, not provide mailing-list functionality.

One technical remark: why do you need to make copy of $this->stuff in $stuff everywhere ? Most often you can directly work on $this->stuff. The same for subarrays, why make a copy ? you may use so many indexes you like as: $secondary[$j]['service']

Mostly for readability. I do use multidimensional array references where it is obvious what I am doing. Sometimes copying $this->variable is just convenient and sometimes it is necessary because I am changing the copied variable but do not want to change the class variable.

The stateDiff() method could follow both arrays in one loop not need to double loop and reset !

Actually, it is necessary to act destructively on both arrays in the first set of nested loops. This is because the behavior of the alert call is to only return data when there is a problem. So, leftover problem states from the previous set of tests have to be identified to be reported as having changed to OK. This means removing all intersecting states, reindexing the previous state array, and then traversing the array setting the message and code to "OK." If the system were reporting everything, including successes, a recursive version of array_diff_assoc would work here. However, it is less efficient and really less desirable to report all the OK states, since OK states are only reported when they are a *change* from a non-OK state.

Please, understand it's no offense here. I'm certainly not coding better ! Anyway I go on in my review. Bye

Thanks for your interest and enthusiasm. :) Best, Robert