Address review comments

Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>
deshsidd · Oct 19, 2023 · 1b4290f · 1b4290f
1 parent 2eb4a3d
commit 1b4290f
Show file tree

Hide file tree

Showing 7 changed files with 28 additions and 26 deletions.
diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java
@@ -26,11 +26,11 @@
  * Class to categorize the search queries based on the type and increment the relevant counters.
  * Class also logs the query shape.
  */
-public final class SearchQueryCategorizer {
+final class SearchQueryCategorizer {
 
     private static final Logger log = LogManager.getLogger(SearchQueryCategorizer.class);
 
-    public final SearchQueryCounters searchQueryCounters;
+    final SearchQueryCounters searchQueryCounters;
 
     public SearchQueryCategorizer(MetricsRegistry metricsRegistry) {
         searchQueryCounters = new SearchQueryCounters(metricsRegistry);
@@ -47,7 +47,7 @@ public void categorize(SearchSourceBuilder source) {
 
     private void incrementQuerySortCounters(List<SortBuilder<?>> sorts) {
         if (sorts != null && sorts.size() > 0) {
-            for (ListIterator<SortBuilder<?>> it = sorts.listIterator(); it.hasNext(); ) {
+            for (ListIterator<SortBuilder<?>> it = sorts.listIterator(); it.hasNext();) {
                 SortBuilder sortBuilder = it.next();
                 String sortOrder = sortBuilder.order().toString();
                 searchQueryCounters.sortCounter.add(1, Tags.create().addTag("sort_order", sortOrder));

diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java
@@ -27,7 +27,7 @@
  * Class to visit the querybuilder tree and also track the level information.
  * Increments the counters related to Search Query type.
  */
-public class SearchQueryCategorizingVisitor implements QueryBuilderVisitor {
+final class SearchQueryCategorizingVisitor implements QueryBuilderVisitor {
     private static final String LEVEL_TAG = "level";
     private final int level;
     private final SearchQueryCounters searchQueryCounters;

diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java
@@ -14,7 +14,8 @@
 /**
  * Class contains all the Counters related to search query types.
  */
-public class SearchQueryCounters {
+final class SearchQueryCounters {
+    private static final String UNIT = "1";
     private final MetricsRegistry metricsRegistry;
 
     // Counters related to Query types
@@ -40,77 +41,77 @@ public SearchQueryCounters(MetricsRegistry metricsRegistry) {
         this.aggCounter = metricsRegistry.createCounter(
             "search.query.type.agg.count",
             "Counter for the number of top level agg search queries",
-            "1"
+            UNIT
         );
         this.boolCounter = metricsRegistry.createCounter(
             "search.query.type.bool.count",
             "Counter for the number of top level and nested bool search queries",
-            "1"
+            UNIT
         );
         this.functionScoreCounter = metricsRegistry.createCounter(
             "search.query.type.functionscore.count",
             "Counter for the number of top level and nested function score search queries",
-            "1"
+            UNIT
         );
         this.matchCounter = metricsRegistry.createCounter(
             "search.query.type.match.count",
             "Counter for the number of top level and nested match search queries",
-            "1"
+            UNIT
         );
         this.matchPhrasePrefixCounter = metricsRegistry.createCounter(
             "search.query.type.matchphrase.count",
             "Counter for the number of top level and nested match phrase prefix search queries",
-            "1"
+            UNIT
         );
         this.multiMatchCounter = metricsRegistry.createCounter(
             "search.query.type.multimatch.count",
             "Counter for the number of top level and nested multi match search queries",
-            "1"
+            UNIT
         );
         this.otherQueryCounter = metricsRegistry.createCounter(
             "search.query.type.other.count",
             "Counter for the number of top level and nested search queries that do not match any other categories",
-            "1"
+            UNIT
         );
         this.queryStringQueryCounter = metricsRegistry.createCounter(
             "search.query.type.querystringquery.count",
             "Counter for the number of top level and nested queryStringQuery search queries",
-            "1"
+            UNIT
         );
         this.rangeCounter = metricsRegistry.createCounter(
             "search.query.type.range.count",
             "Counter for the number of top level and nested range search queries",
-            "1"
+            UNIT
         );
         this.regexCounter = metricsRegistry.createCounter(
             "search.query.type.regex.count",
             "Counter for the number of top level and nested regex search queries",
-            "1"
+            UNIT
         );
         this.skippedCounter = metricsRegistry.createCounter(
             "search.query.type.skipped.count",
             "Counter for the number queries skipped due to error",
-            "1"
+            UNIT
         );
         this.sortCounter = metricsRegistry.createCounter(
             "search.query.type.sort.count",
             "Counter for the number of top level sort search queries",
-            "1"
+            UNIT
         );
         this.termCounter = metricsRegistry.createCounter(
             "search.query.type.term.count",
             "Counter for the number of top level and nested term search queries",
-            "1"
+            UNIT
         );
         this.totalCounter = metricsRegistry.createCounter(
             "search.query.type.total.count",
             "Counter for the number of top level and nested search queries",
-            "1"
+            UNIT
         );
         this.wildcardCounter = metricsRegistry.createCounter(
             "search.query.type.wildcard.count",
             "Counter for the number of top level and nested wildcard search queries",
-            "1"
+            UNIT
         );
     }
 }
diff --git a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java
@@ -234,7 +234,7 @@ public TransportSearchAction(
 
     private void setSearchQueryMetricsEnabled(boolean searchQueryMetricsEnabled) {
         this.searchQueryMetricsEnabled = searchQueryMetricsEnabled;
-        if ((this.searchQueryMetricsEnabled == true ) && this.searchQueryCategorizer == null) {
+        if ((this.searchQueryMetricsEnabled == true) && this.searchQueryCategorizer == null) {
             this.searchQueryCategorizer = new SearchQueryCategorizer(metricsRegistry);
         }
     }

diff --git a/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java b/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java
@@ -20,7 +20,7 @@
 /**
  * Class to traverse the QueryBuilder tree and capture the query shape
  */
-public class QueryShapeVisitor implements QueryBuilderVisitor {
+final class QueryShapeVisitor implements QueryBuilderVisitor {
     private final SetOnce<String> queryType = new SetOnce<>();
     private final Map<BooleanClause.Occur, List<QueryShapeVisitor>> childVisitors = new EnumMap<>(BooleanClause.Occur.class);
 

diff --git a/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java b/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java
@@ -42,7 +42,7 @@
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.when;
 
-public class SearchQueryCategorizerTests extends OpenSearchTestCase {
+final class SearchQueryCategorizerTests extends OpenSearchTestCase {
 
     private MetricsRegistry metricsRegistry;
 
@@ -211,11 +211,12 @@ public void testComplexQuery() {
         TermQueryBuilder termQueryBuilder = QueryBuilders.termQuery("field", "value2");
         MatchQueryBuilder matchQueryBuilder = QueryBuilders.matchQuery("tags", "php");
         RegexpQueryBuilder regexpQueryBuilder = new RegexpQueryBuilder("field", "text");
-        BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder().must(termQueryBuilder).filter(matchQueryBuilder).should(regexpQueryBuilder);
+        BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder().must(termQueryBuilder)
+            .filter(matchQueryBuilder)
+            .should(regexpQueryBuilder);
         sourceBuilder.query(boolQueryBuilder);
         sourceBuilder.aggregation(new RangeAggregationBuilder("agg1").field("num"));
 
-
         searchQueryCategorizer.categorize(sourceBuilder);
 
         Mockito.verify(searchQueryCategorizer.searchQueryCounters.termCounter).add(eq(1.0d), any(Tags.class));

diff --git a/server/src/test/java/org/opensearch/index/query/QueryShapeVisitorTests.java b/server/src/test/java/org/opensearch/index/query/QueryShapeVisitorTests.java
@@ -12,7 +12,7 @@
 
 import static org.junit.Assert.assertEquals;
 
-public class QueryShapeVisitorTests extends OpenSearchTestCase {
+final class QueryShapeVisitorTests extends OpenSearchTestCase {
     public void testQueryShapeVisitor() {
         QueryBuilder builder = new BoolQueryBuilder().must(new TermQueryBuilder("foo", "bar"))
             .filter(new ConstantScoreQueryBuilder(new RangeQueryBuilder("timestamp").from("12345677").to("2345678")))