Skip to content

Commit

Permalink
Refactor ProfileResult class and add more tests
Browse files Browse the repository at this point in the history 
Signed-off-by: Ticheng Lin <ticheng@amazon.com>
  • Loading branch information
@ticheng-aws
ticheng-aws committed Jul 29, 2023
1 parent f53ead2 commit 7fd4d69
Show file tree
Hide file tree
Showing 7 changed files with 102 additions and 114 deletions.
60 changes: 40 additions & 20 deletions ...rnalClusterTest/java/org/opensearch/search/profile/aggregation/AggregationProfilerIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ public void testSimpleProfile() {
assertThat(histoAggResult.getTime(), greaterThan(0L));
Map<String, Long> breakdown = histoAggResult.getTimeBreakdown();
assertThat(breakdown, notNullValue());
if (histoAggResult.isConcurrent()) {
if (histoAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(breakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(breakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand Down Expand Up @@ -261,7 +262,8 @@ public void testMultiLevelProfile() {
assertThat(histoAggResult.getTime(), greaterThan(0L));
Map<String, Long> histoBreakdown = histoAggResult.getTimeBreakdown();
assertThat(histoBreakdown, notNullValue());
if (histoAggResult.isConcurrent()) {
if (histoAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(histoBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(histoBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -283,7 +285,8 @@ public void testMultiLevelProfile() {
assertThat(termsAggResult.getTime(), greaterThan(0L));
Map<String, Long> termsBreakdown = termsAggResult.getTimeBreakdown();
assertThat(termsBreakdown, notNullValue());
if (termsAggResult.isConcurrent()) {
if (termsAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(termsBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(termsBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -302,7 +305,8 @@ public void testMultiLevelProfile() {
assertThat(avgAggResult.getTime(), greaterThan(0L));
Map<String, Long> avgBreakdown = termsAggResult.getTimeBreakdown();
assertThat(avgBreakdown, notNullValue());
if (avgAggResult.isConcurrent()) {
if (avgAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(avgBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(avgBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand Down Expand Up @@ -359,7 +363,8 @@ public void testMultiLevelProfileBreadthFirst() {
assertThat(histoAggResult.getTime(), greaterThan(0L));
Map<String, Long> histoBreakdown = histoAggResult.getTimeBreakdown();
assertThat(histoBreakdown, notNullValue());
if (histoAggResult.isConcurrent()) {
if (histoAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(histoBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(histoBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -381,7 +386,8 @@ public void testMultiLevelProfileBreadthFirst() {
assertThat(termsAggResult.getTime(), greaterThan(0L));
Map<String, Long> termsBreakdown = termsAggResult.getTimeBreakdown();
assertThat(termsBreakdown, notNullValue());
if (termsAggResult.isConcurrent()) {
if (termsAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(termsBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(termsBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -400,7 +406,8 @@ public void testMultiLevelProfileBreadthFirst() {
assertThat(avgAggResult.getTime(), greaterThan(0L));
Map<String, Long> avgBreakdown = avgAggResult.getTimeBreakdown();
assertThat(avgBreakdown, notNullValue());
if (avgAggResult.isConcurrent()) {
if (avgAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(avgBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(avgBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand Down Expand Up @@ -442,7 +449,8 @@ public void testDiversifiedAggProfile() {
assertThat(diversifyAggResult.getTime(), greaterThan(0L));
Map<String, Long> diversifyBreakdown = diversifyAggResult.getTimeBreakdown();
assertThat(diversifyBreakdown, notNullValue());
if (diversifyAggResult.isConcurrent()) {
if (diversifyAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(diversifyBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(diversifyBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -463,7 +471,8 @@ public void testDiversifiedAggProfile() {
assertThat(maxAggResult.getTime(), greaterThan(0L));
Map<String, Long> maxBreakdown = maxAggResult.getTimeBreakdown();
assertThat(maxBreakdown, notNullValue());
if (maxAggResult.isConcurrent()) {
if (maxAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(maxBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(maxBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand Down Expand Up @@ -520,7 +529,8 @@ public void testComplexProfile() {
assertThat(histoAggResult.getTime(), greaterThan(0L));
Map<String, Long> histoBreakdown = histoAggResult.getTimeBreakdown();
assertThat(histoBreakdown, notNullValue());
if (histoAggResult.isConcurrent()) {
if (histoAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(histoBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(histoBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -547,7 +557,8 @@ public void testComplexProfile() {
assertThat(tagsAggResult.getTime(), greaterThan(0L));
Map<String, Long> tagsBreakdown = tagsAggResult.getTimeBreakdown();
assertThat(tagsBreakdown, notNullValue());
if (tagsAggResult.isConcurrent()) {
if (tagsAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(tagsBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(tagsBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -571,7 +582,8 @@ public void testComplexProfile() {
assertThat(avgAggResult.getTime(), greaterThan(0L));
Map<String, Long> avgBreakdown = avgAggResult.getTimeBreakdown();
assertThat(avgBreakdown, notNullValue());
if (avgAggResult.isConcurrent()) {
if (avgAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(avgBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(avgBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -591,7 +603,8 @@ public void testComplexProfile() {
assertThat(maxAggResult.getTime(), greaterThan(0L));
Map<String, Long> maxBreakdown = maxAggResult.getTimeBreakdown();
assertThat(maxBreakdown, notNullValue());
if (maxAggResult.isConcurrent()) {
if (maxAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(maxBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(maxBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -611,7 +624,8 @@ public void testComplexProfile() {
assertThat(stringsAggResult.getTime(), greaterThan(0L));
Map<String, Long> stringsBreakdown = stringsAggResult.getTimeBreakdown();
assertThat(stringsBreakdown, notNullValue());
if (stringsAggResult.isConcurrent()) {
if (stringsAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(stringsBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(stringsBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -635,7 +649,8 @@ public void testComplexProfile() {
assertThat(avgAggResult.getTime(), greaterThan(0L));
avgBreakdown = avgAggResult.getTimeBreakdown();
assertThat(avgBreakdown, notNullValue());
if (avgAggResult.isConcurrent()) {
if (avgAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(avgBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(avgBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -655,7 +670,8 @@ public void testComplexProfile() {
assertThat(maxAggResult.getTime(), greaterThan(0L));
maxBreakdown = maxAggResult.getTimeBreakdown();
assertThat(maxBreakdown, notNullValue());
if (maxAggResult.isConcurrent()) {
if (maxAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(maxBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(maxBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -676,7 +692,8 @@ public void testComplexProfile() {
assertThat(tagsAggResult.getTime(), greaterThan(0L));
tagsBreakdown = tagsAggResult.getTimeBreakdown();
assertThat(tagsBreakdown, notNullValue());
if (tagsAggResult.isConcurrent()) {
if (tagsAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(tagsBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(tagsBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -700,7 +717,8 @@ public void testComplexProfile() {
assertThat(avgAggResult.getTime(), greaterThan(0L));
avgBreakdown = avgAggResult.getTimeBreakdown();
assertThat(avgBreakdown, notNullValue());
if (avgAggResult.isConcurrent()) {
if (avgAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(avgBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(avgBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand All @@ -720,7 +738,8 @@ public void testComplexProfile() {
assertThat(maxAggResult.getTime(), greaterThan(0L));
maxBreakdown = maxAggResult.getTimeBreakdown();
assertThat(maxBreakdown, notNullValue());
if (maxAggResult.isConcurrent()) {
if (maxAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertThat(maxBreakdown.keySet(), equalTo(CONCURRENT_SEARCH_BREAKDOWN_KEYS));
} else {
assertThat(maxBreakdown.keySet(), equalTo(BREAKDOWN_KEYS));
Expand Down Expand Up @@ -821,7 +840,8 @@ public void testGlobalAggWithStatsSubAggregatorProfile() {
assertThat(globalAggResult.getTime(), greaterThan(0L));
Map<String, Long> breakdown = globalAggResult.getTimeBreakdown();
assertThat(breakdown, notNullValue());
if (globalAggResult.isConcurrent()) {
if (globalAggResult.getMaxSliceTime() != null) {
// concurrent segment search enabled
assertEquals(CONCURRENT_SEARCH_BREAKDOWN_KEYS, breakdown.keySet());
} else {
assertEquals(BREAKDOWN_KEYS, breakdown.keySet());
Expand Down
79 changes: 35 additions & 44 deletions server/src/main/java/org/opensearch/search/profile/ProfileResult.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,10 @@ public final class ProfileResult implements Writeable, ToXContentObject {
private final Map<String, Long> breakdown;
private final Map<String, Object> debug;
private final long nodeTime;
private final long maxSliceNodeTime;
private final long minSliceNodeTime;
private final long avgSliceNodeTime;
private Long maxSliceNodeTime;
private Long minSliceNodeTime;
private Long avgSliceNodeTime;
private final List<ProfileResult> children;
private final boolean concurrent;

public ProfileResult(
String type,
Expand All @@ -100,7 +99,7 @@ public ProfileResult(
long nodeTime,
List<ProfileResult> children
) {
this(type, description, breakdown, debug, nodeTime, children, false, -1, -1, -1);
this(type, description, breakdown, debug, nodeTime, children, null, null, null);
}

public ProfileResult(
Expand All @@ -110,18 +109,16 @@ public ProfileResult(
Map<String, Object> debug,
long nodeTime,
List<ProfileResult> children,
boolean concurrent,
long maxSliceNodeTime,
long minSliceNodeTime,
long avgSliceNodeTime
Long maxSliceNodeTime,
Long minSliceNodeTime,
Long avgSliceNodeTime
) {
this.type = type;
this.description = description;
this.breakdown = Objects.requireNonNull(breakdown, "required breakdown argument missing");
this.debug = debug == null ? Map.of() : debug;
this.children = children == null ? List.of() : children;
this.nodeTime = nodeTime;
this.concurrent = concurrent;
this.maxSliceNodeTime = maxSliceNodeTime;
this.minSliceNodeTime = minSliceNodeTime;
this.avgSliceNodeTime = avgSliceNodeTime;
Expand All @@ -138,21 +135,13 @@ public ProfileResult(StreamInput in) throws IOException {
debug = in.readMap(StreamInput::readString, StreamInput::readGenericValue);
children = in.readList(ProfileResult::new);
if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
this.concurrent = in.readBoolean();
if (concurrent) {
this.maxSliceNodeTime = in.readLong();
this.minSliceNodeTime = in.readLong();
this.avgSliceNodeTime = in.readLong();
} else {
this.maxSliceNodeTime = -1;
this.minSliceNodeTime = -1;
this.avgSliceNodeTime = -1;
}
this.maxSliceNodeTime = in.readOptionalLong();
this.minSliceNodeTime = in.readOptionalLong();
this.avgSliceNodeTime = in.readOptionalLong();
} else {
this.concurrent = false;
this.maxSliceNodeTime = -1;
this.minSliceNodeTime = -1;
this.avgSliceNodeTime = -1;
this.maxSliceNodeTime = null;
this.minSliceNodeTime = null;
this.avgSliceNodeTime = null;
}
}

Expand All @@ -165,12 +154,9 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeMap(debug, StreamOutput::writeString, StreamOutput::writeGenericValue);
out.writeList(children);
if (out.getVersion().onOrAfter(Version.V_3_0_0)) {
out.writeBoolean(concurrent);
if (concurrent) {
out.writeLong(maxSliceNodeTime);
out.writeLong(minSliceNodeTime);
out.writeLong(avgSliceNodeTime);
}
out.writeOptionalLong(maxSliceNodeTime);
out.writeOptionalLong(minSliceNodeTime);
out.writeOptionalLong(avgSliceNodeTime);
}
}

Expand Down Expand Up @@ -211,22 +197,18 @@ public long getTime() {
return nodeTime;
}

public long getMaxSliceTime() {
public Long getMaxSliceTime() {
return maxSliceNodeTime;
}

public long getMinSliceTime() {
public Long getMinSliceTime() {
return minSliceNodeTime;
}

public long getAvgSliceTime() {
public Long getAvgSliceTime() {
return avgSliceNodeTime;
}

public boolean isConcurrent() {
return concurrent;
}

/**
* Returns a list of all profiled children queries
*/
Expand All @@ -241,19 +223,27 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.field(DESCRIPTION.getPreferredName(), description);
if (builder.humanReadable()) {
builder.field(NODE_TIME.getPreferredName(), new TimeValue(getTime(), TimeUnit.NANOSECONDS).toString());
if (concurrent) {
if (getMaxSliceTime() != null) {
builder.field(MAX_SLICE_NODE_TIME.getPreferredName(), new TimeValue(getMaxSliceTime(), TimeUnit.NANOSECONDS).toString());
}
if (getMinSliceTime() != null) {
builder.field(MIN_SLICE_NODE_TIME.getPreferredName(), new TimeValue(getMinSliceTime(), TimeUnit.NANOSECONDS).toString());
}
if (getAvgSliceTime() != null) {
builder.field(AVG_SLICE_NODE_TIME.getPreferredName(), new TimeValue(getAvgSliceTime(), TimeUnit.NANOSECONDS).toString());
}
}
builder.field(NODE_TIME_RAW.getPreferredName(), getTime());
if (concurrent) {
if (getMaxSliceTime() != null) {
builder.field(MAX_SLICE_NODE_TIME_RAW.getPreferredName(), getMaxSliceTime());
}
if (getMinSliceTime() != null) {
builder.field(MIN_SLICE_NODE_TIME_RAW.getPreferredName(), getMinSliceTime());
}
if (getAvgSliceTime() != null) {
builder.field(AVG_SLICE_NODE_TIME_RAW.getPreferredName(), getAvgSliceTime());
}
createBreakownView(builder);
createBreakdownView(builder);
if (false == debug.isEmpty()) {
builder.field(DEBUG.getPreferredName(), debug);
}
Expand All @@ -269,11 +259,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return builder.endObject();
}

private void createBreakownView(XContentBuilder builder) throws IOException {
private void createBreakdownView(XContentBuilder builder) throws IOException {
Map<String, Long> modifiedBreakdown = new LinkedHashMap<>(breakdown);
if (!concurrent) {
removeStartTimeFields(modifiedBreakdown);
}
removeStartTimeFields(modifiedBreakdown);
builder.field(BREAKDOWN.getPreferredName(), modifiedBreakdown);
}

Expand All @@ -300,6 +288,9 @@ static void removeStartTimeFields(Map<String, Long> modifiedBreakdown) {
parser.declareObject(optionalConstructorArg(), (p, c) -> p.map(), DEBUG);
parser.declareLong(constructorArg(), NODE_TIME_RAW);
parser.declareObjectArray(optionalConstructorArg(), (p, c) -> fromXContent(p), CHILDREN);
parser.declareLong(optionalConstructorArg(), MAX_SLICE_NODE_TIME_RAW);
parser.declareLong(optionalConstructorArg(), MIN_SLICE_NODE_TIME_RAW);
parser.declareLong(optionalConstructorArg(), AVG_SLICE_NODE_TIME_RAW);
PARSER = parser.build();
}

Expand Down
1 change: 0 additions & 1 deletion ...rc/main/java/org/opensearch/search/profile/aggregation/ConcurrentAggregationProfiler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ private List<ProfileResult> reduceProfileResultsTree(List<ProfileResult> profile
debug,
nodeTime,
reducedChildrenTree,
true,
maxSliceNodeTime,
minSliceNodeTime,
avgSliceNodeTime
Expand Down
Loading

0 comments on commit 7fd4d69

Please sign in to comment.