From cd39650f7d09ad93a2bc95a29ebd681f4b71d774 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 27 Sep 2023 11:42:11 -0700 Subject: [PATCH 01/35] Search Query Categorizor initial skeleton using QueryBuilderVisitor Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizor.java | 49 +++++++++++++++++++ .../action/search/TransportSearchAction.java | 5 ++ 2 files changed, 54 insertions(+) create mode 100644 server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java new file mode 100644 index 0000000000000..bd2ee837d07cc --- /dev/null +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -0,0 +1,49 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.action.search; + +import org.apache.lucene.search.BooleanClause; +import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.MatchQueryBuilder; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.index.query.QueryBuilderVisitor; +import org.opensearch.index.query.QueryStringQueryBuilder; +import org.opensearch.search.builder.SearchSourceBuilder; + +public class SearchQueryCategorizor { + + public static void categorize(SearchSourceBuilder source) { + QueryBuilder topLevelQueryBuilder = source.query(); + QueryBuilderVisitor queryBuilderVisitor = new QueryBuilderVisitor() { + @Override + public void accept(QueryBuilder qb) { + // This method will be called for every QueryBuilder node in the tree. + // The tree referred to here is the tree of querybuilders for the incoming search + // query with the topLevelQueryBuilder as the root. + + // Increment counter for current QueryBuilder using Metric Framework. + if (qb instanceof BoolQueryBuilder) { + // Increment counter for Bool using Metric Framework. + } else if (qb instanceof MatchQueryBuilder) { + // Increment counter for Match using Metric Framework. + } else if (qb instanceof QueryStringQueryBuilder) { + // Increment counter for QueryStringQuery using Metric Framework. + } + // Similar for other builders + } + + @Override + public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { + return this; + } + }; + topLevelQueryBuilder.visit(queryBuilderVisitor); + } + +} 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 284f71bd9da62..9e6c2335188d1 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -32,6 +32,7 @@ package org.opensearch.action.search; +import org.apache.lucene.search.BooleanClause; import org.opensearch.action.OriginalIndices; import org.opensearch.action.admin.cluster.node.tasks.cancel.CancelTasksRequest; import org.opensearch.action.admin.cluster.shards.ClusterSearchShardsGroup; @@ -72,6 +73,8 @@ import org.opensearch.core.index.shard.ShardId; import org.opensearch.core.indices.breaker.CircuitBreakerService; import org.opensearch.core.tasks.TaskId; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.index.query.QueryBuilderVisitor; import org.opensearch.index.query.Rewriteable; import org.opensearch.search.SearchPhaseResult; import org.opensearch.search.SearchService; @@ -489,6 +492,8 @@ private void executeRequest( return; } + SearchQueryCategorizor.categorize(searchRequest.source()); + ActionListener rewriteListener = ActionListener.wrap(source -> { if (source != searchRequest.source()) { // only set it if it changed - we don't allow null values to be set but it might be already null. this way we catch From ac8e046e1e4681d52bedf76be0ed31c144ccb966 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Tue, 3 Oct 2023 23:04:54 -0700 Subject: [PATCH 02/35] Integrate metrics framework, add counters and log query shape Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizor.java | 51 ++++++++++-- .../action/search/SearchQueryCounters.java | 60 ++++++++++++++ .../index/query/QueryShapeVisitor.java | 82 +++++++++++++++++++ 3 files changed, 188 insertions(+), 5 deletions(-) create mode 100644 server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java create mode 100644 server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index bd2ee837d07cc..2aaeab3afb95d 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -8,18 +8,42 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.lucene.search.BooleanClause; +import org.opensearch.identity.IdentityService; import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.IntervalsSourceProvider; +import org.opensearch.index.query.MatchPhraseQueryBuilder; import org.opensearch.index.query.MatchQueryBuilder; +import org.opensearch.index.query.MultiMatchQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryBuilderVisitor; +import org.opensearch.index.query.QueryShapeVisitor; import org.opensearch.index.query.QueryStringQueryBuilder; +import org.opensearch.index.query.RangeQueryBuilder; +import org.opensearch.index.query.RegexpQueryBuilder; +import org.opensearch.index.query.TermQueryBuilder; +import org.opensearch.index.query.WildcardQueryBuilder; +import org.opensearch.index.query.functionscore.FunctionScoreQueryBuilder; import org.opensearch.search.builder.SearchSourceBuilder; public class SearchQueryCategorizor { + private static final Logger log = LogManager.getLogger(SearchQueryCategorizor.class); + + public static SearchQueryCounters searchQueryCounters = new SearchQueryCounters(); // What metrics registry to use here? + public static void categorize(SearchSourceBuilder source) { QueryBuilder topLevelQueryBuilder = source.query(); + + // Get and log the query shape + QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); + topLevelQueryBuilder.visit(shapeVisitor); + String queryShapeJson = shapeVisitor.prettyPrintTree(" "); + log.info("Query shape : " + queryShapeJson); + + // Increment the query counters using Metric Framework QueryBuilderVisitor queryBuilderVisitor = new QueryBuilderVisitor() { @Override public void accept(QueryBuilder qb) { @@ -28,14 +52,31 @@ public void accept(QueryBuilder qb) { // query with the topLevelQueryBuilder as the root. // Increment counter for current QueryBuilder using Metric Framework. - if (qb instanceof BoolQueryBuilder) { - // Increment counter for Bool using Metric Framework. + if (qb instanceof AggregationQ) { + searchQueryCounters.aggCounter.add(1); + } else if (qb instanceof BoolQueryBuilder) { + searchQueryCounters.boolCounter.add(1); + } else if (qb instanceof FunctionScoreQueryBuilder) { + searchQueryCounters.functionScoreCounter.add(1); } else if (qb instanceof MatchQueryBuilder) { - // Increment counter for Match using Metric Framework. + searchQueryCounters.matchCounter.add(1); + } else if (qb instanceof MatchPhraseQueryBuilder) { + searchQueryCounters.matchPhrasePrefixCounter.add(1); + } else if (qb instanceof MultiMatchQueryBuilder) { + searchQueryCounters.multiMatchCounter.add(1); } else if (qb instanceof QueryStringQueryBuilder) { - // Increment counter for QueryStringQuery using Metric Framework. + searchQueryCounters.queryStringQueryCounter.add(1); + } else if (qb instanceof RangeQueryBuilder) { + searchQueryCounters.rangeCounter.add(1); + } else if (qb instanceof RegexpQueryBuilder) { + searchQueryCounters.regexCounter.add(1); + } else if (qb instanceof TermQueryBuilder) { + searchQueryCounters.termCounter.add(1); + } else if (qb instanceof WildcardQueryBuilder) { + searchQueryCounters.wildcardCounter.add(1); + } else { + searchQueryCounters.otherQueryCounter.add(1); } - // Similar for other builders } @Override diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java new file mode 100644 index 0000000000000..6e4fdb8d21ba5 --- /dev/null +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java @@ -0,0 +1,60 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.action.search; + +public class SearchQueryCounters { + private final MetricsRegistry metricsRegistry; + + // Counters related to Query types + public final Counter aggCounter; + public final Counter boolCounter; + public final Counter functionScoreCounter; + public final Counter matchCounter; + public final Counter matchPhrasePrefixCounter; + public final Counter multiMatchCounter; + public final Counter otherQueryCounter; + public final Counter queryStringQueryCounter; + public final Counter rangeCounter; + public final Counter regexCounter; + public final Counter termCounter; + public final Counter totalCounter; + public final Counter wildcardCounter; + + + + public SearchQueryCounters(MetricsRegistry metricsRegistry) { + this.metricsRegistry = metricsRegistry; + this.aggCounter = metricsRegistry.createCounter("aggSearchQueryCounter", + "Counter for the number of top level and nested agg search queries", "0"); + this.boolCounter = metricsRegistry.createCounter("boolSearchQueryCounter", + "Counter for the number of top level and nested bool search queries", "0"); + this.functionScoreCounter = metricsRegistry.createCounter("functionScoreSearchQueryCounter", + "Counter for the number of top level and nested function score search queries", "0"); + this.matchCounter = metricsRegistry.createCounter("matchSearchQueryCounter", + "Counter for the number of top level and nested match search queries", "0"); + this.matchPhrasePrefixCounter = metricsRegistry.createCounter("matchPhrasePrefixSearchQueryCounter", + "Counter for the number of top level and nested match phrase prefix search queries", "0"); + this.multiMatchCounter = metricsRegistry.createCounter("multiMatchSearchQueryCounter", + "Counter for the number of top level and nested multi match search queries", "0"); + this.otherQueryCounter = metricsRegistry.createCounter("otherSearchQueryCounter", + "Counter for the number of top level and nested search queries that do not match any other categories", "0"); + this.queryStringQueryCounter = metricsRegistry.createCounter("queryStringQuerySearchQueryCounter", + "Counter for the number of top level and nested queryStringQuery search queries", "0"); + this.rangeCounter = metricsRegistry.createCounter("rangeSearchQueryCounter", + "Counter for the number of top level and nested range search queries", "0"); + this.regexCounter = metricsRegistry.createCounter("regexSearchQueryCounter", + "Counter for the number of top level and nested regex search queries", "0"); + this.termCounter = metricsRegistry.createCounter("termSearchQueryCounter", + "Counter for the number of top level and nested term search queries", "0"); + this.totalCounter = metricsRegistry.createCounter("totalSearchQueryCounter", + "Counter for the number of top level and nested search queries", "0"); + this.wildcardCounter = metricsRegistry.createCounter("wildcardSearchQueryCounter", + "Counter for the number of top level and nested wildcard search queries", "0"); + } +} diff --git a/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java b/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java new file mode 100644 index 0000000000000..0b513b35b32c6 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java @@ -0,0 +1,82 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.query; + +import org.apache.lucene.search.BooleanClause; +import org.opensearch.common.SetOnce; + +import java.util.ArrayList; +import java.util.EnumMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +public class QueryShapeVisitor implements QueryBuilderVisitor { + private final SetOnce queryType = new SetOnce<>(); + private final Map> childVisitors = new EnumMap<>(BooleanClause.Occur.class); + + @Override + public void accept(QueryBuilder qb) { + queryType.set(qb.getName()); + } + + @Override + public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { + // Should get called once per Occur value + if (childVisitors.containsKey(occur)) { + throw new IllegalStateException("getChildVisitor already called for " + occur); + } + final List childVisitorList = new ArrayList<>(); + QueryBuilderVisitor childVisitorWrapper = new QueryBuilderVisitor() { + QueryShapeVisitor currentChild; + @Override + public void accept(QueryBuilder qb) { + currentChild = new QueryShapeVisitor(); + childVisitorList.add(currentChild); + currentChild.accept(qb); + } + + @Override + public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { + return currentChild.getChildVisitor(occur); + } + }; + childVisitors.put(occur, childVisitorList); + return childVisitorWrapper; + } + + public String toJson() { + StringBuilder outputBuilder = new StringBuilder("{\"type\":\"").append(queryType.get()).append("\""); + for (Map.Entry> entry : childVisitors.entrySet()) { + outputBuilder.append(",\"").append(entry.getKey().name().toLowerCase(Locale.ROOT)).append("\"["); + boolean first = true; + for (QueryShapeVisitor child : entry.getValue()) { + if (!first) { + outputBuilder.append(","); + } + outputBuilder.append(child.toJson()); + first = false; + } + outputBuilder.append("]"); + } + outputBuilder.append("}"); + return outputBuilder.toString(); + } + + public String prettyPrintTree(String indent) { + StringBuilder outputBuilder = new StringBuilder(indent).append(queryType.get()).append("\n"); + for (Map.Entry> entry : childVisitors.entrySet()) { + outputBuilder.append(indent).append(" ").append(entry.getKey().name().toLowerCase(Locale.ROOT)).append(":\n"); + for (QueryShapeVisitor child : entry.getValue()) { + outputBuilder.append(child.prettyPrintTree(indent + " ")); + } + } + return outputBuilder.toString(); + } +} From b2de31276826c81d4f5666d3c30643aeeb418649 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Tue, 3 Oct 2023 23:37:31 -0700 Subject: [PATCH 03/35] Update changelog Signed-off-by: Siddhant Deshmukh --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ad18b94f31b7..ca324bdfd250c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Remote cluster state] Upload global metadata in cluster state to remote store([#10404](https://github.com/opensearch-project/OpenSearch/pull/10404)) - [Remote cluster state] Download functionality of global metadata from remote store ([#10535](https://github.com/opensearch-project/OpenSearch/pull/10535)) - [Remote cluster state] Restore global metadata from remote store when local state is lost after quorum loss ([#10404](https://github.com/opensearch-project/OpenSearch/pull/10404)) +- Configurable merge policy for index with an option to choose from LogByteSize and Tiered merge policy ([#9992](https://github.com/opensearch-project/OpenSearch/pull/9992)) +- Add search query categorizor ([#10255](https://github.com/opensearch-project/OpenSearch/pull/10255)) ### Dependencies - Bump `log4j-core` from 2.18.0 to 2.19.0 From 507423edb52c80150fe026c130d243d5b3bb0489 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Thu, 5 Oct 2023 00:24:20 -0700 Subject: [PATCH 04/35] Add level attribute to QueryBuilderVisitor and as a tag in Counters Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizor.java | 31 ++++++++++--------- .../index/query/BoolQueryBuilder.java | 12 +++---- .../index/query/BoostingQueryBuilder.java | 8 ++--- .../query/ConstantScoreQueryBuilder.java | 6 ++-- .../index/query/DisMaxQueryBuilder.java | 6 ++-- .../query/FieldMaskingSpanQueryBuilder.java | 6 ++-- .../opensearch/index/query/QueryBuilder.java | 5 +-- .../index/query/QueryBuilderVisitor.java | 5 +-- .../index/query/QueryShapeVisitor.java | 6 ++-- .../query/SpanContainingQueryBuilder.java | 8 ++--- .../index/query/SpanFirstQueryBuilder.java | 6 ++-- .../query/SpanMultiTermQueryBuilder.java | 6 ++-- .../index/query/SpanNearQueryBuilder.java | 6 ++-- .../index/query/SpanNotQueryBuilder.java | 8 ++--- .../index/query/SpanOrQueryBuilder.java | 6 ++-- .../index/query/SpanWithinQueryBuilder.java | 8 ++--- 16 files changed, 68 insertions(+), 65 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index 2aaeab3afb95d..b8f6da0212e03 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -27,6 +27,7 @@ import org.opensearch.index.query.WildcardQueryBuilder; import org.opensearch.index.query.functionscore.FunctionScoreQueryBuilder; import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.telemetry.tracing.attributes.Attributes; public class SearchQueryCategorizor { @@ -39,43 +40,43 @@ public static void categorize(SearchSourceBuilder source) { // Get and log the query shape QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); - topLevelQueryBuilder.visit(shapeVisitor); + topLevelQueryBuilder.visit(shapeVisitor, 0); String queryShapeJson = shapeVisitor.prettyPrintTree(" "); log.info("Query shape : " + queryShapeJson); // Increment the query counters using Metric Framework QueryBuilderVisitor queryBuilderVisitor = new QueryBuilderVisitor() { @Override - public void accept(QueryBuilder qb) { + public void accept(QueryBuilder qb, int level) { // This method will be called for every QueryBuilder node in the tree. // The tree referred to here is the tree of querybuilders for the incoming search // query with the topLevelQueryBuilder as the root. // Increment counter for current QueryBuilder using Metric Framework. if (qb instanceof AggregationQ) { - searchQueryCounters.aggCounter.add(1); + searchQueryCounters.aggCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof BoolQueryBuilder) { - searchQueryCounters.boolCounter.add(1); + searchQueryCounters.boolCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof FunctionScoreQueryBuilder) { - searchQueryCounters.functionScoreCounter.add(1); + searchQueryCounters.functionScoreCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof MatchQueryBuilder) { - searchQueryCounters.matchCounter.add(1); + searchQueryCounters.matchCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof MatchPhraseQueryBuilder) { - searchQueryCounters.matchPhrasePrefixCounter.add(1); + searchQueryCounters.matchPhrasePrefixCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof MultiMatchQueryBuilder) { - searchQueryCounters.multiMatchCounter.add(1); + searchQueryCounters.multiMatchCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof QueryStringQueryBuilder) { - searchQueryCounters.queryStringQueryCounter.add(1); + searchQueryCounters.queryStringQueryCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof RangeQueryBuilder) { - searchQueryCounters.rangeCounter.add(1); + searchQueryCounters.rangeCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof RegexpQueryBuilder) { - searchQueryCounters.regexCounter.add(1); + searchQueryCounters.regexCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof TermQueryBuilder) { - searchQueryCounters.termCounter.add(1); + searchQueryCounters.termCounter.add(1, Attributes.create().addAttribute("level", level)); } else if (qb instanceof WildcardQueryBuilder) { - searchQueryCounters.wildcardCounter.add(1); + searchQueryCounters.wildcardCounter.add(1, Attributes.create().addAttribute("level", level)); } else { - searchQueryCounters.otherQueryCounter.add(1); + searchQueryCounters.otherQueryCounter.add(1, Attributes.create().addAttribute("level", level)); } } @@ -84,7 +85,7 @@ public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { return this; } }; - topLevelQueryBuilder.visit(queryBuilderVisitor); + topLevelQueryBuilder.visit(queryBuilderVisitor, 0); } } diff --git a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java index c44a7ef6a397c..5c0956030e183 100644 --- a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java @@ -429,30 +429,30 @@ private static boolean rewriteClauses( } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); if (mustClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST); for (QueryBuilder mustClause : mustClauses) { - mustClause.visit(subVisitor); + mustClause.visit(subVisitor, level+1); } } if (shouldClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.SHOULD); for (QueryBuilder shouldClause : shouldClauses) { - shouldClause.visit(subVisitor); + shouldClause.visit(subVisitor, level+1); } } if (mustNotClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST_NOT); for (QueryBuilder mustNotClause : mustNotClauses) { - mustNotClause.visit(subVisitor); + mustNotClause.visit(subVisitor, level+1); } } if (filterClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.FILTER); for (QueryBuilder filterClause : filterClauses) { - filterClause.visit(subVisitor); + filterClause.visit(subVisitor, level+1); } } diff --git a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java index 1b52ae2f03605..5fa268770bfe8 100644 --- a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java @@ -255,13 +255,13 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); if (positiveQuery != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery, level+1); } if (negativeQuery != null) { - visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery); + visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery, level+1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java index b2764d29da80a..d27f020a603c9 100644 --- a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java @@ -186,9 +186,9 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); - visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); + visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder, level+1); } } diff --git a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java index bd8ec62f6c43e..36e223d227b52 100644 --- a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java @@ -249,12 +249,12 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); if (queries.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); for (QueryBuilder subQb : queries) { - subVisitor.accept(subQb); + subVisitor.accept(subQb, level+1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java index 1162689a54689..f59d4a8e1b207 100644 --- a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java @@ -210,8 +210,8 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder, level+1); } } diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java index 090f74c5be7fe..7d6c024846a1c 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java @@ -99,9 +99,10 @@ default QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOExc /** * Recurse through the QueryBuilder tree, visiting any child QueryBuilder. * @param visitor a query builder visitor to be called by each query builder in the tree. + * @param level level of the current query builder based on the QueryBuilder tree. 0 is top/root level. */ - default void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + default void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); }; } diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java index af5a125f9dd95..4cae0f00d746d 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java @@ -18,8 +18,9 @@ public interface QueryBuilderVisitor { /** * Accept method is called when the visitor accepts the queryBuilder object to be traversed in the query tree. * @param qb is a queryBuilder object which is accepeted by the visitor. + * @param level level of the current query builder based on the QueryBuilder tree. 0 is top/root level. */ - void accept(QueryBuilder qb); + void accept(QueryBuilder qb, int level); /** * Fetches the child sub visitor from the main QueryBuilderVisitor Object. @@ -35,7 +36,7 @@ public interface QueryBuilderVisitor { */ QueryBuilderVisitor NO_OP_VISITOR = new QueryBuilderVisitor() { @Override - public void accept(QueryBuilder qb) { + public void accept(QueryBuilder qb, int level) { // Do nothing } 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 0b513b35b32c6..4b0491d38f155 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java @@ -22,7 +22,7 @@ public class QueryShapeVisitor implements QueryBuilderVisitor { private final Map> childVisitors = new EnumMap<>(BooleanClause.Occur.class); @Override - public void accept(QueryBuilder qb) { + public void accept(QueryBuilder qb, int level) { queryType.set(qb.getName()); } @@ -36,10 +36,10 @@ public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { QueryBuilderVisitor childVisitorWrapper = new QueryBuilderVisitor() { QueryShapeVisitor currentChild; @Override - public void accept(QueryBuilder qb) { + public void accept(QueryBuilder qb, int level) { currentChild = new QueryShapeVisitor(); childVisitorList.add(currentChild); - currentChild.accept(qb); + currentChild.accept(qb, level+1); } @Override diff --git a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java index 32a19ea3e9b50..f46b2d7fc60f8 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java @@ -191,9 +191,9 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little, level+1); } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java index bcbc64ddf386d..ac3ad8d75c028 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java @@ -189,8 +189,8 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder, level+1); } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java index 96d03c91964e3..31564d7983629 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java @@ -216,10 +216,10 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); if (multiTermQueryBuilder != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder, level+1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java index 30a1c29c29126..b4fcfd015dad2 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java @@ -301,12 +301,12 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); if (this.clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); for (QueryBuilder subQb : this.clauses) { - subVisitor.accept(subQb); + subVisitor.accept(subQb, level+1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java index 59ec5b9d77fc8..d0415c7b24209 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java @@ -287,14 +287,14 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); if (include != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include, level+1); } if (exclude != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST_NOT).accept(exclude); + visitor.getChildVisitor(BooleanClause.Occur.MUST_NOT).accept(exclude, level+1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java index fae1e318c66bd..79deadd7da38e 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java @@ -191,12 +191,12 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); if (clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); for (QueryBuilder subQb : this.clauses) { - subVisitor.accept(subQb); + subVisitor.accept(subQb, level+1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java index 4d5a6dde61a70..fb32dfad76dc0 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java @@ -200,9 +200,9 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little, level+1); } } From aa426a17bbf26a0ea5f988d6ce9d27b637b9cde4 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Thu, 5 Oct 2023 01:14:42 -0700 Subject: [PATCH 05/35] Log query shape as debug log Signed-off-by: Siddhant Deshmukh --- .../org/opensearch/action/search/SearchQueryCategorizor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index b8f6da0212e03..d1995f3da01e6 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -42,7 +42,7 @@ public static void categorize(SearchSourceBuilder source) { QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); topLevelQueryBuilder.visit(shapeVisitor, 0); String queryShapeJson = shapeVisitor.prettyPrintTree(" "); - log.info("Query shape : " + queryShapeJson); + log.debug("Query shape : " + queryShapeJson); // Increment the query counters using Metric Framework QueryBuilderVisitor queryBuilderVisitor = new QueryBuilderVisitor() { From f6db526283bdb5f398ee4c7924a2edf82f973104 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Mon, 9 Oct 2023 01:43:38 -0700 Subject: [PATCH 06/35] Integrate metrics framework, refactor code and update tests Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizor.java | 61 ++++++++++--------- .../action/search/SearchQueryCounters.java | 3 + .../action/search/TransportSearchAction.java | 10 ++- .../index/query/QueryBuilderVisitorTests.java | 2 +- 4 files changed, 44 insertions(+), 32 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index d1995f3da01e6..5d50d42f4e7a3 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -11,9 +11,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.lucene.search.BooleanClause; -import org.opensearch.identity.IdentityService; import org.opensearch.index.query.BoolQueryBuilder; -import org.opensearch.index.query.IntervalsSourceProvider; import org.opensearch.index.query.MatchPhraseQueryBuilder; import org.opensearch.index.query.MatchQueryBuilder; import org.opensearch.index.query.MultiMatchQueryBuilder; @@ -27,56 +25,54 @@ import org.opensearch.index.query.WildcardQueryBuilder; import org.opensearch.index.query.functionscore.FunctionScoreQueryBuilder; import org.opensearch.search.builder.SearchSourceBuilder; -import org.opensearch.telemetry.tracing.attributes.Attributes; +import org.opensearch.telemetry.metrics.MetricsRegistry; +import org.opensearch.telemetry.metrics.tags.Tags; public class SearchQueryCategorizor { private static final Logger log = LogManager.getLogger(SearchQueryCategorizor.class); - public static SearchQueryCounters searchQueryCounters = new SearchQueryCounters(); // What metrics registry to use here? + public static SearchQueryCounters searchQueryCounters; - public static void categorize(SearchSourceBuilder source) { + public SearchQueryCategorizor(MetricsRegistry metricsRegistry) { + searchQueryCounters = new SearchQueryCounters(metricsRegistry); + } + + public void categorize(SearchSourceBuilder source) { QueryBuilder topLevelQueryBuilder = source.query(); - // Get and log the query shape - QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); - topLevelQueryBuilder.visit(shapeVisitor, 0); - String queryShapeJson = shapeVisitor.prettyPrintTree(" "); - log.debug("Query shape : " + queryShapeJson); + logQueryShape(topLevelQueryBuilder); + + incrementQueryCounters(topLevelQueryBuilder); + } + private static void incrementQueryCounters(QueryBuilder topLevelQueryBuilder) { // Increment the query counters using Metric Framework QueryBuilderVisitor queryBuilderVisitor = new QueryBuilderVisitor() { @Override public void accept(QueryBuilder qb, int level) { - // This method will be called for every QueryBuilder node in the tree. - // The tree referred to here is the tree of querybuilders for the incoming search - // query with the topLevelQueryBuilder as the root. - - // Increment counter for current QueryBuilder using Metric Framework. - if (qb instanceof AggregationQ) { - searchQueryCounters.aggCounter.add(1, Attributes.create().addAttribute("level", level)); - } else if (qb instanceof BoolQueryBuilder) { - searchQueryCounters.boolCounter.add(1, Attributes.create().addAttribute("level", level)); + if (qb instanceof BoolQueryBuilder) { + searchQueryCounters.boolCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof FunctionScoreQueryBuilder) { - searchQueryCounters.functionScoreCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.functionScoreCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof MatchQueryBuilder) { - searchQueryCounters.matchCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.matchCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof MatchPhraseQueryBuilder) { - searchQueryCounters.matchPhrasePrefixCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.matchPhrasePrefixCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof MultiMatchQueryBuilder) { - searchQueryCounters.multiMatchCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.multiMatchCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof QueryStringQueryBuilder) { - searchQueryCounters.queryStringQueryCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.queryStringQueryCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof RangeQueryBuilder) { - searchQueryCounters.rangeCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.rangeCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof RegexpQueryBuilder) { - searchQueryCounters.regexCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.regexCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof TermQueryBuilder) { - searchQueryCounters.termCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.termCounter.add(1, Tags.create().addTag("level", level)); } else if (qb instanceof WildcardQueryBuilder) { - searchQueryCounters.wildcardCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.wildcardCounter.add(1, Tags.create().addTag("level", level)); } else { - searchQueryCounters.otherQueryCounter.add(1, Attributes.create().addAttribute("level", level)); + searchQueryCounters.otherQueryCounter.add(1, Tags.create().addTag("level", level)); } } @@ -88,4 +84,11 @@ public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { topLevelQueryBuilder.visit(queryBuilderVisitor, 0); } + private static void logQueryShape(QueryBuilder topLevelQueryBuilder) { + QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); + topLevelQueryBuilder.visit(shapeVisitor, 0); + String queryShapeJson = shapeVisitor.prettyPrintTree(" "); + log.debug("Query shape : " + queryShapeJson); + } + } 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 6e4fdb8d21ba5..43195a9a1afa2 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java @@ -8,6 +8,9 @@ package org.opensearch.action.search; +import org.opensearch.telemetry.metrics.Counter; +import org.opensearch.telemetry.metrics.MetricsRegistry; + public class SearchQueryCounters { private final MetricsRegistry metricsRegistry; 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 9e6c2335188d1..108db277fa7a1 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -97,6 +97,7 @@ import org.opensearch.transport.RemoteTransportException; import org.opensearch.transport.Transport; import org.opensearch.transport.TransportService; +import org.opensearch.telemetry.metrics.MetricsRegistry; import java.util.ArrayList; import java.util.Arrays; @@ -182,6 +183,8 @@ public class TransportSearchAction extends HandledTransportAction) SearchRequest::new); this.client = client; @@ -214,6 +218,7 @@ public TransportSearchAction( this.isRequestStatsEnabled = clusterService.getClusterSettings().get(SEARCH_REQUEST_STATS_ENABLED); clusterService.getClusterSettings().addSettingsUpdateConsumer(SEARCH_REQUEST_STATS_ENABLED, this::setIsRequestStatsEnabled); this.searchRequestStats = searchRequestStats; + this.metricsRegistry = metricsRegistry; } private void setIsRequestStatsEnabled(boolean isRequestStatsEnabled) { @@ -492,7 +497,8 @@ private void executeRequest( return; } - SearchQueryCategorizor.categorize(searchRequest.source()); + SearchQueryCategorizor searchQueryCategorizor = new SearchQueryCategorizor(metricsRegistry); + searchQueryCategorizor.categorize(searchRequest.source()); ActionListener rewriteListener = ActionListener.wrap(source -> { if (source != searchRequest.source()) { diff --git a/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java b/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java index 7849d3985ca59..0fd627fe642b7 100644 --- a/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java +++ b/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java @@ -21,7 +21,7 @@ public void testNoOpsVisitor() { List visitedQueries = new ArrayList<>(); QueryBuilderVisitor qbv = createTestVisitor(visitedQueries); - boolQueryBuilder.visit(qbv); + boolQueryBuilder.visit(qbv, 0); QueryBuilderVisitor subQbv = qbv.getChildVisitor(BooleanClause.Occur.MUST_NOT); assertEquals(0, visitedQueries.size()); assertEquals(qbv, subQbv); From 40ff0164dbca2ed335da1e38afe607471cea54e7 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Mon, 9 Oct 2023 09:38:30 -0700 Subject: [PATCH 07/35] Fix build Signed-off-by: Siddhant Deshmukh --- .../index/query/functionscore/ScriptScoreQueryBuilder.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java index fe9ad200d44f0..08461c323a8b2 100644 --- a/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java @@ -227,11 +227,11 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor) { - visitor.accept(this); + public void visit(QueryBuilderVisitor visitor, int level) { + visitor.accept(this, level); if (query != null) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); - subVisitor.accept(query); + subVisitor.accept(query, level+1); } } } From 5c09d0a2f86370824687c18abdb163c4368c1d46 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Mon, 9 Oct 2023 09:53:14 -0700 Subject: [PATCH 08/35] Add javadocs Signed-off-by: Siddhant Deshmukh --- .../org/opensearch/action/search/SearchQueryCategorizor.java | 4 ++++ .../org/opensearch/action/search/SearchQueryCounters.java | 3 +++ .../java/org/opensearch/index/query/QueryShapeVisitor.java | 3 +++ 3 files changed, 10 insertions(+) diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index 5d50d42f4e7a3..25b360ab69391 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -28,6 +28,10 @@ import org.opensearch.telemetry.metrics.MetricsRegistry; import org.opensearch.telemetry.metrics.tags.Tags; +/** + * Class to categorize the search queries based on the type and increment the relevant counters. + * Class also logs the query shape. + */ public class SearchQueryCategorizor { private static final Logger log = LogManager.getLogger(SearchQueryCategorizor.class); 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 43195a9a1afa2..2ef2eb2b959d0 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java @@ -11,6 +11,9 @@ import org.opensearch.telemetry.metrics.Counter; import org.opensearch.telemetry.metrics.MetricsRegistry; +/** + * Class contains all the Counters related to search query types. + */ public class SearchQueryCounters { private final MetricsRegistry 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 4b0491d38f155..d06b8c1cc9442 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java @@ -17,6 +17,9 @@ import java.util.Locale; import java.util.Map; +/** + * Class to traverse the QueryBuilder tree and capture the query shape + */ public class QueryShapeVisitor implements QueryBuilderVisitor { private final SetOnce queryType = new SetOnce<>(); private final Map> childVisitors = new EnumMap<>(BooleanClause.Occur.class); From 4a5c70e2c4ab4fe6348e251f87addc44d8290d4c Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Mon, 9 Oct 2023 10:25:02 -0700 Subject: [PATCH 09/35] Minor fix Signed-off-by: Siddhant Deshmukh --- .../main/java/org/opensearch/test/AbstractBuilderTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java b/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java index 7a8cf4963c4f1..dc7f8e69d755e 100644 --- a/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java @@ -322,7 +322,7 @@ protected static QueryShardContext createShardContext() { protected static QueryBuilderVisitor createTestVisitor(List visitedQueries) { return new QueryBuilderVisitor() { @Override - public void accept(QueryBuilder qb) { + public void accept(QueryBuilder qb, int level) { visitedQueries.add(qb); } From 255ed77e97bdeb99194844d2ed1b05274c6f4d43 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Tue, 10 Oct 2023 00:17:29 -0700 Subject: [PATCH 10/35] Spotless check changes Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCounters.java | 93 +++++++++++++------ .../action/search/TransportSearchAction.java | 5 +- .../index/query/BoolQueryBuilder.java | 8 +- .../index/query/BoostingQueryBuilder.java | 4 +- .../query/ConstantScoreQueryBuilder.java | 2 +- .../index/query/DisMaxQueryBuilder.java | 2 +- .../query/FieldMaskingSpanQueryBuilder.java | 2 +- .../index/query/QueryShapeVisitor.java | 3 +- .../query/SpanContainingQueryBuilder.java | 4 +- .../index/query/SpanFirstQueryBuilder.java | 2 +- .../query/SpanMultiTermQueryBuilder.java | 2 +- .../index/query/SpanNearQueryBuilder.java | 2 +- .../index/query/SpanNotQueryBuilder.java | 4 +- .../index/query/SpanOrQueryBuilder.java | 2 +- .../index/query/SpanWithinQueryBuilder.java | 4 +- .../ScriptScoreQueryBuilder.java | 2 +- 16 files changed, 88 insertions(+), 53 deletions(-) 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 2ef2eb2b959d0..0bf7bf6e7f960 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java @@ -32,35 +32,72 @@ public class SearchQueryCounters { public final Counter totalCounter; public final Counter wildcardCounter; - - public SearchQueryCounters(MetricsRegistry metricsRegistry) { this.metricsRegistry = metricsRegistry; - this.aggCounter = metricsRegistry.createCounter("aggSearchQueryCounter", - "Counter for the number of top level and nested agg search queries", "0"); - this.boolCounter = metricsRegistry.createCounter("boolSearchQueryCounter", - "Counter for the number of top level and nested bool search queries", "0"); - this.functionScoreCounter = metricsRegistry.createCounter("functionScoreSearchQueryCounter", - "Counter for the number of top level and nested function score search queries", "0"); - this.matchCounter = metricsRegistry.createCounter("matchSearchQueryCounter", - "Counter for the number of top level and nested match search queries", "0"); - this.matchPhrasePrefixCounter = metricsRegistry.createCounter("matchPhrasePrefixSearchQueryCounter", - "Counter for the number of top level and nested match phrase prefix search queries", "0"); - this.multiMatchCounter = metricsRegistry.createCounter("multiMatchSearchQueryCounter", - "Counter for the number of top level and nested multi match search queries", "0"); - this.otherQueryCounter = metricsRegistry.createCounter("otherSearchQueryCounter", - "Counter for the number of top level and nested search queries that do not match any other categories", "0"); - this.queryStringQueryCounter = metricsRegistry.createCounter("queryStringQuerySearchQueryCounter", - "Counter for the number of top level and nested queryStringQuery search queries", "0"); - this.rangeCounter = metricsRegistry.createCounter("rangeSearchQueryCounter", - "Counter for the number of top level and nested range search queries", "0"); - this.regexCounter = metricsRegistry.createCounter("regexSearchQueryCounter", - "Counter for the number of top level and nested regex search queries", "0"); - this.termCounter = metricsRegistry.createCounter("termSearchQueryCounter", - "Counter for the number of top level and nested term search queries", "0"); - this.totalCounter = metricsRegistry.createCounter("totalSearchQueryCounter", - "Counter for the number of top level and nested search queries", "0"); - this.wildcardCounter = metricsRegistry.createCounter("wildcardSearchQueryCounter", - "Counter for the number of top level and nested wildcard search queries", "0"); + this.aggCounter = metricsRegistry.createCounter( + "aggSearchQueryCounter", + "Counter for the number of top level and nested agg search queries", + "0" + ); + this.boolCounter = metricsRegistry.createCounter( + "boolSearchQueryCounter", + "Counter for the number of top level and nested bool search queries", + "0" + ); + this.functionScoreCounter = metricsRegistry.createCounter( + "functionScoreSearchQueryCounter", + "Counter for the number of top level and nested function score search queries", + "0" + ); + this.matchCounter = metricsRegistry.createCounter( + "matchSearchQueryCounter", + "Counter for the number of top level and nested match search queries", + "0" + ); + this.matchPhrasePrefixCounter = metricsRegistry.createCounter( + "matchPhrasePrefixSearchQueryCounter", + "Counter for the number of top level and nested match phrase prefix search queries", + "0" + ); + this.multiMatchCounter = metricsRegistry.createCounter( + "multiMatchSearchQueryCounter", + "Counter for the number of top level and nested multi match search queries", + "0" + ); + this.otherQueryCounter = metricsRegistry.createCounter( + "otherSearchQueryCounter", + "Counter for the number of top level and nested search queries that do not match any other categories", + "0" + ); + this.queryStringQueryCounter = metricsRegistry.createCounter( + "queryStringQuerySearchQueryCounter", + "Counter for the number of top level and nested queryStringQuery search queries", + "0" + ); + this.rangeCounter = metricsRegistry.createCounter( + "rangeSearchQueryCounter", + "Counter for the number of top level and nested range search queries", + "0" + ); + this.regexCounter = metricsRegistry.createCounter( + "regexSearchQueryCounter", + "Counter for the number of top level and nested regex search queries", + "0" + ); + this.termCounter = metricsRegistry.createCounter( + "termSearchQueryCounter", + "Counter for the number of top level and nested term search queries", + "0" + ); + this.totalCounter = metricsRegistry.createCounter( + "totalSearchQueryCounter", + "Counter for the number of top level and nested search queries", + "0" + ); + this.wildcardCounter = metricsRegistry.createCounter( + "wildcardSearchQueryCounter", + "Counter for the number of top level and nested wildcard search queries", + "0" + ); } } 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 108db277fa7a1..0929069393e56 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -32,7 +32,6 @@ package org.opensearch.action.search; -import org.apache.lucene.search.BooleanClause; import org.opensearch.action.OriginalIndices; import org.opensearch.action.admin.cluster.node.tasks.cancel.CancelTasksRequest; import org.opensearch.action.admin.cluster.shards.ClusterSearchShardsGroup; @@ -73,8 +72,6 @@ import org.opensearch.core.index.shard.ShardId; import org.opensearch.core.indices.breaker.CircuitBreakerService; import org.opensearch.core.tasks.TaskId; -import org.opensearch.index.query.QueryBuilder; -import org.opensearch.index.query.QueryBuilderVisitor; import org.opensearch.index.query.Rewriteable; import org.opensearch.search.SearchPhaseResult; import org.opensearch.search.SearchService; @@ -91,13 +88,13 @@ import org.opensearch.search.profile.SearchProfileShardResults; import org.opensearch.tasks.CancellableTask; import org.opensearch.tasks.Task; +import org.opensearch.telemetry.metrics.MetricsRegistry; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.RemoteClusterAware; import org.opensearch.transport.RemoteClusterService; import org.opensearch.transport.RemoteTransportException; import org.opensearch.transport.Transport; import org.opensearch.transport.TransportService; -import org.opensearch.telemetry.metrics.MetricsRegistry; import java.util.ArrayList; import java.util.Arrays; diff --git a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java index 5c0956030e183..e870de23cd5fd 100644 --- a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java @@ -434,25 +434,25 @@ public void visit(QueryBuilderVisitor visitor, int level) { if (mustClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST); for (QueryBuilder mustClause : mustClauses) { - mustClause.visit(subVisitor, level+1); + mustClause.visit(subVisitor, level + 1); } } if (shouldClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.SHOULD); for (QueryBuilder shouldClause : shouldClauses) { - shouldClause.visit(subVisitor, level+1); + shouldClause.visit(subVisitor, level + 1); } } if (mustNotClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST_NOT); for (QueryBuilder mustNotClause : mustNotClauses) { - mustNotClause.visit(subVisitor, level+1); + mustNotClause.visit(subVisitor, level + 1); } } if (filterClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.FILTER); for (QueryBuilder filterClause : filterClauses) { - filterClause.visit(subVisitor, level+1); + filterClause.visit(subVisitor, level + 1); } } diff --git a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java index 5fa268770bfe8..c5070920ab4cd 100644 --- a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java @@ -258,10 +258,10 @@ protected void extractInnerHitBuilders(Map inner public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); if (positiveQuery != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery, level + 1); } if (negativeQuery != null) { - visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery, level+1); + visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery, level + 1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java index d27f020a603c9..2ede60d935210 100644 --- a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java @@ -188,7 +188,7 @@ protected void extractInnerHitBuilders(Map inner @Override public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder, level+1); + visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder, level + 1); } } diff --git a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java index 36e223d227b52..bc26cfed53b07 100644 --- a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java @@ -254,7 +254,7 @@ public void visit(QueryBuilderVisitor visitor, int level) { if (queries.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); for (QueryBuilder subQb : queries) { - subVisitor.accept(subQb, level+1); + subVisitor.accept(subQb, level + 1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java index f59d4a8e1b207..f4859adacc194 100644 --- a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java @@ -212,6 +212,6 @@ public String getWriteableName() { @Override public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder, level + 1); } } 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 d06b8c1cc9442..6fd7a182c1fba 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java @@ -38,11 +38,12 @@ public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { final List childVisitorList = new ArrayList<>(); QueryBuilderVisitor childVisitorWrapper = new QueryBuilderVisitor() { QueryShapeVisitor currentChild; + @Override public void accept(QueryBuilder qb, int level) { currentChild = new QueryShapeVisitor(); childVisitorList.add(currentChild); - currentChild.accept(qb, level+1); + currentChild.accept(qb, level + 1); } @Override diff --git a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java index f46b2d7fc60f8..781f522980511 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java @@ -193,7 +193,7 @@ public String getWriteableName() { @Override public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big, level+1); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big, level + 1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little, level + 1); } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java index ac3ad8d75c028..e51f4457cc0f1 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java @@ -191,6 +191,6 @@ public String getWriteableName() { @Override public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder, level + 1); } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java index 31564d7983629..c27b8682eb8b6 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java @@ -219,7 +219,7 @@ public String getWriteableName() { public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); if (multiTermQueryBuilder != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder, level + 1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java index b4fcfd015dad2..d3573ec3e8684 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java @@ -306,7 +306,7 @@ public void visit(QueryBuilderVisitor visitor, int level) { if (this.clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); for (QueryBuilder subQb : this.clauses) { - subVisitor.accept(subQb, level+1); + subVisitor.accept(subQb, level + 1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java index d0415c7b24209..00d9e8014b533 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java @@ -290,11 +290,11 @@ public String getWriteableName() { public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); if (include != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include, level + 1); } if (exclude != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST_NOT).accept(exclude, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST_NOT).accept(exclude, level + 1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java index 79deadd7da38e..f1b94f40c4bd7 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java @@ -196,7 +196,7 @@ public void visit(QueryBuilderVisitor visitor, int level) { if (clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); for (QueryBuilder subQb : this.clauses) { - subVisitor.accept(subQb, level+1); + subVisitor.accept(subQb, level + 1); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java index fb32dfad76dc0..fb695842864d8 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java @@ -202,7 +202,7 @@ public String getWriteableName() { @Override public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big, level+1); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little, level+1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big, level + 1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little, level + 1); } } diff --git a/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java index 08461c323a8b2..2145873f41f91 100644 --- a/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java @@ -231,7 +231,7 @@ public void visit(QueryBuilderVisitor visitor, int level) { visitor.accept(this, level); if (query != null) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); - subVisitor.accept(query, level+1); + subVisitor.accept(query, level + 1); } } } From 4386264e5bbfae649b294a0fcbd2f727d85b8324 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 11 Oct 2023 00:13:44 -0700 Subject: [PATCH 11/35] Address comments, add agg and sort counters, add feature flag, refactoring Signed-off-by: Siddhant Deshmukh --- .../SearchQueryCategorizingVisitor.java | 71 +++++++++++++++++++ .../action/search/SearchQueryCategorizor.java | 69 ++++++------------ .../action/search/SearchQueryCounters.java | 9 ++- .../action/search/TransportSearchAction.java | 13 +++- .../opensearch/common/util/FeatureFlags.java | 11 +++ .../index/query/BoolQueryBuilder.java | 12 ++-- .../index/query/BoostingQueryBuilder.java | 8 +-- .../query/ConstantScoreQueryBuilder.java | 6 +- .../index/query/DisMaxQueryBuilder.java | 6 +- .../query/FieldMaskingSpanQueryBuilder.java | 6 +- .../opensearch/index/query/QueryBuilder.java | 5 +- .../index/query/QueryBuilderVisitor.java | 5 +- .../index/query/QueryShapeVisitor.java | 6 +- .../query/SpanContainingQueryBuilder.java | 8 +-- .../index/query/SpanFirstQueryBuilder.java | 6 +- .../query/SpanMultiTermQueryBuilder.java | 6 +- .../index/query/SpanNearQueryBuilder.java | 6 +- .../index/query/SpanNotQueryBuilder.java | 8 +-- .../index/query/SpanOrQueryBuilder.java | 6 +- .../index/query/SpanWithinQueryBuilder.java | 8 +-- .../ScriptScoreQueryBuilder.java | 6 +- 21 files changed, 175 insertions(+), 106 deletions(-) create mode 100644 server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java new file mode 100644 index 0000000000000..007480bb2e7bd --- /dev/null +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java @@ -0,0 +1,71 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.action.search; + +import org.apache.lucene.search.BooleanClause; +import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.MatchPhraseQueryBuilder; +import org.opensearch.index.query.MatchQueryBuilder; +import org.opensearch.index.query.MultiMatchQueryBuilder; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.index.query.QueryBuilderVisitor; +import org.opensearch.index.query.QueryStringQueryBuilder; +import org.opensearch.index.query.RangeQueryBuilder; +import org.opensearch.index.query.RegexpQueryBuilder; +import org.opensearch.index.query.TermQueryBuilder; +import org.opensearch.index.query.WildcardQueryBuilder; +import org.opensearch.index.query.functionscore.FunctionScoreQueryBuilder; +import org.opensearch.telemetry.metrics.tags.Tags; + +/** + * 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 { + private final int level; + private final SearchQueryCounters searchQueryCounters; + + public SearchQueryCategorizingVisitor(SearchQueryCounters searchQueryCounters) { + this(searchQueryCounters, 0); + } + + private SearchQueryCategorizingVisitor(SearchQueryCounters counters, int level) { + this.searchQueryCounters = counters; + this.level = level; + } + public void accept(QueryBuilder qb) { + if (qb instanceof BoolQueryBuilder) { + searchQueryCounters.boolCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof FunctionScoreQueryBuilder) { + searchQueryCounters.functionScoreCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof MatchQueryBuilder) { + searchQueryCounters.matchCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof MatchPhraseQueryBuilder) { + searchQueryCounters.matchPhrasePrefixCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof MultiMatchQueryBuilder) { + searchQueryCounters.multiMatchCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof QueryStringQueryBuilder) { + searchQueryCounters.queryStringQueryCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof RangeQueryBuilder) { + searchQueryCounters.rangeCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof RegexpQueryBuilder) { + searchQueryCounters.regexCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof TermQueryBuilder) { + searchQueryCounters.termCounter.add(1, Tags.create().addTag("level", level)); + } else if (qb instanceof WildcardQueryBuilder) { + searchQueryCounters.wildcardCounter.add(1, Tags.create().addTag("level", level)); + } else { + searchQueryCounters.otherQueryCounter.add(1, Tags.create().addTag("level", level)); + } + } + + public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { + return new SearchQueryCategorizingVisitor(searchQueryCounters, level + 1); + } +} diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index 25b360ab69391..64526fd2fc32d 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -10,24 +10,17 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.apache.lucene.search.BooleanClause; -import org.opensearch.index.query.BoolQueryBuilder; -import org.opensearch.index.query.MatchPhraseQueryBuilder; -import org.opensearch.index.query.MatchQueryBuilder; -import org.opensearch.index.query.MultiMatchQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryBuilderVisitor; import org.opensearch.index.query.QueryShapeVisitor; -import org.opensearch.index.query.QueryStringQueryBuilder; -import org.opensearch.index.query.RangeQueryBuilder; -import org.opensearch.index.query.RegexpQueryBuilder; -import org.opensearch.index.query.TermQueryBuilder; -import org.opensearch.index.query.WildcardQueryBuilder; -import org.opensearch.index.query.functionscore.FunctionScoreQueryBuilder; +import org.opensearch.search.aggregations.AggregatorFactories; import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.search.sort.SortBuilder; import org.opensearch.telemetry.metrics.MetricsRegistry; import org.opensearch.telemetry.metrics.tags.Tags; +import java.util.List; + /** * Class to categorize the search queries based on the type and increment the relevant counters. * Class also logs the query shape. @@ -46,51 +39,31 @@ public void categorize(SearchSourceBuilder source) { QueryBuilder topLevelQueryBuilder = source.query(); logQueryShape(topLevelQueryBuilder); + incrementQueryTypeCounters(topLevelQueryBuilder); + incrementQueryAggregationCounters(source.aggregations()); + incrementQuerySortCounters(source.sorts()); + } - incrementQueryCounters(topLevelQueryBuilder); + private void incrementQuerySortCounters(List> sorts) { + if (sorts.size() > 0) { + searchQueryCounters.sortCounter.add(1); + } } - private static void incrementQueryCounters(QueryBuilder topLevelQueryBuilder) { - // Increment the query counters using Metric Framework - QueryBuilderVisitor queryBuilderVisitor = new QueryBuilderVisitor() { - @Override - public void accept(QueryBuilder qb, int level) { - if (qb instanceof BoolQueryBuilder) { - searchQueryCounters.boolCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof FunctionScoreQueryBuilder) { - searchQueryCounters.functionScoreCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof MatchQueryBuilder) { - searchQueryCounters.matchCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof MatchPhraseQueryBuilder) { - searchQueryCounters.matchPhrasePrefixCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof MultiMatchQueryBuilder) { - searchQueryCounters.multiMatchCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof QueryStringQueryBuilder) { - searchQueryCounters.queryStringQueryCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof RangeQueryBuilder) { - searchQueryCounters.rangeCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof RegexpQueryBuilder) { - searchQueryCounters.regexCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof TermQueryBuilder) { - searchQueryCounters.termCounter.add(1, Tags.create().addTag("level", level)); - } else if (qb instanceof WildcardQueryBuilder) { - searchQueryCounters.wildcardCounter.add(1, Tags.create().addTag("level", level)); - } else { - searchQueryCounters.otherQueryCounter.add(1, Tags.create().addTag("level", level)); - } - } + private void incrementQueryAggregationCounters(AggregatorFactories.Builder aggregations) { + if (aggregations != null) { + searchQueryCounters.aggCounter.add(1); + } + } - @Override - public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { - return this; - } - }; - topLevelQueryBuilder.visit(queryBuilderVisitor, 0); + private static void incrementQueryTypeCounters(QueryBuilder topLevelQueryBuilder) { + QueryBuilderVisitor searhQueryVisitor = new SearchQueryCategorizingVisitor(searchQueryCounters); + topLevelQueryBuilder.visit(searhQueryVisitor); } private static void logQueryShape(QueryBuilder topLevelQueryBuilder) { QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); - topLevelQueryBuilder.visit(shapeVisitor, 0); + topLevelQueryBuilder.visit(shapeVisitor); String queryShapeJson = shapeVisitor.prettyPrintTree(" "); log.debug("Query shape : " + queryShapeJson); } 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 0bf7bf6e7f960..c0d8fabc6bf36 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java @@ -28,6 +28,8 @@ public class SearchQueryCounters { public final Counter queryStringQueryCounter; public final Counter rangeCounter; public final Counter regexCounter; + + public final Counter sortCounter; public final Counter termCounter; public final Counter totalCounter; public final Counter wildcardCounter; @@ -36,7 +38,7 @@ public SearchQueryCounters(MetricsRegistry metricsRegistry) { this.metricsRegistry = metricsRegistry; this.aggCounter = metricsRegistry.createCounter( "aggSearchQueryCounter", - "Counter for the number of top level and nested agg search queries", + "Counter for the number of top level agg search queries", "0" ); this.boolCounter = metricsRegistry.createCounter( @@ -84,6 +86,11 @@ public SearchQueryCounters(MetricsRegistry metricsRegistry) { "Counter for the number of top level and nested regex search queries", "0" ); + this.sortCounter = metricsRegistry.createCounter( + "sortSearchQueryCounter", + "Counter for the number of top level sort search queries", + "0" + ); this.termCounter = metricsRegistry.createCounter( "termSearchQueryCounter", "Counter for the number of top level and nested term search queries", 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 0929069393e56..4f9f7a558e4fb 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -60,6 +60,7 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AtomicArray; import org.opensearch.common.util.concurrent.CountDown; import org.opensearch.core.action.ActionListener; @@ -182,6 +183,8 @@ public class TransportSearchAction extends HandledTransportAction rewriteListener = ActionListener.wrap(source -> { if (source != searchRequest.source()) { diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 4e9b417e3433b..8f5bc00f0f7eb 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -60,6 +60,11 @@ public class FeatureFlags { */ public static final String DATETIME_FORMATTER_CACHING = "opensearch.experimental.optimization.datetime_formatter_caching.enabled"; + /** + * Gates the query categorization by type feature. + */ + public static final String QUERY_CATEOGORIZATION = "opensearch.experimental.feature.query_categorization.enabled"; + /** * Should store the settings from opensearch.yml. */ @@ -122,4 +127,10 @@ public static boolean isEnabled(Setting featureFlag) { true, Property.NodeScope ); + + public static final Setting QUERY_CATEGORIZATION_SETTING = Setting.boolSetting( + QUERY_CATEOGORIZATION, + false, + Property.NodeScope + ); } diff --git a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java index e870de23cd5fd..c44a7ef6a397c 100644 --- a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java @@ -429,30 +429,30 @@ private static boolean rewriteClauses( } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); if (mustClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST); for (QueryBuilder mustClause : mustClauses) { - mustClause.visit(subVisitor, level + 1); + mustClause.visit(subVisitor); } } if (shouldClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.SHOULD); for (QueryBuilder shouldClause : shouldClauses) { - shouldClause.visit(subVisitor, level + 1); + shouldClause.visit(subVisitor); } } if (mustNotClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST_NOT); for (QueryBuilder mustNotClause : mustNotClauses) { - mustNotClause.visit(subVisitor, level + 1); + mustNotClause.visit(subVisitor); } } if (filterClauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.FILTER); for (QueryBuilder filterClause : filterClauses) { - filterClause.visit(subVisitor, level + 1); + filterClause.visit(subVisitor); } } diff --git a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java index c5070920ab4cd..1b52ae2f03605 100644 --- a/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoostingQueryBuilder.java @@ -255,13 +255,13 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); if (positiveQuery != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery, level + 1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery); } if (negativeQuery != null) { - visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery, level + 1); + visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery); } } } diff --git a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java index 2ede60d935210..b2764d29da80a 100644 --- a/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/ConstantScoreQueryBuilder.java @@ -186,9 +186,9 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder, level + 1); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.FILTER).accept(filterBuilder); } } diff --git a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java index bc26cfed53b07..bd8ec62f6c43e 100644 --- a/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/DisMaxQueryBuilder.java @@ -249,12 +249,12 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); if (queries.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); for (QueryBuilder subQb : queries) { - subVisitor.accept(subQb, level + 1); + subVisitor.accept(subQb); } } } diff --git a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java index f4859adacc194..1162689a54689 100644 --- a/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/FieldMaskingSpanQueryBuilder.java @@ -210,8 +210,8 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder, level + 1); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(queryBuilder); } } diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java index 7d6c024846a1c..090f74c5be7fe 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilder.java @@ -99,10 +99,9 @@ default QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOExc /** * Recurse through the QueryBuilder tree, visiting any child QueryBuilder. * @param visitor a query builder visitor to be called by each query builder in the tree. - * @param level level of the current query builder based on the QueryBuilder tree. 0 is top/root level. */ - default void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + default void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); }; } diff --git a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java index 4cae0f00d746d..af5a125f9dd95 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryBuilderVisitor.java @@ -18,9 +18,8 @@ public interface QueryBuilderVisitor { /** * Accept method is called when the visitor accepts the queryBuilder object to be traversed in the query tree. * @param qb is a queryBuilder object which is accepeted by the visitor. - * @param level level of the current query builder based on the QueryBuilder tree. 0 is top/root level. */ - void accept(QueryBuilder qb, int level); + void accept(QueryBuilder qb); /** * Fetches the child sub visitor from the main QueryBuilderVisitor Object. @@ -36,7 +35,7 @@ public interface QueryBuilderVisitor { */ QueryBuilderVisitor NO_OP_VISITOR = new QueryBuilderVisitor() { @Override - public void accept(QueryBuilder qb, int level) { + public void accept(QueryBuilder qb) { // Do nothing } 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 6fd7a182c1fba..10f5774219032 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java +++ b/server/src/main/java/org/opensearch/index/query/QueryShapeVisitor.java @@ -25,7 +25,7 @@ public class QueryShapeVisitor implements QueryBuilderVisitor { private final Map> childVisitors = new EnumMap<>(BooleanClause.Occur.class); @Override - public void accept(QueryBuilder qb, int level) { + public void accept(QueryBuilder qb) { queryType.set(qb.getName()); } @@ -40,10 +40,10 @@ public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { QueryShapeVisitor currentChild; @Override - public void accept(QueryBuilder qb, int level) { + public void accept(QueryBuilder qb) { currentChild = new QueryShapeVisitor(); childVisitorList.add(currentChild); - currentChild.accept(qb, level + 1); + currentChild.accept(qb); } @Override diff --git a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java index 781f522980511..32a19ea3e9b50 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanContainingQueryBuilder.java @@ -191,9 +191,9 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big, level + 1); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little, level + 1); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java index e51f4457cc0f1..bcbc64ddf386d 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanFirstQueryBuilder.java @@ -189,8 +189,8 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder, level + 1); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(matchBuilder); } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java index c27b8682eb8b6..96d03c91964e3 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanMultiTermQueryBuilder.java @@ -216,10 +216,10 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); if (multiTermQueryBuilder != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder, level + 1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(multiTermQueryBuilder); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java index d3573ec3e8684..30a1c29c29126 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNearQueryBuilder.java @@ -301,12 +301,12 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); if (this.clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); for (QueryBuilder subQb : this.clauses) { - subVisitor.accept(subQb, level + 1); + subVisitor.accept(subQb); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java index 00d9e8014b533..59ec5b9d77fc8 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanNotQueryBuilder.java @@ -287,14 +287,14 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); if (include != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include, level + 1); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(include); } if (exclude != null) { - visitor.getChildVisitor(BooleanClause.Occur.MUST_NOT).accept(exclude, level + 1); + visitor.getChildVisitor(BooleanClause.Occur.MUST_NOT).accept(exclude); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java index f1b94f40c4bd7..fae1e318c66bd 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanOrQueryBuilder.java @@ -191,12 +191,12 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); if (clauses.isEmpty() == false) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.SHOULD); for (QueryBuilder subQb : this.clauses) { - subVisitor.accept(subQb, level + 1); + subVisitor.accept(subQb); } } } diff --git a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java index fb695842864d8..4d5a6dde61a70 100644 --- a/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/SpanWithinQueryBuilder.java @@ -200,9 +200,9 @@ public String getWriteableName() { } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big, level + 1); - visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little, level + 1); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(big); + visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(little); } } diff --git a/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java index 2145873f41f91..fe9ad200d44f0 100644 --- a/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/functionscore/ScriptScoreQueryBuilder.java @@ -227,11 +227,11 @@ protected void extractInnerHitBuilders(Map inner } @Override - public void visit(QueryBuilderVisitor visitor, int level) { - visitor.accept(this, level); + public void visit(QueryBuilderVisitor visitor) { + visitor.accept(this); if (query != null) { QueryBuilderVisitor subVisitor = visitor.getChildVisitor(BooleanClause.Occur.MUST); - subVisitor.accept(query, level + 1); + subVisitor.accept(query); } } } From 21184bcea8991c523912474d030027cb87ff8501 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 11 Oct 2023 01:50:10 -0700 Subject: [PATCH 12/35] Build fix Signed-off-by: Siddhant Deshmukh --- .../main/java/org/opensearch/test/AbstractBuilderTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java b/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java index dc7f8e69d755e..7a8cf4963c4f1 100644 --- a/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java @@ -322,7 +322,7 @@ protected static QueryShardContext createShardContext() { protected static QueryBuilderVisitor createTestVisitor(List visitedQueries) { return new QueryBuilderVisitor() { @Override - public void accept(QueryBuilder qb, int level) { + public void accept(QueryBuilder qb) { visitedQueries.add(qb); } From 7b729b528a7e90ebb4ff07ae53b67b6c71aabe58 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 11 Oct 2023 02:18:13 -0700 Subject: [PATCH 13/35] spotless check Signed-off-by: Siddhant Deshmukh --- .../opensearch/action/search/SearchQueryCategorizingVisitor.java | 1 + .../org/opensearch/action/search/SearchQueryCategorizor.java | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) 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 007480bb2e7bd..ac0dcf701a2ed 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java @@ -39,6 +39,7 @@ private SearchQueryCategorizingVisitor(SearchQueryCounters counters, int level) this.searchQueryCounters = counters; this.level = level; } + public void accept(QueryBuilder qb) { if (qb instanceof BoolQueryBuilder) { searchQueryCounters.boolCounter.add(1, Tags.create().addTag("level", level)); diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index 64526fd2fc32d..b30ea600ce12c 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -17,7 +17,6 @@ import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.search.sort.SortBuilder; import org.opensearch.telemetry.metrics.MetricsRegistry; -import org.opensearch.telemetry.metrics.tags.Tags; import java.util.List; From d86845e77b89344876c1d5395366b4e033f274ff Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 11 Oct 2023 02:41:47 -0700 Subject: [PATCH 14/35] Fix tests Signed-off-by: Siddhant Deshmukh --- .../org/opensearch/index/query/QueryBuilderVisitorTests.java | 2 +- .../java/org/opensearch/snapshots/SnapshotResiliencyTests.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java b/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java index 0fd627fe642b7..7849d3985ca59 100644 --- a/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java +++ b/server/src/test/java/org/opensearch/index/query/QueryBuilderVisitorTests.java @@ -21,7 +21,7 @@ public void testNoOpsVisitor() { List visitedQueries = new ArrayList<>(); QueryBuilderVisitor qbv = createTestVisitor(visitedQueries); - boolQueryBuilder.visit(qbv, 0); + boolQueryBuilder.visit(qbv); QueryBuilderVisitor subQbv = qbv.getChildVisitor(BooleanClause.Occur.MUST_NOT); assertEquals(0, visitedQueries.size()); assertEquals(qbv, subQbv); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 97c5d23831965..73758f3852227 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -2302,6 +2302,7 @@ public void onFailure(final Exception e) { List.of(), client ), + null, null ) ); From 4e05a39f5591bfe2dfa9b4d55478d90a3f2153b7 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Thu, 12 Oct 2023 03:10:11 -0700 Subject: [PATCH 15/35] Dynamic feature flag with callback Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizor.java | 2 +- .../action/search/TransportSearchAction.java | 21 ++++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index b30ea600ce12c..0da95845d1b3b 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -44,7 +44,7 @@ public void categorize(SearchSourceBuilder source) { } private void incrementQuerySortCounters(List> sorts) { - if (sorts.size() > 0) { + if (sorts != null && sorts.size() > 0) { searchQueryCounters.sortCounter.add(1); } } 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 4f9f7a558e4fb..c43bf565f9c3d 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -60,7 +60,6 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AtomicArray; import org.opensearch.common.util.concurrent.CountDown; import org.opensearch.core.action.ActionListener; @@ -139,6 +138,13 @@ public class TransportSearchAction extends HandledTransportAction SEARCH_QUERY_CATEGORIZATION_ENABLED_SETTING = Setting.boolSetting( + "search.query.categorization.enabled", + false, + Setting.Property.NodeScope, + Setting.Property.Dynamic + ); + // cluster level setting for timeout based search cancellation. If search request level parameter is present then that will take // precedence over the cluster setting value public static final String SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING_KEY = "search.cancel_after_time_interval"; @@ -179,6 +185,8 @@ public class TransportSearchAction extends HandledTransportAction Date: Thu, 12 Oct 2023 03:31:50 -0700 Subject: [PATCH 16/35] Minor fix Signed-off-by: Siddhant Deshmukh --- .../java/org/opensearch/common/settings/ClusterSettings.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index 90f91dcb7c553..13b8971a0b5de 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -377,6 +377,7 @@ public void apply(Settings value, Settings current, Settings previous) { TransportSearchAction.SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING, TransportSearchAction.SEARCH_REQUEST_STATS_ENABLED, TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED, + TransportSearchAction.SEARCH_QUERY_CATEGORIZATION_ENABLED_SETTING, RemoteClusterService.REMOTE_CLUSTER_SKIP_UNAVAILABLE, SniffConnectionStrategy.REMOTE_CONNECTIONS_PER_CLUSTER, RemoteClusterService.REMOTE_INITIAL_CONNECTION_TIMEOUT_SETTING, From 792d4c43989d0afb8e094d7117ffd04d8c1f25fb Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Fri, 13 Oct 2023 02:25:59 -0700 Subject: [PATCH 17/35] Add initialization in callback Signed-off-by: Siddhant Deshmukh --- .../action/search/TransportSearchAction.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 c43bf565f9c3d..11deabd59f74e 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -191,7 +191,7 @@ public class TransportSearchAction extends HandledTransportAction Date: Fri, 13 Oct 2023 03:40:31 -0700 Subject: [PATCH 18/35] Address comments Signed-off-by: Siddhant Deshmukh --- .../org/opensearch/action/search/SearchQueryCategorizor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index 0da95845d1b3b..a7311f403c0ad 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -28,7 +28,7 @@ public class SearchQueryCategorizor { private static final Logger log = LogManager.getLogger(SearchQueryCategorizor.class); - public static SearchQueryCounters searchQueryCounters; + public SearchQueryCounters searchQueryCounters; public SearchQueryCategorizor(MetricsRegistry metricsRegistry) { searchQueryCounters = new SearchQueryCounters(metricsRegistry); From 254578853188391e3f635e3a79ae5e4126397a10 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Mon, 16 Oct 2023 13:22:09 -0700 Subject: [PATCH 19/35] Add exception handling Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizor.java | 16 ++++++++++++---- .../action/search/SearchQueryCounters.java | 6 ++++++ .../action/search/TransportSearchAction.java | 6 +++++- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java index a7311f403c0ad..9f6e4de95a704 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java @@ -55,12 +55,20 @@ private void incrementQueryAggregationCounters(AggregatorFactories.Builder aggre } } - private static void incrementQueryTypeCounters(QueryBuilder topLevelQueryBuilder) { - QueryBuilderVisitor searhQueryVisitor = new SearchQueryCategorizingVisitor(searchQueryCounters); - topLevelQueryBuilder.visit(searhQueryVisitor); + private void incrementQueryTypeCounters(QueryBuilder topLevelQueryBuilder) { + if (topLevelQueryBuilder == null) { + searchQueryCounters.skippedCounter.add(1); + return; + } + QueryBuilderVisitor searchQueryVisitor = new SearchQueryCategorizingVisitor(searchQueryCounters); + topLevelQueryBuilder.visit(searchQueryVisitor); } - private static void logQueryShape(QueryBuilder topLevelQueryBuilder) { + private void logQueryShape(QueryBuilder topLevelQueryBuilder) { + if (topLevelQueryBuilder == null) { + searchQueryCounters.skippedCounter.add(1); + return; + } QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); topLevelQueryBuilder.visit(shapeVisitor); String queryShapeJson = shapeVisitor.prettyPrintTree(" "); 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 c0d8fabc6bf36..e44c88262e712 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java @@ -30,6 +30,7 @@ public class SearchQueryCounters { public final Counter regexCounter; public final Counter sortCounter; + public final Counter skippedCounter; public final Counter termCounter; public final Counter totalCounter; public final Counter wildcardCounter; @@ -86,6 +87,11 @@ public SearchQueryCounters(MetricsRegistry metricsRegistry) { "Counter for the number of top level and nested regex search queries", "0" ); + this.skippedCounter = metricsRegistry.createCounter( + "skippedQueryCounter", + "Counter for the number queries skipped due to error", + "0" + ); this.sortCounter = metricsRegistry.createCounter( "sortSearchQueryCounter", "Counter for the number of top level sort search queries", 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 11deabd59f74e..632abad2b06d1 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -516,7 +516,11 @@ private void executeRequest( } if (isSearchQueryCategorizationEnabled) { - searchQueryCategorizor.categorize(searchRequest.source()); + try { + searchQueryCategorizor.categorize(searchRequest.source()); + } catch (Exception e) { + logger.error("Error while trying to categorize the query."); + } } ActionListener rewriteListener = ActionListener.wrap(source -> { From ddf4594e660ddf3ec529e8133c79ac6b8ea07d0f Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Mon, 16 Oct 2023 14:32:46 -0700 Subject: [PATCH 20/35] Refactoring and renaming Signed-off-by: Siddhant Deshmukh --- ...Categorizor.java => SearchQueryCategorizer.java} | 10 +++++----- .../action/search/TransportSearchAction.java | 8 ++++---- .../org/opensearch/common/util/FeatureFlags.java | 13 +------------ 3 files changed, 10 insertions(+), 21 deletions(-) rename server/src/main/java/org/opensearch/action/search/{SearchQueryCategorizor.java => SearchQueryCategorizer.java} (90%) diff --git a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java similarity index 90% rename from server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java rename to server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java index 9f6e4de95a704..bfe96be3e05d1 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java @@ -24,13 +24,13 @@ * Class to categorize the search queries based on the type and increment the relevant counters. * Class also logs the query shape. */ -public class SearchQueryCategorizor { +public class SearchQueryCategorizer { - private static final Logger log = LogManager.getLogger(SearchQueryCategorizor.class); + private static final Logger log = LogManager.getLogger(SearchQueryCategorizer.class); public SearchQueryCounters searchQueryCounters; - public SearchQueryCategorizor(MetricsRegistry metricsRegistry) { + public SearchQueryCategorizer(MetricsRegistry metricsRegistry) { searchQueryCounters = new SearchQueryCounters(metricsRegistry); } @@ -71,8 +71,8 @@ private void logQueryShape(QueryBuilder topLevelQueryBuilder) { } QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); topLevelQueryBuilder.visit(shapeVisitor); - String queryShapeJson = shapeVisitor.prettyPrintTree(" "); - log.debug("Query shape : " + queryShapeJson); + String indentedQueryShape = shapeVisitor.prettyPrintTree(" "); + log.debug("Query shape : " + indentedQueryShape); } } 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 632abad2b06d1..cf04d9b2618df 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -191,7 +191,7 @@ public class TransportSearchAction extends HandledTransportAction featureFlag) { true, Property.NodeScope ); - - public static final Setting QUERY_CATEGORIZATION_SETTING = Setting.boolSetting( - QUERY_CATEOGORIZATION, - false, - Property.NodeScope - ); } From 88660f42ae5abadf73f5b8037f9b951b46a2cc89 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Mon, 16 Oct 2023 14:59:12 -0700 Subject: [PATCH 21/35] Minor fix Signed-off-by: Siddhant Deshmukh --- .../org/opensearch/action/search/SearchQueryCategorizer.java | 2 -- .../src/main/java/org/opensearch/common/util/FeatureFlags.java | 2 +- 2 files changed, 1 insertion(+), 3 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 bfe96be3e05d1..83e4ecbc7750d 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java @@ -57,7 +57,6 @@ private void incrementQueryAggregationCounters(AggregatorFactories.Builder aggre private void incrementQueryTypeCounters(QueryBuilder topLevelQueryBuilder) { if (topLevelQueryBuilder == null) { - searchQueryCounters.skippedCounter.add(1); return; } QueryBuilderVisitor searchQueryVisitor = new SearchQueryCategorizingVisitor(searchQueryCounters); @@ -66,7 +65,6 @@ private void incrementQueryTypeCounters(QueryBuilder topLevelQueryBuilder) { private void logQueryShape(QueryBuilder topLevelQueryBuilder) { if (topLevelQueryBuilder == null) { - searchQueryCounters.skippedCounter.add(1); return; } QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 0fb5cbb18df19..4e9b417e3433b 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -59,7 +59,7 @@ public class FeatureFlags { * Gates the optimization of datetime formatters caching along with change in default datetime formatter. */ public static final String DATETIME_FORMATTER_CACHING = "opensearch.experimental.optimization.datetime_formatter_caching.enabled"; - + /** * Should store the settings from opensearch.yml. */ From 3e2fedda989c000726326122de05b46fb3fd8c5e Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Mon, 16 Oct 2023 17:11:25 -0700 Subject: [PATCH 22/35] Fix changelog and minor refactoring Signed-off-by: Siddhant Deshmukh --- CHANGELOG.md | 2 ++ .../org/opensearch/action/search/SearchQueryCategorizer.java | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca324bdfd250c..c0930a68ed489 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Added - Per request phase latency ([#10351](https://github.com/opensearch-project/OpenSearch/issues/10351)) - [Remote Store] Add repository stats for remote store([#10567](https://github.com/opensearch-project/OpenSearch/pull/10567)) +- Add search query categorizer ([#10255](https://github.com/opensearch-project/OpenSearch/pull/10255)) + ### Dependencies - Bump `com.google.api.grpc:proto-google-common-protos` from 2.10.0 to 2.25.1 ([#10208](https://github.com/opensearch-project/OpenSearch/pull/10208), [#10298](https://github.com/opensearch-project/OpenSearch/pull/10298)) 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 83e4ecbc7750d..50b2eaa54f0af 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java @@ -28,7 +28,7 @@ public class SearchQueryCategorizer { private static final Logger log = LogManager.getLogger(SearchQueryCategorizer.class); - public SearchQueryCounters searchQueryCounters; + public final SearchQueryCounters searchQueryCounters; public SearchQueryCategorizer(MetricsRegistry metricsRegistry) { searchQueryCounters = new SearchQueryCounters(metricsRegistry); From b9fa336ed79c3dcf30fb99742f0431920e7e2573 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Tue, 17 Oct 2023 12:19:10 -0700 Subject: [PATCH 23/35] Address review comments Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizer.java | 3 +- .../SearchQueryCategorizingVisitor.java | 23 +++++++------- .../action/search/SearchQueryCounters.java | 30 +++++++++---------- .../action/search/TransportSearchAction.java | 18 +++++------ 4 files changed, 37 insertions(+), 37 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 50b2eaa54f0af..f9a1c0c4393f3 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java @@ -69,8 +69,7 @@ private void logQueryShape(QueryBuilder topLevelQueryBuilder) { } QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); topLevelQueryBuilder.visit(shapeVisitor); - String indentedQueryShape = shapeVisitor.prettyPrintTree(" "); - log.debug("Query shape : " + indentedQueryShape); + log.debug("Query shape : {}", shapeVisitor.prettyPrintTree(" ")); } } 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 ac0dcf701a2ed..b15ffb0e86ce4 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java @@ -28,6 +28,7 @@ * Increments the counters related to Search Query type. */ public class SearchQueryCategorizingVisitor implements QueryBuilderVisitor { + public static final String LEVEL_TAG = "level"; private final int level; private final SearchQueryCounters searchQueryCounters; @@ -42,27 +43,27 @@ private SearchQueryCategorizingVisitor(SearchQueryCounters counters, int level) public void accept(QueryBuilder qb) { if (qb instanceof BoolQueryBuilder) { - searchQueryCounters.boolCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.boolCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof FunctionScoreQueryBuilder) { - searchQueryCounters.functionScoreCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.functionScoreCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof MatchQueryBuilder) { - searchQueryCounters.matchCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.matchCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof MatchPhraseQueryBuilder) { - searchQueryCounters.matchPhrasePrefixCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.matchPhrasePrefixCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof MultiMatchQueryBuilder) { - searchQueryCounters.multiMatchCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.multiMatchCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof QueryStringQueryBuilder) { - searchQueryCounters.queryStringQueryCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.queryStringQueryCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof RangeQueryBuilder) { - searchQueryCounters.rangeCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.rangeCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof RegexpQueryBuilder) { - searchQueryCounters.regexCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.regexCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof TermQueryBuilder) { - searchQueryCounters.termCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.termCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else if (qb instanceof WildcardQueryBuilder) { - searchQueryCounters.wildcardCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.wildcardCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } else { - searchQueryCounters.otherQueryCounter.add(1, Tags.create().addTag("level", level)); + searchQueryCounters.otherQueryCounter.add(1, Tags.create().addTag(LEVEL_TAG, level)); } } 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 e44c88262e712..da3a24af3e2a0 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java @@ -38,77 +38,77 @@ public class SearchQueryCounters { public SearchQueryCounters(MetricsRegistry metricsRegistry) { this.metricsRegistry = metricsRegistry; this.aggCounter = metricsRegistry.createCounter( - "aggSearchQueryCounter", + "search.query.type.agg.count", "Counter for the number of top level agg search queries", "0" ); this.boolCounter = metricsRegistry.createCounter( - "boolSearchQueryCounter", + "search.query.type.bool.count", "Counter for the number of top level and nested bool search queries", "0" ); this.functionScoreCounter = metricsRegistry.createCounter( - "functionScoreSearchQueryCounter", + "search.query.type.functionscore.count", "Counter for the number of top level and nested function score search queries", "0" ); this.matchCounter = metricsRegistry.createCounter( - "matchSearchQueryCounter", + "search.query.type.match.count", "Counter for the number of top level and nested match search queries", "0" ); this.matchPhrasePrefixCounter = metricsRegistry.createCounter( - "matchPhrasePrefixSearchQueryCounter", + "search.query.type.matchphrase.count", "Counter for the number of top level and nested match phrase prefix search queries", "0" ); this.multiMatchCounter = metricsRegistry.createCounter( - "multiMatchSearchQueryCounter", + "search.query.type.multimatch.count", "Counter for the number of top level and nested multi match search queries", "0" ); this.otherQueryCounter = metricsRegistry.createCounter( - "otherSearchQueryCounter", + "search.query.type.other.count", "Counter for the number of top level and nested search queries that do not match any other categories", "0" ); this.queryStringQueryCounter = metricsRegistry.createCounter( - "queryStringQuerySearchQueryCounter", + "search.query.type.querystringquery.count", "Counter for the number of top level and nested queryStringQuery search queries", "0" ); this.rangeCounter = metricsRegistry.createCounter( - "rangeSearchQueryCounter", + "search.query.type.range.count", "Counter for the number of top level and nested range search queries", "0" ); this.regexCounter = metricsRegistry.createCounter( - "regexSearchQueryCounter", + "search.query.type.regex.count", "Counter for the number of top level and nested regex search queries", "0" ); this.skippedCounter = metricsRegistry.createCounter( - "skippedQueryCounter", + "search.query.type.skipped.count", "Counter for the number queries skipped due to error", "0" ); this.sortCounter = metricsRegistry.createCounter( - "sortSearchQueryCounter", + "search.query.type.sort.count", "Counter for the number of top level sort search queries", "0" ); this.termCounter = metricsRegistry.createCounter( - "termSearchQueryCounter", + "search.query.type.term.count", "Counter for the number of top level and nested term search queries", "0" ); this.totalCounter = metricsRegistry.createCounter( - "totalSearchQueryCounter", + "search.query.type.total.count", "Counter for the number of top level and nested search queries", "0" ); this.wildcardCounter = metricsRegistry.createCounter( - "wildcardSearchQueryCounter", + "search.query.type.wildcard.count", "Counter for the number of top level and nested wildcard search queries", "0" ); 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 cf04d9b2618df..9bde1fa889ee4 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -138,8 +138,8 @@ public class TransportSearchAction extends HandledTransportAction SEARCH_QUERY_CATEGORIZATION_ENABLED_SETTING = Setting.boolSetting( - "search.query.categorization.enabled", + public static final Setting SEARCH_QUERY_METRICS_ENABLED_SETTING = Setting.boolSetting( + "search.query.metrics.enabled", false, Setting.Property.NodeScope, Setting.Property.Dynamic @@ -185,7 +185,7 @@ public class TransportSearchAction extends HandledTransportAction Date: Tue, 17 Oct 2023 14:07:43 -0700 Subject: [PATCH 24/35] Add unit tests Signed-off-by: Siddhant Deshmukh --- .../search/SearchQueryCategorizerTests.java | 205 ++++++++++++++++++ .../index/query/QueryShapeVisitorTests.java | 32 +++ 2 files changed, 237 insertions(+) create mode 100644 server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java create mode 100644 server/src/test/java/org/opensearch/index/query/QueryShapeVisitorTests.java diff --git a/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java b/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java new file mode 100644 index 0000000000000..05ab78f9d8872 --- /dev/null +++ b/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java @@ -0,0 +1,205 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.action.search; + +import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.BoostingQueryBuilder; +import org.opensearch.index.query.MatchNoneQueryBuilder; +import org.opensearch.index.query.MatchQueryBuilder; +import org.opensearch.index.query.MultiMatchQueryBuilder; +import org.opensearch.index.query.QueryBuilders; +import org.opensearch.index.query.QueryStringQueryBuilder; +import org.opensearch.index.query.RangeQueryBuilder; +import org.opensearch.index.query.RegexpQueryBuilder; +import org.opensearch.index.query.TermQueryBuilder; +import org.opensearch.index.query.WildcardQueryBuilder; +import org.opensearch.index.query.functionscore.FunctionScoreQueryBuilder; +import org.opensearch.search.aggregations.bucket.terms.MultiTermsAggregationBuilder; +import org.opensearch.search.aggregations.support.MultiTermsValuesSourceConfig; +import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.search.sort.ScoreSortBuilder; +import org.opensearch.search.sort.SortOrder; +import org.opensearch.telemetry.metrics.Counter; +import org.opensearch.telemetry.metrics.MetricsRegistry; +import org.opensearch.telemetry.metrics.tags.Tags; +import org.opensearch.test.OpenSearchTestCase; +import org.junit.Before; + +import java.util.Arrays; + +import org.mockito.Mockito; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.when; + +public class SearchQueryCategorizerTests extends OpenSearchTestCase { + + private MetricsRegistry metricsRegistry; + + private SearchQueryCategorizer searchQueryCategorizer; + + @Before + public void setup() { + metricsRegistry = mock(MetricsRegistry.class); + when(metricsRegistry.createCounter(any(String.class), any(String.class), any(String.class))).thenAnswer( + invocation -> mock(Counter.class) + ); + searchQueryCategorizer = new SearchQueryCategorizer(metricsRegistry); + } + + public void testAggregationsQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.aggregation( + new MultiTermsAggregationBuilder("agg1").terms( + Arrays.asList( + new MultiTermsValuesSourceConfig.Builder().setFieldName("username").build(), + new MultiTermsValuesSourceConfig.Builder().setFieldName("rating").build() + ) + ) + ); + sourceBuilder.size(0); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.aggCounter).add(eq(1.0d)); + } + + public void testBoolQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + sourceBuilder.query(new BoolQueryBuilder().must(new MatchQueryBuilder("searchText", "fox"))); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.boolCounter).add(eq(1.0d), any(Tags.class)); + Mockito.verify(searchQueryCategorizer.searchQueryCounters.matchCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testFunctionScoreQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + sourceBuilder.query(new FunctionScoreQueryBuilder(QueryBuilders.prefixQuery("text", "bro"))); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.functionScoreCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testMatchQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + sourceBuilder.query(QueryBuilders.matchQuery("tags", "php")); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.matchCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testMatchPhraseQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + sourceBuilder.query(QueryBuilders.matchPhraseQuery("tags", "php")); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.matchPhrasePrefixCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testMultiMatchQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + sourceBuilder.query(new MultiMatchQueryBuilder("foo bar", "myField")); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.multiMatchCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testOtherQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + BoostingQueryBuilder queryBuilder = new BoostingQueryBuilder( + new TermQueryBuilder("unmapped_field", "foo"), + new MatchNoneQueryBuilder() + ); + sourceBuilder.query(queryBuilder); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.otherQueryCounter, times(2)).add(eq(1.0d), any(Tags.class)); + Mockito.verify(searchQueryCategorizer.searchQueryCounters.termCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testQueryStringQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + QueryStringQueryBuilder queryBuilder = new QueryStringQueryBuilder("foo:*"); + sourceBuilder.query(queryBuilder); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.queryStringQueryCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testRangeQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + RangeQueryBuilder rangeQuery = new RangeQueryBuilder("date"); + rangeQuery.gte("1970-01-01"); + rangeQuery.lt("1982-01-01"); + sourceBuilder.query(rangeQuery); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.rangeCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testRegexQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.query(new RegexpQueryBuilder("field", "text")); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.regexCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testSortQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.query(QueryBuilders.matchQuery("tags", "ruby")); + sourceBuilder.sort("creationDate", SortOrder.DESC); + sourceBuilder.sort(new ScoreSortBuilder()); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.matchCounter).add(eq(1.0d), any(Tags.class)); + Mockito.verify(searchQueryCategorizer.searchQueryCounters.sortCounter).add(eq(1.0d)); + } + + public void testTermQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + sourceBuilder.query(QueryBuilders.termQuery("field", "value2")); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.termCounter).add(eq(1.0d), any(Tags.class)); + } + + public void testWildcardQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + sourceBuilder.query(new WildcardQueryBuilder("field", "text")); + + searchQueryCategorizer.categorize(sourceBuilder); + + Mockito.verify(searchQueryCategorizer.searchQueryCounters.wildcardCounter).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 new file mode 100644 index 0000000000000..561025e197b0b --- /dev/null +++ b/server/src/test/java/org/opensearch/index/query/QueryShapeVisitorTests.java @@ -0,0 +1,32 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.query; + +import org.opensearch.test.OpenSearchTestCase; + +import static org.junit.Assert.assertEquals; + +public 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"))) + .should( + new BoolQueryBuilder().must(new MatchQueryBuilder("text", "this is some text")) + .mustNot(new RegexpQueryBuilder("color", "red.*")) + ) + .must(new TermsQueryBuilder("genre", "action", "drama", "romance")); + QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); + builder.visit(shapeVisitor); + // {"type":"bool","must"[{"type":"term"},{"type":"terms"}],"filter"[{"type":"constant_score","filter"[{"type":"range"}]}],"should"[{"type":"bool","must"[{"type":"match"}],"must_not"[{"type":"regexp"}]}]} + assertEquals( + "{\"type\":\"bool\",\"must\"[{\"type\":\"term\"},{\"type\":\"terms\"}],\"filter\"[{\"type\":\"constant_score\",\"filter\"[{\"type\":\"range\"}]}],\"should\"[{\"type\":\"bool\",\"must\"[{\"type\":\"match\"}],\"must_not\"[{\"type\":\"regexp\"}]}]}", + shapeVisitor.toJson() + ); + } +} From 00fdef638c51826ead6e7e2a6f7bc9d89fbbac34 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Tue, 17 Oct 2023 22:34:55 -0700 Subject: [PATCH 25/35] Address review comments and add complex query unit test Signed-off-by: Siddhant Deshmukh --- .../action/search/SearchQueryCategorizer.java | 2 +- .../SearchQueryCategorizingVisitor.java | 2 +- .../action/search/SearchQueryCounters.java | 30 +++++++++---------- .../action/search/TransportSearchAction.java | 14 ++++----- .../index/query/QueryShapeVisitor.java | 4 +-- .../search/SearchQueryCategorizerTests.java | 22 ++++++++++++++ 6 files changed, 48 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 f9a1c0c4393f3..df6a034d826bd 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java @@ -24,7 +24,7 @@ * Class to categorize the search queries based on the type and increment the relevant counters. * Class also logs the query shape. */ -public class SearchQueryCategorizer { +public final class SearchQueryCategorizer { private static final Logger log = LogManager.getLogger(SearchQueryCategorizer.class); 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 b15ffb0e86ce4..f95c28a1d11e3 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizingVisitor.java @@ -28,7 +28,7 @@ * Increments the counters related to Search Query type. */ public class SearchQueryCategorizingVisitor implements QueryBuilderVisitor { - public static final String LEVEL_TAG = "level"; + 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 da3a24af3e2a0..441c32256ef4a 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCounters.java @@ -40,77 +40,77 @@ public SearchQueryCounters(MetricsRegistry metricsRegistry) { this.aggCounter = metricsRegistry.createCounter( "search.query.type.agg.count", "Counter for the number of top level agg search queries", - "0" + "1" ); this.boolCounter = metricsRegistry.createCounter( "search.query.type.bool.count", "Counter for the number of top level and nested bool search queries", - "0" + "1" ); this.functionScoreCounter = metricsRegistry.createCounter( "search.query.type.functionscore.count", "Counter for the number of top level and nested function score search queries", - "0" + "1" ); this.matchCounter = metricsRegistry.createCounter( "search.query.type.match.count", "Counter for the number of top level and nested match search queries", - "0" + "1" ); this.matchPhrasePrefixCounter = metricsRegistry.createCounter( "search.query.type.matchphrase.count", "Counter for the number of top level and nested match phrase prefix search queries", - "0" + "1" ); this.multiMatchCounter = metricsRegistry.createCounter( "search.query.type.multimatch.count", "Counter for the number of top level and nested multi match search queries", - "0" + "1" ); 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", - "0" + "1" ); this.queryStringQueryCounter = metricsRegistry.createCounter( "search.query.type.querystringquery.count", "Counter for the number of top level and nested queryStringQuery search queries", - "0" + "1" ); this.rangeCounter = metricsRegistry.createCounter( "search.query.type.range.count", "Counter for the number of top level and nested range search queries", - "0" + "1" ); this.regexCounter = metricsRegistry.createCounter( "search.query.type.regex.count", "Counter for the number of top level and nested regex search queries", - "0" + "1" ); this.skippedCounter = metricsRegistry.createCounter( "search.query.type.skipped.count", "Counter for the number queries skipped due to error", - "0" + "1" ); this.sortCounter = metricsRegistry.createCounter( "search.query.type.sort.count", "Counter for the number of top level sort search queries", - "0" + "1" ); this.termCounter = metricsRegistry.createCounter( "search.query.type.term.count", "Counter for the number of top level and nested term search queries", - "0" + "1" ); this.totalCounter = metricsRegistry.createCounter( "search.query.type.total.count", "Counter for the number of top level and nested search queries", - "0" + "1" ); this.wildcardCounter = metricsRegistry.createCounter( "search.query.type.wildcard.count", "Counter for the number of top level and nested wildcard search queries", - "0" + "1" ); } } 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 9bde1fa889ee4..8e36352e58b8b 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -185,7 +185,7 @@ public class TransportSearchAction extends HandledTransportAction childVisitorList = new ArrayList<>(); QueryBuilderVisitor childVisitorWrapper = new QueryBuilderVisitor() { @@ -55,7 +55,7 @@ public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) { return childVisitorWrapper; } - public String toJson() { + String toJson() { StringBuilder outputBuilder = new StringBuilder("{\"type\":\"").append(queryType.get()).append("\""); for (Map.Entry> entry : childVisitors.entrySet()) { outputBuilder.append(",\"").append(entry.getKey().name().toLowerCase(Locale.ROOT)).append("\"["); 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 05ab78f9d8872..b990dfcee699e 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java @@ -20,6 +20,7 @@ import org.opensearch.index.query.TermQueryBuilder; import org.opensearch.index.query.WildcardQueryBuilder; import org.opensearch.index.query.functionscore.FunctionScoreQueryBuilder; +import org.opensearch.search.aggregations.bucket.range.RangeAggregationBuilder; import org.opensearch.search.aggregations.bucket.terms.MultiTermsAggregationBuilder; import org.opensearch.search.aggregations.support.MultiTermsValuesSourceConfig; import org.opensearch.search.builder.SearchSourceBuilder; @@ -202,4 +203,25 @@ public void testWildcardQuery() { Mockito.verify(searchQueryCategorizer.searchQueryCounters.wildcardCounter).add(eq(1.0d), any(Tags.class)); } + + public void testComplexQuery() { + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.size(50); + + 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); + 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)); + Mockito.verify(searchQueryCategorizer.searchQueryCounters.matchCounter).add(eq(1.0d), any(Tags.class)); + Mockito.verify(searchQueryCategorizer.searchQueryCounters.regexCounter).add(eq(1.0d), any(Tags.class)); + Mockito.verify(searchQueryCategorizer.searchQueryCounters.boolCounter).add(eq(1.0d), any(Tags.class)); + Mockito.verify(searchQueryCategorizer.searchQueryCounters.aggCounter).add(eq(1.0d)); + } } From 0d7bba17869aaed58267bffd01609ce7ebb76ec3 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Tue, 17 Oct 2023 22:42:51 -0700 Subject: [PATCH 26/35] Add sort order as a tag to sort counter Signed-off-by: Siddhant Deshmukh --- .../opensearch/action/search/SearchQueryCategorizer.java | 8 +++++++- .../action/search/SearchQueryCategorizerTests.java | 2 +- 2 files changed, 8 insertions(+), 2 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 df6a034d826bd..1049cf18a0ff1 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java +++ b/server/src/main/java/org/opensearch/action/search/SearchQueryCategorizer.java @@ -17,8 +17,10 @@ import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.search.sort.SortBuilder; import org.opensearch.telemetry.metrics.MetricsRegistry; +import org.opensearch.telemetry.metrics.tags.Tags; import java.util.List; +import java.util.ListIterator; /** * Class to categorize the search queries based on the type and increment the relevant counters. @@ -45,7 +47,11 @@ public void categorize(SearchSourceBuilder source) { private void incrementQuerySortCounters(List> sorts) { if (sorts != null && sorts.size() > 0) { - searchQueryCounters.sortCounter.add(1); + 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/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java b/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java index b990dfcee699e..568ab5a7dbc8c 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchQueryCategorizerTests.java @@ -181,7 +181,7 @@ public void testSortQuery() { searchQueryCategorizer.categorize(sourceBuilder); Mockito.verify(searchQueryCategorizer.searchQueryCounters.matchCounter).add(eq(1.0d), any(Tags.class)); - Mockito.verify(searchQueryCategorizer.searchQueryCounters.sortCounter).add(eq(1.0d)); + Mockito.verify(searchQueryCategorizer.searchQueryCounters.sortCounter, times(2)).add(eq(1.0d), any(Tags.class)); } public void testTermQuery() { From 8b21e6f65a1cdfea1bf565cf8e256b4b3f66a757 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 09:17:21 -0700 Subject: [PATCH 27/35] 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"))) From 6c59f8d256597d8ca43aae43e0ccf6c0589c2696 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 09:51:57 -0700 Subject: [PATCH 28/35] Address final comments Signed-off-by: Siddhant Deshmukh --- .../org/opensearch/index/query/QueryShapeVisitorTests.java | 1 - .../java/org/opensearch/snapshots/SnapshotResiliencyTests.java | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) 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 53875cf078941..3c989bbe28595 100644 --- a/server/src/test/java/org/opensearch/index/query/QueryShapeVisitorTests.java +++ b/server/src/test/java/org/opensearch/index/query/QueryShapeVisitorTests.java @@ -23,7 +23,6 @@ public void testQueryShapeVisitor() { .must(new TermsQueryBuilder("genre", "action", "drama", "romance")); QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(); builder.visit(shapeVisitor); - // {"type":"bool","must"[{"type":"term"},{"type":"terms"}],"filter"[{"type":"constant_score","filter"[{"type":"range"}]}],"should"[{"type":"bool","must"[{"type":"match"}],"must_not"[{"type":"regexp"}]}]} assertEquals( "{\"type\":\"bool\",\"must\"[{\"type\":\"term\"},{\"type\":\"terms\"}],\"filter\"[{\"type\":\"constant_score\",\"filter\"[{\"type\":\"range\"}]}],\"should\"[{\"type\":\"bool\",\"must\"[{\"type\":\"match\"}],\"must_not\"[{\"type\":\"regexp\"}]}]}", shapeVisitor.toJson() diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 73758f3852227..2f9f38d18a064 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -222,6 +222,7 @@ import org.opensearch.search.query.QueryPhase; import org.opensearch.snapshots.mockstore.MockEventuallyConsistentRepository; import org.opensearch.tasks.TaskResourceTrackingService; +import org.opensearch.telemetry.metrics.noop.NoopMetricsRegistry; import org.opensearch.telemetry.tracing.noop.NoopTracer; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.disruption.DisruptableMockTransport; @@ -2303,7 +2304,7 @@ public void onFailure(final Exception e) { client ), null, - null + NoopMetricsRegistry.INSTANCE ) ); actions.put( From f73ec2a372bffc77c6b78826360dffdefce5c761 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 09:58:05 -0700 Subject: [PATCH 29/35] Build fix Signed-off-by: Siddhant Deshmukh --- .../main/java/org/opensearch/index/query/QueryShapeVisitor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e484da0f48a2c..3ba13bc7a2da4 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 */ -final class QueryShapeVisitor implements QueryBuilderVisitor { +public final class QueryShapeVisitor implements QueryBuilderVisitor { private final SetOnce queryType = new SetOnce<>(); private final Map> childVisitors = new EnumMap<>(BooleanClause.Occur.class); From f0395d50d0add9122fa4cd4330e4b81b5e72e368 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 10:52:18 -0700 Subject: [PATCH 30/35] Fix build tests failure Signed-off-by: Siddhant Deshmukh --- .../opensearch/action/search/SearchQueryCategorizerTests.java | 2 +- .../java/org/opensearch/index/query/QueryShapeVisitorTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 30c9937c98abc..a2e301143d694 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; -final class SearchQueryCategorizerTests extends OpenSearchTestCase { +public final class SearchQueryCategorizerTests extends OpenSearchTestCase { private MetricsRegistry metricsRegistry; 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 3c989bbe28595..18b814aec61c2 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; -final class QueryShapeVisitorTests extends OpenSearchTestCase { +public 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"))) From a402aa0dc8d42b2e4b51b5c7463ed073873e8bb6 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 17:24:41 -0700 Subject: [PATCH 31/35] Minor fix Signed-off-by: Siddhant Deshmukh --- .../java/org/opensearch/common/settings/ClusterSettings.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index 13b8971a0b5de..76883c200542e 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -377,7 +377,7 @@ public void apply(Settings value, Settings current, Settings previous) { TransportSearchAction.SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING, TransportSearchAction.SEARCH_REQUEST_STATS_ENABLED, TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED, - TransportSearchAction.SEARCH_QUERY_CATEGORIZATION_ENABLED_SETTING, + TransportSearchAction.SEARCH_QUERY_METRICS_ENABLED_SETTING, RemoteClusterService.REMOTE_CLUSTER_SKIP_UNAVAILABLE, SniffConnectionStrategy.REMOTE_CONNECTIONS_PER_CLUSTER, RemoteClusterService.REMOTE_INITIAL_CONNECTION_TIMEOUT_SETTING, From fb08ddac10d025720e7c990ccd3367d3e50ea3bb Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 17:26:25 -0700 Subject: [PATCH 32/35] Minor fix Signed-off-by: Siddhant Deshmukh --- CHANGELOG.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0930a68ed489..1c22640d45c9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,8 +18,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Remote cluster state] Upload global metadata in cluster state to remote store([#10404](https://github.com/opensearch-project/OpenSearch/pull/10404)) - [Remote cluster state] Download functionality of global metadata from remote store ([#10535](https://github.com/opensearch-project/OpenSearch/pull/10535)) - [Remote cluster state] Restore global metadata from remote store when local state is lost after quorum loss ([#10404](https://github.com/opensearch-project/OpenSearch/pull/10404)) -- Configurable merge policy for index with an option to choose from LogByteSize and Tiered merge policy ([#9992](https://github.com/opensearch-project/OpenSearch/pull/9992)) -- Add search query categorizor ([#10255](https://github.com/opensearch-project/OpenSearch/pull/10255)) ### Dependencies - Bump `log4j-core` from 2.18.0 to 2.19.0 From c6a27a6b5073fc70db795cd03af3eed97161d50d Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 18:11:17 -0700 Subject: [PATCH 33/35] Empty commit Signed-off-by: Siddhant Deshmukh From 3ee92360b732b3867311c14b1ed49a0088ca038a Mon Sep 17 00:00:00 2001 From: Michael Froh Date: Wed, 18 Oct 2023 21:05:50 -0700 Subject: [PATCH 34/35] Remove extra newline Signed-off-by: Michael Froh --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c22640d45c9f..552c277789dd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,7 +93,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Remote Store] Add repository stats for remote store([#10567](https://github.com/opensearch-project/OpenSearch/pull/10567)) - Add search query categorizer ([#10255](https://github.com/opensearch-project/OpenSearch/pull/10255)) - ### Dependencies - Bump `com.google.api.grpc:proto-google-common-protos` from 2.10.0 to 2.25.1 ([#10208](https://github.com/opensearch-project/OpenSearch/pull/10208), [#10298](https://github.com/opensearch-project/OpenSearch/pull/10298)) - Bump Lucene from 9.7.0 to 9.8.0 ([10276](https://github.com/opensearch-project/OpenSearch/pull/10276)) From b17245058bc9a494adf36d117a99437a82a96df9 Mon Sep 17 00:00:00 2001 From: Siddhant Deshmukh Date: Wed, 18 Oct 2023 18:11:17 -0700 Subject: [PATCH 35/35] Empty commit Signed-off-by: Siddhant Deshmukh