[Monitoring] Remove support for setting interval -1 (elastic/x-pack-elasticsearch#4035)

This removes the ability to set `xpack.monitoring.collection.interval` to
`-1`.

Original commit: elastic/x-pack-elasticsearch@60f2db4bd1

docs/en/release-notes/7.0.0-alpha1.asciidoc

@@ -9,6 +9,11 @@ Machine Learning::
 * The `max_running_jobs` node property is removed in this release. Use the
 `xpack.ml.max_open_jobs` setting instead. For more information, see <<ml-settings>>.
 
+Monitoring::
+* The `xpack.monitoring.collection.interval` setting can no longer be set to `-1`
+to disable monitoring data collection. Use `xpack.monitoring.collection.enabled`
+and set it to `false` (its default), which was added in 6.3.0.
+
 Security::
 * The fields returned as part of the mappings section by get index, get
 mappings, get field mappings and field capabilities API are now only the

docs/en/settings/monitoring-settings.asciidoc

@@ -41,6 +41,19 @@ Enable the collection of monitoring data. Defaults to `false`. added[6.3.0]
 You can update this setting through the
 <<cluster-update-settings,Cluster Update Settings API>>.
 
+`xpack.monitoring.collection.interval`::
+
+Setting to `-1` to disable data collection is no longer supported beginning with
+7.0.0. deprecated[6.3.0, Use `xpack.monitoring.collection.enabled` set to
+`false` instead.]
++
+Controls how often data samples are collected. Defaults to `10s`. If you
+modify the collection interval, set the `xpack.monitoring.min_interval_seconds`
+option in `kibana.yml` to the same value.
++
+You can update this setting through the
+<<cluster-update-settings,Cluster Update Settings API>>.
+
 `xpack.monitoring.collection.cluster.stats.timeout`::
 
 Sets the timeout for collecting the cluster statistics. Defaults to `10s`.
@@ -73,18 +86,6 @@ collect only active recoveries. Defaults to `false`.
 
 Sets the timeout for collecting the recovery information. Defaults to `10s`.
 
-`xpack.monitoring.collection.interval`::
-
-Setting to `-1` to disable data collection has been deprecated. added[6.3.0,
-Use `xpack.monitoring.collection.enabled` set to `false` instead.]
-
-Controls how often data samples are collected. Defaults to `10s`. If you
-modify the collection interval, set the `xpack.monitoring.min_interval_seconds`
-option in `kibana.yml` to the same value.
-+
-You can update this setting through the
-<<cluster-update-settings,Cluster Update Settings API>>.
-
 `xpack.monitoring.history.duration`::
 
 Sets the retention duration beyond which the indices created by a Monitoring

plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/MonitoringService.java

@@ -5,14 +5,12 @@
  */
 package org.elasticsearch.xpack.monitoring;
 
-import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.logging.log4j.util.Supplier;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.component.AbstractLifecycleComponent;
-import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
@@ -40,24 +38,6 @@ import java.util.concurrent.atomic.AtomicBoolean;
  */
 public class MonitoringService extends AbstractLifecycleComponent {
 
-    /**
-     * Log a deprecation warning if {@code value} is -1.
-     * <p>
-     * This should be removed in 7.0.
-     *
-     * @param value The value being set for the collection interval.
-     */
-    private static void deprecateMinusOne(final TimeValue value) {
-        if (TimeValue.MINUS_ONE.equals(value)) {
-            final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(MonitoringService.class));
-
-            deprecationLogger.deprecated(
-                "Setting [xpack.monitoring.collection.interval] to [-1] has been deprecated as the way to disable collection. Use " +
-                "[xpack.monitoring.collection.enabled] set to [false] instead."
-            );
-        }
-    }
-
     /**
      * Minimum value for sampling interval (1 second)
      */
@@ -73,16 +53,8 @@ public class MonitoringService extends AbstractLifecycleComponent {
     /**
      * Sampling interval between two collections (default to 10s)
      */
-    public static final Setting<TimeValue> INTERVAL = new Setting<>("xpack.monitoring.collection.interval", "10s",
+    public static final Setting<TimeValue> INTERVAL =
-            (s) -> {
+            Setting.timeSetting("xpack.monitoring.collection.interval", TimeValue.timeValueSeconds(10), MIN_INTERVAL,
-                TimeValue value = TimeValue.parseTimeValue(s, null, "xpack.monitoring.collection.interval");
-                if (TimeValue.MINUS_ONE.equals(value) || value.millis() >= MIN_INTERVAL.millis()) {
-                    deprecateMinusOne(value);
-
-                    return value;
-                }
-                throw new IllegalArgumentException("Failed to parse monitoring interval [" + s + "], value must be >= " + MIN_INTERVAL);
-            },
                                 Setting.Property.Dynamic, Setting.Property.NodeScope);
 
     /** State of the monitoring service, either started or stopped **/
@@ -129,10 +101,7 @@ public class MonitoringService extends AbstractLifecycleComponent {
     }
 
     public boolean isMonitoringActive() {
-        return isStarted()
+        return isStarted() && enabled;
-                && enabled
-                && interval != null
-                && interval.millis() >= MIN_INTERVAL.millis();
     }
 
     private String threadPoolName() {

plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/MonitoringServiceTests.java

@@ -78,12 +78,6 @@ public class MonitoringServiceTests extends ESTestCase {
         monitoringService.setMonitoringActive(true);
         assertTrue(monitoringService.isMonitoringActive());
 
-        monitoringService.setInterval(TimeValue.MINUS_ONE);
-        assertFalse(monitoringService.isMonitoringActive());
-
-        monitoringService.setInterval(TimeValue.timeValueSeconds(10));
-        assertTrue(monitoringService.isMonitoringActive());
-
         monitoringService.stop();
         assertBusy(() -> assertFalse(monitoringService.isStarted()));
         assertFalse(monitoringService.isMonitoringActive());
@@ -100,31 +94,25 @@ public class MonitoringServiceTests extends ESTestCase {
     public void testInterval() throws Exception {
         final Settings settings =
                 Settings.builder()
-                        .put(MonitoringService.ENABLED.getKey(), true)
+                        .put("xpack.monitoring.collection.interval", MonitoringService.MIN_INTERVAL)
-                        .put(MonitoringService.INTERVAL.getKey(), TimeValue.MINUS_ONE)
                         .build();
 
         CountingExporter exporter = new CountingExporter();
         monitoringService = new MonitoringService(settings, clusterService, threadPool, emptySet(), exporter);
 
-        assertWarnings(
-            "Setting [xpack.monitoring.collection.interval] to [-1] has been deprecated as the way to disable collection. Use " +
-            "[xpack.monitoring.collection.enabled] set to [false] instead.");
-
         monitoringService.start();
         assertBusy(() -> assertTrue(monitoringService.isStarted()));
-        assertFalse("interval -1 does not start the monitoring execution", monitoringService.isMonitoringActive());
+        assertFalse("interval does not start the monitoring execution", monitoringService.isMonitoringActive());
         assertEquals(0, exporter.getExportsCount());
 
-        monitoringService.setInterval(TimeValue.timeValueSeconds(1));
+        monitoringService.setMonitoringActive(true);
         assertTrue(monitoringService.isMonitoringActive());
+
+        // now the interval should take place
         assertBusy(() -> assertThat(exporter.getExportsCount(), greaterThan(0)));
 
-        monitoringService.setInterval(TimeValue.timeValueMillis(100));
+        // take down threads
-        assertFalse(monitoringService.isMonitoringActive());
+        monitoringService.setMonitoringActive(false);
-
-        monitoringService.setInterval(TimeValue.MINUS_ONE);
-        assertFalse(monitoringService.isMonitoringActive());
     }
 
     public void testSkipExecution() throws Exception {
@@ -132,8 +120,8 @@ public class MonitoringServiceTests extends ESTestCase {
         final BlockingExporter exporter = new BlockingExporter(latch);
         final Settings settings =
                 Settings.builder()
-                        .put(MonitoringService.ENABLED.getKey(), true)
+                        .put("xpack.monitoring.collection.enabled", true)
-                        .put(MonitoringService.INTERVAL.getKey(), MonitoringService.MIN_INTERVAL)
+                        .put("xpack.monitoring.collection.interval", MonitoringService.MIN_INTERVAL)
                         .build();
 
         monitoringService = new MonitoringService(settings, clusterService, threadPool, emptySet(), exporter);