Refactor Exporters and Readers #210

ansjcy · 2025-01-24T01:45:37Z

Description

There are multiple issues with the current exporter.

Currently, we have different exporters config for different metrics.
https://opensearch.org/docs/latest/observing-your-data/query-insights/top-n-queries/#configuring-a-local-index-exporter
This is not a good pattern since we don't have specific use cases when users enabled multiple top n metrics, but they only want to exporter certain of them. We need to move the exporter from top queries service level to query insights service level.
The safeguard to decide if an index is created by query insights service is not strong. We only check the index name. We should add certain metadata in the index to indicate "this is our index". [FEATURE] Enhance the safegard when deleting top queries records #213
There's no index mapping and index level sorting defined for the index, and we only allow default size (10?) when searching on the index. This will become a problem when the frontend wants lots of data (basically, everything exceed 10 will go wrong) [FEATURE] Exporters should have index mappings defined #212
There are a lot of duplicate records in index when multiple metrics are enabled. We need to do deduplication.
when LocalIndexReader uses generateLocalIndexDateHash() to get historical data, since generateLocalIndexDateHash only consider current time, it's not generating correct hash for older date.
We are overusing static methods, and not complying with the ExporterFactory Model. Need to refactor the node.

Issues Resolved

TODO

tests

TODO

enable top n queries for all metrics and local index exporter with

 curl -X PUT 'localhost:9200/_cluster/settings' -H 'Content-Type: application/json' -d'
{
    "persistent" : {
        "search.insights.top_queries.latency.enabled" : "true",
        "search.insights.top_queries.cpu.enabled" : "true",
        "search.insights.top_queries.memory.enabled" : "true",
        "search.insights.top_queries.exporter.type" : "local_index"
    }
}'
{"acknowledged":true,"persistent":{"search":{"insights":{"top_queries":{"cpu":{"enabled":"true"},"exporter":{"type":"local_index"},"memory":{"enabled":"true"},"latency":{"enabled":"true"}}}}},"transient":{}}

do search
check local index, data are exported without errors

➜  OpenSearch git:(main) ✗ curl -X POST "localhost:9200/top_queries-*/_search?pretty"
{
  "took" : 15,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 6,
      "relation" : "eq"
    },
    "max_score" : 1.0,
    "hits" : [
      {
        "_index" : "top_queries-2025.01.24-62114",
        "_id" : "l1L7lZQBbSnBp-lpBZtY",
        "_score" : 1.0,
        "_source" : {
          "timestamp" : 1737683015018,
          "id" : "ba5087e8-47f6-4882-b4e0-94b0c455df83",
          "task_resource_usages" : [
            {
              "action" : "indices:data/read/search[phase/query]",
              "taskId" : 117,
              "parentTaskId" : 116,
              "nodeId" : "aBdathzfR_O-o7ct8LqHrA",
              "taskResourceUsage" : {
                "cpu_time_in_nanos" : 577000,
                "memory_in_bytes" : 33160
              }
            },
            {
              "action" : "indices:data/read/search",
              "taskId" : 116,
              "parentTaskId" : -1,
              "nodeId" : "aBdathzfR_O-o7ct8LqHrA",
              "taskResourceUsage" : {
                "cpu_time_in_nanos" : 85000,
                "memory_in_bytes" : 3672
              }
            }
          ],
          "search_type" : "query_then_fetch",
          "total_shards" : 1,
          "indices" : [
            "my-index-0"
          ],
          "group_by" : "NONE",
          "labels" : { },
          "phase_latency_map" : {
            "expand" : 0,
            "query" : 1,
            "fetch" : 0
          },
          "source" : {
            "size" : 20,
            "query" : {
              "term" : {
                "user.id" : {
                  "value" : "cyji",
                  "boost" : 1.0
                }
              }
            }
          },
          "measurements" : {
            "memory" : {
              "number" : 36832,
              "count" : 1,
              "aggregationType" : "NONE"
            },
            "latency" : {
              "number" : 1,
              "count" : 1,
              "aggregationType" : "NONE"
            },
            "cpu" : {
              "number" : 662000,
              "count" : 1,
              "aggregationType" : "NONE"
            }
          }
        }
      },
    ...

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.

deshsidd

Added a few comments and went over this - looks good at a high level. Might need to give one more pass soon.

deshsidd · 2025-01-24T22:13:29Z

src/main/java/org/opensearch/plugin/insights/core/exporter/LocalIndexExporter.java

-        String currentDate = DateTimeFormatter.ofPattern("yyyy-MM-dd", Locale.ROOT)
-            .format(Instant.now().atOffset(ZoneOffset.UTC).toLocalDate());
+    private boolean checkIndexExists(String indexName) {
+        ClusterState clusterState = clusterService.state();


Can there be a corner scenario where the cluster state does not have up to date information? Are we handling this?

Yes, In theory the cluster state should be most up to date but there are scenarios when 2 threads are trying to create / delete the same index for the same time. If you look into the code I already handled with something like

Throwable cause = ExceptionsHelper.unwrapCause(e); if (cause instanceof ResourceAlreadyExistsException) {

deshsidd · 2025-01-24T22:15:19Z

src/main/java/org/opensearch/plugin/insights/core/reader/LocalIndexReader.java

@@ -104,7 +116,7 @@ public List<SearchQueryRecord> read(final String from, final String to, String i
        while (curr.isBefore(end.plusDays(1).toLocalDate().atStartOfDay(end.getZone()))) {
            String indexName = buildLocalIndexName(curr);
            SearchRequest searchRequest = new SearchRequest(indexName);
-            SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
+            SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder().size(1000);


Might want to add a constant so this does not get lost in the code in case we want to change this in the future

deshsidd · 2025-01-24T22:18:29Z

src/main/java/org/opensearch/plugin/insights/core/service/QueryInsightsService.java

+        // This method is invoked when sink type is changed
+        // Clear local indices if exporter is of type LocalIndexExporter
+        if (topQueriesExporter != null && topQueriesExporter.getClass() == LocalIndexExporter.class) {
+            deleteAllTopNIndices(client, indexMetadataMap, (LocalIndexExporter) topQueriesExporter);


If we toggle between exports and back to local index exporter do we want to lose all the older data in this scenario?

Yes, that's the expectation for now. Disabling local index exporter will delete all historical data. It was originally implemented in #172

deshsidd · 2025-01-24T22:23:48Z

src/main/java/org/opensearch/plugin/insights/core/service/TopQueriesService.java

-                .execute(() -> ((LocalIndexExporter) exporter).deleteExpiredTopNIndices(indexMetadataMap));
-        }
-    }
+    public static boolean isTopQueriesIndex(String indexName, IndexMetadata indexMetadata) {


nit: Do we want to add this to a helper/util?

I think it should belongs to TopQueriesService though since we are checking if an index belongs to top n queries.

deshsidd · 2025-01-24T22:25:27Z

src/main/java/org/opensearch/plugin/insights/settings/QueryInsightsSettings.java

-     * Prefix for top n queries by memory exporters
-     */
-    private static final String TOP_N_MEMORY_QUERIES_EXPORTER_PREFIX = TOP_N_MEMORY_QUERIES_PREFIX + ".exporter.";
+    private static final String TOP_N_QUERIES_EXPORTER_PREFIX = TOP_N_QUERIES_SETTING_PREFIX + ".exporter";


Self note: Will have to update the documentation since the current docs are now invalid.

yes, I'll send a PR to update the document later.

deshsidd · 2025-01-24T22:27:16Z

src/test/java/org/opensearch/plugin/insights/core/exporter/LocalIndexExporterTests.java

-        verify(adminClient, times(3)).indices();
-        verify(indicesAdminClient, times(3)).delete(any(), any());
-    }
+    // public void testDeleteExpiredTopNIndices() {


Why is this commented out?

It is moved to queryInsightsTests.

deshsidd · 2025-01-24T22:28:26Z

src/test/java/org/opensearch/plugin/insights/core/service/QueryInsightsServiceTests.java

+        verify(localIndexExporter, times(9)).deleteSingleIndex(any(), any());
+    }
+
+    // TODO: comment out for now. Need to reenable this before mering


nit: Don't forget to resolve this

Signed-off-by: Chenyang Ji <[email protected]>

deshsidd

LGTM overall with some minor comments.

deshsidd · 2025-01-27T18:55:45Z

src/main/java/org/opensearch/plugin/insights/core/exporter/QueryInsightsExporter.java

@@ -22,4 +22,6 @@ public interface QueryInsightsExporter extends Closeable {
     * @param records list of {@link SearchQueryRecord}
     */
    void export(final List<SearchQueryRecord> records);
+
+    String getId();


nit: good to add some comments here since this is an interface and the intent may get lost over time

deshsidd · 2025-01-27T19:00:29Z

src/main/java/org/opensearch/plugin/insights/core/exporter/LocalIndexExporter.java

-            .format(Instant.now().atOffset(ZoneOffset.UTC).toLocalDate());
+    private boolean checkIndexExists(String indexName) {
+        ClusterState clusterState = clusterService.state();
+        return clusterState.getRoutingTable().hasIndex(indexName);


Hope we don't need to handle any potential errors here

Good point.. I saw the same patter is used in multiple places in OpenSearch. I think we should be good.
https://github.com/opensearch-project/OpenSearch/blob/e6fc6008e408c41e2369ca308d5a763575ef8c89/server/src/main/java/org/opensearch/cluster/health/ClusterStateHealth.java#L211

deshsidd · 2025-01-27T19:06:39Z

src/main/java/org/opensearch/plugin/insights/core/service/QueryInsightsService.java

+            }
+        } else {
+            // Disable exporter if exporter type is set to null
+            try {


nit: Duplicated code few lines above, could refactor this out

deshsidd · 2025-01-27T19:12:21Z

src/test/java/org/opensearch/plugin/insights/core/service/QueryInsightsServiceTests.java

+        }
+
+        queryInsightsService.deleteAllTopNIndices(client, indexMetadataMap, localIndexExporter);
+        // All 10 indices should be deleted


9 indices should be deleted?

ansjcy · 2025-01-27T21:11:50Z

@deshsidd Thanks for the review, I'll merge it for now to unblock the frontend, but we need to iterate from here to add missing doc/ refactor some code into util functions etc.

* Move exporter config to query insights level Signed-off-by: Chenyang Ji <[email protected]> * use index mapping and define default index metadata for top n queries Signed-off-by: Chenyang Ji <[email protected]> * improve safegard for deleting top queries indices with index mapping Signed-off-by: Chenyang Ji <[email protected]> * fix unit tests Signed-off-by: Chenyang Ji <[email protected]> * fix bug on local index reader with generateLocalIndexDateHash Signed-off-by: Chenyang Ji <[email protected]> * fix exception in query grouper Signed-off-by: Chenyang Ji <[email protected]> * update PR based on comments and re-enable unit tests Signed-off-by: Chenyang Ji <[email protected]> --------- Signed-off-by: Chenyang Ji <[email protected]> (cherry picked from commit 2d14764) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Move exporter config to query insights level * use index mapping and define default index metadata for top n queries * improve safegard for deleting top queries indices with index mapping * fix unit tests * fix bug on local index reader with generateLocalIndexDateHash * fix exception in query grouper * update PR based on comments and re-enable unit tests --------- (cherry picked from commit 2d14764) Signed-off-by: Chenyang Ji <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

ansjcy requested review from jainankitk, deshsidd and dzane17 as code owners January 24, 2025 01:45

ansjcy force-pushed the refactor-exporter-and-reader branch 10 times, most recently from 7f3d20d to 851d174 Compare January 24, 2025 07:07

deshsidd reviewed Jan 24, 2025

View reviewed changes

ansjcy added 5 commits January 25, 2025 15:37

Move exporter config to query insights level

3a6b46a

Signed-off-by: Chenyang Ji <[email protected]>

use index mapping and define default index metadata for top n queries

ab95ebf

Signed-off-by: Chenyang Ji <[email protected]>

improve safegard for deleting top queries indices with index mapping

f6847dc

Signed-off-by: Chenyang Ji <[email protected]>

fix unit tests

cd14db4

Signed-off-by: Chenyang Ji <[email protected]>

fix bug on local index reader with generateLocalIndexDateHash

9584b38

Signed-off-by: Chenyang Ji <[email protected]>

ansjcy force-pushed the refactor-exporter-and-reader branch from 851d174 to 9584b38 Compare January 26, 2025 00:54

ansjcy added 2 commits January 25, 2025 17:47

fix exception in query grouper

2833004

Signed-off-by: Chenyang Ji <[email protected]>

update PR based on comments and re-enable unit tests

0761668

Signed-off-by: Chenyang Ji <[email protected]>

deshsidd approved these changes Jan 27, 2025

View reviewed changes

ansjcy merged commit 2d14764 into opensearch-project:main Jan 27, 2025
12 checks passed

ansjcy added the backport 2.x label Jan 27, 2025

opensearch-trigger-bot bot mentioned this pull request Jan 27, 2025

[Backport 2.x] Refactor Exporters and Readers #215

Merged

ansjcy mentioned this pull request Jan 28, 2025

[FEATURE] Enhance the safegard when deleting top queries records #213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Exporters and Readers #210

Refactor Exporters and Readers #210

ansjcy commented Jan 24, 2025 •

edited

Loading

deshsidd left a comment

deshsidd Jan 24, 2025

ansjcy Jan 26, 2025

deshsidd Jan 24, 2025

deshsidd Jan 24, 2025

ansjcy Jan 26, 2025

deshsidd Jan 24, 2025

ansjcy Jan 26, 2025

deshsidd Jan 24, 2025

ansjcy Jan 26, 2025

deshsidd Jan 24, 2025

ansjcy Jan 26, 2025

deshsidd Jan 24, 2025

ansjcy Jan 26, 2025

deshsidd left a comment

deshsidd Jan 27, 2025

deshsidd Jan 27, 2025

ansjcy Jan 27, 2025

deshsidd Jan 27, 2025

deshsidd Jan 27, 2025

ansjcy commented Jan 27, 2025

Refactor Exporters and Readers #210

Refactor Exporters and Readers #210

Conversation

ansjcy commented Jan 24, 2025 • edited Loading

Description

Issues Resolved

tests

deshsidd 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

Choose a reason for hiding this comment

deshsidd 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

ansjcy commented Jan 27, 2025

ansjcy commented Jan 24, 2025 •

edited

Loading