Skip to content
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

Do not request "search_pipelines" metrics by default in NodesInfoRequest #12497

Merged
merged 1 commit into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Add task completion count in search backpressure stats API ([#10028](https://github.com/opensearch-project/OpenSearch/pull/10028/))
- Deprecate CamelCase `PathHierarchy` tokenizer name in favor to lowercase `path_hierarchy` ([#10894](https://github.com/opensearch-project/OpenSearch/pull/10894))
- Switched to more reliable OpenSearch Lucene snapshot location([#11728](https://github.com/opensearch-project/OpenSearch/pull/11728))
- Breaking change: Do not request "search_pipelines" metrics by default in NodesInfoRequest ([#12497](https://github.com/opensearch-project/OpenSearch/pull/12497))

### Deprecated

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
@PublicApi(since = "1.0.0")
public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {

private Set<String> requestedMetrics = Metric.allMetrics();
private Set<String> requestedMetrics = Metric.defaultMetrics();

/**
* Create a new NodeInfoRequest from a {@link StreamInput} object.
Expand All @@ -73,7 +73,7 @@ public NodesInfoRequest(StreamInput in) throws IOException {
*/
public NodesInfoRequest(String... nodesIds) {
super(nodesIds);
all();
defaultMetrics();
}

/**
Expand All @@ -85,13 +85,24 @@ public NodesInfoRequest clear() {
}

/**
* Sets to return all the data.
* Sets to return data for all the metrics.
* See {@link Metric}
*/
public NodesInfoRequest all() {
requestedMetrics.addAll(Metric.allMetrics());
return this;
}

/**
* Sets to return data for default metrics only.
* See {@link Metric}
* See {@link Metric#defaultMetrics()}.
*/
public NodesInfoRequest defaultMetrics() {
requestedMetrics.addAll(Metric.defaultMetrics());
return this;
}

/**
* Get the names of requested metrics
*/
Expand Down Expand Up @@ -156,7 +167,7 @@ public void writeTo(StreamOutput out) throws IOException {

/**
* An enumeration of the "core" sections of metrics that may be requested
* from the nodes information endpoint. Eventually this list list will be
* from the nodes information endpoint. Eventually this list will be
* pluggable.
*/
public enum Metric {
Expand Down Expand Up @@ -187,8 +198,25 @@ boolean containedIn(Set<String> metricNames) {
return metricNames.contains(this.metricName());
}

/**
* Return all available metrics.
* See {@link Metric}
*/
public static Set<String> allMetrics() {
return Arrays.stream(values()).map(Metric::metricName).collect(Collectors.toSet());
}

/**
* Return "the default" set of metrics.
* Similar to {@link #allMetrics()} except {@link Metric#SEARCH_PIPELINES} metric is not included.
* <br>
* The motivation to define the default set of metrics was to keep the default response
* size at bay. Metrics that are NOT included in the default set were typically introduced later
* and are considered to contain specific type of information that is not usually useful unless you
* know that you really need it.
*/
public static Set<String> defaultMetrics() {
return allMetrics().stream().filter(metric -> !(metric.equals(SEARCH_PIPELINES.metricName()))).collect(Collectors.toSet());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
ClusterState state,
ActionListener<AcknowledgedResponse> listener
) throws Exception {
NodesInfoRequest nodesInfoRequest = new NodesInfoRequest();
NodesInfoRequest nodesInfoRequest = new NodesInfoRequest().clear().addMetric(NodesInfoRequest.Metric.SEARCH_PIPELINES.metricName());

Check warning on line 85 in server/src/main/java/org/opensearch/action/search/PutSearchPipelineTransportAction.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/search/PutSearchPipelineTransportAction.java#L85

Added line #L85 was not covered by tests
client.admin().cluster().nodesInfo(nodesInfoRequest, ActionListener.wrap(nodeInfos -> {
Map<DiscoveryNode, SearchPipelineInfo> searchPipelineInfos = new HashMap<>();
for (NodeInfo nodeInfo : nodeInfos.getNodes()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,15 +86,18 @@ public void testRemoveSingleMetric() throws Exception {
}

/**
* Test that a newly constructed NodesInfoRequestObject requests all of the
* possible metrics defined in {@link NodesInfoRequest.Metric}.
* Test that a newly constructed NodesInfoRequestObject does not request all the
* possible metrics defined in {@link NodesInfoRequest.Metric} but only the default metrics
* according to {@link NodesInfoRequest.Metric#defaultMetrics()}.
*/
public void testNodesInfoRequestDefaults() {
NodesInfoRequest defaultNodesInfoRequest = new NodesInfoRequest(randomAlphaOfLength(8));
NodesInfoRequest allMetricsNodesInfoRequest = new NodesInfoRequest(randomAlphaOfLength(8));
allMetricsNodesInfoRequest.all();
NodesInfoRequest requestOOTB = new NodesInfoRequest(randomAlphaOfLength(8));
NodesInfoRequest requestAll = new NodesInfoRequest(randomAlphaOfLength(8)).all();
NodesInfoRequest requestDefault = new NodesInfoRequest(randomAlphaOfLength(8)).defaultMetrics();

assertThat(defaultNodesInfoRequest.requestedMetrics(), equalTo(allMetricsNodesInfoRequest.requestedMetrics()));
assertTrue(requestAll.requestedMetrics().size() > requestOOTB.requestedMetrics().size());
assertTrue(requestDefault.requestedMetrics().size() == requestOOTB.requestedMetrics().size());
assertThat(requestOOTB.requestedMetrics(), equalTo(requestDefault.requestedMetrics()));
}

/**
Expand All @@ -107,6 +110,21 @@ public void testNodesInfoRequestAll() throws Exception {
assertThat(request.requestedMetrics(), equalTo(NodesInfoRequest.Metric.allMetrics()));
}

/**
* Test that the {@link NodesInfoRequest#defaultMetrics()} method enables default metrics.
*/
public void testNodesInfoRequestDefault() {
NodesInfoRequest request = new NodesInfoRequest("node");
request.defaultMetrics();

assertEquals(11, request.requestedMetrics().size());
assertThat(request.requestedMetrics(), equalTo(NodesInfoRequest.Metric.defaultMetrics()));
assertTrue(request.requestedMetrics().contains(NodesInfoRequest.Metric.JVM.metricName()));
assertTrue(request.requestedMetrics().contains(NodesInfoRequest.Metric.AGGREGATIONS.metricName()));
// search_pipelines metrics are not included
assertFalse(request.requestedMetrics().contains(NodesInfoRequest.Metric.SEARCH_PIPELINES.metricName()));
}

/**
* Test that the {@link NodesInfoRequest#clear()} method disables all metrics.
*/
Expand Down
Loading