From 47a167fa6110c08edbbb4095ea20723fd99ca1c2 Mon Sep 17 00:00:00 2001 From: Chenyang Ji Date: Thu, 7 Dec 2023 16:06:10 -0800 Subject: [PATCH 1/7] support dynamically adding SearchRequestOperationsListener Signed-off-by: Chenyang Ji --- .../SearchRequestOperationsListener.java | 23 ++++++ .../action/search/SearchRequestSlowLog.java | 27 +++++++ .../action/search/SearchRequestStats.java | 29 +++++++- .../action/search/TransportSearchAction.java | 71 ++++++++++--------- .../common/settings/ClusterSettings.java | 3 +- .../main/java/org/opensearch/node/Node.java | 2 +- .../snapshots/SnapshotResiliencyTests.java | 2 - 7 files changed, 118 insertions(+), 39 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java index 19ce0beb3c493..9132aac387085 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java @@ -21,6 +21,7 @@ */ @InternalApi abstract class SearchRequestOperationsListener { + protected boolean enabled; abstract void onPhaseStart(SearchPhaseContext context); @@ -32,6 +33,28 @@ void onRequestStart(SearchRequestContext searchRequestContext) {} void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {} + public void setEnabled(boolean enabled) { + this.enabled = enabled; + if (enabled) { + register(); + } else { + deregister(); + } + } + + + /** + * Handler function to register this listener to certain components + * This function will be called when the listener is enabled. + */ + protected void register() {} + + /** + * Handler function to deregister this listener from certain components + * This function will be called when the listener is disabled. + */ + protected void deregister() {} + /** * Holder of Composite Listeners * diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java index a55cfd463a78f..0971e252996a3 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java @@ -160,6 +160,22 @@ void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequest } } + /** + * register this listener to TransportSearchAction + */ + @Override + protected void register() { + TransportSearchAction.addSearchOperationsListener(this); + } + + /** + * deregister this listener to TransportSearchAction + */ + @Override + protected void deregister() { + TransportSearchAction.removeSearchOperationsListener(this); + } + /** * Search request slow log message * @@ -233,18 +249,22 @@ private static String escapeJson(String text) { void setWarnThreshold(TimeValue warnThreshold) { this.warnThreshold = warnThreshold.nanos(); + changeEnabledIfNeeded(); } void setInfoThreshold(TimeValue infoThreshold) { this.infoThreshold = infoThreshold.nanos(); + changeEnabledIfNeeded(); } void setDebugThreshold(TimeValue debugThreshold) { this.debugThreshold = debugThreshold.nanos(); + changeEnabledIfNeeded(); } void setTraceThreshold(TimeValue traceThreshold) { this.traceThreshold = traceThreshold.nanos(); + changeEnabledIfNeeded(); } void setLevel(SlowLogLevel level) { @@ -270,4 +290,11 @@ protected long getTraceThreshold() { SlowLogLevel getLevel() { return level; } + + private void changeEnabledIfNeeded() { + super.setEnabled(this.warnThreshold >= 0 + || this.debugThreshold >= 0 + || this.infoThreshold >= 0 + || this.traceThreshold >= 0); + } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java index 262750849eaa9..2a1b62b235301 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -8,10 +8,12 @@ package org.opensearch.action.search; +import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.annotation.PublicApi; import org.opensearch.common.inject.Inject; import org.opensearch.common.metrics.CounterMetric; import org.opensearch.common.metrics.MeanMetric; +import org.opensearch.common.settings.Setting; import java.util.EnumMap; import java.util.Map; @@ -26,13 +28,38 @@ public final class SearchRequestStats extends SearchRequestOperationsListener { Map phaseStatsMap = new EnumMap<>(SearchPhaseName.class); + public static final String SEARCH_REQUEST_STATS_ENABLED_KEY = "search.request_stats_enabled"; + public static final Setting SEARCH_REQUEST_STATS_ENABLED = Setting.boolSetting( + SEARCH_REQUEST_STATS_ENABLED_KEY, + false, + Setting.Property.Dynamic, + Setting.Property.NodeScope + ); + @Inject - public SearchRequestStats() { + public SearchRequestStats(ClusterService clusterService) { + clusterService.getClusterSettings().addSettingsUpdateConsumer(SEARCH_REQUEST_STATS_ENABLED, this::setEnabled); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { phaseStatsMap.put(searchPhaseName, new StatsHolder()); } } + /** + * register this listener to TransportSearchAction + */ + @Override + protected void register() { + TransportSearchAction.addSearchOperationsListener(this); + } + + /** + * deregister this listener to TransportSearchAction + */ + @Override + protected void deregister() { + TransportSearchAction.removeSearchOperationsListener(this); + } + public long getPhaseCurrent(SearchPhaseName searchPhaseName) { return phaseStatsMap.get(searchPhaseName).current.count(); } 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 05f4308df74fa..5dc4e505e9b05 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -154,13 +154,6 @@ public class TransportSearchAction extends HandledTransportAction SEARCH_REQUEST_STATS_ENABLED = Setting.boolSetting( - SEARCH_REQUEST_STATS_ENABLED_KEY, - false, - Property.Dynamic, - Property.NodeScope - ); public static final String SEARCH_PHASE_TOOK_ENABLED_KEY = "search.phase_took_enabled"; public static final Setting SEARCH_PHASE_TOOK_ENABLED = Setting.boolSetting( @@ -170,6 +163,8 @@ public class TransportSearchAction extends HandledTransportAction searchRequestOperationsListenersList = new ArrayList<>(); + private final NodeClient client; private final ThreadPool threadPool; private final ClusterService clusterService; @@ -182,13 +177,8 @@ public class TransportSearchAction extends HandledTransportAction) SearchRequest::new); @@ -224,10 +212,6 @@ public TransportSearchAction( this.indexNameExpressionResolver = indexNameExpressionResolver; this.namedWriteableRegistry = namedWriteableRegistry; this.searchPipelineService = searchPipelineService; - this.isRequestStatsEnabled = clusterService.getClusterSettings().get(SEARCH_REQUEST_STATS_ENABLED); - clusterService.getClusterSettings().addSettingsUpdateConsumer(SEARCH_REQUEST_STATS_ENABLED, this::setIsRequestStatsEnabled); - this.searchRequestStats = searchRequestStats; - this.searchRequestSlowLog = searchRequestSlowLog; this.metricsRegistry = metricsRegistry; this.searchQueryMetricsEnabled = clusterService.getClusterSettings().get(SEARCH_QUERY_METRICS_ENABLED_SETTING); clusterService.getClusterSettings() @@ -241,10 +225,6 @@ private void setSearchQueryMetricsEnabled(boolean searchQueryMetricsEnabled) { } } - private void setIsRequestStatsEnabled(boolean isRequestStatsEnabled) { - this.isRequestStatsEnabled = isRequestStatsEnabled; - } - private Map buildPerIndexAliasFilter( SearchRequest request, ClusterState clusterState, @@ -1245,11 +1225,7 @@ AbstractSearchAsyncAction asyncSearchAction( } private List createSearchListenerList(SearchRequest searchRequest, SearchTimeProvider timeProvider) { - final List searchListenersList = new ArrayList<>(); - - if (isRequestStatsEnabled) { - searchListenersList.add(searchRequestStats); - } + final List searchListenersList = new ArrayList<>(searchRequestOperationsListenersList); // phase_took is enabled with request param and/or cluster setting Boolean phaseTookRequestParam = searchRequest.isPhaseTook(); @@ -1263,13 +1239,6 @@ private List createSearchListenerList(SearchReq searchListenersList.add(timeProvider); } - if (searchRequestSlowLog.getWarnThreshold() >= 0 - || searchRequestSlowLog.getInfoThreshold() >= 0 - || searchRequestSlowLog.getDebugThreshold() >= 0 - || searchRequestSlowLog.getTraceThreshold() >= 0) { - searchListenersList.add(searchRequestSlowLog); - } - return searchListenersList; } @@ -1540,4 +1509,38 @@ static List getLocalLocalShardsIteratorFromPointInTime( } return iterators; } + + + /** + * Add a {@link SearchRequestOperationsListener} to the searchRequestOperationsListenersList, + * which will be executed during each search request. + * + * @param listener A SearchRequestOperationsListener object to add. + * @throws IllegalArgumentException if the input listener is null or already exists in the list. + */ + public static void addSearchOperationsListener(SearchRequestOperationsListener listener) { + if (listener == null) { + throw new IllegalArgumentException("listener must not be null"); + } + if (searchRequestOperationsListenersList.contains(listener)) { + throw new IllegalArgumentException("listener already added"); + } + searchRequestOperationsListenersList.add(listener); + } + + /** + * Remove a {@link SearchRequestOperationsListener} from the searchRequestOperationsListenersList, + * + * @param listener A SearchRequestOperationsListener object to remove. + * @throws IllegalArgumentException if the input listener is null or already exists in the list. + */ + public static void removeSearchOperationsListener(SearchRequestOperationsListener listener) { + if (listener == null) { + throw new IllegalArgumentException("listener must not be null"); + } + if (!searchRequestOperationsListenersList.contains(listener)) { + throw new IllegalArgumentException("listener does not exist in the listeners list"); + } + searchRequestOperationsListenersList.remove(listener); + } } 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 fa4b0f475edc5..8b00d7290cfdd 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -36,6 +36,7 @@ import org.opensearch.action.admin.indices.close.TransportCloseIndexAction; import org.opensearch.action.search.CreatePitController; import org.opensearch.action.search.SearchRequestSlowLog; +import org.opensearch.action.search.SearchRequestStats; import org.opensearch.action.search.TransportSearchAction; import org.opensearch.action.support.AutoCreateIndex; import org.opensearch.action.support.DestructiveOperations; @@ -380,9 +381,9 @@ public void apply(Settings value, Settings current, Settings previous) { SearchService.DEFAULT_ALLOW_PARTIAL_SEARCH_RESULTS, TransportSearchAction.SHARD_COUNT_LIMIT_SETTING, TransportSearchAction.SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING, - TransportSearchAction.SEARCH_REQUEST_STATS_ENABLED, TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED, TransportSearchAction.SEARCH_QUERY_METRICS_ENABLED_SETTING, + SearchRequestStats.SEARCH_REQUEST_STATS_ENABLED, RemoteClusterService.REMOTE_CLUSTER_SKIP_UNAVAILABLE, SniffConnectionStrategy.REMOTE_CONNECTIONS_PER_CLUSTER, RemoteClusterService.REMOTE_INITIAL_CONNECTION_TIMEOUT_SETTING, diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 4cbf8dc191a9d..7fe6f03951113 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -783,7 +783,7 @@ protected Node( threadPool ); - final SearchRequestStats searchRequestStats = new SearchRequestStats(); + final SearchRequestStats searchRequestStats = new SearchRequestStats(clusterService); final SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); remoteStoreStatsTrackerFactory = new RemoteStoreStatsTrackerFactory(clusterService, settings); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 9fe1f8294fc74..281ae14193308 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -2310,8 +2310,6 @@ public void onFailure(final Exception e) { List.of(), client ), - null, - new SearchRequestSlowLog(clusterService), NoopMetricsRegistry.INSTANCE ) ); From 8056938ce8f63ddc6d9ec8fe220467588fb1d154 Mon Sep 17 00:00:00 2001 From: Chenyang Ji Date: Mon, 11 Dec 2023 17:41:36 -0800 Subject: [PATCH 2/7] Dynamically add search request operation listeners with SearchRequestListenerManager Signed-off-by: Chenyang Ji --- .../search/SearchRequestListenerManager.java | 124 +++++++++++++++++ .../SearchRequestOperationsListener.java | 15 +- .../action/search/SearchRequestSlowLog.java | 39 ++---- .../action/search/SearchRequestStats.java | 22 +-- .../action/search/TransportSearchAction.java | 77 ++--------- .../common/settings/ClusterSettings.java | 7 +- .../main/java/org/opensearch/node/Node.java | 7 +- .../cluster/node/stats/NodeStatsTests.java | 12 +- .../AbstractSearchAsyncActionTests.java | 27 +++- .../SearchRequestListenerManagerTests.java | 129 ++++++++++++++++++ .../search/SearchRequestSlowLogTests.java | 33 +++-- .../search/SearchRequestStatsTests.java | 43 +++++- .../index/search/stats/SearchStatsTests.java | 17 ++- .../indices/NodeIndicesStatsTests.java | 12 +- .../snapshots/SnapshotResiliencyTests.java | 5 +- 15 files changed, 421 insertions(+), 148 deletions(-) create mode 100644 server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java create mode 100644 server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java b/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java new file mode 100644 index 0000000000000..b1c275555349f --- /dev/null +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java @@ -0,0 +1,124 @@ +/* + * 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.logging.log4j.Logger; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.inject.Inject; +import org.opensearch.common.settings.Setting; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + + +/** + * SearchRequestListenerManager manages listeners registered to search requests, + * and is responsible for creating the {@link SearchRequestOperationsListener.CompositeListener} + * with the all listeners enabled at cluster-level and request-level. + * + * + * @opensearch.internal + */ +public class SearchRequestListenerManager { + + private final ClusterService clusterService; + public static final String SEARCH_PHASE_TOOK_ENABLED_KEY = "search.phase_took_enabled"; + public static final Setting SEARCH_PHASE_TOOK_ENABLED = Setting.boolSetting( + SEARCH_PHASE_TOOK_ENABLED_KEY, + false, + Setting.Property.Dynamic, + Setting.Property.NodeScope + ); + private final List searchRequestListenersList; + + @Inject + public SearchRequestListenerManager( + ClusterService clusterService + ) { + this.clusterService = clusterService; + searchRequestListenersList = new ArrayList<>(); + } + + /** + * Add a {@link SearchRequestOperationsListener} to the searchRequestListenersList, + * which will be executed during each search request. + * + * @param listener A SearchRequestOperationsListener object to add. + * @throws IllegalArgumentException if the input listener is null or already exists in the list. + */ + public void addListener(SearchRequestOperationsListener listener) { + if (listener == null) { + throw new IllegalArgumentException("listener must not be null"); + } + if (searchRequestListenersList.contains(listener)) { + throw new IllegalArgumentException("listener already added"); + } + searchRequestListenersList.add(listener); + } + + /** + * Remove a {@link SearchRequestOperationsListener} from the searchRequestListenersList, + * + * @param listener A SearchRequestOperationsListener object to remove. + * @throws IllegalArgumentException if the input listener is null or already exists in the list. + */ + public void removeListener(SearchRequestOperationsListener listener) { + if (listener == null) { + throw new IllegalArgumentException("listener must not be null"); + } + if (!searchRequestListenersList.contains(listener)) { + throw new IllegalArgumentException("listener does not exist in the listeners list"); + } + searchRequestListenersList.remove(listener); + } + + /** + * Get searchRequestListenersList, + * + * @return List of SearchRequestOperationsListener + * @throws IllegalArgumentException if the input listener is null or already exists in the list. + */ + public List getListeners() { + return searchRequestListenersList; + } + + + /** + * Create the {@link SearchRequestOperationsListener.CompositeListener} + * with the all listeners enabled at cluster-level and request-level. + * + * @param searchRequest The SearchRequest object. SearchRequestListenerManager will decide which request-level listeners to add based on states/flags of the request + * @param logger Logger to be attached to the {@link SearchRequestOperationsListener.CompositeListener} + * @param perRequestListeners the per-request listeners that can be optionally added to the returned CompositeListener list. + * @return SearchRequestOperationsListener.CompositeListener + */ + public SearchRequestOperationsListener.CompositeListener buildCompositeListener( + SearchRequest searchRequest, + Logger logger, + SearchRequestOperationsListener... perRequestListeners + ) { + final List searchListenersList = new ArrayList<>(searchRequestListenersList); + + Arrays.stream(perRequestListeners).parallel().forEach((listener) -> { + if (listener != null && listener.getClass() == TransportSearchAction.SearchTimeProvider.class) { + TransportSearchAction.SearchTimeProvider timeProvider = (TransportSearchAction.SearchTimeProvider) listener; + // phase_took is enabled with request param and/or cluster setting + boolean phaseTookEnabled = (searchRequest.isPhaseTook() != null && searchRequest.isPhaseTook()) || + clusterService.getClusterSettings().get(SEARCH_PHASE_TOOK_ENABLED); + if (phaseTookEnabled) { + timeProvider.setPhaseTook(true); + searchListenersList.add(timeProvider); + } + } + }); + return new SearchRequestOperationsListener.CompositeListener(searchListenersList, logger); + } + +} diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java index 9132aac387085..59bdc10a32978 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java @@ -21,7 +21,7 @@ */ @InternalApi abstract class SearchRequestOperationsListener { - protected boolean enabled; + protected SearchRequestListenerManager searchRequestListenerManager; abstract void onPhaseStart(SearchPhaseContext context); @@ -34,7 +34,6 @@ void onRequestStart(SearchRequestContext searchRequestContext) {} void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {} public void setEnabled(boolean enabled) { - this.enabled = enabled; if (enabled) { register(); } else { @@ -47,13 +46,21 @@ public void setEnabled(boolean enabled) { * Handler function to register this listener to certain components * This function will be called when the listener is enabled. */ - protected void register() {} + protected void register() { + if (this.searchRequestListenerManager != null) { + this.searchRequestListenerManager.addListener(this); + } + } /** * Handler function to deregister this listener from certain components * This function will be called when the listener is disabled. */ - protected void deregister() {} + protected void deregister() { + if (this.searchRequestListenerManager != null) { + this.searchRequestListenerManager.removeListener(this); + } + } /** * Holder of Composite Listeners diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java index 0971e252996a3..f06619ec6f263 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java @@ -37,6 +37,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.inject.Inject; import org.opensearch.common.logging.Loggers; import org.opensearch.common.logging.OpenSearchLogMessage; import org.opensearch.common.logging.SlowLogLevel; @@ -108,12 +109,18 @@ public final class SearchRequestSlowLog extends SearchRequestOperationsListener private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false")); - public SearchRequestSlowLog(ClusterService clusterService) { - this(clusterService, LogManager.getLogger(CLUSTER_SEARCH_REQUEST_SLOWLOG_PREFIX)); // logger configured in log4j2.properties + @Inject + public SearchRequestSlowLog( + ClusterService clusterService, + SearchRequestListenerManager searchRequestListenerManager + ) { + this(clusterService, searchRequestListenerManager, LogManager.getLogger(CLUSTER_SEARCH_REQUEST_SLOWLOG_PREFIX)); // logger configured in log4j2.properties } - SearchRequestSlowLog(ClusterService clusterService, Logger logger) { + @Inject + SearchRequestSlowLog(ClusterService clusterService, SearchRequestListenerManager searchRequestListenerManager, Logger logger) { this.logger = logger; + this.searchRequestListenerManager = searchRequestListenerManager; Loggers.setLevel(this.logger, SlowLogLevel.TRACE.name()); this.warnThreshold = clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_WARN_SETTING).nanos(); @@ -160,22 +167,6 @@ void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequest } } - /** - * register this listener to TransportSearchAction - */ - @Override - protected void register() { - TransportSearchAction.addSearchOperationsListener(this); - } - - /** - * deregister this listener to TransportSearchAction - */ - @Override - protected void deregister() { - TransportSearchAction.removeSearchOperationsListener(this); - } - /** * Search request slow log message * @@ -249,22 +240,22 @@ private static String escapeJson(String text) { void setWarnThreshold(TimeValue warnThreshold) { this.warnThreshold = warnThreshold.nanos(); - changeEnabledIfNeeded(); + setEnabled(); } void setInfoThreshold(TimeValue infoThreshold) { this.infoThreshold = infoThreshold.nanos(); - changeEnabledIfNeeded(); + setEnabled(); } void setDebugThreshold(TimeValue debugThreshold) { this.debugThreshold = debugThreshold.nanos(); - changeEnabledIfNeeded(); + setEnabled(); } void setTraceThreshold(TimeValue traceThreshold) { this.traceThreshold = traceThreshold.nanos(); - changeEnabledIfNeeded(); + setEnabled(); } void setLevel(SlowLogLevel level) { @@ -291,7 +282,7 @@ SlowLogLevel getLevel() { return level; } - private void changeEnabledIfNeeded() { + private void setEnabled() { super.setEnabled(this.warnThreshold >= 0 || this.debugThreshold >= 0 || this.infoThreshold >= 0 diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java index 2a1b62b235301..185ac2a66a859 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -37,27 +37,15 @@ public final class SearchRequestStats extends SearchRequestOperationsListener { ); @Inject - public SearchRequestStats(ClusterService clusterService) { + public SearchRequestStats( + ClusterService clusterService, + SearchRequestListenerManager searchRequestListenerManager + ) { clusterService.getClusterSettings().addSettingsUpdateConsumer(SEARCH_REQUEST_STATS_ENABLED, this::setEnabled); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { phaseStatsMap.put(searchPhaseName, new StatsHolder()); } - } - - /** - * register this listener to TransportSearchAction - */ - @Override - protected void register() { - TransportSearchAction.addSearchOperationsListener(this); - } - - /** - * deregister this listener to TransportSearchAction - */ - @Override - protected void deregister() { - TransportSearchAction.removeSearchOperationsListener(this); + this.searchRequestListenerManager = searchRequestListenerManager; } public long getPhaseCurrent(SearchPhaseName searchPhaseName) { 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 5dc4e505e9b05..17b220ae324a4 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -154,17 +154,6 @@ public class TransportSearchAction extends HandledTransportAction SEARCH_PHASE_TOOK_ENABLED = Setting.boolSetting( - SEARCH_PHASE_TOOK_ENABLED_KEY, - false, - Property.Dynamic, - Property.NodeScope - ); - - private static final List searchRequestOperationsListenersList = new ArrayList<>(); - private final NodeClient client; private final ThreadPool threadPool; private final ClusterService clusterService; @@ -176,6 +165,7 @@ public class TransportSearchAction extends HandledTransportAction) SearchRequest::new); this.client = client; @@ -214,6 +205,7 @@ public TransportSearchAction( this.searchPipelineService = searchPipelineService; this.metricsRegistry = metricsRegistry; this.searchQueryMetricsEnabled = clusterService.getClusterSettings().get(SEARCH_QUERY_METRICS_ENABLED_SETTING); + this.searchRequestListenerManager = searchRequestListenerManager; clusterService.getClusterSettings() .addSettingsUpdateConsumer(SEARCH_QUERY_METRICS_ENABLED_SETTING, this::setSearchQueryMetricsEnabled); } @@ -470,11 +462,12 @@ private void executeRequest( relativeStartNanos, System::nanoTime ); - - final List searchListenersList = createSearchListenerList(originalSearchRequest, timeProvider); - SearchRequestContext searchRequestContext = new SearchRequestContext( - new SearchRequestOperationsListener.CompositeListener(searchListenersList, logger) + SearchRequestOperationsListener.CompositeListener requestOperationsListeners = searchRequestListenerManager.buildCompositeListener( + originalSearchRequest, + logger, + timeProvider ); + SearchRequestContext searchRequestContext = new SearchRequestContext(requestOperationsListeners); searchRequestContext.getSearchRequestOperationsListener().onRequestStart(searchRequestContext); PipelinedRequest searchRequest; @@ -1224,24 +1217,6 @@ AbstractSearchAsyncAction asyncSearchAction( ); } - private List createSearchListenerList(SearchRequest searchRequest, SearchTimeProvider timeProvider) { - final List searchListenersList = new ArrayList<>(searchRequestOperationsListenersList); - - // phase_took is enabled with request param and/or cluster setting - Boolean phaseTookRequestParam = searchRequest.isPhaseTook(); - if (phaseTookRequestParam == null) { // check cluster setting only when request param is undefined - if (clusterService.getClusterSettings().get(TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED)) { - timeProvider.setPhaseTook(true); - searchListenersList.add(timeProvider); - } - } else if (phaseTookRequestParam == true) { - timeProvider.setPhaseTook(true); - searchListenersList.add(timeProvider); - } - - return searchListenersList; - } - private AbstractSearchAsyncAction searchAsyncAction( SearchTask task, SearchRequest searchRequest, @@ -1509,38 +1484,4 @@ static List getLocalLocalShardsIteratorFromPointInTime( } return iterators; } - - - /** - * Add a {@link SearchRequestOperationsListener} to the searchRequestOperationsListenersList, - * which will be executed during each search request. - * - * @param listener A SearchRequestOperationsListener object to add. - * @throws IllegalArgumentException if the input listener is null or already exists in the list. - */ - public static void addSearchOperationsListener(SearchRequestOperationsListener listener) { - if (listener == null) { - throw new IllegalArgumentException("listener must not be null"); - } - if (searchRequestOperationsListenersList.contains(listener)) { - throw new IllegalArgumentException("listener already added"); - } - searchRequestOperationsListenersList.add(listener); - } - - /** - * Remove a {@link SearchRequestOperationsListener} from the searchRequestOperationsListenersList, - * - * @param listener A SearchRequestOperationsListener object to remove. - * @throws IllegalArgumentException if the input listener is null or already exists in the list. - */ - public static void removeSearchOperationsListener(SearchRequestOperationsListener listener) { - if (listener == null) { - throw new IllegalArgumentException("listener must not be null"); - } - if (!searchRequestOperationsListenersList.contains(listener)) { - throw new IllegalArgumentException("listener does not exist in the listeners list"); - } - searchRequestOperationsListenersList.remove(listener); - } } 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 8b00d7290cfdd..12145bf298f63 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -34,10 +34,7 @@ import org.apache.logging.log4j.LogManager; import org.opensearch.action.admin.cluster.configuration.TransportAddVotingConfigExclusionsAction; import org.opensearch.action.admin.indices.close.TransportCloseIndexAction; -import org.opensearch.action.search.CreatePitController; -import org.opensearch.action.search.SearchRequestSlowLog; -import org.opensearch.action.search.SearchRequestStats; -import org.opensearch.action.search.TransportSearchAction; +import org.opensearch.action.search.*; import org.opensearch.action.support.AutoCreateIndex; import org.opensearch.action.support.DestructiveOperations; import org.opensearch.action.support.replication.TransportReplicationAction; @@ -381,8 +378,8 @@ public void apply(Settings value, Settings current, Settings previous) { SearchService.DEFAULT_ALLOW_PARTIAL_SEARCH_RESULTS, TransportSearchAction.SHARD_COUNT_LIMIT_SETTING, TransportSearchAction.SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING, - TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED, TransportSearchAction.SEARCH_QUERY_METRICS_ENABLED_SETTING, + SearchRequestListenerManager.SEARCH_PHASE_TOOK_ENABLED, SearchRequestStats.SEARCH_REQUEST_STATS_ENABLED, RemoteClusterService.REMOTE_CLUSTER_SKIP_UNAVAILABLE, SniffConnectionStrategy.REMOTE_CONNECTIONS_PER_CLUSTER, diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 7fe6f03951113..59672eaf183bc 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -46,6 +46,7 @@ import org.opensearch.action.admin.cluster.snapshots.status.TransportNodesSnapshotsStatus; import org.opensearch.action.search.SearchExecutionStatsCollector; import org.opensearch.action.search.SearchPhaseController; +import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchRequestSlowLog; import org.opensearch.action.search.SearchRequestStats; import org.opensearch.action.search.SearchTransportService; @@ -782,9 +783,10 @@ protected Node( repositoriesServiceReference::get, threadPool ); + final SearchRequestListenerManager searchRequestListenerManager = new SearchRequestListenerManager(clusterService); - final SearchRequestStats searchRequestStats = new SearchRequestStats(clusterService); - final SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); + final SearchRequestStats searchRequestStats = new SearchRequestStats(clusterService, searchRequestListenerManager); + final SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, searchRequestListenerManager); remoteStoreStatsTrackerFactory = new RemoteStoreStatsTrackerFactory(clusterService, settings); final IndicesService indicesService = new IndicesService( @@ -1275,6 +1277,7 @@ protected Node( b.bind(RemoteClusterStateService.class).toProvider(() -> remoteClusterStateService); b.bind(PersistedStateRegistry.class).toInstance(persistedStateRegistry); b.bind(SegmentReplicationStatsTracker.class).toInstance(segmentReplicationStatsTracker); + b.bind(SearchRequestListenerManager.class).toInstance(searchRequestListenerManager); }); injector = modules.createInjector(); diff --git a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java index b8ab5c935fa34..1704238868907 100644 --- a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java +++ b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java @@ -34,6 +34,7 @@ import org.opensearch.action.admin.indices.stats.CommonStats; import org.opensearch.action.admin.indices.stats.CommonStatsFlags; +import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchRequestStats; import org.opensearch.cluster.coordination.PendingClusterStateStats; import org.opensearch.cluster.coordination.PersistedStateStats; @@ -41,9 +42,12 @@ import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.routing.WeightedRoutingStats; import org.opensearch.cluster.service.ClusterManagerThrottlingStats; +import org.opensearch.cluster.service.ClusterService; import org.opensearch.cluster.service.ClusterStateStats; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.metrics.OperationStats; +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.index.shard.ShardId; import org.opensearch.core.indices.breaker.AllCircuitBreakerStats; @@ -961,7 +965,13 @@ public void apply(String action, AdmissionControlActionType admissionControlActi private static NodeIndicesStats getNodeIndicesStats(boolean remoteStoreStats) { NodeIndicesStats indicesStats = null; if (remoteStoreStats) { - indicesStats = new NodeIndicesStats(new CommonStats(CommonStatsFlags.ALL), new HashMap<>(), new SearchRequestStats()); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + indicesStats = new NodeIndicesStats(new CommonStats(CommonStatsFlags.ALL), new HashMap<>(), new SearchRequestStats(clusterService, listenerManager)); RemoteSegmentStats remoteSegmentStats = indicesStats.getSegments().getRemoteSegmentStats(); remoteSegmentStats.addUploadBytesStarted(10L); remoteSegmentStats.addUploadBytesSucceeded(10L); diff --git a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java index e17fbab32a12e..d835fbda107c4 100644 --- a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java @@ -36,8 +36,11 @@ import org.opensearch.action.support.IndicesOptions; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.routing.GroupShardsIterator; +import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.UUIDs; import org.opensearch.common.collect.Tuple; +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.AtomicArray; import org.opensearch.common.util.concurrent.OpenSearchExecutors; import org.opensearch.common.util.set.Sets; @@ -328,7 +331,13 @@ public void testSendSearchResponseDisallowPartialFailures() { } public void testOnPhaseFailureAndVerifyListeners() { - SearchRequestStats testListener = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testListener = new SearchRequestStats(clusterService, listenerManager); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); SearchQueryThenFetchAsyncAction action = createSearchQueryThenFetchAsyncAction(requestOperationListeners); @@ -591,7 +600,13 @@ public void onFailure(Exception e) { } public void testOnPhaseListenersWithQueryAndThenFetchType() throws InterruptedException { - SearchRequestStats testListener = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testListener = new SearchRequestStats(clusterService, listenerManager); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); long delay = (randomIntBetween(1, 5)); @@ -640,7 +655,13 @@ public void testOnPhaseListenersWithQueryAndThenFetchType() throws InterruptedEx } public void testOnPhaseListenersWithDfsType() throws InterruptedException { - SearchRequestStats testListener = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testListener = new SearchRequestStats(clusterService, listenerManager); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); SearchDfsQueryThenFetchAsyncAction searchDfsQueryThenFetchAsyncAction = createSearchDfsQueryThenFetchAsyncAction( diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java new file mode 100644 index 0000000000000..7cd1010f1e362 --- /dev/null +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java @@ -0,0 +1,129 @@ +/* + * 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.cluster.service.ClusterService; +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; +import org.opensearch.index.query.QueryBuilders; +import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.test.OpenSearchTestCase; + +import java.lang.reflect.Field; +import java.util.List; + + +public class SearchRequestListenerManagerTests extends OpenSearchTestCase { + public void testAddAndGetListeners() { + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestOperationsListener testListener = createTestSearchRequestOperationsListener(); + listenerManager.addListener(testListener); + assertEquals(1, listenerManager.getListeners().size()); + assertEquals(testListener, listenerManager.getListeners().get(0)); + } + + public void testRemoveListeners() { + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); + listenerManager.addListener(testListener1); + listenerManager.addListener(testListener2); + assertEquals(2, listenerManager.getListeners().size()); + listenerManager.removeListener(testListener2); + assertEquals(1, listenerManager.getListeners().size()); + assertEquals(testListener1, listenerManager.getListeners().get(0)); + } + + public void testBuildCompositeListenersWithTimeProvider() throws NoSuchFieldException, IllegalAccessException { + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( + 0, + System.nanoTime(), + System::nanoTime + ); + listenerManager.addListener(testListener1); + SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); + SearchRequest searchRequest = new SearchRequest().source(source); + searchRequest.setPhaseTook(true); + SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( + searchRequest, + logger, + timeProviderListener + ); + Field listenersField = SearchRequestOperationsListener.CompositeListener.class.getDeclaredField("listeners"); + listenersField.setAccessible(true); + List listeners = (List) listenersField.get(compositeListener); + assertEquals(2, listeners.size()); + assertEquals(testListener1, listeners.get(0)); + assertEquals(timeProviderListener, listeners.get(1)); + assertEquals(1, listenerManager.getListeners().size()); + assertEquals(testListener1, listenerManager.getListeners().get(0)); + } + + public void testBuildCompositeListenersWithPhaseTookDisabled() throws NoSuchFieldException, IllegalAccessException { + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( + 0, + System.nanoTime(), + System::nanoTime + ); + listenerManager.addListener(testListener1); + SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); + SearchRequest searchRequest = new SearchRequest().source(source); + searchRequest.setPhaseTook(false); + SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( + searchRequest, + logger, + timeProviderListener + ); + Field listenersField = SearchRequestOperationsListener.CompositeListener.class.getDeclaredField("listeners"); + listenersField.setAccessible(true); + List listeners = (List) listenersField.get(compositeListener); + assertEquals(1, listeners.size()); + assertEquals(testListener1, listeners.get(0)); + assertEquals(1, listenerManager.getListeners().size()); + assertEquals(testListener1, listenerManager.getListeners().get(0)); + } + + + public SearchRequestOperationsListener createTestSearchRequestOperationsListener() { + return new SearchRequestOperationsListener() { + @Override + void onPhaseStart(SearchPhaseContext context) {} + + @Override + void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {} + + @Override + void onPhaseFailure(SearchPhaseContext context) {} + }; + } +} diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java index e23f08c9415eb..fc8300e0174cf 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java @@ -95,7 +95,8 @@ public void testMultipleSlowLoggersUseSingleLog4jLogger() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestSlowLog searchRequestSlowLog1 = new SearchRequestSlowLog(clusterService1); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService1); + SearchRequestSlowLog searchRequestSlowLog1 = new SearchRequestSlowLog(clusterService1, listenerManager); int numberOfLoggersBefore = context.getLoggers().size(); SearchPhaseContext searchPhaseContext2 = new MockSearchPhaseContext(1); @@ -104,7 +105,8 @@ public void testMultipleSlowLoggersUseSingleLog4jLogger() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestSlowLog searchRequestSlowLog2 = new SearchRequestSlowLog(clusterService2); + SearchRequestListenerManager listenerManager2 = new SearchRequestListenerManager(clusterService2); + SearchRequestSlowLog searchRequestSlowLog2 = new SearchRequestSlowLog(clusterService2, listenerManager2); int numberOfLoggersAfter = context.getLoggers().size(); assertThat(numberOfLoggersAfter, equalTo(numberOfLoggersBefore)); @@ -124,7 +126,8 @@ public void testOnRequestEnd() throws InterruptedException { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, logger); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager, logger); final List searchListenersList = new ArrayList<>(List.of(searchRequestSlowLog)); when(searchRequestContext.getSearchRequestOperationsListener()).thenReturn( @@ -157,7 +160,8 @@ public void testConcurrentOnRequestEnd() throws InterruptedException { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, logger); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager, logger); final List searchListenersList = new ArrayList<>(List.of(searchRequestSlowLog)); when(searchPhaseContext.getRequest()).thenReturn(searchRequest); @@ -308,7 +312,8 @@ public void testLevelSettingWarn() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); assertEquals(level, searchRequestSlowLog.getLevel()); } @@ -319,7 +324,8 @@ public void testLevelSettingDebug() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); assertEquals(level, searchRequestSlowLog.getLevel().toString()); } @@ -330,9 +336,10 @@ public void testLevelSettingFail() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); try { - new SearchRequestSlowLog(clusterService); + new SearchRequestSlowLog(clusterService, listenerManager); fail(); } catch (IllegalArgumentException ex) { final String expected = "No enum constant org.opensearch.common.logging.SlowLogLevel.NOT A LEVEL"; @@ -350,7 +357,8 @@ public void testSetThresholds() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); assertEquals(TimeValue.timeValueMillis(400).nanos(), searchRequestSlowLog.getWarnThreshold()); assertEquals(TimeValue.timeValueMillis(300).nanos(), searchRequestSlowLog.getInfoThreshold()); assertEquals(TimeValue.timeValueMillis(200).nanos(), searchRequestSlowLog.getDebugThreshold()); @@ -367,7 +375,8 @@ public void testSetThresholdsUnits() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); assertEquals(TimeValue.timeValueSeconds(400).nanos(), searchRequestSlowLog.getWarnThreshold()); assertEquals(TimeValue.timeValueMillis(300).nanos(), searchRequestSlowLog.getInfoThreshold()); assertEquals(TimeValue.timeValueNanos(200000).nanos(), searchRequestSlowLog.getDebugThreshold()); @@ -382,7 +391,8 @@ public void testSetThresholdsDefaults() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); assertEquals(TimeValue.timeValueMillis(400).nanos(), searchRequestSlowLog.getWarnThreshold()); assertEquals(TimeValue.timeValueMillis(-1).nanos(), searchRequestSlowLog.getInfoThreshold()); assertEquals(TimeValue.timeValueMillis(200).nanos(), searchRequestSlowLog.getDebugThreshold()); @@ -396,9 +406,10 @@ public void testSetThresholdsError() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); try { - new SearchRequestSlowLog(clusterService); + new SearchRequestSlowLog(clusterService, listenerManager); fail(); } catch (IllegalArgumentException ex) { final String expected = diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java index 93cf77933fdd5..6ac7eb6040f13 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -8,6 +8,9 @@ package org.opensearch.action.search; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; import org.opensearch.test.OpenSearchTestCase; import java.util.HashMap; @@ -22,7 +25,13 @@ public class SearchRequestStatsTests extends OpenSearchTestCase { public void testSearchRequestPhaseFailure() { - SearchRequestStats testRequestStats = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -37,7 +46,13 @@ public void testSearchRequestPhaseFailure() { } public void testSearchRequestStats() { - SearchRequestStats testRequestStats = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); @@ -58,7 +73,13 @@ public void testSearchRequestStats() { } public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedException { - SearchRequestStats testRequestStats = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); @@ -85,7 +106,13 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE } public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedException { - SearchRequestStats testRequestStats = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); @@ -121,7 +148,13 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc } public void testSearchRequestStatsOnPhaseFailureConcurrently() throws InterruptedException { - SearchRequestStats testRequestStats = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); diff --git a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java index 52b272094cd86..612d3a39a1149 100644 --- a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java +++ b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java @@ -32,11 +32,10 @@ package org.opensearch.index.search.stats; -import org.opensearch.action.search.SearchPhase; -import org.opensearch.action.search.SearchPhaseContext; -import org.opensearch.action.search.SearchPhaseName; -import org.opensearch.action.search.SearchRequestOperationsListenerSupport; -import org.opensearch.action.search.SearchRequestStats; +import org.opensearch.action.search.*; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; import org.opensearch.index.search.stats.SearchStats.Stats; import org.opensearch.test.OpenSearchTestCase; @@ -77,7 +76,13 @@ public void testShardLevelSearchGroupStats() throws Exception { long paramValue = randomIntBetween(2, 50); // Testing for request stats - SearchRequestStats testRequestStats = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); SearchPhaseContext ctx = mock(SearchPhaseContext.class); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhase mockSearchPhase = mock(SearchPhase.class); diff --git a/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java b/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java index 6f36d22b7e17b..174dbc895206b 100644 --- a/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java +++ b/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java @@ -33,7 +33,11 @@ package org.opensearch.indices; import org.opensearch.action.admin.indices.stats.CommonStats; +import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchRequestStats; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.test.OpenSearchTestCase; @@ -46,7 +50,13 @@ public class NodeIndicesStatsTests extends OpenSearchTestCase { public void testInvalidLevel() { CommonStats oldStats = new CommonStats(); - SearchRequestStats requestStats = new SearchRequestStats(); + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestStats requestStats = new SearchRequestStats(clusterService, listenerManager); final NodeIndicesStats stats = new NodeIndicesStats(oldStats, Collections.emptyMap(), requestStats); final String level = randomAlphaOfLength(16); final ToXContent.Params params = new ToXContent.MapParams(Collections.singletonMap("level", level)); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 281ae14193308..519250d40cae6 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -91,6 +91,7 @@ import org.opensearch.action.search.SearchPhaseController; import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchRequestSlowLog; +import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchResponse; import org.opensearch.action.search.SearchTransportService; import org.opensearch.action.search.TransportSearchAction; @@ -2285,6 +2286,7 @@ public void onFailure(final Exception e) { writableRegistry(), searchService::aggReduceContextBuilder ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); actions.put( SearchAction.INSTANCE, new TransportSearchAction( @@ -2310,7 +2312,8 @@ public void onFailure(final Exception e) { List.of(), client ), - NoopMetricsRegistry.INSTANCE + NoopMetricsRegistry.INSTANCE, + listenerManager ) ); actions.put( From cc0489e5c3c4571c25098fe62b2e325d9c46b1a5 Mon Sep 17 00:00:00 2001 From: Chenyang Ji Date: Tue, 12 Dec 2023 14:25:13 -0800 Subject: [PATCH 3/7] improve support for multithreading Signed-off-by: Chenyang Ji --- CHANGELOG.md | 1 + .../search/SearchRequestListenerManager.java | 31 ++++---- .../SearchRequestOperationsListener.java | 39 ++++------ .../action/search/SearchRequestSlowLog.java | 11 +-- .../action/search/SearchRequestStats.java | 4 +- .../common/settings/ClusterSettings.java | 6 +- .../main/java/org/opensearch/node/Node.java | 12 ++- .../cluster/node/stats/NodeStatsTests.java | 4 +- .../AbstractSearchAsyncActionTests.java | 9 +-- .../SearchRequestListenerManagerTests.java | 74 +++++++++++++++++-- .../search/SearchRequestSlowLogTests.java | 32 +++----- .../search/SearchRequestStatsTests.java | 15 ++-- .../index/search/stats/SearchStatsTests.java | 9 ++- .../indices/NodeIndicesStatsTests.java | 4 +- .../snapshots/SnapshotResiliencyTests.java | 1 - 15 files changed, 142 insertions(+), 110 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b638b63dd52a0..64f71157dbfde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -193,6 +193,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add deleted doc count in _cat/shards ([#11678](https://github.com/opensearch-project/OpenSearch/pull/11678)) - Capture information for additional query types and aggregation types ([#11582](https://github.com/opensearch-project/OpenSearch/pull/11582)) - Use slice_size == shard_size heuristic in terms aggs for concurrent segment search and properly calculate the doc_count_error ([#11732](https://github.com/opensearch-project/OpenSearch/pull/11732)) +- Added Support for dynamically adding SearchRequestOperationsListeners ([#11526](https://github.com/opensearch-project/OpenSearch/pull/11526)) ### Deprecated diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java b/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java index b1c275555349f..25e44a0a562e8 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java @@ -10,12 +10,12 @@ import org.apache.logging.log4j.Logger; import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.inject.Inject; import org.opensearch.common.settings.Setting; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.stream.Collectors; /** @@ -38,7 +38,6 @@ public class SearchRequestListenerManager { ); private final List searchRequestListenersList; - @Inject public SearchRequestListenerManager( ClusterService clusterService ) { @@ -47,20 +46,22 @@ public SearchRequestListenerManager( } /** - * Add a {@link SearchRequestOperationsListener} to the searchRequestListenersList, - * which will be executed during each search request. + * Add multiple {@link SearchRequestOperationsListener} to the searchRequestListenersList. + * Those enabled listeners will be executed during each search request. * - * @param listener A SearchRequestOperationsListener object to add. - * @throws IllegalArgumentException if the input listener is null or already exists in the list. + * @param listeners Multiple SearchRequestOperationsListener object to add. + * @throws IllegalArgumentException if any input listener is null or already exists in the list. */ - public void addListener(SearchRequestOperationsListener listener) { - if (listener == null) { - throw new IllegalArgumentException("listener must not be null"); - } - if (searchRequestListenersList.contains(listener)) { - throw new IllegalArgumentException("listener already added"); + public void addListeners(SearchRequestOperationsListener... listeners) { + for (SearchRequestOperationsListener listener : listeners) { + if (listener == null) { + throw new IllegalArgumentException("listener must not be null"); + } + if (searchRequestListenersList.contains(listener)) { + throw new IllegalArgumentException("listener already added"); + } + searchRequestListenersList.add(listener); } - searchRequestListenersList.add(listener); } /** @@ -104,9 +105,9 @@ public SearchRequestOperationsListener.CompositeListener buildCompositeListener( Logger logger, SearchRequestOperationsListener... perRequestListeners ) { - final List searchListenersList = new ArrayList<>(searchRequestListenersList); + final List searchListenersList = searchRequestListenersList.stream().filter(SearchRequestOperationsListener::getEnabled).collect(Collectors.toList()); - Arrays.stream(perRequestListeners).parallel().forEach((listener) -> { + Arrays.stream(perRequestListeners).forEach((listener) -> { if (listener != null && listener.getClass() == TransportSearchAction.SearchTimeProvider.class) { TransportSearchAction.SearchTimeProvider timeProvider = (TransportSearchAction.SearchTimeProvider) listener; // phase_took is enabled with request param and/or cluster setting diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java index 59bdc10a32978..7e318a542c812 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java @@ -20,8 +20,15 @@ * @opensearch.internal */ @InternalApi -abstract class SearchRequestOperationsListener { - protected SearchRequestListenerManager searchRequestListenerManager; +public abstract class SearchRequestOperationsListener { + private volatile boolean enabled; + + protected SearchRequestOperationsListener() { + this.enabled = false; + } + protected SearchRequestOperationsListener(boolean enabled) { + this.enabled = enabled; + } abstract void onPhaseStart(SearchPhaseContext context); @@ -33,33 +40,13 @@ void onRequestStart(SearchRequestContext searchRequestContext) {} void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {} - public void setEnabled(boolean enabled) { - if (enabled) { - register(); - } else { - deregister(); - } - } - - /** - * Handler function to register this listener to certain components - * This function will be called when the listener is enabled. - */ - protected void register() { - if (this.searchRequestListenerManager != null) { - this.searchRequestListenerManager.addListener(this); - } + boolean getEnabled() { + return enabled; } - /** - * Handler function to deregister this listener from certain components - * This function will be called when the listener is disabled. - */ - protected void deregister() { - if (this.searchRequestListenerManager != null) { - this.searchRequestListenerManager.removeListener(this); - } + void setEnabled(boolean enabled) { + this.enabled = enabled; } /** diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java index f06619ec6f263..0fadbda6b62a9 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java @@ -37,7 +37,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.inject.Inject; import org.opensearch.common.logging.Loggers; import org.opensearch.common.logging.OpenSearchLogMessage; import org.opensearch.common.logging.SlowLogLevel; @@ -109,18 +108,14 @@ public final class SearchRequestSlowLog extends SearchRequestOperationsListener private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false")); - @Inject public SearchRequestSlowLog( - ClusterService clusterService, - SearchRequestListenerManager searchRequestListenerManager + ClusterService clusterService ) { - this(clusterService, searchRequestListenerManager, LogManager.getLogger(CLUSTER_SEARCH_REQUEST_SLOWLOG_PREFIX)); // logger configured in log4j2.properties + this(clusterService, LogManager.getLogger(CLUSTER_SEARCH_REQUEST_SLOWLOG_PREFIX)); // logger configured in log4j2.properties } - @Inject - SearchRequestSlowLog(ClusterService clusterService, SearchRequestListenerManager searchRequestListenerManager, Logger logger) { + SearchRequestSlowLog(ClusterService clusterService, Logger logger) { this.logger = logger; - this.searchRequestListenerManager = searchRequestListenerManager; Loggers.setLevel(this.logger, SlowLogLevel.TRACE.name()); this.warnThreshold = clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_WARN_SETTING).nanos(); diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java index 185ac2a66a859..19cb9dc4b73d5 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -38,14 +38,12 @@ public final class SearchRequestStats extends SearchRequestOperationsListener { @Inject public SearchRequestStats( - ClusterService clusterService, - SearchRequestListenerManager searchRequestListenerManager + ClusterService clusterService ) { clusterService.getClusterSettings().addSettingsUpdateConsumer(SEARCH_REQUEST_STATS_ENABLED, this::setEnabled); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { phaseStatsMap.put(searchPhaseName, new StatsHolder()); } - this.searchRequestListenerManager = searchRequestListenerManager; } public long getPhaseCurrent(SearchPhaseName searchPhaseName) { 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 12145bf298f63..17bd4b7544e54 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -34,7 +34,11 @@ import org.apache.logging.log4j.LogManager; import org.opensearch.action.admin.cluster.configuration.TransportAddVotingConfigExclusionsAction; import org.opensearch.action.admin.indices.close.TransportCloseIndexAction; -import org.opensearch.action.search.*; +import org.opensearch.action.search.CreatePitController; +import org.opensearch.action.search.SearchRequestListenerManager; +import org.opensearch.action.search.SearchRequestSlowLog; +import org.opensearch.action.search.SearchRequestStats; +import org.opensearch.action.search.TransportSearchAction; import org.opensearch.action.support.AutoCreateIndex; import org.opensearch.action.support.DestructiveOperations; import org.opensearch.action.support.replication.TransportReplicationAction; diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 59672eaf183bc..6acc5105b7b29 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -783,10 +783,16 @@ protected Node( repositoriesServiceReference::get, threadPool ); - final SearchRequestListenerManager searchRequestListenerManager = new SearchRequestListenerManager(clusterService); - final SearchRequestStats searchRequestStats = new SearchRequestStats(clusterService, searchRequestListenerManager); - final SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, searchRequestListenerManager); + final SearchRequestStats searchRequestStats = new SearchRequestStats(clusterService); + final SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); + + // register all standard SearchRequestOperationsListeners to the SearchRequestListenerManager + final SearchRequestListenerManager searchRequestListenerManager = new SearchRequestListenerManager(clusterService); + searchRequestListenerManager.addListeners( + searchRequestStats, + searchRequestSlowLog + ); remoteStoreStatsTrackerFactory = new RemoteStoreStatsTrackerFactory(clusterService, settings); final IndicesService indicesService = new IndicesService( diff --git a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java index 1704238868907..04593fc0b66be 100644 --- a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java +++ b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java @@ -34,7 +34,6 @@ import org.opensearch.action.admin.indices.stats.CommonStats; import org.opensearch.action.admin.indices.stats.CommonStatsFlags; -import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchRequestStats; import org.opensearch.cluster.coordination.PendingClusterStateStats; import org.opensearch.cluster.coordination.PersistedStateStats; @@ -970,8 +969,7 @@ private static NodeIndicesStats getNodeIndicesStats(boolean remoteStoreStats) { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - indicesStats = new NodeIndicesStats(new CommonStats(CommonStatsFlags.ALL), new HashMap<>(), new SearchRequestStats(clusterService, listenerManager)); + indicesStats = new NodeIndicesStats(new CommonStats(CommonStatsFlags.ALL), new HashMap<>(), new SearchRequestStats(clusterService)); RemoteSegmentStats remoteSegmentStats = indicesStats.getSegments().getRemoteSegmentStats(); remoteSegmentStats.addUploadBytesStarted(10L); remoteSegmentStats.addUploadBytesSucceeded(10L); diff --git a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java index d835fbda107c4..d1362acdc3e34 100644 --- a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java @@ -336,8 +336,7 @@ public void testOnPhaseFailureAndVerifyListeners() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testListener = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testListener = new SearchRequestStats(clusterService); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); SearchQueryThenFetchAsyncAction action = createSearchQueryThenFetchAsyncAction(requestOperationListeners); @@ -605,8 +604,7 @@ public void testOnPhaseListenersWithQueryAndThenFetchType() throws InterruptedEx new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testListener = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testListener = new SearchRequestStats(clusterService); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); long delay = (randomIntBetween(1, 5)); @@ -660,8 +658,7 @@ public void testOnPhaseListenersWithDfsType() throws InterruptedException { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testListener = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testListener = new SearchRequestStats(clusterService); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); SearchDfsQueryThenFetchAsyncAction searchDfsQueryThenFetchAsyncAction = createSearchDfsQueryThenFetchAsyncAction( diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java index 7cd1010f1e362..348049dacf667 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java @@ -28,7 +28,7 @@ public void testAddAndGetListeners() { ); SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); SearchRequestOperationsListener testListener = createTestSearchRequestOperationsListener(); - listenerManager.addListener(testListener); + listenerManager.addListeners(testListener); assertEquals(1, listenerManager.getListeners().size()); assertEquals(testListener, listenerManager.getListeners().get(0)); } @@ -42,15 +42,14 @@ public void testRemoveListeners() { SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); - listenerManager.addListener(testListener1); - listenerManager.addListener(testListener2); + listenerManager.addListeners(testListener1, testListener2); assertEquals(2, listenerManager.getListeners().size()); listenerManager.removeListener(testListener2); assertEquals(1, listenerManager.getListeners().size()); assertEquals(testListener1, listenerManager.getListeners().get(0)); } - public void testBuildCompositeListenersWithTimeProvider() throws NoSuchFieldException, IllegalAccessException { + public void testStandardListenersEnabled() throws NoSuchFieldException, IllegalAccessException { ClusterService clusterService = new ClusterService( Settings.EMPTY, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), @@ -58,12 +57,40 @@ public void testBuildCompositeListenersWithTimeProvider() throws NoSuchFieldExce ); SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); + testListener2.setEnabled(true); + listenerManager.addListeners(testListener1, testListener2); + SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); + SearchRequest searchRequest = new SearchRequest().source(source); + SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( + searchRequest, + logger + ); + Field listenersField = SearchRequestOperationsListener.CompositeListener.class.getDeclaredField("listeners"); + listenersField.setAccessible(true); + List listeners = (List) listenersField.get(compositeListener); + assertEquals(1, listeners.size()); + assertEquals(testListener2, listeners.get(0)); + assertEquals(2, listenerManager.getListeners().size()); + assertEquals(testListener1, listenerManager.getListeners().get(0)); + assertEquals(testListener2, listenerManager.getListeners().get(1)); + } + + public void testStandardListenersAndTimeProvider() throws NoSuchFieldException, IllegalAccessException { + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + testListener1.setEnabled(true); SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( 0, System.nanoTime(), System::nanoTime ); - listenerManager.addListener(testListener1); + listenerManager.addListeners(testListener1); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(true); @@ -82,7 +109,39 @@ public void testBuildCompositeListenersWithTimeProvider() throws NoSuchFieldExce assertEquals(testListener1, listenerManager.getListeners().get(0)); } - public void testBuildCompositeListenersWithPhaseTookDisabled() throws NoSuchFieldException, IllegalAccessException { + public void testStandardListenersDisabledAndTimeProvider() throws NoSuchFieldException, IllegalAccessException { + ClusterService clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + null + ); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( + 0, + System.nanoTime(), + System::nanoTime + ); + listenerManager.addListeners(testListener1); + SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); + SearchRequest searchRequest = new SearchRequest().source(source); + searchRequest.setPhaseTook(true); + SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( + searchRequest, + logger, + timeProviderListener + ); + Field listenersField = SearchRequestOperationsListener.CompositeListener.class.getDeclaredField("listeners"); + listenersField.setAccessible(true); + List listeners = (List) listenersField.get(compositeListener); + assertEquals(1, listeners.size()); + assertEquals(timeProviderListener, listeners.get(0)); + assertEquals(1, listenerManager.getListeners().size()); + assertEquals(testListener1, listenerManager.getListeners().get(0)); + assertFalse(listenerManager.getListeners().get(0).getEnabled()); + } + + public void testStandardListenerAndTimeProviderDisabled() throws NoSuchFieldException, IllegalAccessException { ClusterService clusterService = new ClusterService( Settings.EMPTY, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), @@ -90,12 +149,13 @@ public void testBuildCompositeListenersWithPhaseTookDisabled() throws NoSuchFiel ); SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + testListener1.setEnabled(true); SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( 0, System.nanoTime(), System::nanoTime ); - listenerManager.addListener(testListener1); + listenerManager.addListeners(testListener1); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(false); diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java index fc8300e0174cf..5456ef02b9b8e 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java @@ -95,8 +95,7 @@ public void testMultipleSlowLoggersUseSingleLog4jLogger() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService1); - SearchRequestSlowLog searchRequestSlowLog1 = new SearchRequestSlowLog(clusterService1, listenerManager); + SearchRequestSlowLog searchRequestSlowLog1 = new SearchRequestSlowLog(clusterService1); int numberOfLoggersBefore = context.getLoggers().size(); SearchPhaseContext searchPhaseContext2 = new MockSearchPhaseContext(1); @@ -106,7 +105,7 @@ public void testMultipleSlowLoggersUseSingleLog4jLogger() { null ); SearchRequestListenerManager listenerManager2 = new SearchRequestListenerManager(clusterService2); - SearchRequestSlowLog searchRequestSlowLog2 = new SearchRequestSlowLog(clusterService2, listenerManager2); + SearchRequestSlowLog searchRequestSlowLog2 = new SearchRequestSlowLog(clusterService2); int numberOfLoggersAfter = context.getLoggers().size(); assertThat(numberOfLoggersAfter, equalTo(numberOfLoggersBefore)); @@ -126,8 +125,7 @@ public void testOnRequestEnd() throws InterruptedException { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager, logger); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, logger); final List searchListenersList = new ArrayList<>(List.of(searchRequestSlowLog)); when(searchRequestContext.getSearchRequestOperationsListener()).thenReturn( @@ -160,8 +158,7 @@ public void testConcurrentOnRequestEnd() throws InterruptedException { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager, logger); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, logger); final List searchListenersList = new ArrayList<>(List.of(searchRequestSlowLog)); when(searchPhaseContext.getRequest()).thenReturn(searchRequest); @@ -312,8 +309,7 @@ public void testLevelSettingWarn() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); assertEquals(level, searchRequestSlowLog.getLevel()); } @@ -324,8 +320,7 @@ public void testLevelSettingDebug() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); assertEquals(level, searchRequestSlowLog.getLevel().toString()); } @@ -336,10 +331,9 @@ public void testLevelSettingFail() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); try { - new SearchRequestSlowLog(clusterService, listenerManager); + new SearchRequestSlowLog(clusterService); fail(); } catch (IllegalArgumentException ex) { final String expected = "No enum constant org.opensearch.common.logging.SlowLogLevel.NOT A LEVEL"; @@ -357,8 +351,7 @@ public void testSetThresholds() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); assertEquals(TimeValue.timeValueMillis(400).nanos(), searchRequestSlowLog.getWarnThreshold()); assertEquals(TimeValue.timeValueMillis(300).nanos(), searchRequestSlowLog.getInfoThreshold()); assertEquals(TimeValue.timeValueMillis(200).nanos(), searchRequestSlowLog.getDebugThreshold()); @@ -375,8 +368,7 @@ public void testSetThresholdsUnits() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); assertEquals(TimeValue.timeValueSeconds(400).nanos(), searchRequestSlowLog.getWarnThreshold()); assertEquals(TimeValue.timeValueMillis(300).nanos(), searchRequestSlowLog.getInfoThreshold()); assertEquals(TimeValue.timeValueNanos(200000).nanos(), searchRequestSlowLog.getDebugThreshold()); @@ -391,8 +383,7 @@ public void testSetThresholdsDefaults() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService, listenerManager); + SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); assertEquals(TimeValue.timeValueMillis(400).nanos(), searchRequestSlowLog.getWarnThreshold()); assertEquals(TimeValue.timeValueMillis(-1).nanos(), searchRequestSlowLog.getInfoThreshold()); assertEquals(TimeValue.timeValueMillis(200).nanos(), searchRequestSlowLog.getDebugThreshold()); @@ -406,10 +397,9 @@ public void testSetThresholdsError() { Settings settings = settingsBuilder.build(); ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = new ClusterService(settings, clusterSettings, null); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); try { - new SearchRequestSlowLog(clusterService, listenerManager); + new SearchRequestSlowLog(clusterService); fail(); } catch (IllegalArgumentException ex) { final String expected = diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java index 6ac7eb6040f13..e2b21689bdb03 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -30,8 +30,7 @@ public void testSearchRequestPhaseFailure() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -51,8 +50,7 @@ public void testSearchRequestStats() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); @@ -78,8 +76,7 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); @@ -111,8 +108,7 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); @@ -153,8 +149,7 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); diff --git a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java index 612d3a39a1149..ea370ebc6cf76 100644 --- a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java +++ b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java @@ -32,7 +32,11 @@ package org.opensearch.index.search.stats; -import org.opensearch.action.search.*; +import org.opensearch.action.search.SearchPhase; +import org.opensearch.action.search.SearchPhaseContext; +import org.opensearch.action.search.SearchPhaseName; +import org.opensearch.action.search.SearchRequestOperationsListenerSupport; +import org.opensearch.action.search.SearchRequestStats; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; @@ -81,8 +85,7 @@ public void testShardLevelSearchGroupStats() throws Exception { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); SearchPhaseContext ctx = mock(SearchPhaseContext.class); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhase mockSearchPhase = mock(SearchPhase.class); diff --git a/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java b/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java index 174dbc895206b..b07dc6d298045 100644 --- a/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java +++ b/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java @@ -33,7 +33,6 @@ package org.opensearch.indices; import org.opensearch.action.admin.indices.stats.CommonStats; -import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchRequestStats; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.ClusterSettings; @@ -55,8 +54,7 @@ public void testInvalidLevel() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestStats requestStats = new SearchRequestStats(clusterService, listenerManager); + SearchRequestStats requestStats = new SearchRequestStats(clusterService); final NodeIndicesStats stats = new NodeIndicesStats(oldStats, Collections.emptyMap(), requestStats); final String level = randomAlphaOfLength(16); final ToXContent.Params params = new ToXContent.MapParams(Collections.singletonMap("level", level)); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 519250d40cae6..bf1a6e1593f4c 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -90,7 +90,6 @@ import org.opensearch.action.search.SearchExecutionStatsCollector; import org.opensearch.action.search.SearchPhaseController; import org.opensearch.action.search.SearchRequest; -import org.opensearch.action.search.SearchRequestSlowLog; import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchResponse; import org.opensearch.action.search.SearchTransportService; From e7ce64663f995af7a149184ff5f2cd96f6d56c60 Mon Sep 17 00:00:00 2001 From: Chenyang Ji Date: Mon, 18 Dec 2023 14:20:09 -0800 Subject: [PATCH 4/7] make the logic to decide enabled self-contained in each listener Signed-off-by: Chenyang Ji --- .../search/SearchWeightedRoutingIT.java | 2 +- .../search/stats/SearchStatsIT.java | 2 +- .../search/SearchRequestListenerManager.java | 62 ++--------- .../SearchRequestOperationsListener.java | 7 +- .../action/search/SearchRequestSlowLog.java | 29 ++--- .../action/search/SearchRequestStats.java | 5 +- .../action/search/TransportSearchAction.java | 31 ++++-- .../common/settings/ClusterSettings.java | 3 +- .../main/java/org/opensearch/node/Node.java | 18 +-- .../cluster/node/stats/NodeStatsTests.java | 6 +- .../SearchRequestListenerManagerTests.java | 104 ++++-------------- .../search/SearchRequestSlowLogTests.java | 2 +- .../snapshots/SnapshotResiliencyTests.java | 2 +- 13 files changed, 94 insertions(+), 179 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/search/SearchWeightedRoutingIT.java b/server/src/internalClusterTest/java/org/opensearch/search/SearchWeightedRoutingIT.java index aa1fe695ecc12..d1e66c19c28e2 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/SearchWeightedRoutingIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/SearchWeightedRoutingIT.java @@ -57,7 +57,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import static org.opensearch.action.search.TransportSearchAction.SEARCH_REQUEST_STATS_ENABLED_KEY; +import static org.opensearch.action.search.SearchRequestStats.SEARCH_REQUEST_STATS_ENABLED_KEY; import static org.opensearch.search.aggregations.AggregationBuilders.terms; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; diff --git a/server/src/internalClusterTest/java/org/opensearch/search/stats/SearchStatsIT.java b/server/src/internalClusterTest/java/org/opensearch/search/stats/SearchStatsIT.java index 253a8b2b14824..8fb3c57dd7680 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/stats/SearchStatsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/stats/SearchStatsIT.java @@ -64,7 +64,7 @@ import java.util.Set; import java.util.function.Function; -import static org.opensearch.action.search.TransportSearchAction.SEARCH_REQUEST_STATS_ENABLED_KEY; +import static org.opensearch.action.search.SearchRequestStats.SEARCH_REQUEST_STATS_ENABLED_KEY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; import static org.opensearch.index.query.QueryBuilders.matchAllQuery; diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java b/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java index 25e44a0a562e8..884a09a60ec49 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java @@ -9,14 +9,12 @@ package org.opensearch.action.search; import org.apache.logging.log4j.Logger; -import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.settings.Setting; import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; - +import java.util.stream.Stream; /** * SearchRequestListenerManager manages listeners registered to search requests, @@ -27,32 +25,18 @@ * @opensearch.internal */ public class SearchRequestListenerManager { - - private final ClusterService clusterService; - public static final String SEARCH_PHASE_TOOK_ENABLED_KEY = "search.phase_took_enabled"; - public static final Setting SEARCH_PHASE_TOOK_ENABLED = Setting.boolSetting( - SEARCH_PHASE_TOOK_ENABLED_KEY, - false, - Setting.Property.Dynamic, - Setting.Property.NodeScope - ); private final List searchRequestListenersList; - public SearchRequestListenerManager( - ClusterService clusterService - ) { - this.clusterService = clusterService; - searchRequestListenersList = new ArrayList<>(); - } - /** - * Add multiple {@link SearchRequestOperationsListener} to the searchRequestListenersList. + * Create the SearchRequestListenerManager and add multiple {@link SearchRequestOperationsListener} + * to the searchRequestListenersList. * Those enabled listeners will be executed during each search request. * * @param listeners Multiple SearchRequestOperationsListener object to add. * @throws IllegalArgumentException if any input listener is null or already exists in the list. */ - public void addListeners(SearchRequestOperationsListener... listeners) { + public SearchRequestListenerManager(SearchRequestOperationsListener... listeners) { + searchRequestListenersList = new ArrayList<>(); for (SearchRequestOperationsListener listener : listeners) { if (listener == null) { throw new IllegalArgumentException("listener must not be null"); @@ -64,22 +48,6 @@ public void addListeners(SearchRequestOperationsListener... listeners) { } } - /** - * Remove a {@link SearchRequestOperationsListener} from the searchRequestListenersList, - * - * @param listener A SearchRequestOperationsListener object to remove. - * @throws IllegalArgumentException if the input listener is null or already exists in the list. - */ - public void removeListener(SearchRequestOperationsListener listener) { - if (listener == null) { - throw new IllegalArgumentException("listener must not be null"); - } - if (!searchRequestListenersList.contains(listener)) { - throw new IllegalArgumentException("listener does not exist in the listeners list"); - } - searchRequestListenersList.remove(listener); - } - /** * Get searchRequestListenersList, * @@ -90,35 +58,23 @@ public List getListeners() { return searchRequestListenersList; } - /** * Create the {@link SearchRequestOperationsListener.CompositeListener} * with the all listeners enabled at cluster-level and request-level. * - * @param searchRequest The SearchRequest object. SearchRequestListenerManager will decide which request-level listeners to add based on states/flags of the request * @param logger Logger to be attached to the {@link SearchRequestOperationsListener.CompositeListener} * @param perRequestListeners the per-request listeners that can be optionally added to the returned CompositeListener list. * @return SearchRequestOperationsListener.CompositeListener */ public SearchRequestOperationsListener.CompositeListener buildCompositeListener( - SearchRequest searchRequest, Logger logger, SearchRequestOperationsListener... perRequestListeners ) { - final List searchListenersList = searchRequestListenersList.stream().filter(SearchRequestOperationsListener::getEnabled).collect(Collectors.toList()); + final List searchListenersList = Stream.concat( + searchRequestListenersList.stream(), + Arrays.stream(perRequestListeners) + ).filter(SearchRequestOperationsListener::getEnabled).collect(Collectors.toList()); - Arrays.stream(perRequestListeners).forEach((listener) -> { - if (listener != null && listener.getClass() == TransportSearchAction.SearchTimeProvider.class) { - TransportSearchAction.SearchTimeProvider timeProvider = (TransportSearchAction.SearchTimeProvider) listener; - // phase_took is enabled with request param and/or cluster setting - boolean phaseTookEnabled = (searchRequest.isPhaseTook() != null && searchRequest.isPhaseTook()) || - clusterService.getClusterSettings().get(SEARCH_PHASE_TOOK_ENABLED); - if (phaseTookEnabled) { - timeProvider.setPhaseTook(true); - searchListenersList.add(timeProvider); - } - } - }); return new SearchRequestOperationsListener.CompositeListener(searchListenersList, logger); } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java index 7e318a542c812..ecebabcf2b8ae 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java @@ -26,6 +26,7 @@ public abstract class SearchRequestOperationsListener { protected SearchRequestOperationsListener() { this.enabled = false; } + protected SearchRequestOperationsListener(boolean enabled) { this.enabled = enabled; } @@ -40,7 +41,6 @@ void onRequestStart(SearchRequestContext searchRequestContext) {} void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {} - boolean getEnabled() { return enabled; } @@ -62,6 +62,7 @@ static final class CompositeListener extends SearchRequestOperationsListener { CompositeListener(List listeners, Logger logger) { this.listeners = listeners; this.logger = logger; + this.setEnabled(true); } @Override @@ -118,5 +119,9 @@ public void onRequestEnd(SearchPhaseContext context, SearchRequestContext search } } } + + public List getListeners() { + return listeners; + } } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java index 0fadbda6b62a9..7f25f9026f215 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java @@ -108,9 +108,7 @@ public final class SearchRequestSlowLog extends SearchRequestOperationsListener private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false")); - public SearchRequestSlowLog( - ClusterService clusterService - ) { + public SearchRequestSlowLog(ClusterService clusterService) { this(clusterService, LogManager.getLogger(CLUSTER_SEARCH_REQUEST_SLOWLOG_PREFIX)); // logger configured in log4j2.properties } @@ -118,11 +116,11 @@ public SearchRequestSlowLog( this.logger = logger; Loggers.setLevel(this.logger, SlowLogLevel.TRACE.name()); - this.warnThreshold = clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_WARN_SETTING).nanos(); - this.infoThreshold = clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_INFO_SETTING).nanos(); - this.debugThreshold = clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_DEBUG_SETTING).nanos(); - this.traceThreshold = clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_TRACE_SETTING).nanos(); - this.level = clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_LEVEL); + this.setWarnThreshold(clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_WARN_SETTING)); + this.setInfoThreshold(clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_INFO_SETTING)); + this.setDebugThreshold(clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_DEBUG_SETTING)); + this.setTraceThreshold(clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_TRACE_SETTING)); + this.setLevel(clusterService.getClusterSettings().get(CLUSTER_SEARCH_REQUEST_SLOWLOG_LEVEL)); clusterService.getClusterSettings() .addSettingsUpdateConsumer(CLUSTER_SEARCH_REQUEST_SLOWLOG_THRESHOLD_WARN_SETTING, this::setWarnThreshold); @@ -235,22 +233,22 @@ private static String escapeJson(String text) { void setWarnThreshold(TimeValue warnThreshold) { this.warnThreshold = warnThreshold.nanos(); - setEnabled(); + setEnabledIfThresholdExceed(); } void setInfoThreshold(TimeValue infoThreshold) { this.infoThreshold = infoThreshold.nanos(); - setEnabled(); + setEnabledIfThresholdExceed(); } void setDebugThreshold(TimeValue debugThreshold) { this.debugThreshold = debugThreshold.nanos(); - setEnabled(); + setEnabledIfThresholdExceed(); } void setTraceThreshold(TimeValue traceThreshold) { this.traceThreshold = traceThreshold.nanos(); - setEnabled(); + setEnabledIfThresholdExceed(); } void setLevel(SlowLogLevel level) { @@ -277,10 +275,7 @@ SlowLogLevel getLevel() { return level; } - private void setEnabled() { - super.setEnabled(this.warnThreshold >= 0 - || this.debugThreshold >= 0 - || this.infoThreshold >= 0 - || this.traceThreshold >= 0); + private void setEnabledIfThresholdExceed() { + super.setEnabled(this.warnThreshold >= 0 || this.debugThreshold >= 0 || this.infoThreshold >= 0 || this.traceThreshold >= 0); } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java index 19cb9dc4b73d5..2b01be8149331 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -37,9 +37,8 @@ public final class SearchRequestStats extends SearchRequestOperationsListener { ); @Inject - public SearchRequestStats( - ClusterService clusterService - ) { + public SearchRequestStats(ClusterService clusterService) { + this.setEnabled(clusterService.getClusterSettings().get(SEARCH_REQUEST_STATS_ENABLED)); clusterService.getClusterSettings().addSettingsUpdateConsumer(SEARCH_REQUEST_STATS_ENABLED, this::setEnabled); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { phaseStatsMap.put(searchPhaseName, new StatsHolder()); 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 17b220ae324a4..2f03d21723b27 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -154,6 +154,14 @@ public class TransportSearchAction extends HandledTransportAction SEARCH_PHASE_TOOK_ENABLED = Setting.boolSetting( + SEARCH_PHASE_TOOK_ENABLED_KEY, + false, + Setting.Property.Dynamic, + Setting.Property.NodeScope + ); + private final NodeClient client; private final ThreadPool threadPool; private final ClusterService clusterService; @@ -277,7 +285,6 @@ static final class SearchTimeProvider extends SearchRequestOperationsListener { private final long absoluteStartMillis; private final long relativeStartNanos; private final LongSupplier relativeCurrentNanosProvider; - private boolean phaseTook = false; /** * Instantiates a new search time provider. The absolute start time is the real clock time @@ -304,12 +311,8 @@ long buildTookInMillis() { return TimeUnit.NANOSECONDS.toMillis(relativeCurrentNanosProvider.getAsLong() - relativeStartNanos); } - public void setPhaseTook(boolean phaseTook) { - this.phaseTook = phaseTook; - } - SearchResponse.PhaseTook getPhaseTook() { - if (phaseTook) { + if (getEnabled()) { Map phaseTookMap = new HashMap<>(); // Convert Map to Map for SearchResponse() for (SearchPhaseName searchPhaseName : phaseStatsMap.keySet()) { @@ -323,6 +326,20 @@ SearchResponse.PhaseTook getPhaseTook() { Map phaseStatsMap = new EnumMap<>(SearchPhaseName.class); + /** + * Set if this listener is enabled based on the cluster level setting + * and per request enable flags. + * + * @param enabledAtClusterLevel if the SearchTimeProvider listener is enabled at cluster level + * @param searchRequest the original Search Request + * @opensearch.internal + */ + + void setEnabled(boolean enabledAtClusterLevel, SearchRequest searchRequest) { + // phase_took is enabled wi th request param and/or cluster setting + super.setEnabled(enabledAtClusterLevel || (searchRequest.isPhaseTook() != null && searchRequest.isPhaseTook())); + } + @Override void onPhaseStart(SearchPhaseContext context) {} @@ -462,8 +479,8 @@ private void executeRequest( relativeStartNanos, System::nanoTime ); + timeProvider.setEnabled(clusterService.getClusterSettings().get(SEARCH_PHASE_TOOK_ENABLED), originalSearchRequest); SearchRequestOperationsListener.CompositeListener requestOperationsListeners = searchRequestListenerManager.buildCompositeListener( - originalSearchRequest, logger, timeProvider ); 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 17bd4b7544e54..277286ae1ff1b 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -35,7 +35,6 @@ import org.opensearch.action.admin.cluster.configuration.TransportAddVotingConfigExclusionsAction; import org.opensearch.action.admin.indices.close.TransportCloseIndexAction; import org.opensearch.action.search.CreatePitController; -import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchRequestSlowLog; import org.opensearch.action.search.SearchRequestStats; import org.opensearch.action.search.TransportSearchAction; @@ -383,7 +382,7 @@ public void apply(Settings value, Settings current, Settings previous) { TransportSearchAction.SHARD_COUNT_LIMIT_SETTING, TransportSearchAction.SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING, TransportSearchAction.SEARCH_QUERY_METRICS_ENABLED_SETTING, - SearchRequestListenerManager.SEARCH_PHASE_TOOK_ENABLED, + TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED, SearchRequestStats.SEARCH_REQUEST_STATS_ENABLED, RemoteClusterService.REMOTE_CLUSTER_SKIP_UNAVAILABLE, SniffConnectionStrategy.REMOTE_CONNECTIONS_PER_CLUSTER, diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 6acc5105b7b29..aadac4324211d 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -47,6 +47,7 @@ import org.opensearch.action.search.SearchExecutionStatsCollector; import org.opensearch.action.search.SearchPhaseController; import org.opensearch.action.search.SearchRequestListenerManager; +import org.opensearch.action.search.SearchRequestOperationsListener; import org.opensearch.action.search.SearchRequestSlowLog; import org.opensearch.action.search.SearchRequestStats; import org.opensearch.action.search.SearchTransportService; @@ -787,13 +788,6 @@ protected Node( final SearchRequestStats searchRequestStats = new SearchRequestStats(clusterService); final SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); - // register all standard SearchRequestOperationsListeners to the SearchRequestListenerManager - final SearchRequestListenerManager searchRequestListenerManager = new SearchRequestListenerManager(clusterService); - searchRequestListenerManager.addListeners( - searchRequestStats, - searchRequestSlowLog - ); - remoteStoreStatsTrackerFactory = new RemoteStoreStatsTrackerFactory(clusterService, settings); final IndicesService indicesService = new IndicesService( settings, @@ -887,6 +881,16 @@ protected Node( ) .collect(Collectors.toList()); + // register all standard SearchRequestOperationsListeners to the SearchRequestListenerManager + final SearchRequestListenerManager searchRequestListenerManager = new SearchRequestListenerManager( + Stream.concat( + Stream.of(searchRequestStats, searchRequestSlowLog), + pluginComponents.stream() + .filter(p -> p instanceof SearchRequestOperationsListener) + .map(p -> (SearchRequestOperationsListener) p) + ).toArray(SearchRequestOperationsListener[]::new) + ); + ActionModule actionModule = new ActionModule( settings, clusterModule.getIndexNameExpressionResolver(), diff --git a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java index 04593fc0b66be..c59342d0f4292 100644 --- a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java +++ b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java @@ -969,7 +969,11 @@ private static NodeIndicesStats getNodeIndicesStats(boolean remoteStoreStats) { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - indicesStats = new NodeIndicesStats(new CommonStats(CommonStatsFlags.ALL), new HashMap<>(), new SearchRequestStats(clusterService)); + indicesStats = new NodeIndicesStats( + new CommonStats(CommonStatsFlags.ALL), + new HashMap<>(), + new SearchRequestStats(clusterService) + ); RemoteSegmentStats remoteSegmentStats = indicesStats.getSegments().getRemoteSegmentStats(); remoteSegmentStats.addUploadBytesStarted(10L); remoteSegmentStats.addUploadBytesSucceeded(10L); diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java index 348049dacf667..c16841ae1ee17 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java @@ -8,67 +8,27 @@ package org.opensearch.action.search; -import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.settings.ClusterSettings; -import org.opensearch.common.settings.Settings; import org.opensearch.index.query.QueryBuilders; import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.test.OpenSearchTestCase; -import java.lang.reflect.Field; import java.util.List; - public class SearchRequestListenerManagerTests extends OpenSearchTestCase { public void testAddAndGetListeners() { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); SearchRequestOperationsListener testListener = createTestSearchRequestOperationsListener(); - listenerManager.addListeners(testListener); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener); assertEquals(1, listenerManager.getListeners().size()); assertEquals(testListener, listenerManager.getListeners().get(0)); } - public void testRemoveListeners() { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); - SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); - SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); - listenerManager.addListeners(testListener1, testListener2); - assertEquals(2, listenerManager.getListeners().size()); - listenerManager.removeListener(testListener2); - assertEquals(1, listenerManager.getListeners().size()); - assertEquals(testListener1, listenerManager.getListeners().get(0)); - } - - public void testStandardListenersEnabled() throws NoSuchFieldException, IllegalAccessException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + public void testStandardListenersEnabled() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener1, testListener2); testListener2.setEnabled(true); - listenerManager.addListeners(testListener1, testListener2); - SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); - SearchRequest searchRequest = new SearchRequest().source(source); - SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( - searchRequest, - logger - ); - Field listenersField = SearchRequestOperationsListener.CompositeListener.class.getDeclaredField("listeners"); - listenersField.setAccessible(true); - List listeners = (List) listenersField.get(compositeListener); + SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener(logger); + List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); assertEquals(testListener2, listeners.get(0)); assertEquals(2, listenerManager.getListeners().size()); @@ -76,32 +36,25 @@ public void testStandardListenersEnabled() throws NoSuchFieldException, IllegalA assertEquals(testListener2, listenerManager.getListeners().get(1)); } - public void testStandardListenersAndTimeProvider() throws NoSuchFieldException, IllegalAccessException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + public void testStandardListenersAndTimeProvider() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener1); + testListener1.setEnabled(true); - SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( + TransportSearchAction.SearchTimeProvider timeProviderListener = new TransportSearchAction.SearchTimeProvider( 0, System.nanoTime(), System::nanoTime ); - listenerManager.addListeners(testListener1); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(true); + timeProviderListener.setEnabled(false, searchRequest); SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( - searchRequest, logger, timeProviderListener ); - Field listenersField = SearchRequestOperationsListener.CompositeListener.class.getDeclaredField("listeners"); - listenersField.setAccessible(true); - List listeners = (List) listenersField.get(compositeListener); + List listeners = compositeListener.getListeners(); assertEquals(2, listeners.size()); assertEquals(testListener1, listeners.get(0)); assertEquals(timeProviderListener, listeners.get(1)); @@ -109,31 +62,23 @@ public void testStandardListenersAndTimeProvider() throws NoSuchFieldException, assertEquals(testListener1, listenerManager.getListeners().get(0)); } - public void testStandardListenersDisabledAndTimeProvider() throws NoSuchFieldException, IllegalAccessException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + public void testStandardListenersDisabledAndTimeProvider() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); - SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener1); + TransportSearchAction.SearchTimeProvider timeProviderListener = new TransportSearchAction.SearchTimeProvider( 0, System.nanoTime(), System::nanoTime ); - listenerManager.addListeners(testListener1); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(true); + timeProviderListener.setEnabled(false, searchRequest); SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( - searchRequest, logger, timeProviderListener ); - Field listenersField = SearchRequestOperationsListener.CompositeListener.class.getDeclaredField("listeners"); - listenersField.setAccessible(true); - List listeners = (List) listenersField.get(compositeListener); + List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); assertEquals(timeProviderListener, listeners.get(0)); assertEquals(1, listenerManager.getListeners().size()); @@ -141,39 +86,30 @@ public void testStandardListenersDisabledAndTimeProvider() throws NoSuchFieldExc assertFalse(listenerManager.getListeners().get(0).getEnabled()); } - public void testStandardListenerAndTimeProviderDisabled() throws NoSuchFieldException, IllegalAccessException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + public void testStandardListenerAndTimeProviderDisabled() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener1); + testListener1.setEnabled(true); SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( 0, System.nanoTime(), System::nanoTime ); - listenerManager.addListeners(testListener1); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(false); SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( - searchRequest, logger, timeProviderListener ); - Field listenersField = SearchRequestOperationsListener.CompositeListener.class.getDeclaredField("listeners"); - listenersField.setAccessible(true); - List listeners = (List) listenersField.get(compositeListener); + List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); assertEquals(testListener1, listeners.get(0)); assertEquals(1, listenerManager.getListeners().size()); assertEquals(testListener1, listenerManager.getListeners().get(0)); } - public SearchRequestOperationsListener createTestSearchRequestOperationsListener() { return new SearchRequestOperationsListener() { @Override diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java index 5456ef02b9b8e..e70bba36445e0 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java @@ -104,7 +104,7 @@ public void testMultipleSlowLoggersUseSingleLog4jLogger() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager2 = new SearchRequestListenerManager(clusterService2); + SearchRequestListenerManager listenerManager2 = new SearchRequestListenerManager(); SearchRequestSlowLog searchRequestSlowLog2 = new SearchRequestSlowLog(clusterService2); int numberOfLoggersAfter = context.getLoggers().size(); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index bf1a6e1593f4c..e631d3e67a484 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -2285,7 +2285,7 @@ public void onFailure(final Exception e) { writableRegistry(), searchService::aggReduceContextBuilder ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(clusterService); + SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(); actions.put( SearchAction.INSTANCE, new TransportSearchAction( From ac218536097326639387456c5d76ac3852437eea Mon Sep 17 00:00:00 2001 From: Chenyang Ji Date: Tue, 2 Jan 2024 14:18:07 -0800 Subject: [PATCH 5/7] rename SearchRequestListenerManager to SearchRequestOperationsListeners Signed-off-by: Chenyang Ji --- ... => SearchRequestOperationsListeners.java} | 13 +++--- .../action/search/SearchRequestStats.java | 8 ++-- .../action/search/TransportSearchAction.java | 12 +++-- .../main/java/org/opensearch/node/Node.java | 10 ++--- .../cluster/node/stats/NodeStatsTests.java | 9 +--- .../AbstractSearchAsyncActionTests.java | 25 +++-------- ...earchRequestOperationsListenersTests.java} | 44 +++++++++---------- .../search/SearchRequestSlowLogTests.java | 2 +- .../search/SearchRequestStatsTests.java | 41 +++++------------ .../index/search/stats/SearchStatsTests.java | 9 +--- .../indices/NodeIndicesStatsTests.java | 9 +--- .../snapshots/SnapshotResiliencyTests.java | 6 +-- 12 files changed, 67 insertions(+), 121 deletions(-) rename server/src/main/java/org/opensearch/action/search/{SearchRequestListenerManager.java => SearchRequestOperationsListeners.java} (84%) rename server/src/test/java/org/opensearch/action/search/{SearchRequestListenerManagerTests.java => SearchRequestOperationsListenersTests.java} (72%) diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java similarity index 84% rename from server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java rename to server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java index 884a09a60ec49..f20c52b9d97d4 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestListenerManager.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java @@ -17,33 +17,30 @@ import java.util.stream.Stream; /** - * SearchRequestListenerManager manages listeners registered to search requests, + * SearchRequestOperationsListeners contains listeners registered to search requests, * and is responsible for creating the {@link SearchRequestOperationsListener.CompositeListener} * with the all listeners enabled at cluster-level and request-level. * * * @opensearch.internal */ -public class SearchRequestListenerManager { +public class SearchRequestOperationsListeners { private final List searchRequestListenersList; /** - * Create the SearchRequestListenerManager and add multiple {@link SearchRequestOperationsListener} + * Create the SearchRequestOperationsListeners and add multiple {@link SearchRequestOperationsListener} * to the searchRequestListenersList. * Those enabled listeners will be executed during each search request. * * @param listeners Multiple SearchRequestOperationsListener object to add. - * @throws IllegalArgumentException if any input listener is null or already exists in the list. + * @throws IllegalArgumentException if any input listener is null. */ - public SearchRequestListenerManager(SearchRequestOperationsListener... listeners) { + public SearchRequestOperationsListeners(SearchRequestOperationsListener... listeners) { searchRequestListenersList = new ArrayList<>(); for (SearchRequestOperationsListener listener : listeners) { if (listener == null) { throw new IllegalArgumentException("listener must not be null"); } - if (searchRequestListenersList.contains(listener)) { - throw new IllegalArgumentException("listener already added"); - } searchRequestListenersList.add(listener); } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java index 2b01be8149331..88d599a0dcdaa 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -8,11 +8,11 @@ package org.opensearch.action.search; -import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.annotation.PublicApi; import org.opensearch.common.inject.Inject; import org.opensearch.common.metrics.CounterMetric; import org.opensearch.common.metrics.MeanMetric; +import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Setting; import java.util.EnumMap; @@ -37,9 +37,9 @@ public final class SearchRequestStats extends SearchRequestOperationsListener { ); @Inject - public SearchRequestStats(ClusterService clusterService) { - this.setEnabled(clusterService.getClusterSettings().get(SEARCH_REQUEST_STATS_ENABLED)); - clusterService.getClusterSettings().addSettingsUpdateConsumer(SEARCH_REQUEST_STATS_ENABLED, this::setEnabled); + public SearchRequestStats(ClusterSettings clusterSettings) { + this.setEnabled(clusterSettings.get(SEARCH_REQUEST_STATS_ENABLED)); + clusterSettings.addSettingsUpdateConsumer(SEARCH_REQUEST_STATS_ENABLED, this::setEnabled); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { phaseStatsMap.put(searchPhaseName, new StatsHolder()); } 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 2f03d21723b27..ca7853cd78031 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -173,7 +173,7 @@ public class TransportSearchAction extends HandledTransportAction) SearchRequest::new); this.client = client; @@ -213,7 +213,7 @@ public TransportSearchAction( this.searchPipelineService = searchPipelineService; this.metricsRegistry = metricsRegistry; this.searchQueryMetricsEnabled = clusterService.getClusterSettings().get(SEARCH_QUERY_METRICS_ENABLED_SETTING); - this.searchRequestListenerManager = searchRequestListenerManager; + this.searchRequestOperationsListeners = searchRequestOperationsListeners; clusterService.getClusterSettings() .addSettingsUpdateConsumer(SEARCH_QUERY_METRICS_ENABLED_SETTING, this::setSearchQueryMetricsEnabled); } @@ -480,10 +480,8 @@ private void executeRequest( System::nanoTime ); timeProvider.setEnabled(clusterService.getClusterSettings().get(SEARCH_PHASE_TOOK_ENABLED), originalSearchRequest); - SearchRequestOperationsListener.CompositeListener requestOperationsListeners = searchRequestListenerManager.buildCompositeListener( - logger, - timeProvider - ); + SearchRequestOperationsListener.CompositeListener requestOperationsListeners = searchRequestOperationsListeners + .buildCompositeListener(logger, timeProvider); SearchRequestContext searchRequestContext = new SearchRequestContext(requestOperationsListeners); searchRequestContext.getSearchRequestOperationsListener().onRequestStart(searchRequestContext); diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index aadac4324211d..0a08885006809 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -46,8 +46,8 @@ import org.opensearch.action.admin.cluster.snapshots.status.TransportNodesSnapshotsStatus; import org.opensearch.action.search.SearchExecutionStatsCollector; import org.opensearch.action.search.SearchPhaseController; -import org.opensearch.action.search.SearchRequestListenerManager; import org.opensearch.action.search.SearchRequestOperationsListener; +import org.opensearch.action.search.SearchRequestOperationsListeners; import org.opensearch.action.search.SearchRequestSlowLog; import org.opensearch.action.search.SearchRequestStats; import org.opensearch.action.search.SearchTransportService; @@ -785,7 +785,7 @@ protected Node( threadPool ); - final SearchRequestStats searchRequestStats = new SearchRequestStats(clusterService); + final SearchRequestStats searchRequestStats = new SearchRequestStats(clusterService.getClusterSettings()); final SearchRequestSlowLog searchRequestSlowLog = new SearchRequestSlowLog(clusterService); remoteStoreStatsTrackerFactory = new RemoteStoreStatsTrackerFactory(clusterService, settings); @@ -881,8 +881,8 @@ protected Node( ) .collect(Collectors.toList()); - // register all standard SearchRequestOperationsListeners to the SearchRequestListenerManager - final SearchRequestListenerManager searchRequestListenerManager = new SearchRequestListenerManager( + // register all standard SearchRequestOperationsListeners to the SearchRequestOperationsListeners + final SearchRequestOperationsListeners searchRequestOperationsListeners = new SearchRequestOperationsListeners( Stream.concat( Stream.of(searchRequestStats, searchRequestSlowLog), pluginComponents.stream() @@ -1287,7 +1287,7 @@ protected Node( b.bind(RemoteClusterStateService.class).toProvider(() -> remoteClusterStateService); b.bind(PersistedStateRegistry.class).toInstance(persistedStateRegistry); b.bind(SegmentReplicationStatsTracker.class).toInstance(segmentReplicationStatsTracker); - b.bind(SearchRequestListenerManager.class).toInstance(searchRequestListenerManager); + b.bind(SearchRequestOperationsListeners.class).toInstance(searchRequestOperationsListeners); }); injector = modules.createInjector(); diff --git a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java index c59342d0f4292..a5ca08f141560 100644 --- a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java +++ b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java @@ -41,7 +41,6 @@ import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.routing.WeightedRoutingStats; import org.opensearch.cluster.service.ClusterManagerThrottlingStats; -import org.opensearch.cluster.service.ClusterService; import org.opensearch.cluster.service.ClusterStateStats; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.metrics.OperationStats; @@ -964,15 +963,11 @@ public void apply(String action, AdmissionControlActionType admissionControlActi private static NodeIndicesStats getNodeIndicesStats(boolean remoteStoreStats) { NodeIndicesStats indicesStats = null; if (remoteStoreStats) { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); indicesStats = new NodeIndicesStats( new CommonStats(CommonStatsFlags.ALL), new HashMap<>(), - new SearchRequestStats(clusterService) + new SearchRequestStats(clusterSettings) ); RemoteSegmentStats remoteSegmentStats = indicesStats.getSegments().getRemoteSegmentStats(); remoteSegmentStats.addUploadBytesStarted(10L); diff --git a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java index d1362acdc3e34..e7d3f4108a4b9 100644 --- a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java @@ -36,7 +36,6 @@ import org.opensearch.action.support.IndicesOptions; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.routing.GroupShardsIterator; -import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.UUIDs; import org.opensearch.common.collect.Tuple; import org.opensearch.common.settings.ClusterSettings; @@ -331,12 +330,8 @@ public void testSendSearchResponseDisallowPartialFailures() { } public void testOnPhaseFailureAndVerifyListeners() { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testListener = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testListener = new SearchRequestStats(clusterSettings); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); SearchQueryThenFetchAsyncAction action = createSearchQueryThenFetchAsyncAction(requestOperationListeners); @@ -599,12 +594,8 @@ public void onFailure(Exception e) { } public void testOnPhaseListenersWithQueryAndThenFetchType() throws InterruptedException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testListener = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testListener = new SearchRequestStats(clusterSettings); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); long delay = (randomIntBetween(1, 5)); @@ -653,12 +644,8 @@ public void testOnPhaseListenersWithQueryAndThenFetchType() throws InterruptedEx } public void testOnPhaseListenersWithDfsType() throws InterruptedException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testListener = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testListener = new SearchRequestStats(clusterSettings); final List requestOperationListeners = new ArrayList<>(List.of(testListener)); SearchDfsQueryThenFetchAsyncAction searchDfsQueryThenFetchAsyncAction = createSearchDfsQueryThenFetchAsyncAction( diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java similarity index 72% rename from server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java rename to server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java index c16841ae1ee17..d935431f935f9 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestListenerManagerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java @@ -14,31 +14,31 @@ import java.util.List; -public class SearchRequestListenerManagerTests extends OpenSearchTestCase { +public class SearchRequestOperationsListenersTests extends OpenSearchTestCase { public void testAddAndGetListeners() { SearchRequestOperationsListener testListener = createTestSearchRequestOperationsListener(); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener); - assertEquals(1, listenerManager.getListeners().size()); - assertEquals(testListener, listenerManager.getListeners().get(0)); + SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener); + assertEquals(1, requestListeners.getListeners().size()); + assertEquals(testListener, requestListeners.getListeners().get(0)); } public void testStandardListenersEnabled() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener1, testListener2); + SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1, testListener2); testListener2.setEnabled(true); - SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener(logger); + SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener(logger); List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); assertEquals(testListener2, listeners.get(0)); - assertEquals(2, listenerManager.getListeners().size()); - assertEquals(testListener1, listenerManager.getListeners().get(0)); - assertEquals(testListener2, listenerManager.getListeners().get(1)); + assertEquals(2, requestListeners.getListeners().size()); + assertEquals(testListener1, requestListeners.getListeners().get(0)); + assertEquals(testListener2, requestListeners.getListeners().get(1)); } public void testStandardListenersAndTimeProvider() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener1); + SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); testListener1.setEnabled(true); TransportSearchAction.SearchTimeProvider timeProviderListener = new TransportSearchAction.SearchTimeProvider( @@ -50,7 +50,7 @@ public void testStandardListenersAndTimeProvider() { SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(true); timeProviderListener.setEnabled(false, searchRequest); - SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( + SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener( logger, timeProviderListener ); @@ -58,13 +58,13 @@ public void testStandardListenersAndTimeProvider() { assertEquals(2, listeners.size()); assertEquals(testListener1, listeners.get(0)); assertEquals(timeProviderListener, listeners.get(1)); - assertEquals(1, listenerManager.getListeners().size()); - assertEquals(testListener1, listenerManager.getListeners().get(0)); + assertEquals(1, requestListeners.getListeners().size()); + assertEquals(testListener1, requestListeners.getListeners().get(0)); } public void testStandardListenersDisabledAndTimeProvider() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener1); + SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); TransportSearchAction.SearchTimeProvider timeProviderListener = new TransportSearchAction.SearchTimeProvider( 0, System.nanoTime(), @@ -74,21 +74,21 @@ public void testStandardListenersDisabledAndTimeProvider() { SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(true); timeProviderListener.setEnabled(false, searchRequest); - SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( + SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener( logger, timeProviderListener ); List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); assertEquals(timeProviderListener, listeners.get(0)); - assertEquals(1, listenerManager.getListeners().size()); - assertEquals(testListener1, listenerManager.getListeners().get(0)); - assertFalse(listenerManager.getListeners().get(0).getEnabled()); + assertEquals(1, requestListeners.getListeners().size()); + assertEquals(testListener1, requestListeners.getListeners().get(0)); + assertFalse(requestListeners.getListeners().get(0).getEnabled()); } public void testStandardListenerAndTimeProviderDisabled() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(testListener1); + SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); testListener1.setEnabled(true); SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( @@ -99,15 +99,15 @@ public void testStandardListenerAndTimeProviderDisabled() { SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(false); - SearchRequestOperationsListener.CompositeListener compositeListener = listenerManager.buildCompositeListener( + SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener( logger, timeProviderListener ); List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); assertEquals(testListener1, listeners.get(0)); - assertEquals(1, listenerManager.getListeners().size()); - assertEquals(testListener1, listenerManager.getListeners().get(0)); + assertEquals(1, requestListeners.getListeners().size()); + assertEquals(testListener1, requestListeners.getListeners().get(0)); } public SearchRequestOperationsListener createTestSearchRequestOperationsListener() { diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java index e70bba36445e0..d2e5c95862fd9 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java @@ -104,7 +104,7 @@ public void testMultipleSlowLoggersUseSingleLog4jLogger() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestListenerManager listenerManager2 = new SearchRequestListenerManager(); + SearchRequestOperationsListeners searchRequestListeners = new SearchRequestOperationsListeners(); SearchRequestSlowLog searchRequestSlowLog2 = new SearchRequestSlowLog(clusterService2); int numberOfLoggersAfter = context.getLoggers().size(); diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java index e2b21689bdb03..25559482d095c 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -8,7 +8,6 @@ package org.opensearch.action.search; -import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; import org.opensearch.test.OpenSearchTestCase; @@ -25,12 +24,8 @@ public class SearchRequestStatsTests extends OpenSearchTestCase { public void testSearchRequestPhaseFailure() { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -45,12 +40,8 @@ public void testSearchRequestPhaseFailure() { } public void testSearchRequestStats() { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); @@ -71,12 +62,8 @@ public void testSearchRequestStats() { } public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); @@ -103,12 +90,8 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE } public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); @@ -144,12 +127,8 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc } public void testSearchRequestStatsOnPhaseFailureConcurrently() throws InterruptedException { - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); diff --git a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java index ea370ebc6cf76..5656b77445772 100644 --- a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java +++ b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java @@ -37,7 +37,6 @@ import org.opensearch.action.search.SearchPhaseName; import org.opensearch.action.search.SearchRequestOperationsListenerSupport; import org.opensearch.action.search.SearchRequestStats; -import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; import org.opensearch.index.search.stats.SearchStats.Stats; @@ -80,12 +79,8 @@ public void testShardLevelSearchGroupStats() throws Exception { long paramValue = randomIntBetween(2, 50); // Testing for request stats - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats testRequestStats = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); SearchPhaseContext ctx = mock(SearchPhaseContext.class); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhase mockSearchPhase = mock(SearchPhase.class); diff --git a/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java b/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java index b07dc6d298045..2424e38636466 100644 --- a/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java +++ b/server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java @@ -34,7 +34,6 @@ import org.opensearch.action.admin.indices.stats.CommonStats; import org.opensearch.action.search.SearchRequestStats; -import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; import org.opensearch.core.xcontent.ToXContent; @@ -49,12 +48,8 @@ public class NodeIndicesStatsTests extends OpenSearchTestCase { public void testInvalidLevel() { CommonStats oldStats = new CommonStats(); - ClusterService clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - null - ); - SearchRequestStats requestStats = new SearchRequestStats(clusterService); + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats requestStats = new SearchRequestStats(clusterSettings); final NodeIndicesStats stats = new NodeIndicesStats(oldStats, Collections.emptyMap(), requestStats); final String level = randomAlphaOfLength(16); final ToXContent.Params params = new ToXContent.MapParams(Collections.singletonMap("level", level)); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index e631d3e67a484..752d88b677a9c 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -90,7 +90,7 @@ import org.opensearch.action.search.SearchExecutionStatsCollector; import org.opensearch.action.search.SearchPhaseController; import org.opensearch.action.search.SearchRequest; -import org.opensearch.action.search.SearchRequestListenerManager; +import org.opensearch.action.search.SearchRequestOperationsListeners; import org.opensearch.action.search.SearchResponse; import org.opensearch.action.search.SearchTransportService; import org.opensearch.action.search.TransportSearchAction; @@ -2285,7 +2285,7 @@ public void onFailure(final Exception e) { writableRegistry(), searchService::aggReduceContextBuilder ); - SearchRequestListenerManager listenerManager = new SearchRequestListenerManager(); + SearchRequestOperationsListeners searchRequestOperationsListeners = new SearchRequestOperationsListeners(); actions.put( SearchAction.INSTANCE, new TransportSearchAction( @@ -2312,7 +2312,7 @@ public void onFailure(final Exception e) { client ), NoopMetricsRegistry.INSTANCE, - listenerManager + searchRequestOperationsListeners ) ); actions.put( From bb64cd2c4503bf50dc4461e9cab96a9d836f788c Mon Sep 17 00:00:00 2001 From: Chenyang Ji Date: Thu, 4 Jan 2024 18:05:05 -0800 Subject: [PATCH 6/7] refactor to remove the TimeProvider listener Signed-off-by: Chenyang Ji --- .../search/AbstractSearchAsyncAction.java | 4 +- .../action/search/SearchRequestContext.java | 19 ++++- .../SearchRequestOperationsListener.java | 11 ++- .../SearchRequestOperationsListeners.java | 6 +- .../action/search/SearchResponseMerger.java | 6 +- .../action/search/TransportSearchAction.java | 83 +++++-------------- ...SearchRequestOperationsListenersTests.java | 57 ++++++------- .../search/SearchResponseMergerTests.java | 25 +++--- .../search/SearchTimeProviderTests.java | 54 ------------ .../search/TransportSearchActionTests.java | 18 ++-- 10 files changed, 106 insertions(+), 177 deletions(-) delete mode 100644 server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java diff --git a/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java b/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java index f18bbb8a1cc13..5b41c2a13b596 100644 --- a/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java +++ b/server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java @@ -214,7 +214,7 @@ public final void start() { 0, 0, buildTookInMillis(), - timeProvider.getPhaseTook(), + searchRequestContext.getPhaseTook(), ShardSearchFailure.EMPTY_ARRAY, clusters, null @@ -670,7 +670,7 @@ protected final SearchResponse buildSearchResponse( successfulOps.get(), skippedOps.get(), buildTookInMillis(), - timeProvider.getPhaseTook(), + searchRequestContext.getPhaseTook(), failures, clusters, searchContextId diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java b/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java index 674363600b251..7f2e71afec5ce 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java @@ -31,18 +31,25 @@ class SearchRequestContext { private TotalHits totalHits; private final EnumMap shardStats; + private final boolean phaseTookEnabled; + /** * This constructor is for testing only */ SearchRequestContext() { - this(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger())); + this(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), false); } - SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener) { + SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener, boolean phaseTookEnabled) { this.searchRequestOperationsListener = searchRequestOperationsListener; this.absoluteStartNanos = System.nanoTime(); this.phaseTookMap = new HashMap<>(); this.shardStats = new EnumMap<>(ShardStatsFieldNames.class); + this.phaseTookEnabled = phaseTookEnabled; + } + + SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener) { + this(searchRequestOperationsListener, false); } SearchRequestOperationsListener getSearchRequestOperationsListener() { @@ -57,6 +64,14 @@ Map phaseTookMap() { return phaseTookMap; } + SearchResponse.PhaseTook getPhaseTook() { + if (phaseTookEnabled) { + return new SearchResponse.PhaseTook(phaseTookMap); + } else { + return null; + } + } + /** * Override absoluteStartNanos set in constructor. * For testing only diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java index ecebabcf2b8ae..99619d9f9c900 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java @@ -24,7 +24,7 @@ public abstract class SearchRequestOperationsListener { private volatile boolean enabled; protected SearchRequestOperationsListener() { - this.enabled = false; + this.enabled = true; } protected SearchRequestOperationsListener(boolean enabled) { @@ -41,11 +41,15 @@ void onRequestStart(SearchRequestContext searchRequestContext) {} void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {} - boolean getEnabled() { + boolean isEnabled(SearchRequest searchRequest) { + return isEnabled(); + } + + boolean isEnabled() { return enabled; } - void setEnabled(boolean enabled) { + protected void setEnabled(boolean enabled) { this.enabled = enabled; } @@ -62,7 +66,6 @@ static final class CompositeListener extends SearchRequestOperationsListener { CompositeListener(List listeners, Logger logger) { this.listeners = listeners; this.logger = logger; - this.setEnabled(true); } @Override diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java index f20c52b9d97d4..237d25169bdd2 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java @@ -59,18 +59,22 @@ public List getListeners() { * Create the {@link SearchRequestOperationsListener.CompositeListener} * with the all listeners enabled at cluster-level and request-level. * + * @param searchRequest The SearchRequest object used to decide which request-level listeners to add based on states/flags * @param logger Logger to be attached to the {@link SearchRequestOperationsListener.CompositeListener} * @param perRequestListeners the per-request listeners that can be optionally added to the returned CompositeListener list. * @return SearchRequestOperationsListener.CompositeListener */ public SearchRequestOperationsListener.CompositeListener buildCompositeListener( + SearchRequest searchRequest, Logger logger, SearchRequestOperationsListener... perRequestListeners ) { final List searchListenersList = Stream.concat( searchRequestListenersList.stream(), Arrays.stream(perRequestListeners) - ).filter(SearchRequestOperationsListener::getEnabled).collect(Collectors.toList()); + ) + .filter((searchRequestOperationsListener -> searchRequestOperationsListener.isEnabled(searchRequest))) + .collect(Collectors.toList()); return new SearchRequestOperationsListener.CompositeListener(searchListenersList, logger); } diff --git a/server/src/main/java/org/opensearch/action/search/SearchResponseMerger.java b/server/src/main/java/org/opensearch/action/search/SearchResponseMerger.java index 054bd578cc56c..538e7fd54e2c3 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchResponseMerger.java +++ b/server/src/main/java/org/opensearch/action/search/SearchResponseMerger.java @@ -110,7 +110,7 @@ final class SearchResponseMerger { /** * Add a search response to the list of responses to be merged together into one. * Merges currently happen at once when all responses are available and - * {@link #getMergedResponse(SearchResponse.Clusters)} )} is called. + * {@link #getMergedResponse(SearchResponse.Clusters, SearchRequestContext)} )} is called. * That may change in the future as it's possible to introduce incremental merges as responses come in if necessary. */ void add(SearchResponse searchResponse) { @@ -126,7 +126,7 @@ int numResponses() { * Returns the merged response. To be called once all responses have been added through {@link #add(SearchResponse)} * so that all responses are merged into a single one. */ - SearchResponse getMergedResponse(SearchResponse.Clusters clusters) { + SearchResponse getMergedResponse(SearchResponse.Clusters clusters, SearchRequestContext searchRequestContext) { // if the search is only across remote clusters, none of them are available, and all of them have skip_unavailable set to true, // we end up calling merge without anything to merge, we just return an empty search response if (searchResponses.size() == 0) { @@ -236,7 +236,7 @@ SearchResponse getMergedResponse(SearchResponse.Clusters clusters) { successfulShards, skippedShards, tookInMillis, - searchTimeProvider.getPhaseTook(), + searchRequestContext.getPhaseTook(), shardFailures, clusters, null 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 ca7853cd78031..0738ce0c5ca68 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -98,7 +98,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.EnumMap; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -269,8 +268,6 @@ private Map resolveIndexBoosts(SearchRequest searchRequest, Clust } /** - * Listener to track request-level tookTime and phase tookTimes from the coordinator. - * * Search operations need two clocks. One clock is to fulfill real clock needs (e.g., resolving * "now" to an index name). Another clock is needed for measuring how long a search operation * took. These two uses are at odds with each other. There are many issues with using a real @@ -280,8 +277,7 @@ private Map resolveIndexBoosts(SearchRequest searchRequest, Clust * * @opensearch.internal */ - static final class SearchTimeProvider extends SearchRequestOperationsListener { - + static final class SearchTimeProvider { private final long absoluteStartMillis; private final long relativeStartNanos; private final LongSupplier relativeCurrentNanosProvider; @@ -310,53 +306,6 @@ long getAbsoluteStartMillis() { long buildTookInMillis() { return TimeUnit.NANOSECONDS.toMillis(relativeCurrentNanosProvider.getAsLong() - relativeStartNanos); } - - SearchResponse.PhaseTook getPhaseTook() { - if (getEnabled()) { - Map phaseTookMap = new HashMap<>(); - // Convert Map to Map for SearchResponse() - for (SearchPhaseName searchPhaseName : phaseStatsMap.keySet()) { - phaseTookMap.put(searchPhaseName.getName(), phaseStatsMap.get(searchPhaseName)); - } - return new SearchResponse.PhaseTook(phaseTookMap); - } else { - return null; - } - } - - Map phaseStatsMap = new EnumMap<>(SearchPhaseName.class); - - /** - * Set if this listener is enabled based on the cluster level setting - * and per request enable flags. - * - * @param enabledAtClusterLevel if the SearchTimeProvider listener is enabled at cluster level - * @param searchRequest the original Search Request - * @opensearch.internal - */ - - void setEnabled(boolean enabledAtClusterLevel, SearchRequest searchRequest) { - // phase_took is enabled wi th request param and/or cluster setting - super.setEnabled(enabledAtClusterLevel || (searchRequest.isPhaseTook() != null && searchRequest.isPhaseTook())); - } - - @Override - void onPhaseStart(SearchPhaseContext context) {} - - @Override - void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - phaseStatsMap.put( - context.getCurrentPhase().getSearchPhaseName(), - TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos()) - ); - } - - @Override - void onPhaseFailure(SearchPhaseContext context) {} - - public Long getPhaseTookTime(SearchPhaseName searchPhaseName) { - return phaseStatsMap.get(searchPhaseName); - } } @Override @@ -479,10 +428,15 @@ private void executeRequest( relativeStartNanos, System::nanoTime ); - timeProvider.setEnabled(clusterService.getClusterSettings().get(SEARCH_PHASE_TOOK_ENABLED), originalSearchRequest); + final boolean phaseTookEnabled; + if (originalSearchRequest.isPhaseTook() == null) { + phaseTookEnabled = clusterService.getClusterSettings().get(SEARCH_PHASE_TOOK_ENABLED); + } else { + phaseTookEnabled = originalSearchRequest.isPhaseTook(); + } SearchRequestOperationsListener.CompositeListener requestOperationsListeners = searchRequestOperationsListeners - .buildCompositeListener(logger, timeProvider); - SearchRequestContext searchRequestContext = new SearchRequestContext(requestOperationsListeners); + .buildCompositeListener(originalSearchRequest, logger); + SearchRequestContext searchRequestContext = new SearchRequestContext(requestOperationsListeners, phaseTookEnabled); searchRequestContext.getSearchRequestOperationsListener().onRequestStart(searchRequestContext); PipelinedRequest searchRequest; @@ -587,7 +541,8 @@ private ActionListener buildRewriteListener( searchContext, searchAsyncActionProvider, searchRequestContext - ) + ), + searchRequestContext ); } else { AtomicInteger skippedClusters = new AtomicInteger(0); @@ -675,7 +630,8 @@ static void ccsRemoteReduce( RemoteClusterService remoteClusterService, ThreadPool threadPool, ActionListener listener, - BiConsumer> localSearchConsumer + BiConsumer> localSearchConsumer, + SearchRequestContext searchRequestContext ) { if (localIndices == null && remoteIndices.size() == 1) { @@ -717,7 +673,7 @@ public void onResponse(SearchResponse searchResponse) { searchResponse.getSuccessfulShards(), searchResponse.getSkippedShards(), timeProvider.buildTookInMillis(), - timeProvider.getPhaseTook(), + searchRequestContext.getPhaseTook(), searchResponse.getShardFailures(), new SearchResponse.Clusters(1, 1, 0), searchResponse.pointInTimeId() @@ -763,7 +719,8 @@ public void onFailure(Exception e) { exceptions, searchResponseMerger, totalClusters, - listener + listener, + searchRequestContext ); Client remoteClusterClient = remoteClusterService.getRemoteClusterClient(threadPool, clusterAlias); remoteClusterClient.search(ccsSearchRequest, ccsListener); @@ -777,7 +734,8 @@ public void onFailure(Exception e) { exceptions, searchResponseMerger, totalClusters, - listener + listener, + searchRequestContext ); SearchRequest ccsLocalSearchRequest = SearchRequest.subSearchRequest( searchRequest, @@ -872,7 +830,8 @@ private static ActionListener createCCSListener( AtomicReference exceptions, SearchResponseMerger searchResponseMerger, int totalClusters, - ActionListener originalListener + ActionListener originalListener, + SearchRequestContext searchRequestContext ) { return new CCSActionListener( clusterAlias, @@ -894,7 +853,7 @@ SearchResponse createFinalResponse() { searchResponseMerger.numResponses(), skippedClusters.get() ); - return searchResponseMerger.getMergedResponse(clusters); + return searchResponseMerger.getMergedResponse(clusters, searchRequestContext); } }; } diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java index d935431f935f9..e762e795d88d2 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java @@ -25,9 +25,14 @@ public void testAddAndGetListeners() { public void testStandardListenersEnabled() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); + testListener1.setEnabled(false); SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1, testListener2); - testListener2.setEnabled(true); - SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener(logger); + SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); + SearchRequest searchRequest = new SearchRequest().source(source); + SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener( + searchRequest, + logger + ); List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); assertEquals(testListener2, listeners.get(0)); @@ -36,72 +41,62 @@ public void testStandardListenersEnabled() { assertEquals(testListener2, requestListeners.getListeners().get(1)); } - public void testStandardListenersAndTimeProvider() { + public void testStandardListenersAndPerRequestListener() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); - + SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); testListener1.setEnabled(true); - TransportSearchAction.SearchTimeProvider timeProviderListener = new TransportSearchAction.SearchTimeProvider( - 0, - System.nanoTime(), - System::nanoTime - ); + testListener2.setEnabled(true); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(true); - timeProviderListener.setEnabled(false, searchRequest); SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener( + searchRequest, logger, - timeProviderListener + testListener2 ); List listeners = compositeListener.getListeners(); assertEquals(2, listeners.size()); assertEquals(testListener1, listeners.get(0)); - assertEquals(timeProviderListener, listeners.get(1)); + assertEquals(testListener2, listeners.get(1)); assertEquals(1, requestListeners.getListeners().size()); assertEquals(testListener1, requestListeners.getListeners().get(0)); } - public void testStandardListenersDisabledAndTimeProvider() { + public void testStandardListenersDisabledAndPerRequestListener() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); + testListener1.setEnabled(false); SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); - TransportSearchAction.SearchTimeProvider timeProviderListener = new TransportSearchAction.SearchTimeProvider( - 0, - System.nanoTime(), - System::nanoTime - ); + SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); - searchRequest.setPhaseTook(true); - timeProviderListener.setEnabled(false, searchRequest); SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener( + searchRequest, logger, - timeProviderListener + testListener2 ); List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); - assertEquals(timeProviderListener, listeners.get(0)); + assertEquals(testListener2, listeners.get(0)); assertEquals(1, requestListeners.getListeners().size()); assertEquals(testListener1, requestListeners.getListeners().get(0)); - assertFalse(requestListeners.getListeners().get(0).getEnabled()); + assertFalse(requestListeners.getListeners().get(0).isEnabled()); } - public void testStandardListenerAndTimeProviderDisabled() { + public void testStandardListenerAndPerRequestListenerDisabled() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); - testListener1.setEnabled(true); - SearchRequestOperationsListener timeProviderListener = new TransportSearchAction.SearchTimeProvider( - 0, - System.nanoTime(), - System::nanoTime - ); + SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); + testListener2.setEnabled(false); + SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); searchRequest.setPhaseTook(false); SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener( + searchRequest, logger, - timeProviderListener + testListener2 ); List listeners = compositeListener.getListeners(); assertEquals(1, listeners.size()); diff --git a/server/src/test/java/org/opensearch/action/search/SearchResponseMergerTests.java b/server/src/test/java/org/opensearch/action/search/SearchResponseMergerTests.java index 1004965c0d50e..71e4236de8510 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchResponseMergerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchResponseMergerTests.java @@ -132,7 +132,7 @@ public void testMergeTookInMillis() throws InterruptedException { addResponse(merger, searchResponse); } awaitResponsesAdded(); - SearchResponse searchResponse = merger.getMergedResponse(SearchResponse.Clusters.EMPTY); + SearchResponse searchResponse = merger.getMergedResponse(SearchResponse.Clusters.EMPTY, new SearchRequestContext()); assertEquals(TimeUnit.NANOSECONDS.toMillis(currentRelativeTime), searchResponse.getTook().millis()); } @@ -184,7 +184,7 @@ public void testMergeShardFailures() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, merger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = merger.getMergedResponse(clusters); + SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -235,7 +235,7 @@ public void testMergeShardFailuresNullShardTarget() throws InterruptedException awaitResponsesAdded(); assertEquals(numResponses, merger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = merger.getMergedResponse(clusters); + SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -281,7 +281,8 @@ public void testMergeShardFailuresNullShardId() throws InterruptedException { } awaitResponsesAdded(); assertEquals(numResponses, merger.numResponses()); - ShardSearchFailure[] shardFailures = merger.getMergedResponse(SearchResponse.Clusters.EMPTY).getShardFailures(); + ShardSearchFailure[] shardFailures = merger.getMergedResponse(SearchResponse.Clusters.EMPTY, new SearchRequestContext()) + .getShardFailures(); assertThat(Arrays.asList(shardFailures), containsInAnyOrder(expectedFailures.toArray(ShardSearchFailure.EMPTY_ARRAY))); } @@ -315,7 +316,7 @@ public void testMergeProfileResults() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, merger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = merger.getMergedResponse(clusters); + SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -377,7 +378,7 @@ public void testMergeCompletionSuggestions() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, searchResponseMerger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters); + SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters, new SearchRequestContext()); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -449,7 +450,7 @@ public void testMergeCompletionSuggestionsTieBreak() throws InterruptedException awaitResponsesAdded(); assertEquals(numResponses, searchResponseMerger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters); + SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters, new SearchRequestContext()); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -523,7 +524,7 @@ public void testMergeAggs() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, searchResponseMerger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters); + SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters, new SearchRequestContext()); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -680,7 +681,7 @@ public void testMergeSearchHits() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, searchResponseMerger.numResponses()); final SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse searchResponse = searchResponseMerger.getMergedResponse(clusters); + SearchResponse searchResponse = searchResponseMerger.getMergedResponse(clusters, new SearchRequestContext()); assertEquals(TimeUnit.NANOSECONDS.toMillis(currentRelativeTime), searchResponse.getTook().millis()); assertEquals(expectedTotal, searchResponse.getTotalShards()); @@ -740,7 +741,7 @@ public void testMergeNoResponsesAdded() { SearchResponseMerger merger = new SearchResponseMerger(0, 10, Integer.MAX_VALUE, timeProvider, emptyReduceContextBuilder()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); assertEquals(0, merger.numResponses()); - SearchResponse response = merger.getMergedResponse(clusters); + SearchResponse response = merger.getMergedResponse(clusters, new SearchRequestContext()); assertSame(clusters, response.getClusters()); assertEquals(TimeUnit.NANOSECONDS.toMillis(currentRelativeTime), response.getTook().millis()); assertEquals(0, response.getTotalShards()); @@ -813,7 +814,7 @@ public void testMergeEmptySearchHitsWithNonEmpty() { merger.add(searchResponse); } assertEquals(2, merger.numResponses()); - SearchResponse mergedResponse = merger.getMergedResponse(clusters); + SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); assertEquals(10, mergedResponse.getHits().getTotalHits().value); assertEquals(10, mergedResponse.getHits().getHits().length); assertEquals(2, mergedResponse.getTotalShards()); @@ -855,7 +856,7 @@ public void testMergeOnlyEmptyHits() { ); merger.add(searchResponse); } - SearchResponse mergedResponse = merger.getMergedResponse(clusters); + SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); assertEquals(expectedTotalHits, mergedResponse.getHits().getTotalHits()); } diff --git a/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java b/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java deleted file mode 100644 index 4d8a44417a3ee..0000000000000 --- a/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * 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.test.OpenSearchTestCase; - -import java.util.concurrent.TimeUnit; - -import static org.hamcrest.Matchers.greaterThanOrEqualTo; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -public class SearchTimeProviderTests extends OpenSearchTestCase { - - public void testSearchTimeProviderPhaseFailure() { - TransportSearchAction.SearchTimeProvider testTimeProvider = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - SearchPhaseContext ctx = mock(SearchPhaseContext.class); - SearchPhase mockSearchPhase = mock(SearchPhase.class); - when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); - testTimeProvider.onPhaseStart(ctx); - assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName)); - testTimeProvider.onPhaseFailure(ctx); - assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName)); - } - } - - public void testSearchTimeProviderPhaseEnd() { - TransportSearchAction.SearchTimeProvider testTimeProvider = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - - SearchPhaseContext ctx = mock(SearchPhaseContext.class); - SearchPhase mockSearchPhase = mock(SearchPhase.class); - when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); - long tookTimeInMillis = randomIntBetween(1, 100); - testTimeProvider.onPhaseStart(ctx); - long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis); - when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); - assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName)); - testTimeProvider.onPhaseEnd(ctx, new SearchRequestContext()); - assertThat(testTimeProvider.getPhaseTookTime(searchPhaseName), greaterThanOrEqualTo(tookTimeInMillis)); - } - } -} diff --git a/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java index c4bf8a5d87172..bed9bd37c49b4 100644 --- a/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java @@ -483,7 +483,8 @@ public void testCCSRemoteReduceMergeFails() throws Exception { remoteClusterService, threadPool, listener, - (r, l) -> setOnce.set(Tuple.tuple(r, l)) + (r, l) -> setOnce.set(Tuple.tuple(r, l)), + new SearchRequestContext() ); if (localIndices == null) { assertNull(setOnce.get()); @@ -541,7 +542,8 @@ public void testCCSRemoteReduce() throws Exception { remoteClusterService, threadPool, listener, - (r, l) -> setOnce.set(Tuple.tuple(r, l)) + (r, l) -> setOnce.set(Tuple.tuple(r, l)), + new SearchRequestContext() ); if (localIndices == null) { assertNull(setOnce.get()); @@ -578,7 +580,8 @@ public void testCCSRemoteReduce() throws Exception { remoteClusterService, threadPool, listener, - (r, l) -> setOnce.set(Tuple.tuple(r, l)) + (r, l) -> setOnce.set(Tuple.tuple(r, l)), + new SearchRequestContext() ); if (localIndices == null) { assertNull(setOnce.get()); @@ -636,7 +639,8 @@ public void onNodeDisconnected(DiscoveryNode node, Transport.Connection connecti remoteClusterService, threadPool, listener, - (r, l) -> setOnce.set(Tuple.tuple(r, l)) + (r, l) -> setOnce.set(Tuple.tuple(r, l)), + new SearchRequestContext() ); if (localIndices == null) { assertNull(setOnce.get()); @@ -676,7 +680,8 @@ public void onNodeDisconnected(DiscoveryNode node, Transport.Connection connecti remoteClusterService, threadPool, listener, - (r, l) -> setOnce.set(Tuple.tuple(r, l)) + (r, l) -> setOnce.set(Tuple.tuple(r, l)), + new SearchRequestContext() ); if (localIndices == null) { assertNull(setOnce.get()); @@ -727,7 +732,8 @@ public void onNodeDisconnected(DiscoveryNode node, Transport.Connection connecti remoteClusterService, threadPool, listener, - (r, l) -> setOnce.set(Tuple.tuple(r, l)) + (r, l) -> setOnce.set(Tuple.tuple(r, l)), + new SearchRequestContext() ); if (localIndices == null) { assertNull(setOnce.get()); From 96c8388ddabe9d8b08b1844a1a13eeae4f99aa7c Mon Sep 17 00:00:00 2001 From: Chenyang Ji Date: Tue, 9 Jan 2024 18:17:17 -0800 Subject: [PATCH 7/7] include SearchRequest in SearchContext and rename to SearchRequestOperationsCompositeListenerFactory Signed-off-by: Chenyang Ji --- CHANGELOG.md | 2 +- .../action/search/SearchRequestContext.java | 21 +--- ...stOperationsCompositeListenerFactory.java} | 15 ++- .../SearchRequestOperationsListener.java | 4 +- .../action/search/TransportSearchAction.java | 15 ++- .../main/java/org/opensearch/node/Node.java | 23 ++--- .../AbstractSearchAsyncActionTests.java | 13 ++- .../CanMatchPreFilterSearchPhaseTests.java | 31 ++++-- .../action/search/SearchAsyncActionTests.java | 12 ++- .../SearchQueryThenFetchAsyncActionTests.java | 7 +- ...rationsCompositeListenerFactoryTests.java} | 23 +++-- ...earchRequestOperationsListenerSupport.java | 12 ++- .../search/SearchRequestSlowLogTests.java | 25 +++-- .../search/SearchRequestStatsTests.java | 18 +++- .../search/SearchResponseMergerTests.java | 98 ++++++++++++++++--- .../search/TransportSearchActionTests.java | 31 ++++-- .../snapshots/SnapshotResiliencyTests.java | 7 +- 17 files changed, 257 insertions(+), 100 deletions(-) rename server/src/main/java/org/opensearch/action/search/{SearchRequestOperationsListeners.java => SearchRequestOperationsCompositeListenerFactory.java} (83%) rename server/src/test/java/org/opensearch/action/search/{SearchRequestOperationsListenersTests.java => SearchRequestOperationsCompositeListenerFactoryTests.java} (85%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64f71157dbfde..1f64dcf82bd4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -193,7 +193,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add deleted doc count in _cat/shards ([#11678](https://github.com/opensearch-project/OpenSearch/pull/11678)) - Capture information for additional query types and aggregation types ([#11582](https://github.com/opensearch-project/OpenSearch/pull/11582)) - Use slice_size == shard_size heuristic in terms aggs for concurrent segment search and properly calculate the doc_count_error ([#11732](https://github.com/opensearch-project/OpenSearch/pull/11732)) -- Added Support for dynamically adding SearchRequestOperationsListeners ([#11526](https://github.com/opensearch-project/OpenSearch/pull/11526)) +- Added Support for dynamically adding SearchRequestOperationsListeners with SearchRequestOperationsCompositeListenerFactory ([#11526](https://github.com/opensearch-project/OpenSearch/pull/11526)) ### Deprecated diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java b/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java index 7f2e71afec5ce..eceac7204b196 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java @@ -8,13 +8,11 @@ package org.opensearch.action.search; -import org.apache.logging.log4j.LogManager; import org.apache.lucene.search.TotalHits; import org.opensearch.common.annotation.InternalApi; import java.util.EnumMap; import java.util.HashMap; -import java.util.List; import java.util.Locale; import java.util.Map; @@ -31,25 +29,14 @@ class SearchRequestContext { private TotalHits totalHits; private final EnumMap shardStats; - private final boolean phaseTookEnabled; + private final SearchRequest searchRequest; - /** - * This constructor is for testing only - */ - SearchRequestContext() { - this(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), false); - } - - SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener, boolean phaseTookEnabled) { + SearchRequestContext(final SearchRequestOperationsListener searchRequestOperationsListener, final SearchRequest searchRequest) { this.searchRequestOperationsListener = searchRequestOperationsListener; this.absoluteStartNanos = System.nanoTime(); this.phaseTookMap = new HashMap<>(); this.shardStats = new EnumMap<>(ShardStatsFieldNames.class); - this.phaseTookEnabled = phaseTookEnabled; - } - - SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener) { - this(searchRequestOperationsListener, false); + this.searchRequest = searchRequest; } SearchRequestOperationsListener getSearchRequestOperationsListener() { @@ -65,7 +52,7 @@ Map phaseTookMap() { } SearchResponse.PhaseTook getPhaseTook() { - if (phaseTookEnabled) { + if (searchRequest != null && searchRequest.isPhaseTook() != null && searchRequest.isPhaseTook()) { return new SearchResponse.PhaseTook(phaseTookMap); } else { return null; diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsCompositeListenerFactory.java similarity index 83% rename from server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java rename to server/src/main/java/org/opensearch/action/search/SearchRequestOperationsCompositeListenerFactory.java index 237d25169bdd2..db487bf945889 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListeners.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsCompositeListenerFactory.java @@ -17,25 +17,25 @@ import java.util.stream.Stream; /** - * SearchRequestOperationsListeners contains listeners registered to search requests, + * SearchRequestOperationsCompositeListenerFactory contains listeners registered to search requests, * and is responsible for creating the {@link SearchRequestOperationsListener.CompositeListener} * with the all listeners enabled at cluster-level and request-level. * * * @opensearch.internal */ -public class SearchRequestOperationsListeners { +public final class SearchRequestOperationsCompositeListenerFactory { private final List searchRequestListenersList; /** - * Create the SearchRequestOperationsListeners and add multiple {@link SearchRequestOperationsListener} + * Create the SearchRequestOperationsCompositeListenerFactory and add multiple {@link SearchRequestOperationsListener} * to the searchRequestListenersList. * Those enabled listeners will be executed during each search request. * * @param listeners Multiple SearchRequestOperationsListener object to add. * @throws IllegalArgumentException if any input listener is null. */ - public SearchRequestOperationsListeners(SearchRequestOperationsListener... listeners) { + public SearchRequestOperationsCompositeListenerFactory(final SearchRequestOperationsListener... listeners) { searchRequestListenersList = new ArrayList<>(); for (SearchRequestOperationsListener listener : listeners) { if (listener == null) { @@ -49,7 +49,6 @@ public SearchRequestOperationsListeners(SearchRequestOperationsListener... liste * Get searchRequestListenersList, * * @return List of SearchRequestOperationsListener - * @throws IllegalArgumentException if the input listener is null or already exists in the list. */ public List getListeners() { return searchRequestListenersList; @@ -65,9 +64,9 @@ public List getListeners() { * @return SearchRequestOperationsListener.CompositeListener */ public SearchRequestOperationsListener.CompositeListener buildCompositeListener( - SearchRequest searchRequest, - Logger logger, - SearchRequestOperationsListener... perRequestListeners + final SearchRequest searchRequest, + final Logger logger, + final SearchRequestOperationsListener... perRequestListeners ) { final List searchListenersList = Stream.concat( searchRequestListenersList.stream(), diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java index 99619d9f9c900..2a09cc084f79f 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java @@ -27,7 +27,7 @@ protected SearchRequestOperationsListener() { this.enabled = true; } - protected SearchRequestOperationsListener(boolean enabled) { + protected SearchRequestOperationsListener(final boolean enabled) { this.enabled = enabled; } @@ -49,7 +49,7 @@ boolean isEnabled() { return enabled; } - protected void setEnabled(boolean enabled) { + protected void setEnabled(final boolean enabled) { this.enabled = enabled; } 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 0738ce0c5ca68..842c10b700d24 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -172,7 +172,7 @@ public class TransportSearchAction extends HandledTransportAction) SearchRequest::new); this.client = client; @@ -212,7 +212,7 @@ public TransportSearchAction( this.searchPipelineService = searchPipelineService; this.metricsRegistry = metricsRegistry; this.searchQueryMetricsEnabled = clusterService.getClusterSettings().get(SEARCH_QUERY_METRICS_ENABLED_SETTING); - this.searchRequestOperationsListeners = searchRequestOperationsListeners; + this.searchRequestOperationsCompositeListenerFactory = searchRequestOperationsCompositeListenerFactory; clusterService.getClusterSettings() .addSettingsUpdateConsumer(SEARCH_QUERY_METRICS_ENABLED_SETTING, this::setSearchQueryMetricsEnabled); } @@ -428,15 +428,12 @@ private void executeRequest( relativeStartNanos, System::nanoTime ); - final boolean phaseTookEnabled; if (originalSearchRequest.isPhaseTook() == null) { - phaseTookEnabled = clusterService.getClusterSettings().get(SEARCH_PHASE_TOOK_ENABLED); - } else { - phaseTookEnabled = originalSearchRequest.isPhaseTook(); + originalSearchRequest.setPhaseTook(clusterService.getClusterSettings().get(SEARCH_PHASE_TOOK_ENABLED)); } - SearchRequestOperationsListener.CompositeListener requestOperationsListeners = searchRequestOperationsListeners + SearchRequestOperationsListener.CompositeListener requestOperationsListeners = searchRequestOperationsCompositeListenerFactory .buildCompositeListener(originalSearchRequest, logger); - SearchRequestContext searchRequestContext = new SearchRequestContext(requestOperationsListeners, phaseTookEnabled); + SearchRequestContext searchRequestContext = new SearchRequestContext(requestOperationsListeners, originalSearchRequest); searchRequestContext.getSearchRequestOperationsListener().onRequestStart(searchRequestContext); PipelinedRequest searchRequest; diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 0a08885006809..8510122c39fcb 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -46,8 +46,8 @@ import org.opensearch.action.admin.cluster.snapshots.status.TransportNodesSnapshotsStatus; import org.opensearch.action.search.SearchExecutionStatsCollector; import org.opensearch.action.search.SearchPhaseController; +import org.opensearch.action.search.SearchRequestOperationsCompositeListenerFactory; import org.opensearch.action.search.SearchRequestOperationsListener; -import org.opensearch.action.search.SearchRequestOperationsListeners; import org.opensearch.action.search.SearchRequestSlowLog; import org.opensearch.action.search.SearchRequestStats; import org.opensearch.action.search.SearchTransportService; @@ -881,15 +881,16 @@ protected Node( ) .collect(Collectors.toList()); - // register all standard SearchRequestOperationsListeners to the SearchRequestOperationsListeners - final SearchRequestOperationsListeners searchRequestOperationsListeners = new SearchRequestOperationsListeners( - Stream.concat( - Stream.of(searchRequestStats, searchRequestSlowLog), - pluginComponents.stream() - .filter(p -> p instanceof SearchRequestOperationsListener) - .map(p -> (SearchRequestOperationsListener) p) - ).toArray(SearchRequestOperationsListener[]::new) - ); + // register all standard SearchRequestOperationsCompositeListenerFactory to the SearchRequestOperationsCompositeListenerFactory + final SearchRequestOperationsCompositeListenerFactory searchRequestOperationsCompositeListenerFactory = + new SearchRequestOperationsCompositeListenerFactory( + Stream.concat( + Stream.of(searchRequestStats, searchRequestSlowLog), + pluginComponents.stream() + .filter(p -> p instanceof SearchRequestOperationsListener) + .map(p -> (SearchRequestOperationsListener) p) + ).toArray(SearchRequestOperationsListener[]::new) + ); ActionModule actionModule = new ActionModule( settings, @@ -1287,7 +1288,7 @@ protected Node( b.bind(RemoteClusterStateService.class).toProvider(() -> remoteClusterStateService); b.bind(PersistedStateRegistry.class).toInstance(persistedStateRegistry); b.bind(SegmentReplicationStatsTracker.class).toInstance(segmentReplicationStatsTracker); - b.bind(SearchRequestOperationsListeners.class).toInstance(searchRequestOperationsListeners); + b.bind(SearchRequestOperationsCompositeListenerFactory.class).toInstance(searchRequestOperationsCompositeListenerFactory); }); injector = modules.createInjector(); diff --git a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java index e7d3f4108a4b9..76129341fc9a2 100644 --- a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java @@ -32,6 +32,7 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; import org.opensearch.action.OriginalIndices; import org.opensearch.action.support.IndicesOptions; import org.opensearch.cluster.ClusterState; @@ -177,7 +178,7 @@ private AbstractSearchAsyncAction createAction( results, request.getMaxConcurrentShardRequests(), SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), request) ) { @Override protected SearchPhase getNextPhase(final SearchPhaseResults results, SearchPhaseContext context) { @@ -715,7 +716,10 @@ private SearchDfsQueryThenFetchAsyncAction createSearchDfsQueryThenFetchAsyncAct null, task, SearchResponse.Clusters.EMPTY, - new SearchRequestContext(new SearchRequestOperationsListener.CompositeListener(searchRequestOperationsListeners, logger)) + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(searchRequestOperationsListeners, logger), + searchRequest + ) ); } @@ -765,7 +769,10 @@ private SearchQueryThenFetchAsyncAction createSearchQueryThenFetchAsyncAction( null, task, SearchResponse.Clusters.EMPTY, - new SearchRequestContext(new SearchRequestOperationsListener.CompositeListener(searchRequestOperationsListeners, logger)) + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(searchRequestOperationsListeners, logger), + searchRequest + ) ) { @Override ShardSearchFailure[] buildShardFailures() { diff --git a/server/src/test/java/org/opensearch/action/search/CanMatchPreFilterSearchPhaseTests.java b/server/src/test/java/org/opensearch/action/search/CanMatchPreFilterSearchPhaseTests.java index 4ed4797efe604..56dcf66d5607d 100644 --- a/server/src/test/java/org/opensearch/action/search/CanMatchPreFilterSearchPhaseTests.java +++ b/server/src/test/java/org/opensearch/action/search/CanMatchPreFilterSearchPhaseTests.java @@ -31,6 +31,7 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; import org.apache.lucene.util.BytesRef; import org.opensearch.Version; import org.opensearch.action.OriginalIndices; @@ -137,7 +138,10 @@ public void run() throws IOException { } }, SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); canMatchPhase.start(); @@ -229,7 +233,10 @@ public void run() throws IOException { } }, SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); canMatchPhase.start(); @@ -320,7 +327,10 @@ public void sendCanMatch( new ArraySearchPhaseResults<>(iter.size()), randomIntBetween(1, 32), SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ) { @Override @@ -348,7 +358,10 @@ protected void executePhaseOnShard( } }, SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); canMatchPhase.start(); @@ -433,7 +446,10 @@ public void run() { } }, SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); canMatchPhase.start(); @@ -533,7 +549,10 @@ public void run() { } }, SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); canMatchPhase.start(); diff --git a/server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java b/server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java index 7b4fa1d8387df..af7adc4e58fb8 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java @@ -31,6 +31,7 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; import org.opensearch.Version; import org.opensearch.action.OriginalIndices; import org.opensearch.cluster.ClusterState; @@ -61,6 +62,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -136,7 +138,7 @@ public void testSkipSearchShards() throws InterruptedException { new ArraySearchPhaseResults<>(shardsIter.size()), request.getMaxConcurrentShardRequests(), SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), request) ) { @Override @@ -255,7 +257,7 @@ public void testLimitConcurrentShardRequests() throws InterruptedException { new ArraySearchPhaseResults<>(shardsIter.size()), request.getMaxConcurrentShardRequests(), SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), request) ) { @Override @@ -373,7 +375,7 @@ public void sendFreeContext(Transport.Connection connection, ShardSearchContextI new ArraySearchPhaseResults<>(shardsIter.size()), request.getMaxConcurrentShardRequests(), SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), request) ) { TestSearchResponse response = new TestSearchResponse(); @@ -496,7 +498,7 @@ public void sendFreeContext(Transport.Connection connection, ShardSearchContextI new ArraySearchPhaseResults<>(shardsIter.size()), request.getMaxConcurrentShardRequests(), SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), request) ) { TestSearchResponse response = new TestSearchResponse(); @@ -610,7 +612,7 @@ public void testAllowPartialResults() throws InterruptedException { new ArraySearchPhaseResults<>(shardsIter.size()), request.getMaxConcurrentShardRequests(), SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), request) ) { @Override protected void executePhaseOnShard( diff --git a/server/src/test/java/org/opensearch/action/search/SearchQueryThenFetchAsyncActionTests.java b/server/src/test/java/org/opensearch/action/search/SearchQueryThenFetchAsyncActionTests.java index a8c0c43ac5080..faf6f86c69c27 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchQueryThenFetchAsyncActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchQueryThenFetchAsyncActionTests.java @@ -32,6 +32,7 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; import org.apache.lucene.search.FieldDoc; import org.apache.lucene.search.SortField; import org.apache.lucene.search.TopFieldDocs; @@ -63,6 +64,7 @@ import org.opensearch.transport.Transport; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; @@ -215,7 +217,10 @@ public void sendExecuteQuery( null, task, SearchResponse.Clusters.EMPTY, - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ) { @Override protected SearchPhase getNextPhase(SearchPhaseResults results, SearchPhaseContext context) { diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsCompositeListenerFactoryTests.java similarity index 85% rename from server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java rename to server/src/test/java/org/opensearch/action/search/SearchRequestOperationsCompositeListenerFactoryTests.java index e762e795d88d2..78c5ba4412c68 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenersTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsCompositeListenerFactoryTests.java @@ -14,10 +14,12 @@ import java.util.List; -public class SearchRequestOperationsListenersTests extends OpenSearchTestCase { +public class SearchRequestOperationsCompositeListenerFactoryTests extends OpenSearchTestCase { public void testAddAndGetListeners() { SearchRequestOperationsListener testListener = createTestSearchRequestOperationsListener(); - SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener); + SearchRequestOperationsCompositeListenerFactory requestListeners = new SearchRequestOperationsCompositeListenerFactory( + testListener + ); assertEquals(1, requestListeners.getListeners().size()); assertEquals(testListener, requestListeners.getListeners().get(0)); } @@ -26,7 +28,10 @@ public void testStandardListenersEnabled() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); testListener1.setEnabled(false); - SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1, testListener2); + SearchRequestOperationsCompositeListenerFactory requestListeners = new SearchRequestOperationsCompositeListenerFactory( + testListener1, + testListener2 + ); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); SearchRequestOperationsListener.CompositeListener compositeListener = requestListeners.buildCompositeListener( @@ -43,7 +48,9 @@ public void testStandardListenersEnabled() { public void testStandardListenersAndPerRequestListener() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); - SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); + SearchRequestOperationsCompositeListenerFactory requestListeners = new SearchRequestOperationsCompositeListenerFactory( + testListener1 + ); SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); testListener1.setEnabled(true); testListener2.setEnabled(true); @@ -66,7 +73,9 @@ public void testStandardListenersAndPerRequestListener() { public void testStandardListenersDisabledAndPerRequestListener() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); testListener1.setEnabled(false); - SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); + SearchRequestOperationsCompositeListenerFactory requestListeners = new SearchRequestOperationsCompositeListenerFactory( + testListener1 + ); SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); @@ -85,7 +94,9 @@ public void testStandardListenersDisabledAndPerRequestListener() { public void testStandardListenerAndPerRequestListenerDisabled() { SearchRequestOperationsListener testListener1 = createTestSearchRequestOperationsListener(); - SearchRequestOperationsListeners requestListeners = new SearchRequestOperationsListeners(testListener1); + SearchRequestOperationsCompositeListenerFactory requestListeners = new SearchRequestOperationsCompositeListenerFactory( + testListener1 + ); testListener1.setEnabled(true); SearchRequestOperationsListener testListener2 = createTestSearchRequestOperationsListener(); testListener2.setEnabled(false); diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerSupport.java b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerSupport.java index 58a4c4a4e555d..0f737e00478cb 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerSupport.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerSupport.java @@ -8,6 +8,10 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; + +import java.util.List; + /** * Helper interface to access package protected {@link SearchRequestOperationsListener} from test cases. */ @@ -17,6 +21,12 @@ default void onPhaseStart(SearchRequestOperationsListener listener, SearchPhaseC } default void onPhaseEnd(SearchRequestOperationsListener listener, SearchPhaseContext context) { - listener.onPhaseEnd(context, new SearchRequestContext()); + listener.onPhaseEnd( + context, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); } } diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java index d2e5c95862fd9..f009988ffae17 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestSlowLogTests.java @@ -104,7 +104,7 @@ public void testMultipleSlowLoggersUseSingleLog4jLogger() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), null ); - SearchRequestOperationsListeners searchRequestListeners = new SearchRequestOperationsListeners(); + SearchRequestOperationsCompositeListenerFactory searchRequestListeners = new SearchRequestOperationsCompositeListenerFactory(); SearchRequestSlowLog searchRequestSlowLog2 = new SearchRequestSlowLog(clusterService2); int numberOfLoggersAfter = context.getLoggers().size(); @@ -176,7 +176,8 @@ public void testConcurrentOnRequestEnd() throws InterruptedException { ArrayList searchRequestContexts = new ArrayList<>(); for (int i = 0; i < numRequests; i++) { SearchRequestContext searchRequestContext = new SearchRequestContext( - new SearchRequestOperationsListener.CompositeListener(searchListenersList, logger) + new SearchRequestOperationsListener.CompositeListener(searchListenersList, logger), + searchRequest ); searchRequestContext.setAbsoluteStartNanos((i < numRequestsLogged) ? 0 : System.nanoTime()); searchRequestContexts.add(searchRequestContext); @@ -205,7 +206,10 @@ public void testSearchRequestSlowLogHasJsonFields_EmptySearchRequestContext() th SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); SearchPhaseContext searchPhaseContext = new MockSearchPhaseContext(1, searchRequest); - SearchRequestContext searchRequestContext = new SearchRequestContext(); + SearchRequestContext searchRequestContext = new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ); SearchRequestSlowLog.SearchRequestSlowLogMessage p = new SearchRequestSlowLog.SearchRequestSlowLogMessage( searchPhaseContext, 10, @@ -226,7 +230,10 @@ public void testSearchRequestSlowLogHasJsonFields_NotEmptySearchRequestContext() SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); SearchPhaseContext searchPhaseContext = new MockSearchPhaseContext(1, searchRequest); - SearchRequestContext searchRequestContext = new SearchRequestContext(); + SearchRequestContext searchRequestContext = new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ); searchRequestContext.updatePhaseTookMap(SearchPhaseName.FETCH.getName(), 10L); searchRequestContext.updatePhaseTookMap(SearchPhaseName.QUERY.getName(), 50L); searchRequestContext.updatePhaseTookMap(SearchPhaseName.EXPAND.getName(), 5L); @@ -252,7 +259,10 @@ public void testSearchRequestSlowLogHasJsonFields_PartialContext() throws IOExce SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); SearchPhaseContext searchPhaseContext = new MockSearchPhaseContext(1, searchRequest); - SearchRequestContext searchRequestContext = new SearchRequestContext(); + SearchRequestContext searchRequestContext = new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ); searchRequestContext.updatePhaseTookMap(SearchPhaseName.FETCH.getName(), 10L); searchRequestContext.updatePhaseTookMap(SearchPhaseName.QUERY.getName(), 50L); searchRequestContext.updatePhaseTookMap(SearchPhaseName.EXPAND.getName(), 5L); @@ -278,7 +288,10 @@ public void testSearchRequestSlowLogSearchContextPrinterToLog() throws IOExcepti SearchSourceBuilder source = SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()); SearchRequest searchRequest = new SearchRequest().source(source); SearchPhaseContext searchPhaseContext = new MockSearchPhaseContext(1, searchRequest); - SearchRequestContext searchRequestContext = new SearchRequestContext(); + SearchRequestContext searchRequestContext = new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ); searchRequestContext.updatePhaseTookMap(SearchPhaseName.FETCH.getName(), 10L); searchRequestContext.updatePhaseTookMap(SearchPhaseName.QUERY.getName(), 50L); searchRequestContext.updatePhaseTookMap(SearchPhaseName.EXPAND.getName(), 5L); diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java index 25559482d095c..377ccebbfd418 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -8,11 +8,13 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; import org.opensearch.test.OpenSearchTestCase; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Phaser; @@ -54,7 +56,13 @@ public void testSearchRequestStats() { long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis); when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName)); - testRequestStats.onPhaseEnd(ctx, new SearchRequestContext()); + testRequestStats.onPhaseEnd( + ctx, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName)); assertEquals(1, testRequestStats.getPhaseTotal(searchPhaseName)); assertThat(testRequestStats.getPhaseMetric(searchPhaseName), greaterThanOrEqualTo(tookTimeInMillis)); @@ -108,7 +116,13 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc for (int i = 0; i < numTasks; i++) { threads[i] = new Thread(() -> { phaser.arriveAndAwaitAdvance(); - testRequestStats.onPhaseEnd(ctx, new SearchRequestContext()); + testRequestStats.onPhaseEnd( + ctx, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); countDownLatch.countDown(); }); threads[i].start(); diff --git a/server/src/test/java/org/opensearch/action/search/SearchResponseMergerTests.java b/server/src/test/java/org/opensearch/action/search/SearchResponseMergerTests.java index 71e4236de8510..ce4d5ca4f7091 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchResponseMergerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchResponseMergerTests.java @@ -32,6 +32,7 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; import org.apache.lucene.search.SortField; import org.apache.lucene.search.TotalHits; import org.opensearch.OpenSearchException; @@ -132,7 +133,13 @@ public void testMergeTookInMillis() throws InterruptedException { addResponse(merger, searchResponse); } awaitResponsesAdded(); - SearchResponse searchResponse = merger.getMergedResponse(SearchResponse.Clusters.EMPTY, new SearchRequestContext()); + SearchResponse searchResponse = merger.getMergedResponse( + SearchResponse.Clusters.EMPTY, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertEquals(TimeUnit.NANOSECONDS.toMillis(currentRelativeTime), searchResponse.getTook().millis()); } @@ -184,7 +191,13 @@ public void testMergeShardFailures() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, merger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse mergedResponse = merger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -235,7 +248,13 @@ public void testMergeShardFailuresNullShardTarget() throws InterruptedException awaitResponsesAdded(); assertEquals(numResponses, merger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse mergedResponse = merger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -281,8 +300,13 @@ public void testMergeShardFailuresNullShardId() throws InterruptedException { } awaitResponsesAdded(); assertEquals(numResponses, merger.numResponses()); - ShardSearchFailure[] shardFailures = merger.getMergedResponse(SearchResponse.Clusters.EMPTY, new SearchRequestContext()) - .getShardFailures(); + ShardSearchFailure[] shardFailures = merger.getMergedResponse( + SearchResponse.Clusters.EMPTY, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ).getShardFailures(); assertThat(Arrays.asList(shardFailures), containsInAnyOrder(expectedFailures.toArray(ShardSearchFailure.EMPTY_ARRAY))); } @@ -316,7 +340,13 @@ public void testMergeProfileResults() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, merger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse mergedResponse = merger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -378,7 +408,13 @@ public void testMergeCompletionSuggestions() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, searchResponseMerger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse mergedResponse = searchResponseMerger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -450,7 +486,13 @@ public void testMergeCompletionSuggestionsTieBreak() throws InterruptedException awaitResponsesAdded(); assertEquals(numResponses, searchResponseMerger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse mergedResponse = searchResponseMerger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -524,7 +566,13 @@ public void testMergeAggs() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, searchResponseMerger.numResponses()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse mergedResponse = searchResponseMerger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse mergedResponse = searchResponseMerger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertSame(clusters, mergedResponse.getClusters()); assertEquals(numResponses, mergedResponse.getTotalShards()); assertEquals(numResponses, mergedResponse.getSuccessfulShards()); @@ -681,7 +729,13 @@ public void testMergeSearchHits() throws InterruptedException { awaitResponsesAdded(); assertEquals(numResponses, searchResponseMerger.numResponses()); final SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); - SearchResponse searchResponse = searchResponseMerger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse searchResponse = searchResponseMerger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertEquals(TimeUnit.NANOSECONDS.toMillis(currentRelativeTime), searchResponse.getTook().millis()); assertEquals(expectedTotal, searchResponse.getTotalShards()); @@ -741,7 +795,13 @@ public void testMergeNoResponsesAdded() { SearchResponseMerger merger = new SearchResponseMerger(0, 10, Integer.MAX_VALUE, timeProvider, emptyReduceContextBuilder()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); assertEquals(0, merger.numResponses()); - SearchResponse response = merger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse response = merger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertSame(clusters, response.getClusters()); assertEquals(TimeUnit.NANOSECONDS.toMillis(currentRelativeTime), response.getTook().millis()); assertEquals(0, response.getTotalShards()); @@ -814,7 +874,13 @@ public void testMergeEmptySearchHitsWithNonEmpty() { merger.add(searchResponse); } assertEquals(2, merger.numResponses()); - SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse mergedResponse = merger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertEquals(10, mergedResponse.getHits().getTotalHits().value); assertEquals(10, mergedResponse.getHits().getHits().length); assertEquals(2, mergedResponse.getTotalShards()); @@ -856,7 +922,13 @@ public void testMergeOnlyEmptyHits() { ); merger.add(searchResponse); } - SearchResponse mergedResponse = merger.getMergedResponse(clusters, new SearchRequestContext()); + SearchResponse mergedResponse = merger.getMergedResponse( + clusters, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest() + ) + ); assertEquals(expectedTotalHits, mergedResponse.getHits().getTotalHits()); } diff --git a/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java index bed9bd37c49b4..da19c839f3826 100644 --- a/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java @@ -32,6 +32,7 @@ package org.opensearch.action.search; +import org.apache.logging.log4j.LogManager; import org.apache.lucene.search.TotalHits; import org.opensearch.Version; import org.opensearch.action.LatchedActionListener; @@ -484,7 +485,10 @@ public void testCCSRemoteReduceMergeFails() throws Exception { threadPool, listener, (r, l) -> setOnce.set(Tuple.tuple(r, l)), - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); if (localIndices == null) { assertNull(setOnce.get()); @@ -543,7 +547,10 @@ public void testCCSRemoteReduce() throws Exception { threadPool, listener, (r, l) -> setOnce.set(Tuple.tuple(r, l)), - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); if (localIndices == null) { assertNull(setOnce.get()); @@ -581,7 +588,10 @@ public void testCCSRemoteReduce() throws Exception { threadPool, listener, (r, l) -> setOnce.set(Tuple.tuple(r, l)), - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); if (localIndices == null) { assertNull(setOnce.get()); @@ -640,7 +650,10 @@ public void onNodeDisconnected(DiscoveryNode node, Transport.Connection connecti threadPool, listener, (r, l) -> setOnce.set(Tuple.tuple(r, l)), - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); if (localIndices == null) { assertNull(setOnce.get()); @@ -681,7 +694,10 @@ public void onNodeDisconnected(DiscoveryNode node, Transport.Connection connecti threadPool, listener, (r, l) -> setOnce.set(Tuple.tuple(r, l)), - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); if (localIndices == null) { assertNull(setOnce.get()); @@ -733,7 +749,10 @@ public void onNodeDisconnected(DiscoveryNode node, Transport.Connection connecti threadPool, listener, (r, l) -> setOnce.set(Tuple.tuple(r, l)), - new SearchRequestContext() + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + searchRequest + ) ); if (localIndices == null) { assertNull(setOnce.get()); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 752d88b677a9c..9bb1f51c51cf6 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -90,7 +90,7 @@ import org.opensearch.action.search.SearchExecutionStatsCollector; import org.opensearch.action.search.SearchPhaseController; import org.opensearch.action.search.SearchRequest; -import org.opensearch.action.search.SearchRequestOperationsListeners; +import org.opensearch.action.search.SearchRequestOperationsCompositeListenerFactory; import org.opensearch.action.search.SearchResponse; import org.opensearch.action.search.SearchTransportService; import org.opensearch.action.search.TransportSearchAction; @@ -2285,7 +2285,8 @@ public void onFailure(final Exception e) { writableRegistry(), searchService::aggReduceContextBuilder ); - SearchRequestOperationsListeners searchRequestOperationsListeners = new SearchRequestOperationsListeners(); + SearchRequestOperationsCompositeListenerFactory searchRequestOperationsCompositeListenerFactory = + new SearchRequestOperationsCompositeListenerFactory(); actions.put( SearchAction.INSTANCE, new TransportSearchAction( @@ -2312,7 +2313,7 @@ public void onFailure(final Exception e) { client ), NoopMetricsRegistry.INSTANCE, - searchRequestOperationsListeners + searchRequestOperationsCompositeListenerFactory ) ); actions.put(