Adds the listener for resource utilization metrics #687

Gaganjuneja · 2024-07-20T12:13:12Z

Adds the listener for resource utilization metrics.

RTFPerformanceAnalyzerSearchListener - Emits the CPU_Utilization and Heap_used metrics for task level query shard operations (both query phase and the fetch phase).
RTFPerformanceAnalyzerTransportInterceptor - Emits the CPU_Utilization metric for BulkShardRequest.

Is your feature request related to a problem? Please provide an existing Issue # , or describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you are proposing
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Check List

Backport Labels added.
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.

Signed-off-by: Gagan Juneja <[email protected]>

src/main/java/org/opensearch/performanceanalyzer/PerformanceAnalyzerPlugin.java

...main/java/org/opensearch/performanceanalyzer/listener/PerformanceAnalyzerSearchListener.java

Signed-off-by: Gagan Juneja <[email protected]>

codecov · 2024-07-23T01:02:00Z

Codecov Report

Attention: Patch coverage is 72.54902% with 70 lines in your changes missing coverage. Please review.

Project coverage is 70.95%. Comparing base (9cb092f) to head (6899623).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...listener/RTFPerformanceAnalyzerSearchListener.java	64.39%	42 Missing and 5 partials ⚠️
...RTFPerformanceAnalyzerTransportRequestHandler.java	65.21%	11 Missing and 5 partials ⚠️
...performanceanalyzer/PerformanceAnalyzerPlugin.java	33.33%	2 Missing ⚠️
...analyzer/config/PerformanceAnalyzerController.java	0.00%	1 Missing ⚠️
...er/listener/PerformanceAnalyzerSearchListener.java	80.00%	0 Missing and 1 partial ⚠️
...rt/PerformanceAnalyzerTransportRequestHandler.java	80.00%	0 Missing and 1 partial ⚠️
...nsport/RTFPerformanceAnalyzerTransportChannel.java	97.91%	0 Missing and 1 partial ⚠️
...rt/RTFPerformanceAnalyzerTransportInterceptor.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #687      +/-   ##
============================================
+ Coverage     70.88%   70.95%   +0.07%     
- Complexity      421      478      +57     
============================================
  Files            49       53       +4     
  Lines          3125     3377     +252     
  Branches        194      217      +23     
============================================
+ Hits           2215     2396     +181     
- Misses          785      844      +59     
- Partials        125      137      +12

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

Signed-off-by: Gagan Juneja <[email protected]>

psychbot

There are some code coverage annotations, can we see if those can be addressed.

src/main/java/org/opensearch/performanceanalyzer/config/PerformanceAnalyzerController.java

...n/java/org/opensearch/performanceanalyzer/listener/RTFPerformanceAnalyzerSearchListener.java

psychbot · 2024-07-23T09:12:34Z

...n/java/org/opensearch/performanceanalyzer/listener/RTFPerformanceAnalyzerSearchListener.java

+        return isSearchListenerEnabled() ? this : NO_OP_SEARCH_LISTENER;
+    }
+
+    private boolean isSearchListenerEnabled() {


isSearchListenerEnabled seems to be a mandatory method for any SearchListeners implementation. Should we add it to the super class and all the subclasses needs to write the logic of this method.

We can do that. Didn't want to change a lot the existing stuff as anyways the plan is to bring this into core at some point in time.

...n/java/org/opensearch/performanceanalyzer/listener/RTFPerformanceAnalyzerSearchListener.java

psychbot · 2024-07-23T09:23:26Z

...n/java/org/opensearch/performanceanalyzer/listener/RTFPerformanceAnalyzerSearchListener.java

+
+    private Histogram createCPUUtilizationHistogram() {
+        MetricsRegistry metricsRegistry = OpenSearchResources.INSTANCE.getMetricsRegistry();
+        if (metricsRegistry != null) {


If the registry is null it does not make sense to create this listener, Do we need to throw exception instead of returning null?

No. We just want to stay silent.

.../opensearch/performanceanalyzer/transport/RTFPerformanceAnalyzerTransportRequestHandler.java

psychbot · 2024-07-23T12:04:11Z

src/main/java/org/opensearch/performanceanalyzer/util/Utils.java

@@ -108,4 +111,19 @@ public static HashMap<ShardId, IndexShard> getShards() {
                    IndexShardState.RECOVERING,
                    IndexShardState.POST_RECOVERY,
                    IndexShardState.STARTED);
+
+    public static double calculateCPUUtilization(


Can we add java doc explaining how we are calculating CPU Utilization

Can the cpuShareFactor be calculated here itself?

No. It's used from 2 places so should be generic.

src/main/java/org/opensearch/performanceanalyzer/util/Utils.java

Signed-off-by: Gagan Juneja <[email protected]>

psychbot · 2024-07-23T15:02:08Z

...n/java/org/opensearch/performanceanalyzer/listener/RTFPerformanceAnalyzerSearchListener.java

+            String operation,
+            boolean isFailed) {
+        long overallStartTime = fetchStartTime;
+        long queryTaskId = threadLocal.get().getOrDefault(QUERY_TASK_ID, 0l);


We can use -1 instead of 0.

psychbot · 2024-07-23T15:02:15Z

...n/java/org/opensearch/performanceanalyzer/listener/RTFPerformanceAnalyzerSearchListener.java

+            long overallStartTime,
+            long operationTime,


As these are being used in the context of search we can change operations to phase and change variable names.

overallStartTime -> startTime operationTime -> phaseTookTime operation -> phase

...ava/org/opensearch/performanceanalyzer/transport/RTFPerformanceAnalyzerTransportChannel.java

Signed-off-by: Gagan Juneja <[email protected]>

psychbot

LGTM!
Do we know the build failure reason and if that needs to be fixed?
If there is no open issue, can we create one and tag it to this PR.

* Adds the listener for resource utilization metrics Signed-off-by: Gagan Juneja <[email protected]> Co-authored-by: Gagan Juneja <[email protected]> (cherry picked from commit c95feb6)

* Adds the listener for resource utilization metrics Signed-off-by: Gagan Juneja <[email protected]> Co-authored-by: Gagan Juneja <[email protected]> (cherry picked from commit c95feb6) Co-authored-by: Gagan Juneja <[email protected]>

* [Feature] adds index_uuid as a tag in node stats all shard metrics (#680) (#689) * Adds index_uuid as a dimension for node stats col Signed-off-by: Atharva Sharma <[email protected]> * updates gauge usage and added index_uuid as tag Signed-off-by: Atharva Sharma <[email protected]> * removed incomplete collector Signed-off-by: Atharva Sharma <[email protected]> * fixed UTs after gauge changes Signed-off-by: Atharva Sharma <[email protected]> * addressed comments Signed-off-by: Atharva Sharma <[email protected]> * reverted gauge related changes Signed-off-by: Atharva Sharma <[email protected]> --------- Signed-off-by: Atharva Sharma <[email protected]> (cherry picked from commit 948b54a) Co-authored-by: Atharva Sharma <[email protected]> * Adds the listener for resource utilization metrics (#687) (#688) * Adds the listener for resource utilization metrics Signed-off-by: Gagan Juneja <[email protected]> Co-authored-by: Gagan Juneja <[email protected]> (cherry picked from commit c95feb6) Co-authored-by: Gagan Juneja <[email protected]> --------- Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: Atharva Sharma <[email protected]>

Adds the listener for resource utilization metrics

d7d5660

Signed-off-by: Gagan Juneja <[email protected]>

Gaganjuneja added the backport 2.x label Jul 20, 2024

Gaganjuneja requested review from khushbr, ansjcy, sbcd90, Shephalimittal, nishchay21, rramachand21 and psychbot as code owners July 20, 2024 12:13

nishchay21 reviewed Jul 22, 2024

View reviewed changes

src/main/java/org/opensearch/performanceanalyzer/PerformanceAnalyzerPlugin.java Show resolved Hide resolved

...main/java/org/opensearch/performanceanalyzer/listener/PerformanceAnalyzerSearchListener.java Show resolved Hide resolved

Gagan Juneja added 4 commits July 22, 2024 19:46

Fixes the same task issue

aad2bfd

Signed-off-by: Gagan Juneja <[email protected]>

Fixes the failing test

cf42943

Signed-off-by: Gagan Juneja <[email protected]>

Fixes the failing test

1cb5c43

Signed-off-by: Gagan Juneja <[email protected]>

Fixes the failing test

b137a47

Signed-off-by: Gagan Juneja <[email protected]>

Fixes the failing test

6899623

Signed-off-by: Gagan Juneja <[email protected]>

nishchay21 previously approved these changes Jul 23, 2024

View reviewed changes

psychbot reviewed Jul 23, 2024

View reviewed changes

Addresses the review comments

34ebcc4

Signed-off-by: Gagan Juneja <[email protected]>

Gaganjuneja dismissed nishchay21’s stale review via 34ebcc4 July 23, 2024 14:11

psychbot reviewed Jul 23, 2024

View reviewed changes

Addresses the review comments

590ba5d

Signed-off-by: Gagan Juneja <[email protected]>

psychbot approved these changes Jul 23, 2024

View reviewed changes

nishchay21 approved these changes Jul 23, 2024

View reviewed changes

nishchay21 merged commit c95feb6 into opensearch-project:main Jul 23, 2024
3 of 5 checks passed

opensearch-trigger-bot bot mentioned this pull request Jul 23, 2024

[Backport 2.x] Adds the listener for resource utilization metrics #688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the listener for resource utilization metrics #687

Adds the listener for resource utilization metrics #687

Gaganjuneja commented Jul 20, 2024

codecov bot commented Jul 23, 2024 •

edited

Loading

psychbot left a comment

psychbot Jul 23, 2024

Gaganjuneja Jul 23, 2024

psychbot Jul 23, 2024

Gaganjuneja Jul 23, 2024

psychbot Jul 23, 2024

psychbot Jul 23, 2024

Gaganjuneja Jul 23, 2024

psychbot Jul 23, 2024

psychbot Jul 23, 2024

psychbot left a comment

Adds the listener for resource utilization metrics #687

Adds the listener for resource utilization metrics #687

Conversation

Gaganjuneja commented Jul 20, 2024

Check List

codecov bot commented Jul 23, 2024 • edited Loading

Codecov Report

psychbot 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

psychbot left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2024 •

edited

Loading