From 1b4290f5b10bc1c82ca082943053f9fad7b4a6bf Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 09:17:21 -0700 Subject: [PATCH] Address review comments Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizer.java | 6 ++-- .../SearchQueryCategorizingVisitor.java | 2 +- .../action/search/SearchQueryCounters.java | 33 ++++++++++--------- .../action/search/TransportSearchAction.java | 2 +- .../index/query/QueryShapeVisitor.java | 2 +- .../search/SearchQueryCategorizerTests.java | 7 ++-- .../index/query/QueryShapeVisitorTests.java | 2 +- 7 files changed, 28 insertions(+), 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 index 1049cf18a0ff1..9cbe2d2ffcb7d 100644 --- 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> sorts) { if (sorts != null && sorts.size() > 0) { - for (ListIterator> it = sorts.listIterator(); it.hasNext(); ) { + for (ListIterator> 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 index f95c28a1d11e3..98f0169e69a5c 100644 --- 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 index 441c32256ef4a..7e0259af07701 100644 --- 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 index 8e36352e58b8b..a6fb8453af4ff 100644 --- 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 index 1159d40bb3e35..e484da0f48a2c 100644 --- 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 queryType = new SetOnce<>(); private final Map> 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 index 568ab5a7dbc8c..30c9937c98abc 100644 --- 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 index 561025e197b0b..53875cf078941 100644 --- 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")))