Populate results and trigggeredDocs field in document level trigger execution context #496
Conversation
eirsep
force-pushed
the
fix-doc-ctx
branch
3 times, most recently
from
17f1695 to
490ec84
Compare
Codecov Report
@@ Coverage Diff @@
## main #496 +/- ##
============================================
- Coverage 76.75% 76.71% -0.04%
Complexity 176 176
============================================
Files 166 166
Lines 8370 8370
Branches 1232 1232
============================================
- Hits 6424 6421 -3
- Misses 1342 1343 +1
- Partials 604 606 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. |
|
Please create an IT test that checks the action output of the message to ensure we get data from |
Added a test to verify that we are seeing results in the alert action output. |
| @Suppress("UNCHECKED_CAST") val actionOutput = (actionResult as Map<String, Map<String, String>>)["output"] | ||
| as Map<String, String> | ||
| assertTrue(actionOutput["subject"].toString().contains(docQuery.id)) | ||
| assertTrue(actionOutput["subject"].toString().contains(docQuery.id)) |
There was a problem hiding this comment.
Check the message as well since subject is being checked twice.
| for (triggerResult in output.objectMap("trigger_results").values) { | ||
| assertEquals(1, triggerResult.objectMap("action_results").values.size) | ||
| for (alertActionResult in triggerResult.objectMap("action_results").values) { | ||
| for (actionResult in alertActionResult.values) { |
There was a problem hiding this comment.
What if alertActionResult.values is 0 or empty? We will never reach the below assertion?
| monitor, trigger, emptyList(), Instant.now(), Instant.now(), | ||
| alerts, emptyList(), emptyList(), null | ||
| monitor, trigger, monitorRunResult.inputResults.results, monitorRunResult.periodStart, monitorRunResult.periodEnd, | ||
| alerts, emptyList(), emptyList(), monitorRunResult.scriptContextError(trigger) |
There was a problem hiding this comment.
Do we need to have a test to surface out the inputResults.error?
There was a problem hiding this comment.
That is a good point, we should check if we get the expected data from there as well.
There was a problem hiding this comment.
I believe we should have a follow up PR or in this PR to have tests to verify every part of the `ctx`` values.
There was a problem hiding this comment.
Lets create an issue and tag it here
| @@ -222,7 +222,7 @@ object DocumentLevelMonitorRunner : MonitorRunner() { | |||
| queryToDocIds: Map<DocLevelQuery, Set<String>>, | |||
| dryrun: Boolean | |||
| ): DocumentLevelTriggerRunResult { | |||
| val triggerCtx = DocumentLevelTriggerExecutionContext(monitor, trigger) | |||
| val triggerCtx = DocumentLevelTriggerExecutionContext(monitor, trigger, monitorResult) | |||
There was a problem hiding this comment.
I think instead of the entire monitorResult, we should rather have the triggerResults set as part of the context for the actions. In DocumentLevelAlerting the two could be different, as trigger results are the final set of document ids sent for alert, post the trigger condition evaluation.
cc: @lezzago
There was a problem hiding this comment.
Oh that is a good point. I was thinking it to be more in line with the other monitor types and have this as the monitor result instead of the trigger result since that is what the other monitors seem to do.
There was a problem hiding this comment.
I agree now that we should use trigger results. The other monitor types did not do this because the trigger results from those other monitor types did not have the granularity and useful information.
There was a problem hiding this comment.
This still needs to address below two concerns - could also be discussed with product for requirements:
-
The monitorResult returned as part of the ctx during the Alerts is the combination of all the documents before the trigger was executed. Since this result ignores the trigger logic and will be repeated across all the Alerts, for the users it could be challenging to map the actual documents which led to those specific Alerts.
-
We also introduced
related_doc_idsas part of the Alerts record, and it is specific to Doc Level Alerts. Since it holds the final result for document Id matching the Alerts, it is more contextual information. However, it introduces an inconsistency for how the users are expected to fetch these compared to other monitor type.
My proposal would be to deprecate related_doc_ids in Alerts record slowly, whole have the ctx hold the documents id specific to those alerts (post trigger execution).
eirsep
force-pushed
the
fix-doc-ctx
branch
5 times, most recently
from
a40837c to
76680d9
Compare
|
@lezzago I have attached the output of successfully running alerting integ test task as I am blocked on the infra issue in opensearch plugins |
eirsep
changed the title
|
@lezzago @getsaurabh02 I will open an issue to add ITs to test let's focus this PR on getting the result and triggered docs field in Document level monitoring. I have added tests for the same. |
…execution context Signed-off-by: Surya Sashank Nistala <[email protected]>
|
Will need to get input from the community on what is the best way to fix this issue. Closing PR until then |
Signed-off-by: Surya Sashank Nistala [email protected]
Issue #, if available:
#479
Description of changes:
Populate results and trigggeredDocs field in document level trigger execution context.
CheckList:
[x] Commits are signed per the DCO using --signoff
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.