GetFindingsAction new optional parameters #563
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ package org.opensearch.alerting.transport | |
| import kotlinx.coroutines.CoroutineScope | ||
| import kotlinx.coroutines.Dispatchers | ||
| import kotlinx.coroutines.launch | ||
| import kotlinx.coroutines.withContext | ||
| import org.apache.logging.log4j.LogManager | ||
| import org.apache.lucene.search.join.ScoreMode | ||
| import org.opensearch.action.ActionListener | ||
|
|
@@ -20,6 +21,9 @@ import org.opensearch.action.support.HandledTransportAction | |
| import org.opensearch.alerting.action.GetFindingsAction | ||
| import org.opensearch.alerting.action.GetFindingsRequest | ||
| import org.opensearch.alerting.action.GetFindingsResponse | ||
| import org.opensearch.alerting.action.GetMonitorAction | ||
| import org.opensearch.alerting.action.GetMonitorRequest | ||
| import org.opensearch.alerting.action.GetMonitorResponse | ||
| import org.opensearch.alerting.alerts.AlertIndices.Companion.ALL_FINDING_INDEX_PATTERN | ||
| import org.opensearch.alerting.model.Finding | ||
| import org.opensearch.alerting.model.FindingDocument | ||
|
|
@@ -40,6 +44,7 @@ import org.opensearch.common.xcontent.XContentParserUtils | |
| import org.opensearch.common.xcontent.XContentType | ||
| import org.opensearch.index.query.Operator | ||
| import org.opensearch.index.query.QueryBuilders | ||
| import org.opensearch.rest.RestRequest | ||
| import org.opensearch.search.builder.SearchSourceBuilder | ||
| import org.opensearch.search.fetch.subphase.FetchSourceContext | ||
| import org.opensearch.search.sort.SortBuilders | ||
|
|
@@ -122,7 +127,8 @@ class TransportGetFindingsSearchAction @Inject constructor( | |
| client.threadPool().threadContext.stashContext().use { | ||
| scope.launch { | ||
| try { | ||
| val indexName = resolveFindingsIndexName(getFindingsRequest) | ||
| val getFindingsResponse = search(searchSourceBuilder, indexName) | ||
| actionListener.onResponse(getFindingsResponse) | ||
| } catch (t: Exception) { | ||
| actionListener.onFailure(AlertingException.wrap(t)) | ||
|
There was a problem hiding this comment. if get monitor fails we would already receive the alerting exception would we need to do something like There was a problem hiding this comment. I added another catch for AlertingException |
||
|
|
@@ -131,10 +137,34 @@ class TransportGetFindingsSearchAction @Inject constructor( | |
| } | ||
| } | ||
|
|
||
| suspend fun resolveFindingsIndexName(findingsRequest: GetFindingsRequest): String { | ||
| var indexName = "" | ||
|
There was a problem hiding this comment. We should start with the default one here, and override based on the availability of the other information being present. |
||
|
|
||
| if (findingsRequest.findingIndexName.isNullOrEmpty() == false) { | ||
|
There was a problem hiding this comment. nit: let's call its |
||
| // findingIndexName has highest priority, so use that if available | ||
| indexName = findingsRequest.findingIndexName | ||
| } else if (findingsRequest.monitorId != null) { | ||
|
There was a problem hiding this comment. let's do an |
||
| // second best is monitorId. | ||
| // 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 = | ||
|
There was a problem hiding this comment. we should catch exceptiton if GetmonitorAction fails and fail the get findings action overall? There was a problem hiding this comment. This should be happening on line 134 right now. I'll add IT for this |
||
| [email protected] { | ||
| execute(GetMonitorAction.INSTANCE, getMonitorRequest, it) | ||
| } | ||
|
Comment on lines
+158
to
+161
There was a problem hiding this comment. How do we handle the failure scenarios here? We would want to log and fallback to the default maybe? There was a problem hiding this comment. 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 |
||
| indexName = getMonitorResponse.monitor?.dataSources?.findingsIndex ?: ALL_FINDING_INDEX_PATTERN | ||
| } | ||
| } else { | ||
| // default is ALL_FINDING_INDEX_PATTERN for bwc and when these 2 parameters are not passed | ||
| indexName = ALL_FINDING_INDEX_PATTERN | ||
| } | ||
| return indexName | ||
| } | ||
|
|
||
| suspend fun search(searchSourceBuilder: SearchSourceBuilder, indexName: String): GetFindingsResponse { | ||
| val searchRequest = SearchRequest() | ||
| .source(searchSourceBuilder) | ||
| .indices(indexName) | ||
| val searchResponse: SearchResponse = client.suspendUntil { client.search(searchRequest, it) } | ||
| val totalFindingCount = searchResponse.hits.totalHits?.value?.toInt() | ||
| val mgetRequest = MultiGetRequest() | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.alerting.action | ||
|
|
||
| import org.opensearch.alerting.model.Table | ||
| import org.opensearch.common.io.stream.BytesStreamOutput | ||
| import org.opensearch.common.io.stream.StreamInput | ||
| import org.opensearch.test.OpenSearchTestCase | ||
|
|
||
| class GetFindingsRequestTests : OpenSearchTestCase() { | ||
|
|
||
| fun `test get findings request`() { | ||
|
|
||
| val table = Table("asc", "sortString", null, 1, 0, "") | ||
|
|
||
| val req = GetFindingsRequest("2121", table, "1", "finding_index_name") | ||
| assertNotNull(req) | ||
|
|
||
| val out = BytesStreamOutput() | ||
| req.writeTo(out) | ||
| val sin = StreamInput.wrap(out.bytes().toBytesRef().bytes) | ||
| val newReq = GetFindingsRequest(sin) | ||
|
|
||
| assertEquals("1", newReq.monitorId) | ||
| assertEquals("2121", newReq.findingId) | ||
| assertEquals("finding_index_name", newReq.findingIndexName) | ||
| assertEquals(table, newReq.table) | ||
| } | ||
|
|
||
| fun `test validate returns null`() { | ||
| val table = Table("asc", "sortString", null, 1, 0, "") | ||
|
|
||
| val req = GetFindingsRequest("2121", table, "1", "active") | ||
| assertNotNull(req) | ||
| assertNull(req.validate()) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Custom data sources are not exposed in rest layer. Let's limit the changes to transport layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alerting Rest level action should not be impact with this change, as SAP will introduce its own rest layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done