Skip to content

Commit

Permalink
[Profiling] Remove legacy aggregation_field (elastic#119770)
Browse files Browse the repository at this point in the history
  • Loading branch information
rockdaboot authored Jan 9, 2025
1 parent 0cf2ebb commit b2f290f
Show file tree
Hide file tree
Showing 13 changed files with 41 additions and 195 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ public void testGetStackTracesUnfiltered() throws Exception {
null,
null,
null,
null,
null
);
GetFlamegraphResponse response = client().execute(GetFlamegraphAction.INSTANCE, request).get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ public void testGetStackTracesUnfiltered() throws Exception {
null,
null,
null,
null,
null
);
request.setAdjustSampleCount(true);
Expand Down Expand Up @@ -68,8 +67,7 @@ public void testGetStackTracesGroupedByServiceName() throws Exception {
null,
null,
null,
"service.name",
null,
new String[] { "service.name" },
null,
null,
null,
Expand Down Expand Up @@ -117,8 +115,7 @@ public void testGetStackTracesFromAPMWithMatchNoDownsampling() throws Exception
// also match an index that does not contain stacktrace ids to ensure it is ignored
new String[] { "apm-test-*", "apm-legacy-test-*" },
"transaction.profiler_stack_trace_ids",
"transaction.name",
null,
new String[] { "transaction.name" },
null,
null,
null,
Expand Down Expand Up @@ -168,7 +165,6 @@ public void testGetStackTracesFromAPMWithMatchAndDownsampling() throws Exception
null,
null,
null,
null,
null
);
// ensures consistent results in the random sampler aggregation that is used internally
Expand Down Expand Up @@ -219,7 +215,6 @@ public void testGetStackTracesFromAPMNoMatch() throws Exception {
null,
null,
null,
null,
null
);
GetStackTracesResponse response = client().execute(GetStackTracesAction.INSTANCE, request).get();
Expand All @@ -242,7 +237,6 @@ public void testGetStackTracesFromAPMIndexNotAvailable() throws Exception {
null,
null,
null,
null,
null
);
GetStackTracesResponse response = client().execute(GetStackTracesAction.INSTANCE, request).get();
Expand All @@ -265,7 +259,6 @@ public void testGetStackTracesFromAPMStackTraceFieldNotAvailable() throws Except
null,
null,
null,
null,
null
);
GetStackTracesResponse response = client().execute(GetStackTracesAction.INSTANCE, request).get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ public void testGetTopNFunctionsUnfiltered() throws Exception {
null,
null,
null,
null,
null
);
request.setAdjustSampleCount(true);
Expand All @@ -42,8 +41,7 @@ public void testGetTopNFunctionsGroupedByServiceName() throws Exception {
null,
null,
null,
"service.name",
null,
new String[] { "service.name" },
null,
null,
null,
Expand All @@ -70,8 +68,7 @@ public void testGetTopNFunctionsFromAPM() throws Exception {
// also match an index that does not contain stacktrace ids to ensure it is ignored
new String[] { "apm-test-*", "apm-legacy-test-*" },
"transaction.profiler_stack_trace_ids",
"transaction.name",
null,
new String[] { "transaction.name" },
null,
null,
null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import org.elasticsearch.action.support.TransportAction;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.core.UpdateForV9;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.tasks.CancellableTask;
import org.elasticsearch.tasks.Task;
Expand Down Expand Up @@ -43,8 +42,6 @@ public class GetStackTracesRequest extends ActionRequest implements IndicesReque
public static final ParseField LIMIT_FIELD = new ParseField("limit");
public static final ParseField INDICES_FIELD = new ParseField("indices");
public static final ParseField STACKTRACE_IDS_FIELD = new ParseField("stacktrace_ids_field");
@UpdateForV9(owner = UpdateForV9.Owner.PROFILING) // Remove this BWC layer and allow only AGGREGATION_FIELDS
public static final ParseField AGGREGATION_FIELD = new ParseField("aggregation_field");
public static final ParseField AGGREGATION_FIELDS = new ParseField("aggregation_fields");
public static final ParseField REQUESTED_DURATION_FIELD = new ParseField("requested_duration");
public static final ParseField AWS_COST_FACTOR_FIELD = new ParseField("aws_cost_factor");
Expand All @@ -62,8 +59,6 @@ public class GetStackTracesRequest extends ActionRequest implements IndicesReque
private String[] indices;
private boolean userProvidedIndices;
private String stackTraceIdsField;
@UpdateForV9(owner = UpdateForV9.Owner.PROFILING) // Remove this BWC layer and allow only aggregationFields
private String aggregationField;
private String[] aggregationFields;
private Double requestedDuration;
private Double awsCostFactor;
Expand All @@ -83,7 +78,7 @@ public class GetStackTracesRequest extends ActionRequest implements IndicesReque
private Integer shardSeed;

public GetStackTracesRequest() {
this(null, null, null, null, null, null, null, null, null, null, null, null, null, null);
this(null, null, null, null, null, null, null, null, null, null, null, null, null);
}

public GetStackTracesRequest(
Expand All @@ -94,7 +89,6 @@ public GetStackTracesRequest(
QueryBuilder query,
String[] indices,
String stackTraceIdsField,
String aggregationField,
String[] aggregationFields,
Double customCO2PerKWH,
Double customDatacenterPUE,
Expand All @@ -110,7 +104,6 @@ public GetStackTracesRequest(
this.indices = indices;
this.userProvidedIndices = indices != null && indices.length > 0;
this.stackTraceIdsField = stackTraceIdsField;
this.aggregationField = aggregationField;
this.aggregationFields = aggregationFields;
this.customCO2PerKWH = customCO2PerKWH;
this.customDatacenterPUE = customDatacenterPUE;
Expand Down Expand Up @@ -184,23 +177,15 @@ public String getStackTraceIdsField() {
return stackTraceIdsField;
}

public String getAggregationField() {
return aggregationField;
}

public String[] getAggregationFields() {
return aggregationField != null ? new String[] { aggregationField } : aggregationFields;
return aggregationFields;
}

public boolean hasAggregationFields() {
String[] f = getAggregationFields();
return f != null && f.length > 0;
}

public boolean isLegacyAggregationField() {
return aggregationField != null;
}

public boolean isAdjustSampleCount() {
return Boolean.TRUE.equals(adjustSampleCount);
}
Expand Down Expand Up @@ -237,8 +222,6 @@ public void parseXContent(XContentParser parser) throws IOException {
this.limit = parser.intValue();
} else if (STACKTRACE_IDS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
this.stackTraceIdsField = parser.text();
} else if (AGGREGATION_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
this.aggregationField = parser.text();
} else if (REQUESTED_DURATION_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
this.requestedDuration = parser.doubleValue();
} else if (AWS_COST_FACTOR_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
Expand Down Expand Up @@ -322,17 +305,6 @@ public ActionRequestValidationException validate() {
);
}
}
if (aggregationField != null && aggregationFields != null) {
validationException = addValidationError(
"["
+ AGGREGATION_FIELD.getPreferredName()
+ "] must not be set when ["
+ AGGREGATION_FIELDS.getPreferredName()
+ "] is also set",
validationException
);

}
if (aggregationFields != null) {
// limit so we avoid an explosion of buckets
if (aggregationFields.length < 1 || aggregationFields.length > 2) {
Expand All @@ -348,13 +320,6 @@ public ActionRequestValidationException validate() {

}

if (aggregationField != null && aggregationField.isBlank()) {
validationException = addValidationError(
"[" + AGGREGATION_FIELD.getPreferredName() + "] must be non-empty",
validationException
);
}

validationException = requirePositive(SAMPLE_SIZE_FIELD, sampleSize, validationException);
validationException = requirePositive(LIMIT_FIELD, limit, validationException);
validationException = requirePositive(REQUESTED_DURATION_FIELD, requestedDuration, validationException);
Expand Down Expand Up @@ -386,7 +351,6 @@ public String getDescription() {
StringBuilder sb = new StringBuilder();
appendField(sb, "indices", indices);
appendField(sb, "stacktrace_ids_field", stackTraceIdsField);
appendField(sb, "aggregation_field", aggregationField);
appendField(sb, "aggregation_fields", aggregationFields);
appendField(sb, "sample_size", sampleSize);
appendField(sb, "limit", limit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,21 @@ public class SubGroup implements ToXContentFragment {
private final String name;
private Long count;
@UpdateForV9(owner = UpdateForV9.Owner.PROFILING) // remove legacy XContent rendering
private final boolean renderLegacyXContent;
private final Map<String, SubGroup> subgroups;

public static SubGroup root(String name, boolean renderLegacyXContent) {
return new SubGroup(name, null, renderLegacyXContent, new HashMap<>());
public static SubGroup root(String name) {
return new SubGroup(name, null, new HashMap<>());
}

public SubGroup(String name, Long count, boolean renderLegacyXContent, Map<String, SubGroup> subgroups) {
public SubGroup(String name, Long count, Map<String, SubGroup> subgroups) {
this.name = name;
this.count = count;
this.renderLegacyXContent = renderLegacyXContent;
this.subgroups = subgroups;
}

public SubGroup addCount(String name, long count) {
if (this.subgroups.containsKey(name) == false) {
this.subgroups.put(name, new SubGroup(name, count, renderLegacyXContent, new HashMap<>()));
this.subgroups.put(name, new SubGroup(name, count, new HashMap<>()));
} else {
SubGroup s = this.subgroups.get(name);
s.count += count;
Expand All @@ -46,7 +44,7 @@ public SubGroup addCount(String name, long count) {

public SubGroup getOrAddChild(String name) {
if (subgroups.containsKey(name) == false) {
this.subgroups.put(name, new SubGroup(name, null, renderLegacyXContent, new HashMap<>()));
this.subgroups.put(name, new SubGroup(name, null, new HashMap<>()));
}
return this.subgroups.get(name);
}
Expand All @@ -65,32 +63,22 @@ public SubGroup copy() {
for (Map.Entry<String, SubGroup> subGroup : subgroups.entrySet()) {
copy.put(subGroup.getKey(), subGroup.getValue().copy());
}
return new SubGroup(name, count, renderLegacyXContent, copy);
return new SubGroup(name, count, copy);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
if (renderLegacyXContent) {
// This assumes that we only have one level of sub groups
if (subgroups != null && subgroups.isEmpty() == false) {
for (SubGroup subgroup : subgroups.values()) {
builder.field(subgroup.name, subgroup.count);
}
}
return builder;
} else {
builder.startObject(name);
// only the root node has no count
if (count != null) {
builder.field("count", count);
}
if (subgroups != null && subgroups.isEmpty() == false) {
for (SubGroup subgroup : subgroups.values()) {
subgroup.toXContent(builder, params);
}
builder.startObject(name);
// only the root node has no count
if (count != null) {
builder.field("count", count);
}
if (subgroups != null && subgroups.isEmpty() == false) {
for (SubGroup subgroup : subgroups.values()) {
subgroup.toXContent(builder, params);
}
return builder.endObject();
}
return builder.endObject();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,15 @@ public final class SubGroupCollector {
private static final Logger log = LogManager.getLogger(SubGroupCollector.class);

private final String[] aggregationFields;
private final boolean legacyAggregationField;

public static SubGroupCollector attach(
AbstractAggregationBuilder<?> parentAggregation,
String[] aggregationFields,
boolean legacyAggregationField
) {
SubGroupCollector c = new SubGroupCollector(aggregationFields, legacyAggregationField);

public static SubGroupCollector attach(AbstractAggregationBuilder<?> parentAggregation, String[] aggregationFields) {
SubGroupCollector c = new SubGroupCollector(aggregationFields);
c.addAggregations(parentAggregation);
return c;
}

private SubGroupCollector(String[] aggregationFields, boolean legacyAggregationField) {
private SubGroupCollector(String[] aggregationFields) {
this.aggregationFields = aggregationFields;
this.legacyAggregationField = legacyAggregationField;
}

private boolean hasAggregationFields() {
Expand Down Expand Up @@ -68,7 +62,7 @@ void collectResults(MultiBucketsAggregation.Bucket bucket, TraceEvent event) {
void collectResults(Bucket bucket, TraceEvent event) {
if (hasAggregationFields()) {
if (event.subGroups == null) {
event.subGroups = SubGroup.root(aggregationFields[0], legacyAggregationField);
event.subGroups = SubGroup.root(aggregationFields[0]);
}
collectInternal(bucket.getAggregations(), event.subGroups, 0);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,7 @@ private void searchGenericEventGroupedByStackTrace(
CountedTermsAggregationBuilder groupByStackTraceId = new CountedTermsAggregationBuilder("group_by").size(
MAX_TRACE_EVENTS_RESULT_SIZE
).field(request.getStackTraceIdsField());
SubGroupCollector subGroups = SubGroupCollector.attach(
groupByStackTraceId,
request.getAggregationFields(),
request.isLegacyAggregationField()
);
SubGroupCollector subGroups = SubGroupCollector.attach(groupByStackTraceId, request.getAggregationFields());
RandomSamplerAggregationBuilder randomSampler = new RandomSamplerAggregationBuilder("sample").setSeed(request.hashCode())
.setProbability(responseBuilder.getSamplingRate())
.subAggregation(groupByStackTraceId);
Expand Down Expand Up @@ -326,11 +322,7 @@ private void searchEventGroupedByStackTrace(
// Especially with high cardinality fields, this makes aggregations really slow.
.executionHint("map")
.subAggregation(new SumAggregationBuilder("count").field("Stacktrace.count"));
SubGroupCollector subGroups = SubGroupCollector.attach(
groupByStackTraceId,
request.getAggregationFields(),
request.isLegacyAggregationField()
);
SubGroupCollector subGroups = SubGroupCollector.attach(groupByStackTraceId, request.getAggregationFields());
client.prepareSearch(eventsIndex.getName())
.setTrackTotalHits(false)
.setSize(0)
Expand Down
Loading

0 comments on commit b2f290f

Please sign in to comment.