From 14204f8381d497c4e98069355ed4165aad6f6419 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Thu, 26 Mar 2020 14:41:49 -0400 Subject: [PATCH] Use set-based interface for NodesStatsRequest (#53637) (#54141) The NodesStatsRequest class uses a set of strings for its internal serialization. This commit updates the class's interface so that we no longer use hard-coded getters and setters, but rather methods that add strings directly. For example, the old way of adding "os" metrics to a request would be to call request.os(true). The new way of doing this is to call request.addMetric("os"). For the time being, the canonical list of metrics is an enum in NodesStatsRequest. This will eventually be replaced with something pluggable. --- build.gradle | 4 +- .../cluster/node/stats/NodesStatsRequest.java | 222 +++++------------- .../node/stats/NodesStatsRequestBuilder.java | 35 ++- .../node/stats/TransportNodesStatsAction.java | 19 +- .../cluster/InternalClusterInfoService.java | 2 +- .../admin/cluster/RestNodesStatsAction.java | 21 +- .../rest/action/cat/RestAllocationAction.java | 3 +- .../rest/action/cat/RestNodesAction.java | 8 +- .../rest/action/cat/RestThreadPoolAction.java | 2 +- .../node/stats/NodesStatsRequestTests.java | 126 +++++----- .../xpack/ml/MachineLearningFeatureSet.java | 3 +- .../TransportGetTrainedModelsStatsAction.java | 3 +- .../collector/node/NodeStatsCollector.java | 11 +- 13 files changed, 189 insertions(+), 270 deletions(-) diff --git a/build.gradle b/build.gradle index 2aa4d4c09f8..b7ebb891bf0 100644 --- a/build.gradle +++ b/build.gradle @@ -224,8 +224,8 @@ task verifyVersions { * after the backport of the backcompat code is complete. */ -boolean bwc_tests_enabled = true -final String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */ +boolean bwc_tests_enabled = false +final String bwc_tests_disabled_issue = "https://github.com/elastic/elasticsearch/pull/54141" /* place a PR link here when committing bwc changes */ if (bwc_tests_enabled == false) { if (bwc_tests_disabled_issue.isEmpty()) { throw new GradleException("bwc_tests_disabled_issue must be set when bwc_tests_enabled == false") diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java index 14c4d95793b..eefa1dfb297 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java @@ -24,11 +24,12 @@ import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags; import org.elasticsearch.action.support.nodes.BaseNodesRequest; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; - import java.io.IOException; import java.util.Arrays; import java.util.HashSet; import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; import java.util.stream.Collectors; /** @@ -49,19 +50,19 @@ public class NodesStatsRequest extends BaseNodesRequest { indices = new CommonStatsFlags(in); requestedMetrics.clear(); if (in.getVersion().before(Version.V_7_7_0)) { - addOrRemoveMetric(in.readBoolean(), Metric.OS.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.PROCESS.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.JVM.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.THREAD_POOL.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.FS.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.TRANSPORT.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.HTTP.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.BREAKER.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.SCRIPT.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.DISCOVERY.metricName()); - addOrRemoveMetric(in.readBoolean(), Metric.INGEST.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.OS.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.PROCESS.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.JVM.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.THREAD_POOL.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.FS.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.TRANSPORT.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.HTTP.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.BREAKER.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.SCRIPT.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.DISCOVERY.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.INGEST.metricName()); if (in.getVersion().onOrAfter(Version.V_6_1_0)) { - addOrRemoveMetric(in.readBoolean(), Metric.ADAPTIVE_SELECTION.metricName()); + optionallyAddMetric(in.readBoolean(), Metric.ADAPTIVE_SELECTION.metricName()); } } else { requestedMetrics.addAll(in.readStringList()); @@ -94,10 +95,21 @@ public class NodesStatsRequest extends BaseNodesRequest { return this; } + /** + * Get indices. Handles separately from other metrics because it may or + * may not have submetrics. + * @return flags indicating which indices stats to return + */ public CommonStatsFlags indices() { return indices; } + /** + * Set indices. Handles separately from other metrics because it may or + * may not involve submetrics. + * @param indices flags indicating which indices stats to return + * @return This object, for request chaining. + */ public NodesStatsRequest indices(CommonStatsFlags indices) { this.indices = indices; return this; @@ -116,178 +128,58 @@ public class NodesStatsRequest extends BaseNodesRequest { } /** - * Should the node OS be returned. + * Get the names of requested metrics, excluding indices, which are + * handled separately. */ - public boolean os() { - return Metric.OS.containedIn(requestedMetrics); + public Set requestedMetrics() { + return new HashSet<>(requestedMetrics); } /** - * Should the node OS be returned. + * Add metric */ - public NodesStatsRequest os(boolean os) { - addOrRemoveMetric(os, Metric.OS.metricName()); + public NodesStatsRequest addMetric(String metric) { + if (Metric.allMetrics().contains(metric) == false) { + throw new IllegalStateException("Used an illegal metric: " + metric); + } + requestedMetrics.add(metric); return this; } /** - * Should the node Process be returned. + * Add an array of metric names */ - public boolean process() { - return Metric.PROCESS.containedIn(requestedMetrics); - } - - /** - * Should the node Process be returned. - */ - public NodesStatsRequest process(boolean process) { - addOrRemoveMetric(process, Metric.PROCESS.metricName()); + public NodesStatsRequest addMetrics(String... metrics) { + // use sorted set for reliable ordering in error messages + SortedSet metricsSet = new TreeSet<>(Arrays.asList(metrics)); + if (Metric.allMetrics().containsAll(metricsSet) == false) { + metricsSet.removeAll(Metric.allMetrics()); + String plural = metricsSet.size() == 1 ? "" : "s"; + throw new IllegalStateException("Used illegal metric" + plural + ": " + metricsSet); + } + requestedMetrics.addAll(metricsSet); return this; } /** - * Should the node JVM be returned. + * Remove metric */ - public boolean jvm() { - return Metric.JVM.containedIn(requestedMetrics); - } - - /** - * Should the node JVM be returned. - */ - public NodesStatsRequest jvm(boolean jvm) { - addOrRemoveMetric(jvm, Metric.JVM.metricName()); + public NodesStatsRequest removeMetric(String metric) { + if (Metric.allMetrics().contains(metric) == false) { + throw new IllegalStateException("Used an illegal metric: " + metric); + } + requestedMetrics.remove(metric); return this; } /** - * Should the node Thread Pool be returned. - */ - public boolean threadPool() { - return Metric.THREAD_POOL.containedIn(requestedMetrics); - } - - /** - * Should the node Thread Pool be returned. - */ - public NodesStatsRequest threadPool(boolean threadPool) { - addOrRemoveMetric(threadPool, Metric.THREAD_POOL.metricName()); - return this; - } - - /** - * Should the node file system stats be returned. - */ - public boolean fs() { - return Metric.FS.containedIn(requestedMetrics); - } - - /** - * Should the node file system stats be returned. - */ - public NodesStatsRequest fs(boolean fs) { - addOrRemoveMetric(fs, Metric.FS.metricName()); - return this; - } - - /** - * Should the node Transport be returned. - */ - public boolean transport() { - return Metric.TRANSPORT.containedIn(requestedMetrics); - } - - /** - * Should the node Transport be returned. - */ - public NodesStatsRequest transport(boolean transport) { - addOrRemoveMetric(transport, Metric.TRANSPORT.metricName()); - return this; - } - - /** - * Should the node HTTP be returned. - */ - public boolean http() { - return Metric.HTTP.containedIn(requestedMetrics); - } - - /** - * Should the node HTTP be returned. - */ - public NodesStatsRequest http(boolean http) { - addOrRemoveMetric(http, Metric.HTTP.metricName()); - return this; - } - - public boolean breaker() { - return Metric.BREAKER.containedIn(requestedMetrics); - } - - /** - * Should the node's circuit breaker stats be returned. - */ - public NodesStatsRequest breaker(boolean breaker) { - addOrRemoveMetric(breaker, Metric.BREAKER.metricName()); - return this; - } - - public boolean script() { - return Metric.SCRIPT.containedIn(requestedMetrics); - } - - public NodesStatsRequest script(boolean script) { - addOrRemoveMetric(script, Metric.SCRIPT.metricName()); - return this; - } - - - public boolean discovery() { - return Metric.DISCOVERY.containedIn(requestedMetrics); - } - - /** - * Should the node's discovery stats be returned. - */ - public NodesStatsRequest discovery(boolean discovery) { - addOrRemoveMetric(discovery, Metric.DISCOVERY.metricName()); - return this; - } - - public boolean ingest() { - return Metric.INGEST.containedIn(requestedMetrics); - } - - /** - * Should ingest statistics be returned. - */ - public NodesStatsRequest ingest(boolean ingest) { - addOrRemoveMetric(ingest, Metric.INGEST.metricName()); - return this; - } - - public boolean adaptiveSelection() { - return Metric.ADAPTIVE_SELECTION.containedIn(requestedMetrics); - } - - /** - * Should adaptiveSelection statistics be returned. - */ - public NodesStatsRequest adaptiveSelection(boolean adaptiveSelection) { - addOrRemoveMetric(adaptiveSelection, Metric.ADAPTIVE_SELECTION.metricName()); - return this; - } - - /** - * Helper method for adding and removing metrics. + * Helper method for adding metrics during deserialization. * @param includeMetric Whether or not to include a metric. - * @param metricName Name of the metric to include or remove. + * @param metricName Name of the metric to add. */ - private void addOrRemoveMetric(boolean includeMetric, String metricName) { + private void optionallyAddMetric(boolean includeMetric, String metricName) { if (includeMetric) { requestedMetrics.add(metricName); - } else { - requestedMetrics.remove(metricName); } } @@ -319,7 +211,7 @@ public class NodesStatsRequest extends BaseNodesRequest { * An enumeration of the "core" sections of metrics that may be requested * from the nodes stats endpoint. Eventually this list will be pluggable. */ - private enum Metric { + public enum Metric { OS("os"), PROCESS("process"), JVM("jvm"), @@ -331,7 +223,7 @@ public class NodesStatsRequest extends BaseNodesRequest { SCRIPT("script"), DISCOVERY("discovery"), INGEST("ingest"), - ADAPTIVE_SELECTION("adaptiveSelection"); + ADAPTIVE_SELECTION("adaptive_selection"); private String metricName; @@ -339,7 +231,7 @@ public class NodesStatsRequest extends BaseNodesRequest { this.metricName = name; } - String metricName() { + public String metricName() { return this.metricName; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestBuilder.java index c5dfb77e6c4..c7144b3c4e7 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestBuilder.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestBuilder.java @@ -55,12 +55,12 @@ public class NodesStatsRequestBuilder } public NodesStatsRequestBuilder setBreaker(boolean breaker) { - request.breaker(breaker); + addOrRemoveMetric(breaker, NodesStatsRequest.Metric.BREAKER); return this; } public NodesStatsRequestBuilder setScript(boolean script) { - request.script(script); + addOrRemoveMetric(script, NodesStatsRequest.Metric.SCRIPT); return this; } @@ -76,7 +76,7 @@ public class NodesStatsRequestBuilder * Should the node OS stats be returned. */ public NodesStatsRequestBuilder setOs(boolean os) { - request.os(os); + addOrRemoveMetric(os, NodesStatsRequest.Metric.OS); return this; } @@ -84,7 +84,7 @@ public class NodesStatsRequestBuilder * Should the node OS stats be returned. */ public NodesStatsRequestBuilder setProcess(boolean process) { - request.process(process); + addOrRemoveMetric(process, NodesStatsRequest.Metric.PROCESS); return this; } @@ -92,7 +92,7 @@ public class NodesStatsRequestBuilder * Should the node JVM stats be returned. */ public NodesStatsRequestBuilder setJvm(boolean jvm) { - request.jvm(jvm); + addOrRemoveMetric(jvm, NodesStatsRequest.Metric.JVM); return this; } @@ -100,7 +100,7 @@ public class NodesStatsRequestBuilder * Should the node thread pool stats be returned. */ public NodesStatsRequestBuilder setThreadPool(boolean threadPool) { - request.threadPool(threadPool); + addOrRemoveMetric(threadPool, NodesStatsRequest.Metric.THREAD_POOL); return this; } @@ -108,7 +108,7 @@ public class NodesStatsRequestBuilder * Should the node file system stats be returned. */ public NodesStatsRequestBuilder setFs(boolean fs) { - request.fs(fs); + addOrRemoveMetric(fs, NodesStatsRequest.Metric.FS); return this; } @@ -116,7 +116,7 @@ public class NodesStatsRequestBuilder * Should the node Transport stats be returned. */ public NodesStatsRequestBuilder setTransport(boolean transport) { - request.transport(transport); + addOrRemoveMetric(transport, NodesStatsRequest.Metric.TRANSPORT); return this; } @@ -124,7 +124,7 @@ public class NodesStatsRequestBuilder * Should the node HTTP stats be returned. */ public NodesStatsRequestBuilder setHttp(boolean http) { - request.http(http); + addOrRemoveMetric(http, NodesStatsRequest.Metric.HTTP); return this; } @@ -132,7 +132,7 @@ public class NodesStatsRequestBuilder * Should the discovery stats be returned. */ public NodesStatsRequestBuilder setDiscovery(boolean discovery) { - request.discovery(discovery); + addOrRemoveMetric(discovery, NodesStatsRequest.Metric.DISCOVERY); return this; } @@ -140,13 +140,24 @@ public class NodesStatsRequestBuilder * Should ingest statistics be returned. */ public NodesStatsRequestBuilder setIngest(boolean ingest) { - request.ingest(ingest); + addOrRemoveMetric(ingest, NodesStatsRequest.Metric.INGEST); return this; } public NodesStatsRequestBuilder setAdaptiveSelection(boolean adaptiveSelection) { - request.adaptiveSelection(adaptiveSelection); + addOrRemoveMetric(adaptiveSelection, NodesStatsRequest.Metric.ADAPTIVE_SELECTION); return this; } + /** + * Helper method for adding metrics to a request + */ + private void addOrRemoveMetric(boolean includeMetric, NodesStatsRequest.Metric metric) { + if (includeMetric) { + request.addMetric(metric.metricName()); + } else { + request.removeMetric(metric.metricName()); + } + } + } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/TransportNodesStatsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/TransportNodesStatsAction.java index d2546813172..b5d16a8f5a7 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/TransportNodesStatsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/TransportNodesStatsAction.java @@ -33,6 +33,7 @@ import org.elasticsearch.transport.TransportService; import java.io.IOException; import java.util.List; +import java.util.Set; public class TransportNodesStatsAction extends TransportNodesAction metrics = request.requestedMetrics(); + return nodeService.stats( + request.indices(), + NodesStatsRequest.Metric.OS.containedIn(metrics), + NodesStatsRequest.Metric.PROCESS.containedIn(metrics), + NodesStatsRequest.Metric.JVM.containedIn(metrics), + NodesStatsRequest.Metric.THREAD_POOL.containedIn(metrics), + NodesStatsRequest.Metric.FS.containedIn(metrics), + NodesStatsRequest.Metric.TRANSPORT.containedIn(metrics), + NodesStatsRequest.Metric.HTTP.containedIn(metrics), + NodesStatsRequest.Metric.BREAKER.containedIn(metrics), + NodesStatsRequest.Metric.SCRIPT.containedIn(metrics), + NodesStatsRequest.Metric.DISCOVERY.containedIn(metrics), + NodesStatsRequest.Metric.INGEST.containedIn(metrics), + NodesStatsRequest.Metric.ADAPTIVE_SELECTION.containedIn(metrics)); } public static class NodeStatsRequest extends BaseNodeRequest { diff --git a/server/src/main/java/org/elasticsearch/cluster/InternalClusterInfoService.java b/server/src/main/java/org/elasticsearch/cluster/InternalClusterInfoService.java index 8c49c3de396..de25666178b 100644 --- a/server/src/main/java/org/elasticsearch/cluster/InternalClusterInfoService.java +++ b/server/src/main/java/org/elasticsearch/cluster/InternalClusterInfoService.java @@ -244,7 +244,7 @@ public class InternalClusterInfoService implements ClusterInfoService, LocalNode final CountDownLatch latch = new CountDownLatch(1); final NodesStatsRequest nodesStatsRequest = new NodesStatsRequest("data:true"); nodesStatsRequest.clear(); - nodesStatsRequest.fs(true); + nodesStatsRequest.addMetric(NodesStatsRequest.Metric.FS.metricName()); nodesStatsRequest.timeout(fetchTimeout); client.admin().cluster().nodesStats(nodesStatsRequest, new LatchedActionListener<>(listener, latch)); return latch; diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java index 73a3e735ead..50586be7b98 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java @@ -58,21 +58,12 @@ public class RestNodesStatsAction extends BaseRestHandler { static final Map> METRICS; static { - final Map> metrics = new HashMap<>(); - metrics.put("os", r -> r.os(true)); - metrics.put("jvm", r -> r.jvm(true)); - metrics.put("thread_pool", r -> r.threadPool(true)); - metrics.put("fs", r -> r.fs(true)); - metrics.put("transport", r -> r.transport(true)); - metrics.put("http", r -> r.http(true)); - metrics.put("indices", r -> r.indices(true)); - metrics.put("process", r -> r.process(true)); - metrics.put("breaker", r -> r.breaker(true)); - metrics.put("script", r -> r.script(true)); - metrics.put("discovery", r -> r.discovery(true)); - metrics.put("ingest", r -> r.ingest(true)); - metrics.put("adaptive_selection", r -> r.adaptiveSelection(true)); - METRICS = Collections.unmodifiableMap(metrics); + Map> map = new HashMap<>(); + for (NodesStatsRequest.Metric metric : NodesStatsRequest.Metric.values()) { + map.put(metric.metricName(), request -> request.addMetric(metric.metricName())); + } + map.put("indices", request -> request.indices(true)); + METRICS = Collections.unmodifiableMap(map); } static final Map> FLAGS; diff --git a/server/src/main/java/org/elasticsearch/rest/action/cat/RestAllocationAction.java b/server/src/main/java/org/elasticsearch/rest/action/cat/RestAllocationAction.java index 1c81d24c21e..9fbb88c2786 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestAllocationAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestAllocationAction.java @@ -75,7 +75,8 @@ public class RestAllocationAction extends AbstractCatAction { @Override public void processResponse(final ClusterStateResponse state) { NodesStatsRequest statsRequest = new NodesStatsRequest(nodes); - statsRequest.clear().fs(true).indices(new CommonStatsFlags(CommonStatsFlags.Flag.Store)); + statsRequest.clear().addMetric(NodesStatsRequest.Metric.FS.metricName()) + .indices(new CommonStatsFlags(CommonStatsFlags.Flag.Store)); client.admin().cluster().nodesStats(statsRequest, new RestResponseListener(channel) { @Override diff --git a/server/src/main/java/org/elasticsearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/elasticsearch/rest/action/cat/RestNodesAction.java index dde182d0df7..8b0aac11464 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestNodesAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestNodesAction.java @@ -113,7 +113,13 @@ public class RestNodesAction extends AbstractCatAction { @Override public void processResponse(final NodesInfoResponse nodesInfoResponse) { NodesStatsRequest nodesStatsRequest = new NodesStatsRequest(); - nodesStatsRequest.clear().jvm(true).os(true).fs(true).indices(true).process(true).script(true); + nodesStatsRequest.clear().indices(true).addMetrics( + NodesStatsRequest.Metric.JVM.metricName(), + NodesStatsRequest.Metric.OS.metricName(), + NodesStatsRequest.Metric.FS.metricName(), + NodesStatsRequest.Metric.PROCESS.metricName(), + NodesStatsRequest.Metric.SCRIPT.metricName() + ); client.admin().cluster().nodesStats(nodesStatsRequest, new RestResponseListener(channel) { @Override public RestResponse buildResponse(NodesStatsResponse nodesStatsResponse) throws Exception { diff --git a/server/src/main/java/org/elasticsearch/rest/action/cat/RestThreadPoolAction.java b/server/src/main/java/org/elasticsearch/rest/action/cat/RestThreadPoolAction.java index 368ec15e58c..ea2182ddfc2 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestThreadPoolAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestThreadPoolAction.java @@ -89,7 +89,7 @@ public class RestThreadPoolAction extends AbstractCatAction { @Override public void processResponse(final NodesInfoResponse nodesInfoResponse) { NodesStatsRequest nodesStatsRequest = new NodesStatsRequest(); - nodesStatsRequest.clear().threadPool(true); + nodesStatsRequest.clear().addMetric(NodesStatsRequest.Metric.THREAD_POOL.metricName()); client.admin().cluster().nodesStats(nodesStatsRequest, new RestResponseListener(channel) { @Override public RestResponse buildResponse(NodesStatsResponse nodesStatsResponse) throws Exception { diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestTests.java index edf33415523..d22648eece2 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestTests.java @@ -24,38 +24,54 @@ import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.test.ESTestCase; +import java.util.HashSet; +import java.util.Set; + +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.in; +import static org.hamcrest.Matchers.not; public class NodesStatsRequestTests extends ESTestCase { /** * Make sure that we can set, serialize, and deserialize arbitrary sets * of metrics. - * - * TODO: Use a looping construct rather than direct API calls */ - public void testMetricsSetters() throws Exception { + public void testAddMetrics() throws Exception { NodesStatsRequest request = new NodesStatsRequest(randomAlphaOfLength(8)); request.indices(randomFrom(CommonStatsFlags.ALL)); - request.os(randomBoolean()); - request.process(randomBoolean()); - request.jvm(randomBoolean()); - request.threadPool(randomBoolean()); - request.fs(randomBoolean()); - request.transport(randomBoolean()); - request.http(randomBoolean()); - request.breaker(randomBoolean()); - request.script(randomBoolean()); - request.discovery(randomBoolean()); - request.ingest(randomBoolean()); - request.adaptiveSelection(randomBoolean()); + String[] metrics = randomSubsetOf(NodesStatsRequest.Metric.allMetrics()).toArray(new String[0]); + request.addMetrics(metrics); NodesStatsRequest deserializedRequest = roundTripRequest(request); assertRequestsEqual(request, deserializedRequest); } /** - * Test that a newly constructed NodesStatsRequestObject requests all of the - * possible metrics defined in {@link NodesStatsRequest}. + * Check that we can add a metric. + */ + public void testAddSingleMetric() throws Exception { + NodesStatsRequest request = new NodesStatsRequest(); + request.addMetric(randomFrom(NodesStatsRequest.Metric.allMetrics())); + NodesStatsRequest deserializedRequest = roundTripRequest(request); + assertRequestsEqual(request, deserializedRequest); + } + + /** + * Check that we can remove a metric. + */ + public void testRemoveSingleMetric() throws Exception { + NodesStatsRequest request = new NodesStatsRequest(); + request.all(); + String metric = randomFrom(NodesStatsRequest.Metric.allMetrics()); + request.removeMetric(metric); + NodesStatsRequest deserializedRequest = roundTripRequest(request); + assertThat(request.requestedMetrics(), equalTo(deserializedRequest.requestedMetrics())); + assertThat(metric, not(in(request.requestedMetrics()))); + } + + /** + * Test that a newly constructed NodesStatsRequestObject requests only index metrics. */ public void testNodesStatsRequestDefaults() { NodesStatsRequest defaultNodesStatsRequest = new NodesStatsRequest(randomAlphaOfLength(8)); @@ -67,53 +83,51 @@ public class NodesStatsRequestTests extends ESTestCase { } /** - * Test that the {@link NodesStatsRequest#all()} method sets all of the - * metrics to {@code true}. - * - * TODO: Use a looping construct rather than direct API calls + * Test that the {@link NodesStatsRequest#all()} method enables all metrics. */ public void testNodesInfoRequestAll() throws Exception { NodesStatsRequest request = new NodesStatsRequest("node"); request.all(); assertThat(request.indices().getFlags(), equalTo(CommonStatsFlags.ALL.getFlags())); - assertTrue(request.os()); - assertTrue(request.process()); - assertTrue(request.jvm()); - assertTrue(request.threadPool()); - assertTrue(request.fs()); - assertTrue(request.transport()); - assertTrue(request.http()); - assertTrue(request.breaker()); - assertTrue(request.script()); - assertTrue(request.discovery()); - assertTrue(request.ingest()); - assertTrue(request.adaptiveSelection()); + assertThat(request.requestedMetrics(), equalTo(NodesStatsRequest.Metric.allMetrics())); } /** - * Test that the {@link NodesStatsRequest#clear()} method sets all of the - * metrics to {@code false}. - * - * TODO: Use a looping construct rather than direct API calls + * Test that the {@link NodesStatsRequest#clear()} method removes all metrics. */ public void testNodesInfoRequestClear() throws Exception { NodesStatsRequest request = new NodesStatsRequest("node"); request.clear(); assertThat(request.indices().getFlags(), equalTo(CommonStatsFlags.NONE.getFlags())); - assertFalse(request.os()); - assertFalse(request.process()); - assertFalse(request.jvm()); - assertFalse(request.threadPool()); - assertFalse(request.fs()); - assertFalse(request.transport()); - assertFalse(request.http()); - assertFalse(request.breaker()); - assertFalse(request.script()); - assertFalse(request.discovery()); - assertFalse(request.ingest()); - assertFalse(request.adaptiveSelection()); + assertThat(request.requestedMetrics(), empty()); + } + + /** + * Test that (for now) we can only add metrics from a set of known metrics. + */ + public void testUnknownMetricsRejected() { + String unknownMetric1 = "unknown_metric1"; + String unknownMetric2 = "unknown_metric2"; + Set unknownMetrics = new HashSet<>(); + unknownMetrics.add(unknownMetric1); + unknownMetrics.addAll(randomSubsetOf(NodesStatsRequest.Metric.allMetrics())); + + NodesStatsRequest request = new NodesStatsRequest(); + + IllegalStateException exception = expectThrows(IllegalStateException.class, () -> request.addMetric(unknownMetric1)); + assertThat(exception.getMessage(), equalTo("Used an illegal metric: " + unknownMetric1)); + + exception = expectThrows(IllegalStateException.class, () -> request.removeMetric(unknownMetric1)); + assertThat(exception.getMessage(), equalTo("Used an illegal metric: " + unknownMetric1)); + + exception = expectThrows(IllegalStateException.class, () -> request.addMetrics(unknownMetrics.toArray(new String[0]))); + assertThat(exception.getMessage(), equalTo("Used illegal metric: [" + unknownMetric1 + "]")); + + unknownMetrics.add(unknownMetric2); + exception = expectThrows(IllegalStateException.class, () -> request.addMetrics(unknownMetrics.toArray(new String[0]))); + assertThat(exception.getMessage(), equalTo("Used illegal metrics: [" + unknownMetric1 + ", " + unknownMetric2 + "]")); } /** @@ -131,19 +145,7 @@ public class NodesStatsRequestTests extends ESTestCase { } private static void assertRequestsEqual(NodesStatsRequest request1, NodesStatsRequest request2) { - // TODO: Use a looping construct rather than direct API calls assertThat(request1.indices().getFlags(), equalTo(request2.indices().getFlags())); - assertThat(request1.os(), equalTo(request2.os())); - assertThat(request1.process(), equalTo(request2.process())); - assertThat(request1.jvm(), equalTo(request2.jvm())); - assertThat(request1.threadPool(), equalTo(request2.threadPool())); - assertThat(request1.fs(), equalTo(request2.fs())); - assertThat(request1.transport(), equalTo(request2.transport())); - assertThat(request1.http(), equalTo(request2.http())); - assertThat(request1.breaker(), equalTo(request2.breaker())); - assertThat(request1.script(), equalTo(request2.script())); - assertThat(request1.discovery(), equalTo(request2.discovery())); - assertThat(request1.ingest(), equalTo(request2.ingest())); - assertThat(request1.adaptiveSelection(), equalTo(request2.adaptiveSelection())); + assertThat(request1.requestedMetrics(), equalTo(request2.requestedMetrics())); } } diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSet.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSet.java index 620e08d0779..848c7f75212 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSet.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSet.java @@ -370,7 +370,8 @@ public class MachineLearningFeatureSet implements XPackFeatureSet { response -> { addDataFrameAnalyticsUsage(response, analyticsUsage); String[] ingestNodes = ingestNodes(state); - NodesStatsRequest nodesStatsRequest = new NodesStatsRequest(ingestNodes).clear().ingest(true); + NodesStatsRequest nodesStatsRequest = + new NodesStatsRequest(ingestNodes).clear().addMetric(NodesStatsRequest.Metric.INGEST.metricName()); client.execute(NodesStatsAction.INSTANCE, nodesStatsRequest, nodesStatsListener); }, listener::onFailure diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetTrainedModelsStatsAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetTrainedModelsStatsAction.java index 33678ab9089..6d1268ab3a8 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetTrainedModelsStatsAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetTrainedModelsStatsAction.java @@ -89,7 +89,8 @@ public class TransportGetTrainedModelsStatsAction extends HandledTransportAction responseBuilder.setExpandedIds(tuple.v2()) .setTotalModelCount(tuple.v1()); String[] ingestNodes = ingestNodes(clusterService.state()); - NodesStatsRequest nodesStatsRequest = new NodesStatsRequest(ingestNodes).clear().ingest(true); + NodesStatsRequest nodesStatsRequest = new NodesStatsRequest(ingestNodes).clear() + .addMetric(NodesStatsRequest.Metric.INGEST.metricName()); executeAsyncWithOrigin(client, ML_ORIGIN, NodesStatsAction.INSTANCE, nodesStatsRequest, nodesStatsListener); }, listener::onFailure diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/node/NodeStatsCollector.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/node/NodeStatsCollector.java index bc816cb9d9a..0354ecf4347 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/node/NodeStatsCollector.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/node/NodeStatsCollector.java @@ -67,11 +67,12 @@ public class NodeStatsCollector extends Collector { final ClusterState clusterState) throws Exception { NodesStatsRequest request = new NodesStatsRequest("_local"); request.indices(FLAGS); - request.os(true); - request.jvm(true); - request.process(true); - request.threadPool(true); - request.fs(true); + request.addMetrics( + NodesStatsRequest.Metric.OS.metricName(), + NodesStatsRequest.Metric.JVM.metricName(), + NodesStatsRequest.Metric.PROCESS.metricName(), + NodesStatsRequest.Metric.THREAD_POOL.metricName(), + NodesStatsRequest.Metric.FS.metricName()); final NodesStatsResponse response = client.admin().cluster().nodesStats(request).actionGet(getCollectionTimeout());