From a41f9be10026344d956f2b861d2439b9c03f2d5b Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 5 Dec 2024 16:40:26 -0800 Subject: [PATCH] Addressed David's comments Signed-off-by: Peter Alfonsi --- .../opensearch/action/search/SearchPhase.java | 10 +-- .../action/search/SearchPhaseName.java | 23 ++---- .../action/search/SearchRequestStats.java | 21 +++--- .../action/search/SearchResponse.java | 1 - .../index/search/stats/SearchStats.java | 2 +- .../search/SearchRequestStatsTests.java | 74 ++++++------------- .../index/search/stats/SearchStatsTests.java | 2 - 7 files changed, 45 insertions(+), 88 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhase.java b/server/src/main/java/org/opensearch/action/search/SearchPhase.java index 9fe6e5f9342ee..0890e9f5de8d4 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhase.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhase.java @@ -69,13 +69,11 @@ public String getName() { } /** - * Returns the SearchPhase name as {@link SearchPhaseName}. If unrecognized, returns the catch-all OTHER_PHASE_TYPES. + * Returns the SearchPhase name as {@link SearchPhaseName}. Exception will come if SearchPhase name is not defined + * in {@link SearchPhaseName} + * @return {@link SearchPhaseName} */ public SearchPhaseName getSearchPhaseName() { - try { - return SearchPhaseName.valueOf(name.toUpperCase(Locale.ROOT)); - } catch (IllegalArgumentException e) { - return SearchPhaseName.OTHER_PHASE_TYPES; - } + return SearchPhaseName.valueOf(name.toUpperCase(Locale.ROOT)); } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java b/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java index 853e0fab88d9a..8cf92934c8a52 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java @@ -17,29 +17,20 @@ */ @PublicApi(since = "2.9.0") public enum SearchPhaseName { - DFS_PRE_QUERY("dfs_pre_query", true), - QUERY("query", true), - FETCH("fetch", true), - DFS_QUERY("dfs_query", true), - EXPAND("expand", true), - CAN_MATCH("can_match", true), - - // A catch-all for other phase types which shouldn't appear in the search phase stats API. - OTHER_PHASE_TYPES("other_phase_types", false); + DFS_PRE_QUERY("dfs_pre_query"), + QUERY("query"), + FETCH("fetch"), + DFS_QUERY("dfs_query"), + EXPAND("expand"), + CAN_MATCH("can_match"); private final String name; - private final boolean shouldTrack; - SearchPhaseName(final String name, final boolean shouldTrack) { + SearchPhaseName(final String name) { this.name = name; - this.shouldTrack = shouldTrack; } public String getName() { return name; } - - public boolean shouldTrack() { - return shouldTrack; - } } 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 cb0a742c6f775..a2722318ac599 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -73,28 +73,31 @@ public long getTookMetric() { @Override protected void onPhaseStart(SearchPhaseContext context) { - SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); - if (phaseName.shouldTrack()) { - phaseStatsMap.get(phaseName).current.inc(); + try { + phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc(); + } catch (IllegalArgumentException ignored) { + // Do nothing if the phase isn't found in SearchPhaseName. } } @Override protected void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); - if (phaseName.shouldTrack()) { - StatsHolder phaseStats = phaseStatsMap.get(phaseName); + try { + StatsHolder phaseStats = phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()); phaseStats.current.dec(); phaseStats.total.inc(); phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos())); + } catch (IllegalArgumentException ignored) { + // Do nothing if the phase isn't found in SearchPhaseName. } } @Override protected void onPhaseFailure(SearchPhaseContext context, Throwable cause) { - SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); - if (phaseName.shouldTrack()) { - phaseStatsMap.get(phaseName).current.dec(); + try { + phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); + } catch (IllegalArgumentException ignored) { + // Do nothing if the phase isn't found in SearchPhaseName. } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchResponse.java b/server/src/main/java/org/opensearch/action/search/SearchResponse.java index 7131d4611ea79..899c71e91e3ab 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchResponse.java +++ b/server/src/main/java/org/opensearch/action/search/SearchResponse.java @@ -705,7 +705,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject(PHASE_TOOK.getPreferredName()); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - if (!searchPhaseName.shouldTrack()) continue; if (phaseTookMap.containsKey(searchPhaseName.getName())) { builder.field(searchPhaseName.getName(), phaseTookMap.get(searchPhaseName.getName())); } else { diff --git a/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java b/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java index f5b4bb38be9b0..d6ea803c9ee13 100644 --- a/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java +++ b/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java @@ -524,7 +524,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { PhaseStatsLongHolder statsLongHolder = requestStatsLongHolder.requestStatsHolder.get(searchPhaseName.getName()); - if (statsLongHolder == null || !searchPhaseName.shouldTrack()) { + if (statsLongHolder == null) { continue; } builder.startObject(searchPhaseName.getName()); 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 5a0d33af51956..2b9a5992c0117 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -13,7 +13,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.test.OpenSearchTestCase; -import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -26,18 +25,6 @@ import static org.mockito.Mockito.when; public class SearchRequestStatsTests extends OpenSearchTestCase { - - static List trackablePhases; - - static { - trackablePhases = new ArrayList<>(); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - if (searchPhaseName.shouldTrack()) { - trackablePhases.add(searchPhaseName); - } - } - } - public void testSearchRequestStats_OnRequestFailure() { ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); @@ -80,7 +67,7 @@ public void testSearchRequestPhaseFailure() { SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); testRequestStats.onPhaseStart(ctx); assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName)); @@ -97,7 +84,7 @@ public void testSearchRequestStats() { SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); long tookTimeInMillis = randomIntBetween(1, 10); testRequestStats.onPhaseStart(ctx); @@ -122,10 +109,10 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE 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 * trackablePhases.size()]; - Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); - for (SearchPhaseName searchPhaseName : trackablePhases) { + Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; + Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -141,7 +128,7 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { assertEquals(numTasks, testRequestStats.getPhaseCurrent(searchPhaseName)); } } @@ -150,11 +137,11 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc 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 * trackablePhases.size()]; - Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); + Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; + Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); Map searchPhaseNameLongMap = new HashMap<>(); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -181,7 +168,7 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { assertEquals(numTasks, testRequestStats.getPhaseTotal(searchPhaseName)); assertThat( testRequestStats.getPhaseMetric(searchPhaseName), @@ -194,10 +181,10 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte 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 * trackablePhases.size()]; - Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); - for (SearchPhaseName searchPhaseName : trackablePhases) { + Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; + Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -214,7 +201,7 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName)); } } @@ -224,15 +211,12 @@ public void testOtherPhaseNamesAreIgnored() { 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); + SearchPhase mockSearchPhase = new SearchPhase("unrecognized_phase") { + @Override + public void run() {} + }; when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - - when(mockSearchPhase.getSearchPhaseName()).thenReturn(SearchPhaseName.OTHER_PHASE_TYPES); testRequestStats.onPhaseStart(ctx); - long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(10); - when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); - // All values should return 0 for untracked phase types - assertEquals(0, testRequestStats.getPhaseCurrent(SearchPhaseName.OTHER_PHASE_TYPES)); testRequestStats.onPhaseEnd( ctx, new SearchRequestContext( @@ -241,21 +225,5 @@ public void testOtherPhaseNamesAreIgnored() { () -> null ) ); - assertEquals(0, testRequestStats.getPhaseCurrent(SearchPhaseName.OTHER_PHASE_TYPES)); - assertEquals(0, testRequestStats.getPhaseTotal(SearchPhaseName.OTHER_PHASE_TYPES)); - assertEquals(0, testRequestStats.getPhaseMetric(SearchPhaseName.OTHER_PHASE_TYPES)); - } - - public void testSearchPhaseCatchAll() { - // Test search phases with unrecognized names return the catch-all OTHER_PHASE_TYPES when getSearchPhaseName() is called. - // These may exist, for example, "create_pit". - String unrecognizedName = "unrecognized_name"; - SearchPhase dummyPhase = new SearchPhase(unrecognizedName) { - @Override - public void run() {} - }; - - assertEquals(unrecognizedName, dummyPhase.getName()); - assertEquals(SearchPhaseName.OTHER_PHASE_TYPES, dummyPhase.getSearchPhaseName()); } } 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 bb8896023fdc7..594700ea60b3e 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 @@ -83,7 +83,6 @@ public void testShardLevelSearchGroupStats() throws Exception { SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); SearchPhaseContext ctx = mock(SearchPhaseContext.class); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - if (!searchPhaseName.shouldTrack()) continue; SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); when(mockSearchPhase.getStartTimeInNanos()).thenReturn(System.nanoTime() - TimeUnit.SECONDS.toNanos(paramValue)); @@ -95,7 +94,6 @@ public void testShardLevelSearchGroupStats() throws Exception { } searchStats1.setSearchRequestStats(testRequestStats); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - if (!searchPhaseName.shouldTrack()) continue; assertEquals( 0, searchStats1.getTotal().getRequestStatsLongHolder().getRequestStatsHolder().get(searchPhaseName.getName()).current