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

Monitor checks #143

Merged
merged 57 commits into from
Mar 18, 2019
Merged

Monitor checks #143

merged 57 commits into from
Mar 18, 2019

Conversation

ruelaidler
Copy link
Contributor

This PR requests to add a new feature to the monitor process.

It adds checkmonitor.q script to its own directory, which is called from monitor.q. It works by making asynchronous calls to target processes and runs them against resultchecker functions. These checks can be user-defined.

The monitor process configuration script has been updated to include this new feature. Documentation has been added under Monitor in Processes.md.

Copy link
Contributor

@tsmyth-aquaq tsmyth-aquaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very neat and well laid out 👍

code/monitor/checkmonitor.q Outdated Show resolved Hide resolved
code/monitor/checkmonitor.q Outdated Show resolved Hide resolved
code/monitor/checkmonitor.q Outdated Show resolved Hide resolved
code/monitor/checkmonitor.q Outdated Show resolved Hide resolved
code/monitor/checkmonitor.q Outdated Show resolved Hide resolved
code/monitor/checkmonitor.q Outdated Show resolved Hide resolved
code/processes/monitor.q Outdated Show resolved Hide resolved
@jonathonmcmurray
Copy link
Member

jonathonmcmurray commented Feb 19, 2019

On a more general note, is it not potentially a bit confusing to have both a binary & plaintext version of the config file, where editing the plaintext version will have no effect once the binary version has been generated? Has it been decided that the potential for confusion is worth the flexibility of what can be done with kdb data structures?

Would an alternative plaintext format be more suited to the data structure here (which I think includes a dictionary inside a table) e.g. json, which allows for more nested structure than csv (although possibly a bit more limited in terms of datatypes)?

@ruelaidler
Copy link
Contributor Author

On a more general note, is it not potentially a bit confusing to have both a binary & plaintext version of the config file, where editing the plaintext version will have no effect once the binary version has been generated? Has it been decided that the potential for confusion is worth

This is something Jonny and I came up with...another alternative I guess would be to override the CSV? I do think saving down the flat table does maintain the cool flexibility of kdb+ tables.

@ruelaidler
Copy link
Contributor Author

hi, can this be reviewed again please? think all the comments have now been addressed.

Copy link
Contributor

@tsmyth-aquaq tsmyth-aquaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As always hear from @jonnypress before you merge.

code/monitor/checkmonitor.q Outdated Show resolved Hide resolved
code/monitor/checkmonitor.q Outdated Show resolved Hide resolved
code/monitor/checkmonitor.q Show resolved Hide resolved
code/monitor/checkmonitor.q Show resolved Hide resolved
Copy link
Contributor

@jonnypress jonnypress left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other bits:

  • change times to use .proc.cp[] where appropriate
  • add api config

code/processes/monitor.q Outdated Show resolved Hide resolved
code/processes/monitor.q Outdated Show resolved Hide resolved
config/settings/monitor.q Outdated Show resolved Hide resolved
.monitor.configstored:@[value;`.monitor.configstored;first .proc.getconfigfile["monitorconfig"]]; //name of stored table for save and reload
.monitor.runcheckinterval:@[value;`.monitor.runcheckinterval;0D00:00:05]; //interval to run checks
.monitor.checkinginterval:@[value;`.monitor.checkinginterval;0D00:00:05]; //interval to make sure checks are not lagging
.monitor.cleartrackinterval::@[value;`.monitor.cleartrackinterval;0D00:00:05];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably default to a bit longer - like only really need to do this every hour or so

.monitor.runcheckinterval:@[value;`.monitor.runcheckinterval;0D00:00:05]; //interval to run checks
.monitor.checkinginterval:@[value;`.monitor.checkinginterval;0D00:00:05]; //interval to make sure checks are not lagging
.monitor.cleartrackinterval::@[value;`.monitor.cleartrackinterval;0D00:00:05];
.monitor.agecheck:@[value;`.monitor.agecheck;0D00:01:00];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a bit longer by default, e.g 12 hours or something. It's useful to keep a history.
This and the one below also need a comment to explain what they are

runcheckinterval:0D00:00:05; //interval to run checks
checkinginterval:0D00:00:07; //interval to identify that checks are not lagging
agecheck:0D00:01:00.000000000; //if check over agecheck, delete from tracker
lagtime:0D00:01:00.000000000; //if check has been running over this time, set to neg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update defaults in here, as per comment above

@ruelaidler ruelaidler merged commit eea0346 into master Mar 18, 2019
@ruelaidler ruelaidler deleted the MonitorChecks branch March 18, 2019 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants