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

Issue #92: Update monitoring with notification throttling #93

Merged
merged 10 commits into from
Dec 11, 2018
Merged

Conversation

jordanpadams
Copy link
Contributor

Resolves #92

Eventually this should probably be a configurable parameter

@jordanpadams jordanpadams requested review from a team as code owners November 2, 2018 22:41
@jordanpadams jordanpadams changed the title Issue #92: Update monitoring with notitication throttling Issue #92: Update monitoring with notification throttling Nov 2, 2018
@aywaldron
Copy link
Contributor

aywaldron commented Nov 21, 2018

The default behavior is now to send one notification upon the first limit trip and no further notifications for any consecutive limit trips. The number of consecutive limit-tripping packets required to trigger a notification can be changed from the default of 1 in the notifications.options.threshold config. A notification reminder frequency can be set in the notifications.options.frequency config if desired, so that a notification will be sent every frequency consecutive limit-tripping packets after the threshold is met.

@aywaldron aywaldron requested a review from MJJoyce November 28, 2018 23:13
@aywaldron aywaldron force-pushed the issue-92 branch 2 times, most recently from 59bfb11 to 2f0fbe0 Compare November 30, 2018 19:13
@aywaldron
Copy link
Contributor

Added a script that tests that the new notifications options work as expected.

@@ -389,8 +389,15 @@ def telem_handler(session):
for k, v in tlm.getDefaultDict().iteritems():
packet_dict[v.uid] = v

notif_thrshld = ait.config.get('notifications.options.threshold')
Copy link
Member

Choose a reason for hiding this comment

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

Let's go ahead and set sane defaults here. Otherwise the behavior could end up being a bit off. For instance, as is the notif_thrshld variable ends up set as None if my config doesn't have a value for notifications.options.threshold and I enable monitoring.

You can do this with a one line for both of these values

notif_thrshld = ait.config.get('notifications.options.threshold', 1)
notif_freq = ait.config.get('notifications.options.frequency', float('inf'))

@MJJoyce
Copy link
Member

MJJoyce commented Dec 2, 2018

I'm +1 besides the above comment. Please make sure to open a ticket so that the relevant config options get added to the core config.yaml file as examples. Also, please open a ticket in AIT GUI so this gets added to the documentation.

@aywaldron
Copy link
Contributor

Pull request for example values added to config.yaml: NASA-AMMOS/AIT-Core#133

jordanpadams and others added 8 commits December 4, 2018 15:21
…otifications

Implements new limit trip notification 'threshold' config.
Notification only sent once per any number of consecutive
limit trips, and only after limit trip 'threshold' (required
number of consecutive tripping packets) has been met.
…otifications

Implements new limit trip notification 'frequency' config
by triggering notifications every 'frequency' consecutive
limit-tripping packet, not taking 'threshold' into account.
@aywaldron aywaldron merged commit 407b231 into master Dec 11, 2018
@aywaldron aywaldron deleted the issue-92 branch December 11, 2018 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants