NoOpTrigger #420
NoOpTrigger #420
Conversation
petardz
force-pushed
the
noop-trigger
branch
2 times, most recently
from
b44b696 to
1c0718a
Compare
|
plz resolve conflicts |
Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #420 +/- ##
============================================
+ Coverage 72.82% 72.87% +0.04%
- Complexity 701 712 +11
============================================
Files 112 113 +1
Lines 4670 4711 +41
Branches 602 606 +4
============================================
+ Hits 3401 3433 +32
- Misses 1019 1024 +5
- Partials 250 254 +4
|
|
Is the idea to use the NoOpTriggers to create error alerts for all monitor types when the monitor fails? |
|
yes |
| import org.opensearch.core.xcontent.XContentParser | ||
| import java.io.IOException | ||
|
|
||
| data class NoOpTrigger( |
There was a problem hiding this comment.
Is the only difference from a normal trigger is that it has no condition, so its technically always true?
There was a problem hiding this comment.
Yea it has id/name so that Alert's fields can be populated. Also, creating monitor with this trigger is not permitted.(require statement in init of monitor model)
How exactly will the NoOptrigger be used in the monitor executions? Also why is this needed for the error alerts? Can't we have a function in Alerting to generate error alerts from monitor failures and have that shared amongst all the monitor runners? Something similar to that is already in place. Also would the idea be for users to make monitors with these noop triggers? |
After discussion with @getsaurabh02 conclusion was that it is safest to keep constraint of mandatory trigger parameter in Alert constructors, hence the NoOpTrigger implementation. You can't make monitors with this type of trigger. I added this constraint inside init block of Monitor class. |
| @@ -357,6 +358,16 @@ class XContentTests { | |||
| assertEquals("Round tripping alert doesn't work", alert, parsedAlert) | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Lets add xcontent tests for NoOp triggers.
Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com>
src/main/kotlin/org/opensearch/commons/alerting/model/NoOpTrigger.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/org/opensearch/commons/alerting/model/XContentTests.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com>
Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com>
* added noop trigger Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> (cherry picked from commit c507ac9)
* added noop trigger Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> (cherry picked from commit c507ac9)
* added noop trigger Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> (cherry picked from commit c507ac9) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com> Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Description
Added NoOpTrigger to be used when constructing monitor error alerts or alerts which are not generated as a result of ran triggers
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.