Skip to content

Commit

Permalink
Addressed David's comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Peter Alfonsi <[email protected]>
  • Loading branch information
Peter Alfonsi committed Dec 6, 2024
1 parent ea30484 commit 2ac0920
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 87 deletions.
10 changes: 4 additions & 6 deletions server/src/main/java/org/opensearch/action/search/SearchPhase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
23 changes: 7 additions & 16 deletions server/src/main/java/org/opensearch/action/search/SearchPhaseName.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
21 changes: 12 additions & 9 deletions server/src/main/java/org/opensearch/action/search/SearchRequestStats.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
}

Expand Down
1 change: 0 additions & 1 deletion server/src/main/java/org/opensearch/action/search/SearchResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/index/search/stats/SearchStats.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
74 changes: 22 additions & 52 deletions server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import org.opensearch.common.settings.Settings;
import org.opensearch.test.OpenSearchTestCase;

import java.util.ArrayList;
import java.io.IOException;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -27,17 +27,6 @@

public class SearchRequestStatsTests extends OpenSearchTestCase {

static List<SearchPhaseName> 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);
Expand Down Expand Up @@ -80,7 +69,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));
Expand All @@ -97,7 +86,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);
Expand All @@ -122,10 +111,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);
Expand All @@ -141,7 +130,7 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE
}
phaser.arriveAndAwaitAdvance();
countDownLatch.await();
for (SearchPhaseName searchPhaseName : trackablePhases) {
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
assertEquals(numTasks, testRequestStats.getPhaseCurrent(searchPhaseName));
}
}
Expand All @@ -150,11 +139,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<SearchPhaseName, Long> 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);
Expand All @@ -181,7 +170,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),
Expand All @@ -194,10 +183,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);
Expand All @@ -214,7 +203,7 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte
}
phaser.arriveAndAwaitAdvance();
countDownLatch.await();
for (SearchPhaseName searchPhaseName : trackablePhases) {
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName));
}
}
Expand All @@ -224,15 +213,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() throws IOException {}
};
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(
Expand All @@ -241,21 +227,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());
}
}
2 changes: 0 additions & 2 deletions server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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
Expand Down

0 comments on commit 2ac0920

Please sign in to comment.