GetFindingsAction new optional parameters #563

petardz · 2022-09-20T18:47:39Z

Signed-off-by: Petar Dzepina petar.dzepina@vroom.com

Issue #, if available:

Description of changes:
Added monitorId and findingsIndexName optinal params to GetFindings Action. These will be used to reference finding index, instead of default hardcoded one. findingsIndexName has higher priority over monitorId

CheckList:
[ ] 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.

…tion Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

eirsep · 2022-09-20T21:01:54Z

alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestGetFindingsAction.kt

@@ -57,7 +58,9 @@ class RestGetFindingsAction : BaseRestHandler() {

        val getFindingsSearchRequest = GetFindingsRequest(
            findingID,
-            table


Custom data sources are not exposed in rest layer. Let's limit the changes to transport layer.

Alerting Rest level action should not be impact with this change, as SAP will introduce its own rest layer.

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

getsaurabh02 · 2022-09-20T21:19:27Z

alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestGetFindingsAction.kt

@@ -57,7 +58,9 @@ class RestGetFindingsAction : BaseRestHandler() {

        val getFindingsSearchRequest = GetFindingsRequest(
            findingID,
-            table


Alerting Rest level action should not be impact with this change, as SAP will introduce its own rest layer.

getsaurabh02 · 2022-09-20T21:20:07Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetFindingsAction.kt

@@ -131,10 +137,34 @@ class TransportGetFindingsSearchAction @Inject constructor(
        }
    }

-    suspend fun search(searchSourceBuilder: SearchSourceBuilder): GetFindingsResponse {
+    suspend fun resolveFindingsIndexName(findingsRequest: GetFindingsRequest): String {
+        var indexName = ""


We should start with the default one here, and override based on the availability of the other information being present.

getsaurabh02 · 2022-09-20T21:59:09Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetFindingsAction.kt

+                val getMonitorResponse: GetMonitorResponse =
+                    this@TransportGetFindingsSearchAction.client.suspendUntil {
+                        execute(GetMonitorAction.INSTANCE, getMonitorRequest, it)
+                    }


How do we handle the failure scenarios here? We would want to log and fallback to the default maybe?
Also moving the fetch config information to a common utility might be a good way to use across. such as for #561

Exception is caught on line 134 and sent back to caller.

Hmm falling back to default one might lead to spurious behavior. It would be better if user knows that GetMonitor Action failed and is retryable then to get back NOT_FOUND without knowing if the desired finding index was searched or not

getsaurabh02 · 2022-09-20T22:09:07Z

alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt

+        // fetch findings - pass monitorId as reference to finding_index
+        val findingsFromAPI1 = getFindings(findings.get(0).id, monitorId, null)
+        assertEquals(
+            "Findings mismatch between manually searched and fetched via GetFindingsAction",
+            findings.get(0).id,
+            findingsFromAPI1.get(0).id
+        )
+        // fetch findings - pass both monitorId and findingIndexName name. Monitor id should be ignored
+        val findingsFromAPI2 = getFindings(findings.get(0).id, "incorrect_monitor_id", customFindingsIndex)
+        assertEquals(
+            "Findings mismatch between manually searched and fetched via GetFindingsAction",
+            findings.get(0).id,
+            findingsFromAPI2.get(0).id
+        )
+        // fetch findings - don't send monitorId or findingIndexName. It should fall back to hardcoded finding index name
+        val findingsFromAPI3 = getFindings(findings.get(0).id, null, null)
+        assertEquals(
+            "Unexpected result when fetching findings from default finding index",
+            0,
+            findingsFromAPI3.size


We should break these into individual findings scenarios - also to include the failure mode tests such as fallback if the name is present while the index is not?

done and added more tests

eirsep

Thanks for making these changes, Peter.

Left a few minor comments.

eirsep · 2022-09-20T21:15:40Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetFindingsAction.kt

+    suspend fun resolveFindingsIndexName(findingsRequest: GetFindingsRequest): String {
+        var indexName = ""
+
+        if (findingsRequest.findingIndexName.isNullOrEmpty() == false) {


nit: let's call its findingsIndex instead of findingsIndexName for consistence with DataSources class.

eirsep · 2022-09-20T21:41:43Z

alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt

@@ -165,6 +165,57 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() {
        assertTrue("Findings saved for test monitor", findings[0].relatedDocIds.contains("1"))
    }

+    fun `test execute monitor with custom findings index and GetFindingsAction`() {


this is not incorrect but instead could we add to existing test methods in this test class where custom findings are being verified

eirsep · 2022-09-20T21:50:10Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetFindingsAction.kt

+        if (findingsRequest.findingIndexName.isNullOrEmpty() == false) {
+            // findingIndexName has highest priority, so use that if available
+            indexName = findingsRequest.findingIndexName
+        } else if (findingsRequest.monitorId != null) {


let's do an monitorId.isNullOrEmpty() == false check here too similar to above

eirsep · 2022-09-20T21:52:36Z

alerting/src/test/kotlin/org/opensearch/alerting/TriggerServiceTests.kt

-        val inputResultsStr = "{\"_shards\":{\"total\":1,\"failed\":0,\"successful\":1,\"skipped\":0},\"hits\":{\"hits\":[{\"_index\":\"sample-http-responses\",\"_type\":\"http\",\"_source\":{\"status_code\":100,\"http_4xx\":0,\"http_3xx\":0,\"http_5xx\":0,\"http_2xx\":0,\"timestamp\":100000,\"http_1xx\":1},\"_id\":1,\"_score\":1}],\"total\":{\"value\":4,\"relation\":\"eq\"},\"max_score\":1},\"took\":37,\"timed_out\":false,\"aggregations\":{\"status_code\":{\"doc_count_error_upper_bound\":0,\"sum_other_doc_count\":0,\"buckets\":[{\"doc_count\":2,\"key\":100},{\"doc_count\":1,\"key\":102},{\"doc_count\":1,\"key\":201}]},\"${trigger.id}\":{\"parent_bucket_path\":\"status_code\",\"bucket_indices\":[0,1,2]}}}"
-
-        val parser = XContentType.JSON.xContent().createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, inputResultsStr)
+        val inputResultsStr = "{\"_shards\":" +


thank you for doing this :)

eirsep · 2022-09-20T22:34:19Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetFindingsAction.kt

+            // We will use it to fetch monitor and then read indexName from dataSources field of monitor
+            withContext(Dispatchers.IO) {
+                val getMonitorRequest = GetMonitorRequest(findingsRequest.monitorId, -3L, RestRequest.Method.GET, null)
+                val getMonitorResponse: GetMonitorResponse =


we should catch exceptiton if GetmonitorAction fails and fail the get findings action overall?

This should be happening on line 134 right now. I'll add IT for this

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

eirsep · 2022-09-20T23:04:27Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetFindingsAction.kt

@@ -122,7 +127,8 @@ class TransportGetFindingsSearchAction @Inject constructor(
        client.threadPool().threadContext.stashContext().use {
            scope.launch {
                try {
-                    val getFindingsResponse = search(searchSourceBuilder)
+                    val indexName = resolveFindingsIndexName(getFindingsRequest)
+                    val getFindingsResponse = search(searchSourceBuilder, indexName)
                    actionListener.onResponse(getFindingsResponse)
                } catch (t: Exception) {
                    actionListener.onFailure(AlertingException.wrap(t))


if get monitor fails we would already receive the alerting exception

would we need to do something like

if (t is AlertingException) { actionListener.onFailure(t) } else { actionListener.onFailure(AlertingException.wrap(t)) }

I added another catch for AlertingException

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

codecov-commenter · 2022-09-21T01:55:35Z

Codecov Report

Merging #563 (1e54b0c) into main (5074f7b) will increase coverage by 0.02%.
The diff coverage is 90.62%.

@@             Coverage Diff              @@
##               main     #563      +/-   ##
============================================
+ Coverage     76.83%   76.85%   +0.02%     
  Complexity      178      178              
============================================
  Files           167      167              
  Lines          8497     8526      +29     
  Branches       1261     1264       +3     
============================================
+ Hits           6529     6553      +24     
- Misses         1337     1341       +4     
- Partials        631      632       +1

Impacted Files	Coverage Δ
...h/alerting/transport/TransportGetFindingsAction.kt	`92.23% <85.71%> (+1.64%)`	⬆️
...g/opensearch/alerting/action/GetFindingsRequest.kt	`100.00% <100.00%> (+50.00%)`	⬆️
...ing/model/destination/DestinationContextFactory.kt	`75.00% <0.00%> (-3.58%)`	⬇️
.../alerting/transport/TransportIndexMonitorAction.kt	`68.29% <0.00%> (-3.14%)`	⬇️
.../kotlin/org/opensearch/alerting/util/IndexUtils.kt	`70.83% <0.00%> (-2.09%)`	⬇️
...ain/kotlin/org/opensearch/alerting/AlertService.kt	`78.19% <0.00%> (-0.48%)`	⬇️
...destinationmigration/DestinationConversionUtils.kt	`71.11% <0.00%> (+1.11%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

* added monitorId and findingsIndexName optinal params to GetFindingsAction Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

added monitorId and findingsIndexName optinal params to GetFindingsAc…

d7fc542

…tion Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

petardz requested a review from a team September 20, 2022 18:47

fixed linter; added unit tests for request

af07d62

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

eirsep reviewed Sep 20, 2022

View reviewed changes

removed new params from rest action

8684ff1

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

getsaurabh02 requested changes Sep 20, 2022

View reviewed changes

eirsep requested changes Sep 20, 2022

View reviewed changes

eirsep mentioned this pull request Sep 20, 2022

Adds support for fetching alerts from custom alert index in Get Alerts transport layer #561

Merged

eirsep reviewed Sep 20, 2022

View reviewed changes

minor tweaks

6f9e0c1

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

eirsep reviewed Sep 20, 2022

View reviewed changes

Petar Dzepina added 3 commits September 21, 2022 01:07

ITs tweaks

7d79dfe

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

added more tests

8d1cadf

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

added exception catch for Alerting exception

1e54b0c

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

Petar Dzepina added 3 commits September 21, 2022 22:16

transport action refactor to support external caller

54a6427

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

reverted transport action to use GetFindingsRequest

fd3f7ae

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

Empty-Commit

1975242

Signed-off-by: Petar Dzepina <petar.dzepina@vroom.com>

eirsep approved these changes Sep 22, 2022

View reviewed changes

getsaurabh02 approved these changes Sep 22, 2022

View reviewed changes

eirsep merged commit 4ca0fb0 into opensearch-project:main Sep 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetFindingsAction new optional parameters #563

GetFindingsAction new optional parameters #563

petardz commented Sep 20, 2022 •

edited

Loading

eirsep Sep 20, 2022

getsaurabh02 Sep 20, 2022

petardz Sep 20, 2022

getsaurabh02 Sep 20, 2022

getsaurabh02 Sep 20, 2022

petardz Sep 20, 2022

getsaurabh02 Sep 20, 2022

petardz Sep 20, 2022

getsaurabh02 Sep 20, 2022

petardz Sep 20, 2022

eirsep left a comment

eirsep Sep 20, 2022

petardz Sep 20, 2022

eirsep Sep 20, 2022

petardz Sep 20, 2022

eirsep Sep 20, 2022

petardz Sep 20, 2022

eirsep Sep 20, 2022

petardz Sep 20, 2022

eirsep Sep 20, 2022

petardz Sep 20, 2022

eirsep Sep 20, 2022

petardz Sep 20, 2022

codecov-commenter commented Sep 21, 2022 •

edited

Loading

GetFindingsAction new optional parameters #563

GetFindingsAction new optional parameters #563

Conversation

petardz commented Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirsep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 21, 2022 • edited Loading

Codecov Report

petardz commented Sep 20, 2022 •

edited

Loading

codecov-commenter commented Sep 21, 2022 •

edited

Loading