-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Profiling] Allow wildcards for custom indices #105372
Conversation
Several profiling APIs allow to specify custom event indices with the `indices` parameter. So far this parameter has allowed only one index and also disallowed wildcards. With this commit we extend this parameter to support multiple indices and each index can use wildcards.
Pinging @elastic/obs-knowledge-team (Team:obs-knowledge) |
Note: I'm labeling this as non-issue as the respective API is exclusive to Kibana and it is not yet in use. |
@@ -32,7 +35,7 @@ | |||
/** | |||
* A request to get profiling details | |||
*/ | |||
public class GetStackTracesRequest extends ActionRequest implements IndicesRequest { | |||
public class GetStackTracesRequest extends ActionRequest implements IndicesRequest.Replaceable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing IndicesRequest.Replaceable
indicates to infrastructure in Elasticsearch that we want to support wildcards, see also
Lines 136 to 146 in 510c851
private static boolean requiresWildcardExpansion(IndicesRequest indicesRequest) { | |
// IndicesAliasesRequest requires special handling because it can have wildcards in request body | |
if (indicesRequest instanceof IndicesAliasesRequest) { | |
return true; | |
} | |
// Replaceable requests always require wildcard expansion | |
if (indicesRequest instanceof IndicesRequest.Replaceable) { | |
return true; | |
} | |
return false; | |
} |
); | ||
} | ||
} | ||
return indices.toArray(new String[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the String array be pre-allocated with indices.size()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually the preferred style in Java, see also https://github.com/JetBrains/intellij-community/blob/master/java/java-impl/src/inspectionDescriptions/ToArrayCallWithZeroLengthArrayArgument.html for the rationale.
@@ -330,7 +350,7 @@ public String[] indices() { | |||
if (this.indices == null) { | |||
indices.addAll(EventsIndex.indexNames()); | |||
} else { | |||
indices.add(this.indices); | |||
indices.addAll(List.of(this.indices)); | |||
} | |||
return indices.toArray(new String[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use pre-allocation here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually the preferred style in Java, see also https://github.com/JetBrains/intellij-community/blob/master/java/java-impl/src/inspectionDescriptions/ToArrayCallWithZeroLengthArrayArgument.html for the rationale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (left some minor comments)
As we set indices now internally in all cases due to wildcard resolution, we need a means to distinguish whether `indices` have been provided originally via the API or whether Elasticsearch has set them internally after wildcard resolution.
Several profiling APIs allow to specify custom event indices with the
indices
parameter. So far this parameter has allowed only one index and also disallowed wildcards. With this commit we extend this parameter to support multiple indices and each index can use wildcards.