Skip to content

Commit 3e07ffa

Browse files
chrisparrinellochrisparrinello
committed
measure can-match coordinator execution for all code paths
1 parent c4aa227 commit 3e07ffa

File tree

5 files changed

+29
-29
lines changed

5 files changed

+29
-29
lines changed

‎server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,7 @@ private CanMatchPreFilterSearchPhase(
9393
SearchTask task,
9494
boolean requireAtLeastOneMatch,
9595
CoordinatorRewriteContextProvider coordinatorRewriteContextProvider,
96-
ActionListener<List<SearchShardIterator>> listener,
97-
SearchResponseMetrics searchResponseMetrics
96+
ActionListener<List<SearchShardIterator>> listener
9897
) {
9998
this.logger = logger;
10099
this.searchTransportService = searchTransportService;
@@ -152,13 +151,7 @@ public static SubscribableListener<List<SearchShardIterator>> execute(
152151
listener.addListener(new ActionListener<>() {
153152
@Override
154153
public void onResponse(List<SearchShardIterator> shardsIts) {
155-
// we only want to record the phase duration if this call to execute is running on the coordinator node
156-
// as part of the can-match phase or a search request. It will be null if this is the data node round trip can-match
157-
// execution
158-
// or an open PIT request
159-
if (searchResponseMetrics != null) {
160-
searchResponseMetrics.recordSearchPhaseDuration(PHASE_NAME, System.nanoTime() - phaseStartTimeInNanos);
161-
}
154+
searchResponseMetrics.recordSearchPhaseDuration(PHASE_NAME, System.nanoTime() - phaseStartTimeInNanos);
162155
}
163156

164157
@Override
@@ -193,8 +186,7 @@ protected void doRun() {
193186
task,
194187
requireAtLeastOneMatch,
195188
coordinatorRewriteContextProvider,
196-
listener,
197-
searchResponseMetrics
189+
listener
198190
).runCoordinatorRewritePhase();
199191
}
200192
});

‎server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ public void runNewSearchPhase(
185185
task,
186186
false,
187187
searchService.getCoordinatorRewriteContextProvider(timeProvider::absoluteStartMillis),
188-
null
188+
searchResponseMetrics
189189
)
190190
.addListener(
191191
listener.delegateFailureAndWrap(

‎server/src/main/java/org/elasticsearch/action/search/TransportSearchShardsAction.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import org.elasticsearch.index.Index;
2626
import org.elasticsearch.index.query.Rewriteable;
2727
import org.elasticsearch.injection.guice.Inject;
28+
import org.elasticsearch.rest.action.search.SearchResponseMetrics;
2829
import org.elasticsearch.search.SearchService;
2930
import org.elasticsearch.search.builder.SearchSourceBuilder;
3031
import org.elasticsearch.search.internal.AliasFilter;
@@ -60,6 +61,7 @@ public class TransportSearchShardsAction extends HandledTransportAction<SearchSh
6061
private final ProjectResolver projectResolver;
6162
private final IndexNameExpressionResolver indexNameExpressionResolver;
6263
private final ThreadPool threadPool;
64+
private final SearchResponseMetrics searchResponseMetrics;
6365

6466
@Inject
6567
public TransportSearchShardsAction(
@@ -70,7 +72,8 @@ public TransportSearchShardsAction(
7072
TransportSearchAction transportSearchAction,
7173
SearchTransportService searchTransportService,
7274
ProjectResolver projectResolver,
73-
IndexNameExpressionResolver indexNameExpressionResolver
75+
IndexNameExpressionResolver indexNameExpressionResolver,
76+
SearchResponseMetrics searchResponseMetrics
7477
) {
7578
super(
7679
TYPE.name(),
@@ -88,6 +91,7 @@ public TransportSearchShardsAction(
8891
this.projectResolver = projectResolver;
8992
this.indexNameExpressionResolver = indexNameExpressionResolver;
9093
this.threadPool = transportService.getThreadPool();
94+
this.searchResponseMetrics = searchResponseMetrics;
9195
}
9296

9397
@Override
@@ -176,7 +180,7 @@ public void searchShards(Task task, SearchShardsRequest searchShardsRequest, Act
176180
(SearchTask) task,
177181
false,
178182
searchService.getCoordinatorRewriteContextProvider(timeProvider::absoluteStartMillis),
179-
null // do not record a phase duration for can_match in search_shards
183+
searchResponseMetrics
180184
)
181185
.addListener(
182186
delegate.map(

‎server/src/test/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhaseTests.java‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.elasticsearch.index.shard.ShardId;
4444
import org.elasticsearch.index.shard.ShardLongFieldRange;
4545
import org.elasticsearch.indices.DateFieldRangeInfo;
46+
import org.elasticsearch.rest.action.search.SearchResponseMetrics;
4647
import org.elasticsearch.search.CanMatchShardResponse;
4748
import org.elasticsearch.search.aggregations.AggregationBuilder;
4849
import org.elasticsearch.search.aggregations.bucket.terms.SignificantTermsAggregationBuilder;
@@ -53,6 +54,7 @@
5354
import org.elasticsearch.search.sort.SortBuilders;
5455
import org.elasticsearch.search.sort.SortOrder;
5556
import org.elasticsearch.search.suggest.SuggestBuilder;
57+
import org.elasticsearch.telemetry.TelemetryProvider;
5658
import org.elasticsearch.test.ESTestCase;
5759
import org.elasticsearch.threadpool.TestThreadPool;
5860
import org.elasticsearch.threadpool.ThreadPool;
@@ -162,7 +164,7 @@ public void sendCanMatch(
162164
null,
163165
true,
164166
EMPTY_CONTEXT_PROVIDER,
165-
null
167+
new SearchResponseMetrics(TelemetryProvider.NOOP.getMeterRegistry())
166168
).addListener(ActionTestUtils.assertNoFailureListener(iter -> {
167169
result.set(iter);
168170
latch.countDown();
@@ -258,7 +260,7 @@ public void sendCanMatch(
258260
null,
259261
true,
260262
EMPTY_CONTEXT_PROVIDER,
261-
null
263+
new SearchResponseMetrics(TelemetryProvider.NOOP.getMeterRegistry())
262264
).addListener(ActionTestUtils.assertNoFailureListener(iter -> {
263265
result.set(iter);
264266
latch.countDown();
@@ -350,7 +352,7 @@ public void sendCanMatch(
350352
null,
351353
true,
352354
EMPTY_CONTEXT_PROVIDER,
353-
null
355+
new SearchResponseMetrics(TelemetryProvider.NOOP.getMeterRegistry())
354356
).addListener(ActionTestUtils.assertNoFailureListener(iter -> {
355357
result.set(iter);
356358
latch.countDown();
@@ -450,7 +452,7 @@ public void sendCanMatch(
450452
null,
451453
shardsIter.size() > shardToSkip.size(),
452454
EMPTY_CONTEXT_PROVIDER,
453-
null
455+
new SearchResponseMetrics(TelemetryProvider.NOOP.getMeterRegistry())
454456
).addListener(ActionTestUtils.assertNoFailureListener(iter -> {
455457
result.set(iter);
456458
latch.countDown();
@@ -1423,7 +1425,7 @@ public void sendCanMatch(
14231425
null,
14241426
true,
14251427
contextProvider,
1426-
null
1428+
new SearchResponseMetrics(TelemetryProvider.NOOP.getMeterRegistry())
14271429
),
14281430
requests
14291431
);

‎server/src/test/java/org/elasticsearch/rest/action/search/SearchPhaseCoordinatorAPMMetricsTests.java‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public void testSearchQueryThenFetch() {
8383
client().prepareSearch(indexName).setSearchType(SearchType.QUERY_THEN_FETCH).setQuery(simpleQueryStringQuery("doc1")),
8484
"1"
8585
);
86-
assertMeasurements(List.of(QUERY_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC));
86+
assertMeasurements(List.of(QUERY_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC), 1);
8787
assertNotMeasured(
8888
List.of(CAN_MATCH_SEARCH_PHASE_METRIC, DFS_SEARCH_PHASE_METRIC, DFS_QUERY_SEARCH_PHASE_METRIC, OPEN_PIT_SEARCH_PHASE_METRIC)
8989
);
@@ -94,7 +94,7 @@ public void testDfsSearch() {
9494
client().prepareSearch(indexName).setSearchType(SearchType.DFS_QUERY_THEN_FETCH).setQuery(simpleQueryStringQuery("doc1")),
9595
"1"
9696
);
97-
assertMeasurements(List.of(DFS_SEARCH_PHASE_METRIC, DFS_QUERY_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC));
97+
assertMeasurements(List.of(DFS_SEARCH_PHASE_METRIC, DFS_QUERY_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC), 1);
9898
assertNotMeasured(List.of(CAN_MATCH_SEARCH_PHASE_METRIC, QUERY_SEARCH_PHASE_METRIC, OPEN_PIT_SEARCH_PHASE_METRIC));
9999
}
100100

@@ -112,8 +112,8 @@ public void testPointInTime() {
112112
.setQuery(simpleQueryStringQuery("doc1")),
113113
"1"
114114
);
115-
assertMeasurements(List.of(OPEN_PIT_SEARCH_PHASE_METRIC, QUERY_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC));
116-
assertNotMeasured(List.of(CAN_MATCH_SEARCH_PHASE_METRIC, DFS_SEARCH_PHASE_METRIC, DFS_QUERY_SEARCH_PHASE_METRIC));
115+
assertMeasurements(List.of(OPEN_PIT_SEARCH_PHASE_METRIC, QUERY_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC), 1);
116+
assertNotMeasured(List.of(DFS_SEARCH_PHASE_METRIC, DFS_QUERY_SEARCH_PHASE_METRIC));
117117
} finally {
118118
client().execute(TransportClosePointInTimeAction.TYPE, new ClosePointInTimeRequest(pointInTimeId)).actionGet();
119119
}
@@ -134,8 +134,10 @@ public void testPointInTimeWithPreFiltering() {
134134
.setQuery(simpleQueryStringQuery("doc1")),
135135
"1"
136136
);
137+
assertMeasurements(List.of(OPEN_PIT_SEARCH_PHASE_METRIC, QUERY_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC), 1);
137138
assertMeasurements(
138-
List.of(OPEN_PIT_SEARCH_PHASE_METRIC, CAN_MATCH_SEARCH_PHASE_METRIC, QUERY_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC)
139+
List.of(CAN_MATCH_SEARCH_PHASE_METRIC),
140+
2 // one during open PIT, one during can-match phase of search
139141
);
140142
assertNotMeasured(List.of(DFS_SEARCH_PHASE_METRIC, DFS_QUERY_SEARCH_PHASE_METRIC));
141143
} finally {
@@ -152,7 +154,7 @@ public void testCanMatchSearch() {
152154
"1"
153155
);
154156

155-
assertMeasurements(List.of(CAN_MATCH_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC, QUERY_SEARCH_PHASE_METRIC));
157+
assertMeasurements(List.of(CAN_MATCH_SEARCH_PHASE_METRIC, FETCH_SEARCH_PHASE_METRIC, QUERY_SEARCH_PHASE_METRIC), 1);
156158
assertNotMeasured(List.of(DFS_SEARCH_PHASE_METRIC, DFS_QUERY_SEARCH_PHASE_METRIC, OPEN_PIT_SEARCH_PHASE_METRIC));
157159
}
158160

@@ -167,15 +169,15 @@ private TestTelemetryPlugin getTestTelemetryPlugin() {
167169
private void assertNotMeasured(Collection<String> metricNames) {
168170
for (var metricName : metricNames) {
169171
List<Measurement> measurements = getTestTelemetryPlugin().getLongHistogramMeasurement(metricName);
170-
assertThat(measurements, hasSize(0));
172+
assertThat(metricName, measurements, hasSize(0));
171173
}
172174
}
173175

174-
private void assertMeasurements(Collection<String> metricNames) {
176+
private void assertMeasurements(Collection<String> metricNames, int numberOfMeasurements) {
175177
for (var metricName : metricNames) {
176178
List<Measurement> measurements = getTestTelemetryPlugin().getLongHistogramMeasurement(metricName);
177-
assertThat(measurements, hasSize(1));
178-
assertThat(measurements.getFirst().getLong(), greaterThanOrEqualTo(0L));
179+
assertThat(metricName, measurements, hasSize(numberOfMeasurements));
180+
assertThat(metricName, measurements.getFirst().getLong(), greaterThanOrEqualTo(0L));
179181
}
180182
}
181183
}

0 commit comments

Comments
 (0)