[Remote Vector Index Build] Add metric collection #2615

owenhalpert · 2025-03-19T01:24:24Z

Description

Adds metric collection for various metrics related to the Remote Vector Index Build service, with stats only being returned in /stats if the feature flag is enabled. Also adds a flag to detect whether the index build is coming from merge or flush to get granular operation-level metrics.

Gist showing stats response with and without feature flag enabled: https://gist.github.com/owenhalpert/a05029924887390bed6c14d60b34bf01

New metrics (only returned if feature flag is enabled):

"remote_vector_index_build_stats" : {
        "repository_stats" : {
          "read_success_count" : 0,
          "read_failure_count" : 0,
          "successful_read_time_in_millis" : 0,
          "write_success_count" : 0,
          "write_failure_count" : 0,
          "successful_write_time_in_millis" : 0
        },
        "client_stats" : {
          "status_request_failure_count" : 0,
          "status_request_success_count" : 0,
          "index_build_success_count" : 0,
          "index_build_failure_count" : 1,
          "build_request_failure_count" : 0,
          "build_request_success_count" : 0,
          "waiting_time_in_ms" : 0
        },
        "build_stats" : {
          "remote_index_build_flush_time_in_millis" : 0,
          "remote_index_build_current_merge_operations" : 0,
          "remote_index_build_current_flush_operations" : 0,
          "remote_index_build_current_merge_size" : 0,
          "remote_index_build_current_flush_size" : 0,
          "remote_index_build_merge_time_in_millis" : 0
        }
},

From @jed326's original PR (#2566):

Complementary to these, existing repository implementations may also collect their own metrics. For example, repository-s3 collects metrics like so:
Existing repo metrics:

     "repositories" : [
        {
          "repository_name" : "vector-repo",
          "repository_type" : "s3",
          "repository_location" : {
            "bucket" : "gpu-acceleration",
            "base_path" : "vectors"
          },
          "request_time_in_millis" : {
            "ListObjects" : 49,
            "PutMultipartObject" : 0,
            "PutObject" : 199,
            "GetObject" : 38,
            "DeleteObjects" : 51
          },
          "request_retry_count_total" : {
            "ListObjects" : 0,
            "PutMultipartObject" : 0,
            "PutObject" : 0,
            "GetObject" : 0,
            "DeleteObjects" : 0
          },
          "request_failures_total" : {
            "ListObjects" : 0,
            "PutMultipartObject" : 0,
            "PutObject" : 0,
            "GetObject" : 0,
            "DeleteObjects" : 0
          },
          "generic_stats" : {
            "LowPriorityPermitsUtilization" : 0,
            "NormalPriorityPermitsUtilization" : 0,
            "LowPriorityQUtilization" : 0,
            "NormalPriorityQUtilization" : 0
          },
          "request_success_total" : {
            "ListObjects" : 1,
            "PutMultipartObject" : 0,
            "PutObject" : 6,
            "GetObject" : 3,
            "DeleteObjects" : 1
          }
        }
      ],

Related Issues

Meta #2391

Check List

New functionality includes testing.
~~- [ ] New functionality has been documented.~~
~~- [ ] API changes companion pull request created.~~
Commits are signed per the DCO using --signoff.
~~- [ ] Public documentation issue/PR created.~~

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.

Signed-off-by: owenhalpert <[email protected]>

src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java

src/main/java/org/opensearch/knn/plugin/stats/KNNStats.java

jed326 · 2025-03-21T17:03:30Z

src/main/java/org/opensearch/knn/index/codec/nativeindex/NativeIndexBuildStrategy.java

+    /**
+     * Build and write index with flush flag. Default implementation calls the non-flag version.
+     * RemoteIndexBuildStrategy will override this to use the flag.
+     *
+     * @param indexInfo {@link BuildIndexParams} containing information about the index to be built
+     * @param isFlush Flag indicating if operation is from flush (vs merge)
+     * @throws IOException if an error occurs during the build process
+     */


I think you can just explain that this is to differentiate the context for metrics purposes rather than explain what it is doing

There doesn't seem to be a need for this method - why can't we add context in BuildIndexParams? the context can be either merge or flush

We can do that, good idea.

@shatejas I added a boolean isFlush field in BuildIndexParams. I considered having this be a string of either merge or flush but I felt that keeping it as a boolean named isFlush is more clear for its usage.

src/main/java/org/opensearch/knn/index/codec/nativeindex/NativeIndexWriter.java

jed326 · 2025-03-21T17:08:30Z

src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java

+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.BUILD_REQUEST_FAILURE_COUNT;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.BUILD_REQUEST_SUCCESS_COUNT;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.INDEX_BUILD_FAILURE_COUNT;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.INDEX_BUILD_SUCCESS_COUNT;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.READ_FAILURE_COUNT;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.READ_SUCCESS_COUNT;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.READ_TIME;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.REMOTE_INDEX_BUILD_CURRENT_FLUSH_OPERATIONS;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.REMOTE_INDEX_BUILD_CURRENT_FLUSH_SIZE;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.REMOTE_INDEX_BUILD_CURRENT_MERGE_OPERATIONS;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.REMOTE_INDEX_BUILD_CURRENT_MERGE_SIZE;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.REMOTE_INDEX_BUILD_FLUSH_TIME;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.REMOTE_INDEX_BUILD_MERGE_TIME;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.WAITING_TIME;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.WRITE_FAILURE_COUNT;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.WRITE_SUCCESS_COUNT;
+import static org.opensearch.knn.plugin.stats.KNNRemoteIndexBuildValue.WRITE_TIME;


Thinking out loud here, should we try to offload all of the stats collection to a separate class? One example could be we have a MetricsCollectingRemoteIndexBuildStrategy extends RemoteIndexBuildStrategy which does all the metrics collection.

Signed-off-by: owenhalpert <[email protected]>

shatejas · 2025-03-25T17:32:15Z

src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java

+            recordBuildRequestSuccess(stopWatch.stop().totalTime().millis(), indexInfo.getFieldName());
+            return remoteBuildResponse;
+        } catch (IOException e) {
+            recordBuildRequestFailure(stopWatch.stop().totalTime().millis(), indexInfo.getFieldName(), e);


We need to do these in finally block. Any other exception and you are not capturing failure. another approach is to catch a generic approach which is generally not recommended. I see the pattern at other places as well.

bool success = false; try { success = true; ...... } finally { recordBuildRequestMetrics(success, stopWatch.millis); }

I see you are passing in the exception and its just used for logging. Please log in the catch block so you don't have to pass the error

owenhalpert force-pushed the remote-metrics branch 9 times, most recently from a795d41 to 97a5c0c Compare March 19, 2025 22:33

Add metric collection to the remote index build.

595fc7c

Signed-off-by: owenhalpert <[email protected]>

owenhalpert force-pushed the remote-metrics branch from 97a5c0c to 595fc7c Compare March 19, 2025 23:07

jed326 reviewed Mar 20, 2025

View reviewed changes

src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/knn/plugin/stats/KNNStats.java Outdated Show resolved Hide resolved

owenhalpert force-pushed the remote-metrics branch 5 times, most recently from 70ca41c to e102dfc Compare March 21, 2025 15:43

This was referenced Mar 21, 2025

[Meta] Remote Vector Index Build Component in OpenSearch Vector Engine #2391

Open

Add metrics for repository interactions to RemoteIndexBuildStrategy #2566

Closed

owenhalpert marked this pull request as ready for review March 21, 2025 16:45

owenhalpert requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners March 21, 2025 16:45

owenhalpert requested review from shatejas, 0ctopus13prime and Vikasht34 as code owners March 21, 2025 16:45

jed326 reviewed Mar 21, 2025

View reviewed changes

owenhalpert force-pushed the remote-metrics branch 3 times, most recently from 0841e10 to 04e1b56 Compare March 25, 2025 00:05

Add flush/merge flag, separate metric logic out from main code flow

201614b

Signed-off-by: owenhalpert <[email protected]>

owenhalpert force-pushed the remote-metrics branch from 04e1b56 to 201614b Compare March 25, 2025 00:10

shatejas reviewed Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Remote Vector Index Build] Add metric collection #2615

[Remote Vector Index Build] Add metric collection #2615

owenhalpert commented Mar 19, 2025 •

edited

Loading

jed326 Mar 21, 2025

shatejas Mar 24, 2025

owenhalpert Mar 24, 2025

owenhalpert Mar 25, 2025

jed326 Mar 21, 2025

shatejas Mar 25, 2025

[Remote Vector Index Build] Add metric collection #2615

Are you sure you want to change the base?

[Remote Vector Index Build] Add metric collection #2615

Conversation

owenhalpert commented Mar 19, 2025 • edited Loading

Description

Related Issues

Check List

jed326 Mar 21, 2025

Choose a reason for hiding this comment

shatejas Mar 24, 2025

Choose a reason for hiding this comment

owenhalpert Mar 24, 2025

Choose a reason for hiding this comment

owenhalpert Mar 25, 2025

Choose a reason for hiding this comment

jed326 Mar 21, 2025

Choose a reason for hiding this comment

shatejas Mar 25, 2025

Choose a reason for hiding this comment

owenhalpert commented Mar 19, 2025 •

edited

Loading