AdmissionControl Split RCA #69

mitalawachat · 2021-09-14T06:13:55Z

Signed-off-by: Mital Awachat [email protected]

Is your feature request related to a problem? Please provide an existing Issue # , or describe.

Split admission control rca into SmallHeap / MediumHeap / LargeHeap rca.
Reduce number of buckets for default configuration request-size controller heap-range to reduce too many auto-tunes for small percentage of heap variation.

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.

codecov-commenter · 2021-09-14T06:27:18Z

Codecov Report

Merging #69 (38971d5) into main (cdcae41) will decrease coverage by 0.03%.
The diff coverage is 77.27%.

❗ Current head 38971d5 differs from pull request most recent head cacbf30. Consider uploading reports for the commit cacbf30 to get more accurate results

@@             Coverage Diff              @@
##               main      #69      +/-   ##
============================================
- Coverage     72.15%   72.11%   -0.04%     
- Complexity     2928     2942      +14     
============================================
  Files           371      376       +5     
  Lines         18627    18680      +53     
  Branches       1415     1429      +14     
============================================
+ Hits          13440    13472      +32     
- Misses         4612     4624      +12     
- Partials        575      584       +9

Impacted Files	Coverage Δ
.../decisionmaker/actions/AdmissionControlAction.java	`80.35% <0.00%> (-1.47%)`	⬇️
...ceanalyzer/metrics/PerformanceAnalyzerMetrics.java	`37.97% <0.00%> (ø)`
...nalyzer/rca/configs/AdmissionControlRcaConfig.java	`0.00% <ø> (ø)`
...ensearch/performanceanalyzer/util/range/Range.java	`55.55% <ø> (ø)`
.../util/range/RequestSizeHeapRangeConfiguration.java	`94.11% <ø> (ø)`
...a/store/rca/admissioncontrol/model/HeapMetric.java	`63.63% <63.63%> (ø)`
...ecisionmaker/deciders/AdmissionControlDecider.java	`79.06% <69.23%> (-8.11%)`	⬇️
...ioncontrol/heap/AdmissionControlByHeapFactory.java	`76.92% <76.92%> (ø)`
...sioncontrol/heap/AdmissionControlByMediumHeap.java	`80.64% <80.64%> (ø)`
...tore/rca/admissioncontrol/AdmissionControlRca.java	`63.79% <82.60%> (-2.07%)`	⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdcae41...cacbf30. Read the comment docs.

sruti1312 · 2021-09-20T22:14:57Z

pa_config/rca.conf

            "upper-bound": 80,
-            "threshold": 12.5
+            "threshold": 15.0


Can we have some comments or more information in the PR's description on how these values are arrived at or more appropriate?

Updated description, and also added in code comments.

sruti1312 · 2021-09-20T22:17:32Z

...a/org/opensearch/performanceanalyzer/rca/store/rca/admissioncontrol/AdmissionControlRca.java

-        return metricValue.get();
+    private <M extends Metric> double getMetric(M metric, Field<String> field, String fieldName) {
+        double response = 0;
+        for (MetricFlowUnit flowUnit : metric.getFlowUnits()) {


Can we run as ./gradlew build as it will fix spotless bugs if any.

sruti1312 · 2021-09-20T22:20:20Z

...a/org/opensearch/performanceanalyzer/rca/store/rca/admissioncontrol/heap/HeapRcaFactory.java

+    private static final String small = "small";
+    private static final String medium = "medium";
+    private static final String large = "large";


static final variables names usually follow upper cases. Also can we be more descriptive with the variable name? SMALL_HEAP?

sruti1312 · 2021-09-22T00:57:02Z

In order to avoid issues like rca flowunits not reaching the deciders that we encountered recently, can we add gauntlet tests to confirm that end-to-end workflow is working as expected?

Also, please add testing details to the PR.

sruti1312 · 2021-09-22T00:59:03Z

...a/org/opensearch/performanceanalyzer/rca/store/rca/admissioncontrol/AdmissionControlRca.java

-                        / BYTES_TO_MEGABYTES;
-        return heapMetrics;
+    private HeapMetric getHeapMetric() {
+        double usedHeap =


Can we name this variables usedHeapInGb?

sruti1312 · 2021-09-22T01:01:27Z

...va/org/opensearch/performanceanalyzer/rca/store/rca/admissioncontrol/heap/MediumHeapRca.java

+
+    private double getThreshold(RangeConfiguration heapRange, double heapPercent) {
+        Range range = heapRange.getRange(heapPercent);
+        return Objects.isNull(range) ? 0 : range.getThreshold();


I think there are more spotless errors, can we fix them?

Successfully completes ./gradlew build

sruti1312 · 2021-09-22T01:14:04Z

One major concern I have is LargeHeapRca, MediumHeapRca and SmallHeapRca are inputs to AdmissionControlRca. Currently, we do not have rca's that reference each other directly. We want to have RCAs independent of each other and hook them up if needed using the analysis graph.

I think LargeHeapRca, MediumHeapRca and SmallHeapRca are more of utility classes that AdmissionControlRca uses for emitting its status flowunits. If yes, can we make them as util classes. One another alternative is to keep them as RCA's and remove the admission control rca.

sruti1312 · 2021-09-22T01:15:28Z

...a/org/opensearch/performanceanalyzer/rca/store/rca/admissioncontrol/AdmissionControlRca.java

-    private double getHeapBasedThreshold(double currentHeapPercent) {
-        Range range = requestSizeHeapRange.getRange(currentHeapPercent);
-        return Objects.isNull(range) ? 0 : range.getThreshold();
+        HeapRca heapRca = HeapRcaFactory.getHeapRca(heapMetric.getMaxHeap());


The max heap value is going to stay the same, can we make this as a one time call?

As PA runs outside of ES JVM, reading max heap here as it might change.

Signed-off-by: Mital Awachat <[email protected]>

* Remove emitting the entry key with the METRICS_WRITE_ERROR metric (#71) Signed-off-by: Sruti Parthiban <[email protected]> * Update commons-io version (#73) Signed-off-by: Sruti Parthiban <[email protected]> * AdmissionControl Split RCA (#69) Signed-off-by: Mital Awachat <[email protected]> Co-authored-by: Mital Awachat <[email protected]> Co-authored-by: Sruti Parthiban <[email protected]> Co-authored-by: Mital Awachat <[email protected]>

mitalawachat force-pushed the feature/admission-control/split-rca branch 4 times, most recently from 7921123 to ef38fe2 Compare September 16, 2021 06:03

sruti1312 reviewed Sep 20, 2021

View reviewed changes

mitalawachat force-pushed the feature/admission-control/split-rca branch from ef38fe2 to c297942 Compare September 21, 2021 05:20

mitalawachat requested a review from sruti1312 September 21, 2021 05:23

sruti1312 reviewed Sep 22, 2021

View reviewed changes

AdmissionControl Split RCA

cacbf30

Signed-off-by: Mital Awachat <[email protected]>

mitalawachat force-pushed the feature/admission-control/split-rca branch from c297942 to cacbf30 Compare September 22, 2021 15:36

mitalawachat requested a review from sruti1312 September 23, 2021 03:13

sruti1312 approved these changes Sep 24, 2021

View reviewed changes

sruti1312 merged commit e4a6261 into opensearch-project:main Sep 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AdmissionControl Split RCA #69

AdmissionControl Split RCA #69

mitalawachat commented Sep 14, 2021 •

edited

Loading

codecov-commenter commented Sep 14, 2021 •

edited

Loading

sruti1312 Sep 20, 2021

mitalawachat Sep 21, 2021

sruti1312 Sep 20, 2021

mitalawachat Sep 21, 2021

sruti1312 Sep 20, 2021

mitalawachat Sep 21, 2021

sruti1312 commented Sep 22, 2021

sruti1312 Sep 22, 2021

sruti1312 Sep 22, 2021

mitalawachat Sep 23, 2021

sruti1312 commented Sep 22, 2021

sruti1312 Sep 22, 2021

mitalawachat Sep 23, 2021

AdmissionControl Split RCA #69

AdmissionControl Split RCA #69

Conversation

mitalawachat commented Sep 14, 2021 • edited Loading

Check List

codecov-commenter commented Sep 14, 2021 • edited Loading

Codecov Report

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 Sep 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sruti1312 commented Sep 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitalawachat commented Sep 14, 2021 •

edited

Loading

codecov-commenter commented Sep 14, 2021 •

edited

Loading