optimize doc-level monitor workflow for index patterns

[sbcd90]

Description of changes:
This pr addresses following performance problems in doc-level monitor execution workflow.

• In the current workflow, if the doc-level monitor is monitoring an index pattern & a new index is introduced which matches the pattern, the doc-level monitor duplicates all the field mappings & queries for that index. This is reproducible using integ test https://github.com/opensearch-project/alerting/pull/1097/files#diff-9b08d53e8cff3d739beb6ba304e4eaf466f08412762c3cd55886a52b722a77f1R503

Now, say, we have 1000 queries & each concrete index behind the index-pattern has 1000 field mappings. Also, lets assume 1 concrete index is generated everyday. We also know the default number of field mappings an index can have is 1000 & today if the no. of field mappings go over 1000 in the query index, we rollover the query index.

This would mean, we create a new rollover query index everyday & keep on ingesting 1000 queries in it everyday. In 30 days, we create 30 indices(which means by default 1 primary & 1 replica shards per index) which contains 30000 duplicate queries.
This causes the data nodes to get full resulting in cluster crash.
opensearch-project/security-analytics#509
https://forum.opensearch.org/t/security-analytics-error/14639/11

We do not notice this problem for small no. of queries but duplication of queries piles up over a period of time.
This pr addresses this issue by continously updating 1 set of queries for all the concrete indices belonging to an index-pattern. this provides huge storage optimization.

• In the current workflow, for every concrete index, we make an update mapping api call in no particular order. So, say, index test1 has fields f1 & f2 & test2 has field f4 & both of them match pattern test*, if we make first update mapping call for test1, then query index gets f1 & f2 but in the next update mapping call for test2 we completely overwrite it with f4. This is reproducible using integ test https://github.com/opensearch-project/alerting/pull/1097/files#diff-9b08d53e8cff3d739beb6ba304e4eaf466f08412762c3cd55886a52b722a77f1R551

This pr addresses this issue by first collecting field mappings of all concrete indices belonging to an index-pattern together, & then making a single call to update mappings api.

• In the current workflow, for every concrete index, we make an update mapping api call in no particular order. So, if there are 100 concrete indices behind an index-pattern we make 100 update mapping api calls.

This pr optimizes the time complexity by making a single call to update mappings api for each index pattern.

• Here is how the optimization looks like
  

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.

[codecov]

Codecov Report

Merging #1097 (6d5b661) into main (778e7ce) will increase coverage by 0.05%.
Report is 2 commits behind head on main.
The diff coverage is 92.15%.

❗ Current head 6d5b661 differs from pull request most recent head 4a85d13. Consider uploading reports for the commit 4a85d13 to get more accurate results

@@             Coverage Diff              @@
##               main    #1097      +/-   ##
============================================
+ Coverage     67.72%   67.77%   +0.05%     
  Complexity      105      105              
============================================
  Files           160      160              
  Lines         10343    10363      +20     
  Branches       1522     1521       -1     
============================================
+ Hits           7005     7024      +19     
- Misses         2672     2673       +1     
  Partials        666      666              

Files Changed	Coverage Δ
.../kotlin/org/opensearch/alerting/util/IndexUtils.kt	79.31% <ø> (-0.69%)	⬇️
...opensearch/alerting/util/DocLevelMonitorQueries.kt	77.27% <88.00%> (+1.79%)	⬆️
.../opensearch/alerting/DocumentLevelMonitorRunner.kt	84.53% <96.15%> (+0.42%)	⬆️

... and 4 files with indirect coverage changes

[eirsep]

why do we do this indexName.replace("*", "_")?

[sbcd90]

this is because query string query fields with * have a different meaning & is also not allowed in some cases.

[eirsep]

1. can we add error log with monitor Id and indices list which were not found
2. in exception why are we logging only 0th index element of indices variable? can we log all elements in array/list

[sbcd90]

changed it now. it was copied from eariler logic.

[eirsep]

why did we change this list being looped from computed concrete indices list to the monitor indices list?

[sbcd90]

this is because we want to expand 1 index pattern & process all its concrete indices separately. Previous logic was we expand all index patterns & process all the concrete indices one by one.

[eirsep]

To create query index mappings for an index pattern would be a very big optimization but it works only under the assumption that an index pattern test* resolves to indices that do not have fields with same names but different data types.

this fails in the following scenario:
indices test1 and test2 are resolved from test* pattern. test1 has a field named f1 of type long and test2 also has a field named f1 BUT with type keyword

when we store the field mapping in query index for f1_test* this would break the query execution due to incorrect mapping

[eirsep]

we can add this optimization for the scenarios where we can identify all indices covered in index pattern are guaranteed to have the same mappings i.e. data streams, rolling indices with index templates.

should we store a field in index mapping called index_template if we are creating index from index template? food for throught

[eirsep]

@sbcd90

To create query index mappings for an index pattern would be a very big optimization but it works only under the assumption that an index pattern test* resolves to indices that do not have fields with same names but different data types.

this fails in the following scenario: indices test1 and test2 are resolved from test* pattern. test1 has a field named f1 of type long and test2 also has a field named f1 BUT with type keyword

when we store the field mapping in query index for f1_test* this would break the query execution due to incorrect mapping

Since we are batching all mappings can we check if the above mentioned field mapping mismatch is happening for any fields and then ingest one for the generic test* and one mapping for the specific index. Similarly while fetching mappings look for both index mappings with test* and test1. If latter is found prefer it. if not found, use the former.

[sbcd90]

@sbcd90

To create query index mappings for an index pattern would be a very big optimization but it works only under the assumption that an index pattern test* resolves to indices that do not have fields with same names but different data types.
this fails in the following scenario: indices test1 and test2 are resolved from test* pattern. test1 has a field named f1 of type long and test2 also has a field named f1 BUT with type keyword
when we store the field mapping in query index for f1_test* this would break the query execution due to incorrect mapping

Since we are batching all mappings can we check if the above mentioned field mapping mismatch is happening for any fields and then ingest one for the generic test* and one mapping for the specific index. Similarly while fetching mappings look for both index mappings with test* and test1. If latter is found prefer it. if not found, use the former.

hi @eirsep , that is the exact fix i'm working on.

[sbcd90]

hi @eirsep , just addressed the comment with the test case https://github.com/opensearch-project/alerting/pull/1097/files#diff-9b08d53e8cff3d739beb6ba304e4eaf466f08412762c3cd55886a52b722a77f1R503.
Can you please look into the pr again?

[eirsep]

the java doc description for this method seems like it should describe adjustMaxFieldLimitForQueryIndex

[eirsep]

can we update the code comments for this method and below method

[sbcd90]

updated it.

[eirsep]

NIT; since you are not explicitly creating index with different mappings
can you plz fetch second index's mapping and assert that the field type is actually different for test_field so as to validate the test scenario. that would make the test more readable. right now it's a bit hard to understand.

[sbcd90]

specifying explicit mappings now.

[eirsep]

can we plz add asserts on the expected mappings and content of the query index. that would be the essence of these tests and help us visualize the changes made in this PR

[sbcd90]

asserts added on actual queries stored in query index.

[eirsep]

this is a very critical piece fo code. can we update java docs description of this method incorporating the new change?

[sbcd90]

updated the documentation for the method.

[eirsep]

does the below flatten logic handle both the nested and non-nested type objects correctly?

what would be the difference after flattening into string in the following 2 cases

"nested_field": {
                      "type": "nested",
                      "properties": {
                        "test1": {
                          "type": "keyword"
                        }
                      }
                      

and

     "nested_field": {
                      
                      "properties": {
                        "test1": {
                          "type": "keyword"
                        }
                      }

[sbcd90]

nested fields dont work with query string queries. https://stackoverflow.com/questions/69857071/how-can-i-use-query-string-to-match-both-nested-and-non-nested-fields-at-the-sam
but if we pair them with an index containing an object field, it will work. https://github.com/opensearch-project/alerting/pull/1097/files#diff-9b08d53e8cff3d739beb6ba304e4eaf466f08412762c3cd55886a52b722a77f1R636

[eirsep]

why do we need field name twice?
can you add some code comments around what is the Triple type object being returned to make it more readable?

[sbcd90]

this is used intentionally because leafNodeProcessor is used at another place where the 2nd param is actually used to pass modified field name.

[eirsep]

thanks for making these changes @sbcd90

i have some minor comments not related to logic but more around readiblity and testsing.

Kindly also respond to earlier comments.

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.x
# Create a new branch
git switch --create backport-1097-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7f0c7c7e77a9213ad5e976c2f1573321bc26b919
# Push it to GitHub
git push --set-upstream origin backport-1097-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport-1097-to-2.x.