add task resource tracking service to cluster service #14681

ansjcy · 2024-07-09T01:36:44Z

Description

This is mainly used in query insights plugin.
Related PR: opensearch-project/query-insights#13
Currently we are not refreshing task level resource usages on coordinator node for searchTasks, which means all coordinator node resource usage will be 0. for example, below node level resource usages in top n queries results:

{
      "timestamp" : 1720486053463,
      "task_resource_usages" : [
        {
          "action" : "indices:data/read/search[phase/query]",
          "taskId" : 96,
          "parentTaskId" : 95,
          "nodeId" : "jLuPXX2pSoeloo5f7u1ASQ",
          "taskResourceUsage" : {
            "cpu_time_in_nanos" : 22570000,
            "memory_in_bytes" : 2563560
          }
        },
        {
          "action" : "indices:data/read/search",
          "taskId" : 95,
          "parentTaskId" : -1,
          "nodeId" : "jLuPXX2pSoeloo5f7u1ASQ",
          "taskResourceUsage" : {
            "cpu_time_in_nanos" : 0,
            "memory_in_bytes" : 0
          }
        }
      ],

This pr is a prerequisite of the fix to refresh task level resource usage before reading it in query insights - basically expose TaskResourceTrackingService for plugins to use.

curl -X GET "localhost:9200/_insights/top_queries?pretty"
{
  "top_queries" : [
    {
      "timestamp" : 1720488258667,
      "source" : "{\"size\":20,\"query\":{\"bool\":{\"must\":[{\"match_phrase\":{\"message\":{\"query\":\"document 2\",\"slop\":0,\"zero_terms_query\":\"NONE\",\"boost\":1.0}}},{\"match\":{\"user.id\":{\"query\":\"cyji\",\"operator\":\"OR\",\"prefix_length\":0,\"max_expansions\":50,\"fuzzy_transpositions\":true,\"lenient\":false,\"zero_terms_query\":\"NONE\",\"auto_generate_synonyms_phrase_query\":true,\"boost\":1.0}}}],\"adjust_pure_negative\":true,\"boost\":1.0}}}",
      "node_id" : "iRnjgZ-dS721NOgtVj5XZg",
      "search_type" : "query_then_fetch",
      "labels" : { },
      "phase_latency_map" : {
        "expand" : 0,
        "query" : 64,
        "fetch" : 3
      },
      "task_resource_usages" : [
        {
          "action" : "indices:data/read/search[phase/query]",
          "taskId" : 57,
          "parentTaskId" : 79,
          "nodeId" : "m9EirNWiSomgjClu3MNu1A",
          "taskResourceUsage" : {
            "cpu_time_in_nanos" : 23660000,
            "memory_in_bytes" : 3352616
          }
        },
        {
          "action" : "indices:data/read/search",
          "taskId" : 79,
          "parentTaskId" : -1,
          "nodeId" : "iRnjgZ-dS721NOgtVj5XZg",
          "taskResourceUsage" : {
            "cpu_time_in_nanos" : 2194000,
            "memory_in_bytes" : 243648
          }
        }
      ],
      "total_shards" : 1,
      "indices" : [
        "my-index-0"
      ],
      "latency" : 77
    },
    {
      "timestamp" : 1720488268193,
      "source" : "{\"size\":20,\"query\":{\"bool\":{\"must\":[{\"match_phrase\":{\"message\":{\"query\":\"document 2\",\"slop\":0,\"zero_terms_query\":\"NONE\",\"boost\":1.0}}},{\"match\":{\"user.id\":{\"query\":\"cyji\",\"operator\":\"OR\",\"prefix_length\":0,\"max_expansions\":50,\"fuzzy_transpositions\":true,\"lenient\":false,\"zero_terms_query\":\"NONE\",\"auto_generate_synonyms_phrase_query\":true,\"boost\":1.0}}}],\"adjust_pure_negative\":true,\"boost\":1.0}}}",
      "node_id" : "iRnjgZ-dS721NOgtVj5XZg",
      "search_type" : "query_then_fetch",
      "labels" : { },
      "phase_latency_map" : {
        "expand" : 0,
        "query" : 33,
        "fetch" : 0
      },
      "task_resource_usages" : [
        {
          "action" : "indices:data/read/search[phase/query]",
          "taskId" : 93,
          "parentTaskId" : 92,
          "nodeId" : "iRnjgZ-dS721NOgtVj5XZg",
          "taskResourceUsage" : {
            "cpu_time_in_nanos" : 26900000,
            "memory_in_bytes" : 2549168
          }
        },
        {
          "action" : "indices:data/read/search",
          "taskId" : 92,
          "parentTaskId" : -1,
          "nodeId" : "iRnjgZ-dS721NOgtVj5XZg",
          "taskResourceUsage" : {
            "cpu_time_in_nanos" : 151000,
            "memory_in_bytes" : 3288
          }
        }
      ],
      "total_shards" : 1,
      "indices" : [
        "my-index-0"
      ],
      "latency" : 35
    }
  ]
}

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

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.

github-actions · 2024-07-09T02:28:04Z

❌ Gradle check result for 31a955e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Chenyang Ji <cyji@amazon.com>

github-actions · 2024-07-09T05:38:46Z

✅ Gradle check result for d1a6038: SUCCESS

codecov · 2024-07-09T05:40:06Z

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.58%. Comparing base (0684342) to head (14ff852).
Report is 21 commits behind head on main.

Files	Patch %	Lines
...org/opensearch/cluster/service/ClusterService.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #14681      +/-   ##
============================================
- Coverage     71.68%   71.58%   -0.10%     
+ Complexity    62365    62327      -38     
============================================
  Files          5148     5148              
  Lines        293297   293301       +4     
  Branches      42384    42384              
============================================
- Hits         210239   209960     -279     
- Misses        65717    65923     +206     
- Partials      17341    17418      +77

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-07-09T08:10:25Z

✅ Gradle check result for bfccfa5: SUCCESS

jainankitk · 2024-07-09T16:52:50Z

...ery-insights/src/main/java/org/opensearch/plugin/insights/rules/model/SearchQueryRecord.java

-        this.attributes = in.readMap(Attribute::readFromStream, StreamInput::readGenericValue);
+        this.attributes = Attribute.readAttributeMap(in);


Has something changed in the ser/deser logic? If yes, can that cause issue while upgrading from 2.15 to 2.16?

Hey @jainankitk ! Actually this is a bug we didn't caught. readGenericValue won't be able to read List<CustomObject>, we need to write our own reader in that case.

And I took this from Siddhant's PR when moving categorization code to the plugin: https://github.com/opensearch-project/OpenSearch/pull/14528/files#r1669900434

@jainankitk This was also discussed in detail in the PR to move categorization to plugin : https://github.com/opensearch-project/OpenSearch/pull/14528/files#r1669900434

Got it, thanks for the details! Given the serialization was failing already, should not result into compatibility issues.

jainankitk · 2024-07-09T16:53:38Z

...ery-insights/src/main/java/org/opensearch/plugin/insights/rules/model/SearchQueryRecord.java

+        out.writeMap(
+            attributes,
+            (stream, attribute) -> Attribute.writeTo(out, attribute),
+            (stream, attributeValue) -> Attribute.writeValueTo(out, attributeValue)


Same goes for the write part as well

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java

Signed-off-by: Chenyang Ji <cyji@amazon.com>

deshsidd · 2024-07-09T18:15:42Z

Curious why the serialize test was passing earlier when we made changes to main and why it was failing only in my branch? @ansjcy

deshsidd · 2024-07-09T18:24:34Z

...sights/src/main/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListener.java

Should we change to attributes.put(Attribute.SOURCE, request.source()); here itself so we can use this in the future?

maybe let's do it as part of your PR? to ensure this PR only contains the necessary urgent fixes.

Makes sense. Thanks!

jainankitk

@ansjcy - The changes look good. Can we just add a test to demonstrate the failure earlier and now it is passing?

github-actions · 2024-07-09T18:50:21Z

❕ Gradle check result for d00b2df: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Signed-off-by: Chenyang Ji <cyji@amazon.com>

github-actions · 2024-07-09T20:25:35Z

✅ Gradle check result for 720cefb: SUCCESS

jainankitk

Thank you adding test. LGTM!

Signed-off-by: Chenyang Ji <cyji@amazon.com>

github-actions · 2024-07-12T20:38:19Z

❕ Gradle check result for 14ff852: UNSTABLE

TEST FAILURES:

      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationDuringQueryPhase

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

mch2 · 2024-07-15T21:54:37Z

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java

@@ -51,6 +52,7 @@
 /**
 * Service that helps track resource usage of tasks running on a node.
 */
+@PublicApi(since = "2.16.0")


I understand why you are marking this public in that it is now exposed publicly from ClusterService. Is this the right level of visibility for this service? Can we instead expose only the required functionality from ClusterService instead the whole of TaskResourceTrackingService? and/or thinking this maybe should be a separate interface for plugins not exposed through ClusterService?

We can do a wrapper of refreshResourceStats in clusterservice, but one argument could be, does it belong to cluster service? In the right level of encapsulation, task level resource usages related operations should only belong to TaskResourceTrackingService. But I agree making the whole service public is also risky. I'm open to suggestions. cc @reta

@ansjcy the TaskResourceTrackingService is internal to OpenSearch (and has no relation to the ClusterService either), should not be exposed to the plugins. Regarding to the issue itself:

Currently we are not refreshing task level resource usages on coordinator node for searchTasks, which means all coordinator node resource usage will be 0.

That seem to be the problem that core implementation has to fix, why the task level resources are not refreshed (on coordinator node)?

Hi @reta ! thanks for the input. Currently the TaskResourceTrackingService only refreshes task usages when a task ends. But in our case we want to get the resource usage in a SearchOperationsListener, which will be triggered before a task finishes.

Let me think about this more. Instead of exposing the TaskResourceTrackingSevice, does it make sense to you If we can do a usage refresh in core before the listeners are called (in this function: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java#L469)?

Thanks @ansjcy

thanks for the input. Currently the TaskResourceTrackingService only refreshes task usages when a task ends.

It does not sound right, the task is still ongoing so its usage won't be correct.

Let me think about this more. Instead of exposing the TaskResourceTrackingSevice, does it make sense to you If we can do a usage refresh in core before the listeners are called (in this function:

The logical point (at least to me) of capturing task usages seems to be the moment task ends. It looks to me you are trying to chime in somewhere in between (while task is still executing), that does not look like the correct way.

No, we add per-request listener instance in the TransportSearchAction::executeRequest. The query insights plugin has nothing to do with it at this moment of time, but we capture the search task resource usage upon request completion, so the tracking data becomes available to everyone (including the query insights plugin). Does it make sense?

@reta We are currently working on a PR based on the above discussion.

For the last point you made, what other justification and work is required to make the API public? We are trying to get all the query insights changes in 2.16 and this is the only PR that is dangling currently. Want to make sure we reach a path forward. Please let us know your suggestions. In the meantime will finalize the above draft PR if there are not concerns with this approach?

Thanks @deshsidd

For the last point you made, what other justification and work is required to make the API public?

I did not design the original APIs, you may ask the contributor if he has any concerns. On the second point, if you need to make it public, apply the @PublicApi annotation accordingly.

Thanks @reta!
cc @buddharajusahil @sgup432 @dzane17 @jainankitk Please let us know your thoughts since you all have contributed to the file.

@reta Looks like you had initially reduced the visibility of the API.

For now I am going to continue with the approach that Reta and Chenyang had discussed above and work on the following PR. Will also make the SearchRequestOperationsListener @publicapi as part of these changes.

@reta Looks like you had initially reduced the visibility of the API.

@deshsidd yes, you will understand why once try to apply @PublicApi to it :-) : it pulls a pile of dependencies with it .... (I am very doubtful it was designed as being public in the first place).

Understood.
@reta and others - Please take a look : #14832 and let me know your thoughts.

jed326 · 2024-07-17T17:27:16Z

server/src/main/java/org/opensearch/node/Node.java

@@ -1109,6 +1109,7 @@ protected Node(
                clusterService.getClusterSettings(),
                threadPool
            );
+            clusterService.setTaskResourceTrackingService(taskResourceTrackingService);


Should we just initialize TaskResourceTrackingService in ClusterService then as we're creating a dependency here anyways.

Also should SetOnce be used?

deshsidd · 2024-07-18T21:17:14Z

Closing in favor of : #14832

ansjcy requested review from anasalkouz, andrross, ashking94, Bukhtawar, CEHENKLE, dblock, dbwiddis, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami and VachaShah as code owners July 9, 2024 01:36

ansjcy added backport 2.x Backport to 2.x branch skip-changelog labels Jul 9, 2024

[bug fix] fix incorrect coordinator node search resource usages

d1a6038

Signed-off-by: Chenyang Ji <cyji@amazon.com>

ansjcy force-pushed the bugfix-insights-coordinator-resource branch from 31a955e to d1a6038 Compare July 9, 2024 04:48

jainankitk reviewed Jul 9, 2024

View reviewed changes

fix bug on serialization when passing task resource usage to coordinator

d00b2df

Signed-off-by: Chenyang Ji <cyji@amazon.com>

ansjcy force-pushed the bugfix-insights-coordinator-resource branch from bfccfa5 to d00b2df Compare July 9, 2024 17:59

deshsidd approved these changes Jul 9, 2024

View reviewed changes

deshsidd reviewed Jul 9, 2024

View reviewed changes

jainankitk reviewed Jul 9, 2024

View reviewed changes

add more unit tests

720cefb

Signed-off-by: Chenyang Ji <cyji@amazon.com>

jainankitk approved these changes Jul 9, 2024

View reviewed changes

ansjcy mentioned this pull request Jul 12, 2024

sync bug fixes from core to the plugin repo opensearch-project/query-insights#13

Merged

remove query insights plugin related code

14ff852

Signed-off-by: Chenyang Ji <cyji@amazon.com>

ansjcy changed the title ~~[bug fix] fix incorrect coordinator node search resource usages~~ add task resource tracking service to cluster service Jul 12, 2024

dzane17 approved these changes Jul 12, 2024

View reviewed changes

mch2 reviewed Jul 15, 2024

View reviewed changes

ansjcy mentioned this pull request Jul 15, 2024

[RELEASE] Release version 2.16.0 opensearch-project/opensearch-build#4771

Closed

74 tasks

jed326 reviewed Jul 17, 2024

View reviewed changes

reta mentioned this pull request Jul 18, 2024

Create listener to refresh search thread resource usage #14832

Merged

3 tasks

deshsidd closed this Jul 18, 2024

This was referenced Sep 11, 2024

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreStatsIT #14310

Open

[AUTOCUT] Gradle Check Flaky Test Report for SearchRestCancellationIT #14311

Open

[AUTOCUT] Gradle Check Flaky Test Report for SegmentReplicationUsingRemoteStoreIT #14329

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add task resource tracking service to cluster service #14681

add task resource tracking service to cluster service #14681

ansjcy commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

codecov bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024

jainankitk Jul 9, 2024

ansjcy Jul 9, 2024

ansjcy Jul 9, 2024

deshsidd Jul 9, 2024 •

edited

Loading

jainankitk Jul 9, 2024

jainankitk Jul 9, 2024

deshsidd commented Jul 9, 2024

deshsidd Jul 9, 2024

ansjcy Jul 9, 2024

deshsidd Jul 9, 2024

jainankitk left a comment

github-actions bot commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

jainankitk left a comment

github-actions bot commented Jul 12, 2024

mch2 Jul 15, 2024

ansjcy Jul 15, 2024 •

edited

Loading

reta Jul 16, 2024 •

edited

Loading

ansjcy Jul 16, 2024 •

edited

Loading

reta Jul 16, 2024 •

edited

Loading

deshsidd Jul 18, 2024

reta Jul 18, 2024 •

edited

Loading

deshsidd Jul 18, 2024

reta Jul 18, 2024

deshsidd Jul 18, 2024

jed326 Jul 17, 2024

deshsidd commented Jul 18, 2024

		this.attributes = in.readMap(Attribute::readFromStream, StreamInput::readGenericValue);
		this.attributes = Attribute.readAttributeMap(in);

add task resource tracking service to cluster service #14681

add task resource tracking service to cluster service #14681

Conversation

ansjcy commented Jul 9, 2024 • edited Loading

Description

Related Issues

Check List

github-actions bot commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

github-actions bot commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deshsidd Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deshsidd commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainankitk left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

jainankitk left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 12, 2024

Choose a reason for hiding this comment

ansjcy Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

reta Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

ansjcy Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

reta Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Jul 18, 2024 • 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

deshsidd commented Jul 18, 2024

ansjcy commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

deshsidd Jul 9, 2024 •

edited

Loading

ansjcy Jul 15, 2024 •

edited

Loading

reta Jul 16, 2024 •

edited

Loading

ansjcy Jul 16, 2024 •

edited

Loading

reta Jul 16, 2024 •

edited

Loading

reta Jul 18, 2024 •

edited

Loading