Task consumer Integration #2293

sruti1312 · 2022-03-01T04:18:12Z

Signed-off-by: Sruti Parthiban partsrut@amazon.com

Description

Integrate task consumers to capture task resource information during unregister.
Add consumer that logs topN expensive search tasks

[2022-02-28T20:12:26,811][INFO ][t.detailslog             ] [runTask-0] taskId[34], type[direct], action[indices:data/read/search[phase/query]], description[shardId[[shakespeare][0]]], start_time_millis[1646107], resource_stats[{memory=100, cpu=100}], metadata[source[{"query":{"query_string":{"query":"king","fields":[],"type":"best_fields","default_operator":"or","max_determinized_states":10000,"enable_position_increments":true,"fuzziness":"AUTO","fuzzy_prefix_length":0,"fuzzy_max_expansions":50,"phrase_slop":0,"analyze_wildcard":false,"escape":false,"auto_generate_synonyms_phrase_query":true,"fuzzy_transpositions":true,"boost":1.0}}}]],

Issues Resolved

#1009

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

opensearch-ci-bot · 2022-03-01T04:20:03Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2022-03-01T04:45:15Z

❌ Gradle Check failure 96388abbca9e2e1c51bb8e1042fbe05a5daa55dc
Log 2892

Reports 2892

server/src/main/java/org/opensearch/tasks/consumer/TopNSearchTaskTracker.java

server/src/main/java/org/opensearch/search/internal/InternalScrollSearchRequest.java

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

server/src/main/java/org/opensearch/tasks/consumer/TopNSearchTaskTracker.java

dblock · 2022-03-21T20:11:14Z

@sruti1312 Take a look at the comments above? @kartg do a CR?

opensearch-ci-bot · 2022-03-22T18:19:44Z

❌ Gradle Check failure 778463abb7369c03397ba48df27b74b519046c9e
Log 3672

Reports 3672

opensearch-ci-bot · 2022-03-25T00:31:52Z

❌ Gradle Check failure 44409d314e27fd683c59b7a36a4540e357333e84
Log 3746

Reports 3746

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

andrross · 2022-03-28T18:59:21Z

server/src/main/java/org/opensearch/tasks/consumer/TopNSearchTasksLogger.java

+        }
+    }
+
+    // TODO: Need performance testing results to understand if we can to use synchronized here.


Does this TODO need to be answered before merging this code? It looks like you're merging this into a feature branch. What is the ultimate plan for the feature branch?

server/src/test/java/org/opensearch/tasks/consumer/TopNSearchTasksLoggerTests.java

andrross · 2022-03-28T19:08:08Z

server/src/main/java/org/opensearch/action/search/SearchShardTask.java

+    // generating metadata in a lazy way since source can be quite big
+    private final Supplier<String> metadataSupplier;


Do you know for sure that doing this lazily is the right approach? It adds complexity and actually results in more work if SearchShardTask#getTaskMetadata happens to be called more than once. The optimal approach would be to memoize the result of the supplier, but that adds more complexity, so I'd suggest doing this only if you know there is a non-trivial perf benefit.

Same/similar question from me - is there a reason why this can't be a simple String instead of a Supplier ?

For SearchShardTask we want to log the query string to be able to correlate the top resource consumers with the query. We have seen query strings can be really huge and with high search workload it can be very expensive to construct it for each shard task. Also depending on the consumers type it may not be needed to get the query string for each shard tasks. Hence, depending upon the consumers (for example in this case TopNConsumer) it will only call this method on the Tasks which are of interest and not on all the Task object.

I agree with the memoization approach but given at the moment there is only one caller expected for this method seems like we can do that optimization as a follow-up ?

As @sohami pointed out this method will only be added once (from the consumer and if query is top resource consumer), there are no other callers to this method. In future if this method is going to be called more than once, we can extend to memoize the result of this supplier. Adding a comment for future reference.

Signed-off-by: sruti1312 <srutiparthiban@gmail.com>

opensearch-ci-bot · 2022-03-29T22:01:51Z

❌ Gradle Check failure eeb754a
Log 3877

Reports 3877

opensearch-ci-bot · 2022-03-30T00:46:23Z

❌ Gradle Check failure c16db7a6d01d62a885caf40646b9aa87b1c5299d
Log 3881

Reports 3881

dblock

This looks good to me overall, made some suggestions. Talk me out of the metadata part, looks like a hack.

Try to get the build to green, address @andrross's comments, etc.

distribution/src/config/log4j2.properties

server/src/main/java/org/opensearch/tasks/consumer/TaskDetailsLogMessage.java

server/src/test/java/org/opensearch/tasks/consumer/TopNSearchTasksLoggerTests.java

server/src/main/java/org/opensearch/tasks/consumer/TopNSearchTasksLogger.java

server/src/test/java/org/opensearch/tasks/consumer/TopNSearchTasksLoggerTests.java

opensearch-ci-bot · 2022-03-31T23:40:56Z

✅ Gradle Check success e9ed69f
Log 3994

Reports 3994

kartg · 2022-04-04T22:02:27Z

server/src/main/java/org/opensearch/action/search/SearchShardTask.java

+    // generating metadata in a lazy way since source can be quite big
+    private final Supplier<String> metadataSupplier;


Same/similar question from me - is there a reason why this can't be a simple String instead of a Supplier ?

server/src/main/java/org/opensearch/tasks/consumer/TaskDetailsLogMessage.java

kartg · 2022-04-04T22:05:47Z

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

+        this.taskResourceConsumer = new ArrayList<Consumer<Task>>() {
+            {
+                add(new TopNSearchTasksLogger(settings));
+            }
+        };


Nitpick - gate this with a check on taskResourceConsumersEnabled ?

Adding TASK_RESOURCE_CONSUMER_SETTING to clusterSetting to enable/disable as required. Also adding a check to see if enabled before publishing to consumers. Do you still think we need to include a gate check here?

As we continue to add more consumers what might help is a control knob per consumer instead of a blanket level knob

@Bukhtawar When we have more consumers, it makes sense to have control knob per consumer! I think blanket level will also be helpful to control all consumers using one knob (like parent level). Going forward we can add the control knobs per consumer. Any thoughts?

sohami · 2022-04-05T02:31:58Z

server/src/main/java/org/opensearch/action/search/SearchShardTask.java

+    // generating metadata in a lazy way since source can be quite big
+    private final Supplier<String> metadataSupplier;


For SearchShardTask we want to log the query string to be able to correlate the top resource consumers with the query. We have seen query strings can be really huge and with high search workload it can be very expensive to construct it for each shard task. Also depending on the consumers type it may not be needed to get the query string for each shard tasks. Hence, depending upon the consumers (for example in this case TopNConsumer) it will only call this method on the Tasks which are of interest and not on all the Task object.

I agree with the memoization approach but given at the moment there is only one caller expected for this method seems like we can do that optimization as a follow-up ?

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

sohami · 2022-04-05T02:51:34Z

server/src/main/java/org/opensearch/tasks/consumer/TopNSearchTasksLogger.java

+        }
+    }
+
+    private synchronized void recordSearchTask(final Task searchTask) {


I think instead of making this method synchronized we should have a queue to hold on to all the tasks which are passed to the consumer. Then a background thread should clear off that queue and manager the topN queue out of that. The reason being unregister can be called on network thread and if logging takes long time (because of configuration or logging module issue) then we will end up blocking the network thread which can result into other issues in the cluster.

sruti1312 · 2022-04-07T01:21:57Z

Performance test results

Workload configuration details

Key	Value
dataset	nyc_taxis
warmupIterations	2
testIterations	3
primaryShards	5
replicaShards	1

Cluster with consumer enabled

Latency (ms)

Operation Type	P50	P90	P99	P100
index	4,761	13,287.1	24,975.9	60,989
query	120	139.1	194.7	233.8

Throughput (req/s)

Operation Type	P0	P50	P100
index	12,153.6	13,335.1	22,659
query	1.543	1.693	1.723

Operation Counts

Operation Type	Op Count	Op Error Count	Error Rate
index	48,272	18	0%
query	1,510	0	0

Usage (%)

Resource	P50	P90	P99	P100
CPU	0	38	64	95
Memory	11.22	61	73.476	78

Cluster with consumer disabled

Latency (ms)

Operation Type	P50	P90	P99	P100
index	4,515.6	11,294.7	24,493.5	61,004
query	112.9	130.6	204.2	261.8

Throughput (req/s)

Operation Type	P0	P50	P100
index	11,058.9	14,221.6	24,538.2
query	1.574	1.695	1.726

Operation Counts

Operation Type	Op Count	Op Error Count	Error Rate
index	47,970	8	0%
query	1,510	0	0

Usage (%)

Resource	P50	P90	P99	P100
CPU	0	46	75	97
Memory	10.96	62	73.85	79

kotwanikunal · 2022-06-14T19:31:55Z

@sruti1312 Can you please address the comments? It would be great to get this PR to a closure.

JeffHuss · 2022-07-21T21:43:20Z

Is this PR going to target the 2.2 release or a later version? I just want to make sure the associated doc ticket for this functionality is accurate and updated so we don't miss the deadline.

github-actions · 2022-08-05T01:04:39Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/1469/
CommitID: 3ad68573e94ed920f0c113b1597d42355214dd0c

Signed-off-by: sruti1312 <srutiparthiban@gmail.com>

github-actions · 2022-08-05T01:11:05Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/1470/
CommitID: 52a616129dea7434ac957882cce432330cc2f6a9

github-actions · 2022-08-05T01:36:52Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/1472/
CommitID: 5f3c262

kartg · 2022-08-05T04:09:58Z

@Bukhtawar can you take a look and merge this if no blockers remain? I believe @sruti1312 would like to get this into 2.2

* Integrate task consumers to capture task resource information during unregister. Add consumer that logs topN expensive search tasks Signed-off-by: sruti1312 <srutiparthiban@gmail.com> (cherry picked from commit fbe93d4)

* Integrate task consumers to capture task resource information during unregister. Add consumer that logs topN expensive search tasks Co-authored-by: Sruti Parthiban <srutiparthiban@gmail.com>

* Integrate task consumers to capture task resource information during unregister. Add consumer that logs topN expensive search tasks Signed-off-by: sruti1312 <srutiparthiban@gmail.com>

sruti1312 requested a review from a team as a code owner March 1, 2022 04:18

sruti1312 changed the title ~~Task consumer Integration~~ [DRAFT] Task consumer Integration Mar 1, 2022

dblock requested a review from kartg March 1, 2022 16:34

sgup432 reviewed Mar 3, 2022

View reviewed changes

sruti1312 force-pushed the feature/consumer-integration branch from 96388ab to 778463a Compare March 22, 2022 18:00

sruti1312 force-pushed the feature/consumer-integration branch from 778463a to 44409d3 Compare March 25, 2022 00:17

sruti1312 changed the title ~~[DRAFT] Task consumer Integration~~ Task consumer Integration Mar 25, 2022

andrross reviewed Mar 28, 2022

View reviewed changes

Add consumer integration

eeb754a

Signed-off-by: sruti1312 <srutiparthiban@gmail.com>

sruti1312 force-pushed the feature/consumer-integration branch from 44409d3 to eeb754a Compare March 29, 2022 21:13

sruti1312 changed the base branch from feature/task-resource-tracking to main March 30, 2022 00:30

dblock reviewed Mar 30, 2022

View reviewed changes

andrross reviewed Mar 30, 2022

View reviewed changes

sruti1312 force-pushed the feature/consumer-integration branch from c16db7a to e9ed69f Compare March 31, 2022 22:55

kartg reviewed Apr 4, 2022

View reviewed changes

sohami reviewed Apr 5, 2022

View reviewed changes

hdhalter mentioned this pull request Jul 12, 2022

[Doc] Server-side rejection of search requests based on resource opensearch-project/documentation-website#795

Closed

JeffHuss mentioned this pull request Jul 19, 2022

[BUG] Document task consumer integration opensearch-project/documentation-website#819

Closed

sruti1312 requested a review from reta as a code owner August 3, 2022 19:39

andrross approved these changes Aug 5, 2022

View reviewed changes

Add static method to create taskManager

5f3c262

Signed-off-by: sruti1312 <srutiparthiban@gmail.com>

sruti1312 force-pushed the feature/consumer-integration branch from 52a6161 to 5f3c262 Compare August 5, 2022 01:10

kartg approved these changes Aug 5, 2022

View reviewed changes

kartg added backport 2.x Backport to 2.x branch v2.2.0 backport 2.2 Backport to 2.2 branch labels Aug 5, 2022

Bukhtawar approved these changes Aug 5, 2022

View reviewed changes

Bukhtawar merged commit fbe93d4 into opensearch-project:main Aug 5, 2022

opensearch-trigger-bot bot mentioned this pull request Aug 5, 2022

[Backport 2.x] Task consumer Integration #4140

Merged

opensearch-trigger-bot bot mentioned this pull request Aug 5, 2022

[Backport 2.2] Task consumer Integration #4141

Merged

sruti1312 deleted the feature/consumer-integration branch August 5, 2022 15:51

This was referenced Mar 16, 2023

[BUG] TopNSearchTasksLogger settings are not recognised #6708

Closed

Add TopNSearchTasksLogger settings to Cluster Settings #6716

Merged

This was referenced Apr 13, 2023

[BUG] Increased search latency with Task Resource Consumer enabled #7146

Open

Add TopNSearchTasksLogger settings to Cluster Settings (#6716) #7241

Merged

Add TopNSearchTasksLogger settings to Cluster Settings (#6716) #7242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task consumer Integration #2293

Task consumer Integration #2293

sruti1312 commented Mar 1, 2022 •

edited

Loading

opensearch-ci-bot commented Mar 1, 2022

opensearch-ci-bot commented Mar 1, 2022

dblock commented Mar 21, 2022

opensearch-ci-bot commented Mar 22, 2022

opensearch-ci-bot commented Mar 25, 2022

andrross Mar 28, 2022

andrross Mar 28, 2022

kartg Apr 4, 2022

sohami Apr 5, 2022

sruti1312 Aug 3, 2022

opensearch-ci-bot commented Mar 29, 2022

opensearch-ci-bot commented Mar 30, 2022

dblock left a comment •

edited

Loading

opensearch-ci-bot commented Mar 31, 2022

kartg Apr 4, 2022

kartg Apr 4, 2022

sruti1312 Aug 3, 2022

Bukhtawar Aug 4, 2022

sruti1312 Aug 5, 2022

sohami Apr 5, 2022

sohami Apr 5, 2022

sruti1312 commented Apr 7, 2022

kotwanikunal commented Jun 14, 2022

JeffHuss commented Jul 21, 2022

github-actions bot commented Aug 5, 2022

github-actions bot commented Aug 5, 2022

github-actions bot commented Aug 5, 2022

kartg commented Aug 5, 2022 •

edited

Loading

		// generating metadata in a lazy way since source can be quite big
		private final Supplier<String> metadataSupplier;

Task consumer Integration #2293

Task consumer Integration #2293

Conversation

sruti1312 commented Mar 1, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Mar 1, 2022

opensearch-ci-bot commented Mar 1, 2022

dblock commented Mar 21, 2022

opensearch-ci-bot commented Mar 22, 2022

opensearch-ci-bot commented Mar 25, 2022

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

opensearch-ci-bot commented Mar 29, 2022

opensearch-ci-bot commented Mar 30, 2022

dblock left a comment • edited Loading

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 31, 2022

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

sruti1312 commented Apr 7, 2022

Performance test results

Workload configuration details

Cluster with consumer enabled

Cluster with consumer disabled

kotwanikunal commented Jun 14, 2022

JeffHuss commented Jul 21, 2022

github-actions bot commented Aug 5, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 5, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Aug 5, 2022

Gradle Check (Jenkins) Run Completed with:

kartg commented Aug 5, 2022 • edited Loading

sruti1312 commented Mar 1, 2022 •

edited

Loading

dblock left a comment •

edited

Loading

kartg commented Aug 5, 2022 •

edited

Loading