[Monitoring] Disable Monitoring Collection by Default (elastic/x-pack-elasticsearch#3962)

This adds a new setting, `xpack.monitoring.collection.enabled`, and
disables it by default (`false`).

Original commit: elastic/x-pack-elasticsearch@4b3a5a1161

docs/en/settings/monitoring-settings.asciidoc

@@ -5,8 +5,11 @@
 <titleabbrev>Monitoring Settings</titleabbrev>
 ++++
 
-Monitoring is enabled by default when you install {xpack}. You can configure
+By default when you install {xpack}, monitoring is enabled but data collection
-these monitoring settings in the `elasticsearch.yml` file.
+is disabled. To enable data collection, use the
+`xpack.monitoring.collection.enabled` setting.
+
+You can configure these monitoring settings in the `elasticsearch.yml` file.
 
 To adjust how monitoring data is displayed in the monitoring UI, configure
 {kibana-ref}/monitoring-settings-kb.html[`xpack.monitoring` settings] in
@@ -31,6 +34,13 @@ Set to `false` to disable {es} {monitoring} for Elasticsearch on the node.
 The `xpack.monitoring.collection` settings control how data is collected from
 your Elasticsearch nodes.
 
+`xpack.monitoring.collection.enabled`::
+
+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.cluster.stats.timeout`::
 
 Sets the timeout for collecting the cluster statistics. Defaults to `10s`.
@@ -43,7 +53,8 @@ example `test*`. You can explicitly include or exclude indices by prepending
 `+` to include the index, or `-` to exclude the index. For example, to include all indices that
 start with `test` except `test3`, you could specify `+test*,-test3`.
 +
-You can update this setting through the Cluster Update Settings API.
+You can update this setting through the
+<<cluster-update-settings,Cluster Update Settings API>>.
 
 `xpack.monitoring.collection.index.stats.timeout`::
 
@@ -67,8 +78,8 @@ Sets the timeout for collecting the recovery information. Defaults to `10s`.
 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. Set to `-1` to temporarily disable
-data collection. You can update this setting through the Cluster Update
+data collection. You can update this setting through the
-Settings API.
+<<cluster-update-settings,Cluster Update Settings API>>.
 
 `xpack.monitoring.history.duration`::
 

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

@@ -170,6 +170,7 @@ public class Monitoring extends Plugin implements ActionPlugin {
         List<Setting<?>> settings = new ArrayList<>();
         settings.add(MonitoringField.HISTORY_DURATION);
         settings.add(CLEAN_WATCHER_HISTORY);
+        settings.add(MonitoringService.ENABLED);
         settings.add(MonitoringService.INTERVAL);
         settings.add(Collector.INDICES);
         settings.add(ClusterStatsCollector.CLUSTER_STATS_TIMEOUT);

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

@@ -41,7 +41,14 @@ public class MonitoringService extends AbstractLifecycleComponent {
     /**
      * Minimum value for sampling interval (1 second)
      */
-    static final TimeValue MIN_INTERVAL = TimeValue.timeValueSeconds(1L);
+    public static final TimeValue MIN_INTERVAL = TimeValue.timeValueSeconds(1L);
+
+    /**
+     * Dynamically controls enabling or disabling the collection of Monitoring data.
+     */
+    public static final Setting<Boolean> ENABLED =
+            Setting.boolSetting("xpack.monitoring.collection.enabled", false,
+                                Setting.Property.Dynamic, Setting.Property.NodeScope);
 
     /**
      * Sampling interval between two collections (default to 10s)
@@ -67,6 +74,7 @@ public class MonitoringService extends AbstractLifecycleComponent {
     private final Set<Collector> collectors;
     private final Exporters exporters;
 
+    private volatile boolean enabled;
     private volatile TimeValue interval;
     private volatile ThreadPool.Cancellable scheduler;
 
@@ -77,11 +85,19 @@ public class MonitoringService extends AbstractLifecycleComponent {
         this.threadPool = Objects.requireNonNull(threadPool);
         this.collectors = Objects.requireNonNull(collectors);
         this.exporters = Objects.requireNonNull(exporters);
+        this.enabled = ENABLED.get(settings);
         this.interval = INTERVAL.get(settings);
+
+        clusterService.getClusterSettings().addSettingsUpdateConsumer(ENABLED, this::setMonitoringActive);
         clusterService.getClusterSettings().addSettingsUpdateConsumer(INTERVAL, this::setInterval);
     }
 
-    void setInterval(TimeValue interval) {
+    void setMonitoringActive(final boolean enabled) {
+        this.enabled = enabled;
+        scheduleExecution();
+    }
+
+    void setInterval(final TimeValue interval) {
         this.interval = interval;
         scheduleExecution();
     }
@@ -92,6 +108,7 @@ public class MonitoringService extends AbstractLifecycleComponent {
 
     public boolean isMonitoringActive() {
         return isStarted()
+                && enabled
                 && interval != null
                 && interval.millis() >= MIN_INTERVAL.millis();
     }

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

@@ -73,6 +73,15 @@ public class MonitoringServiceTests extends ESTestCase {
 
         monitoringService.start();
         assertBusy(() -> assertTrue(monitoringService.isStarted()));
+        assertFalse(monitoringService.isMonitoringActive());
+
+        monitoringService.setMonitoringActive(true);
+        assertTrue(monitoringService.isMonitoringActive());
+
+        monitoringService.setInterval(TimeValue.MINUS_ONE);
+        assertFalse(monitoringService.isMonitoringActive());
+
+        monitoringService.setInterval(TimeValue.timeValueSeconds(10));
         assertTrue(monitoringService.isMonitoringActive());
 
         monitoringService.stop();
@@ -89,7 +98,11 @@ public class MonitoringServiceTests extends ESTestCase {
     }
 
     public void testInterval() throws Exception {
-        Settings settings = Settings.builder().put(MonitoringService.INTERVAL.getKey(), TimeValue.MINUS_ONE).build();
+        final Settings settings =
+                Settings.builder()
+                        .put(MonitoringService.ENABLED.getKey(), true)
+                        .put(MonitoringService.INTERVAL.getKey(), TimeValue.MINUS_ONE)
+                        .build();
 
         CountingExporter exporter = new CountingExporter();
         monitoringService = new MonitoringService(settings, clusterService, threadPool, emptySet(), exporter);
@@ -113,8 +126,12 @@ public class MonitoringServiceTests extends ESTestCase {
     public void testSkipExecution() throws Exception {
         final CountDownLatch latch = new CountDownLatch(1);
         final BlockingExporter exporter = new BlockingExporter(latch);
+        final Settings settings =
+                Settings.builder()
+                        .put(MonitoringService.ENABLED.getKey(), true)
+                        .put(MonitoringService.INTERVAL.getKey(), MonitoringService.MIN_INTERVAL)
+                        .build();
 
-        Settings settings = Settings.builder().put(MonitoringService.INTERVAL.getKey(), MonitoringService.MIN_INTERVAL).build();
         monitoringService = new MonitoringService(settings, clusterService, threadPool, emptySet(), exporter);
 
         monitoringService.start();

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

@@ -27,22 +27,18 @@ import static org.hamcrest.Matchers.instanceOf;
 
 @ClusterScope(scope = Scope.TEST, numDataNodes = 0, numClientNodes = 0, transportClientRatio = 0.0)
 public class MultiNodesStatsTests extends MonitoringIntegTestCase {
-    public MultiNodesStatsTests() throws Exception {
-        super();
-    }
 
     @Override
     protected Settings nodeSettings(int nodeOrdinal) {
         return Settings.builder()
                 .put(super.nodeSettings(nodeOrdinal))
-                .put(MonitoringService.INTERVAL.getKey(), "-1")
                 .put("xpack.monitoring.exporters.default_local.type", "local")
                 .build();
     }
 
     @After
     public void cleanup() throws Exception {
-        disableMonitoringInterval();
+        disableMonitoringCollection();
         wipeMonitoringIndices();
     }
 
@@ -60,7 +56,9 @@ public class MultiNodesStatsTests extends MonitoringIntegTestCase {
 
         n = randomIntBetween(1, 2);
         internalCluster().startNodes(n,
-                Settings.builder().put(Node.NODE_DATA_SETTING.getKey(), false).put(Node.NODE_MASTER_SETTING.getKey(), false)
+                Settings.builder()
+                        .put(Node.NODE_DATA_SETTING.getKey(), false)
+                        .put(Node.NODE_MASTER_SETTING.getKey(), false)
                         .put(Node.NODE_INGEST_SETTING.getKey(), false).build());
         nodes += n;
 
@@ -77,7 +75,7 @@ public class MultiNodesStatsTests extends MonitoringIntegTestCase {
             assertNoTimeout(client().admin().cluster().prepareHealth().setWaitForNodes(Integer.toString(nbNodes)).get());
         });
 
-        updateMonitoringInterval(3L, TimeUnit.SECONDS);
+        enableMonitoringCollection();
         waitForMonitoringIndices();
 
         assertBusy(() -> {

plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTestCase.java

@@ -27,10 +27,6 @@ public abstract class LocalExporterIntegTestCase extends MonitoringIntegTestCase
 
     private static ThreadPool THREADPOOL;
 
-    public LocalExporterIntegTestCase() throws Exception {
-        super();
-    }
-
     @BeforeClass
     public static void setupThreadPool() {
         THREADPOOL = new TestThreadPool(LocalExporterIntegTestCase.class.getName());
@@ -45,7 +41,8 @@ public abstract class LocalExporterIntegTestCase extends MonitoringIntegTestCase
 
     protected Settings localExporterSettings() {
         return Settings.builder()
-                       .put(MonitoringService.INTERVAL.getKey(), "-1")
+                       .put(MonitoringService.ENABLED.getKey(), false)
+                       .put(MonitoringService.INTERVAL.getKey(), "3s")
                        .put("xpack.monitoring.exporters." + exporterName + ".type", LocalExporter.TYPE)
                        .put("xpack.monitoring.exporters." + exporterName +  ".enabled", false)
                        .put("xpack.monitoring.exporters." + exporterName +  ".cluster_alerts.management.enabled", false)

plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java

@@ -56,26 +56,22 @@ import static org.elasticsearch.xpack.core.monitoring.exporter.MonitoringTemplat
 import static org.elasticsearch.xpack.core.monitoring.exporter.MonitoringTemplateUtils.TEMPLATE_VERSION;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.lessThanOrEqualTo;
 
 @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.SUITE,
                               numDataNodes = 1, numClientNodes = 0, transportClientRatio = 0.0, supportsDedicatedMasters = false)
 public class LocalExporterIntegTests extends LocalExporterIntegTestCase {
     private final String indexTimeFormat = randomFrom("YY", "YYYY", "YYYY.MM", "YYYY-MM", "MM.YYYY", "MM", null);
 
-    public LocalExporterIntegTests() throws Exception {
-        super();
-    }
-
     private void stopMonitoring() {
         // Now disabling the monitoring service, so that no more collection are started
         assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(
-                Settings.builder().putNull(MonitoringService.INTERVAL.getKey())
+                Settings.builder().putNull(MonitoringService.ENABLED.getKey())
                                   .putNull("xpack.monitoring.exporters._local.enabled")
                                   .putNull("xpack.monitoring.exporters._local.cluster_alerts.management.enabled")
                                   .putNull("xpack.monitoring.exporters._local.index.name.time_format")));
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/x-pack-elasticsearch/issues/3954")
     public void testExport() throws Exception {
         try {
             if (randomBoolean()) {
@@ -90,7 +86,7 @@ public class LocalExporterIntegTests extends LocalExporterIntegTestCase {
 
             // start the monitoring service so that _xpack/monitoring/_bulk is not ignored
             final Settings.Builder exporterSettings = Settings.builder()
-                    .put(MonitoringService.INTERVAL.getKey(), 3L, TimeUnit.SECONDS)
+                    .put(MonitoringService.ENABLED.getKey(), true)
                     .put("xpack.monitoring.exporters._local.enabled", true)
                     .put("xpack.monitoring.exporters._local.cluster_alerts.management.enabled", false);
 
@@ -119,7 +115,7 @@ public class LocalExporterIntegTests extends LocalExporterIntegTestCase {
                     ensureYellowAndNoInitializingShards(".monitoring-*");
 
                     SearchResponse response = client().prepareSearch(".monitoring-*").get();
-                    assertEquals(nbDocs, response.getHits().getTotalHits());
+                    assertThat((long)nbDocs, lessThanOrEqualTo(response.getHits().getTotalHits()));
                 });
 
                 checkMonitoringTemplates();

plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java

@@ -41,6 +41,7 @@ import org.elasticsearch.xpack.core.monitoring.exporter.MonitoringTemplateUtils;
 import org.elasticsearch.xpack.core.monitoring.MonitoredSystem;
 import org.elasticsearch.xpack.core.monitoring.MonitoringFeatureSetUsage;
 import org.elasticsearch.xpack.monitoring.LocalStateMonitoring;
+import org.elasticsearch.xpack.monitoring.MonitoringService;
 import org.elasticsearch.xpack.monitoring.collector.cluster.ClusterStatsMonitoringDoc;
 import org.elasticsearch.xpack.monitoring.collector.indices.IndexRecoveryMonitoringDoc;
 import org.elasticsearch.xpack.monitoring.collector.indices.IndexStatsMonitoringDoc;
@@ -79,14 +80,12 @@ import static org.hamcrest.Matchers.nullValue;
 
 public class MonitoringIT extends ESSingleNodeTestCase {
 
-    private final TimeValue collectionInterval = TimeValue.timeValueSeconds(3);
-
     @Override
     protected Settings nodeSettings() {
         return Settings.builder()
                        .put(super.nodeSettings())
                        .put(XPackSettings.MACHINE_LEARNING_ENABLED.getKey(), false)
-                       .put("xpack.monitoring.collection.interval", "-1")
+                       .put("xpack.monitoring.collection.interval", MonitoringService.MIN_INTERVAL)
                        .put("xpack.monitoring.exporters._local.type", "local")
                        .put("xpack.monitoring.exporters._local.enabled", false)
                        .put("xpack.monitoring.exporters._local.cluster_alerts.management.enabled", false)
@@ -221,7 +220,7 @@ public class MonitoringIT extends ESSingleNodeTestCase {
                 final Map<String, Object> searchHit = toMap(hit);
                 final String type = (String) extractValue("_source.type", searchHit);
 
-                assertMonitoringDoc(searchHit, MonitoredSystem.ES, type, collectionInterval);
+                assertMonitoringDoc(searchHit, MonitoredSystem.ES, type, MonitoringService.MIN_INTERVAL);
 
                 if (ClusterStatsMonitoringDoc.TYPE.equals(type)) {
                     assertClusterStatsMonitoringDoc(searchHit, createAPMIndex);
@@ -499,7 +498,7 @@ public class MonitoringIT extends ESSingleNodeTestCase {
      */
     private void whenExportersAreReady(final CheckedRunnable<Exception> runnable) throws Exception {
         try {
-            enableMonitoring(collectionInterval);
+            enableMonitoring();
             runnable.run();
         } finally {
             disableMonitoring();
@@ -510,31 +509,22 @@ public class MonitoringIT extends ESSingleNodeTestCase {
      * Enable the monitoring service and the Local exporter, waiting for some monitoring documents
      * to be indexed before it returns.
      */
-    public void enableMonitoring(final TimeValue interval) throws Exception {
+    public void enableMonitoring() throws Exception {
         // delete anything that may happen to already exist
         assertAcked(client().admin().indices().prepareDelete(".monitoring-*").get());
 
-        final XPackUsageResponse usageResponse = new XPackUsageRequestBuilder(client()).execute().get();
+        assertThat("Must be no enabled exporters before enabling monitoring", getMonitoringUsageExporters().isEmpty(), is(true));
-        final Optional<MonitoringFeatureSetUsage> monitoringUsage =
-                usageResponse.getUsages()
-                             .stream()
-                             .filter(usage -> usage instanceof MonitoringFeatureSetUsage)
-                             .map(usage -> (MonitoringFeatureSetUsage)usage)
-                             .findFirst();
-        assertThat("Monitoring feature set does not exist", monitoringUsage.isPresent(), is(true));
-
-        final Map<String, Object> exporters = monitoringUsage.get().getExporters();
-
-        assertThat("List of enabled exporters must be empty before enabling monitoring", exporters.isEmpty(), is(true));
 
         final Settings settings = Settings.builder()
-                .put("xpack.monitoring.collection.interval", interval.getStringRep())
+                .put("xpack.monitoring.collection.enabled", true)
                 .put("xpack.monitoring.exporters._local.enabled", true)
                 .build();
 
         assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(settings));
 
         assertBusy(() -> {
+            assertThat("[_local] exporter not enabled yet", getMonitoringUsageExporters().isEmpty(), is(false));
+
             assertThat("No monitoring documents yet",
                        client().prepareSearch(".monitoring-es-" + TEMPLATE_VERSION + "-*")
                                .setSize(0)
@@ -549,26 +539,15 @@ public class MonitoringIT extends ESSingleNodeTestCase {
     @SuppressWarnings("unchecked")
     public void disableMonitoring() throws Exception {
         final Settings settings = Settings.builder()
-                .put("xpack.monitoring.collection.interval", (String) null)
+                .putNull("xpack.monitoring.collection.enabled")
-                .put("xpack.monitoring.exporters._local.enabled", (String) null)
+                .putNull("xpack.monitoring.exporters._local.enabled")
                 .build();
 
         assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(settings));
 
         assertBusy(() -> {
             try {
-                final XPackUsageResponse usageResponse = new XPackUsageRequestBuilder(client()).execute().get();
+                assertThat("Exporters are not yet stopped", getMonitoringUsageExporters().isEmpty(), is(true));
-                final Optional<MonitoringFeatureSetUsage> monitoringUsage =
-                        usageResponse.getUsages()
-                                .stream()
-                                .filter(usage -> usage instanceof MonitoringFeatureSetUsage)
-                                .map(usage -> (MonitoringFeatureSetUsage)usage)
-                                .findFirst();
-                assertThat("Monitoring feature set does not exist", monitoringUsage.isPresent(), is(true));
-
-                final Map<String, Object> exporters = monitoringUsage.get().getExporters();
-
-                assertThat("Exporters are not yet stopped", exporters.isEmpty(), is(true));
 
                 // now wait until Monitoring has actually stopped
                 final NodesStatsResponse response = client().admin().cluster().prepareNodesStats().clear().setThreadPool(true).get();
@@ -592,6 +571,20 @@ public class MonitoringIT extends ESSingleNodeTestCase {
         });
     }
 
+    private Map<String, Object> getMonitoringUsageExporters() throws Exception {
+        final XPackUsageResponse usageResponse = new XPackUsageRequestBuilder(client()).execute().get();
+        final Optional<MonitoringFeatureSetUsage> monitoringUsage =
+                usageResponse.getUsages()
+                        .stream()
+                        .filter(usage -> usage instanceof MonitoringFeatureSetUsage)
+                        .map(usage -> (MonitoringFeatureSetUsage)usage)
+                        .findFirst();
+
+        assertThat("Monitoring feature set does not exist", monitoringUsage.isPresent(), is(true));
+
+        return monitoringUsage.get().getExporters();
+    }
+
     /**
      * Returns the {@link SearchHit} content as a {@link Map} object.
      */

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

@@ -12,7 +12,6 @@ import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.regex.Regex;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.common.util.concurrent.CountDown;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.index.IndexNotFoundException;
@@ -49,22 +48,11 @@ public abstract class MonitoringIntegTestCase extends ESIntegTestCase {
     protected static final String MONITORING_INDICES_PREFIX = ".monitoring-";
     protected static final String ALL_MONITORING_INDICES = MONITORING_INDICES_PREFIX + "*";
 
-    public MonitoringIntegTestCase() throws Exception {
-//        super();
-//        // The XPackPlugin is sometimes not loaded by the time the plugin components are loaded
-//        // so we do this to ensure that we wont get a NPE
-//        Settings settings = Settings.builder()
-//                .put("path.home", createTempDir())
-//                .build();
-//
-//        // These tests do not load this plugin yet before spinning up some nodes
-//        new XPackPlugin(settings, null);
-    }
-
     @Override
     protected Settings nodeSettings(int nodeOrdinal) {
         Settings.Builder builder = Settings.builder()
                 .put(super.nodeSettings(nodeOrdinal))
+                .put(MonitoringService.INTERVAL.getKey(), MonitoringService.MIN_INTERVAL)
 //                .put(XPackSettings.SECURITY_ENABLED.getKey(), false)
 //                .put(XPackSettings.WATCHER_ENABLED.getKey(), false)
                 // Disable native ML autodetect_process as the c++ controller won't be available
@@ -229,12 +217,14 @@ public abstract class MonitoringIntegTestCase extends ESIntegTestCase {
         assertThat(client().admin().indices().prepareExists(indices).get().isExists(), is(true));
     }
 
-    protected void disableMonitoringInterval() {
+    protected void enableMonitoringCollection() {
-        updateMonitoringInterval(TimeValue.MINUS_ONE.millis(), TimeUnit.MILLISECONDS);
+        assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(
+                    Settings.builder().put(MonitoringService.ENABLED.getKey(), true)));
     }
 
-    protected void updateMonitoringInterval(long value, TimeUnit timeUnit) {
+    protected void disableMonitoringCollection() {
         assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(
-                Settings.builder().put(MonitoringService.INTERVAL.getKey(), value, timeUnit)));
+                    Settings.builder().putNull(MonitoringService.ENABLED.getKey())));
     }
+
 }

plugin/src/test/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java

@@ -129,7 +129,8 @@ public class XPackRestIT extends ESClientYamlSuiteTestCase {
                     XContentMapValues.extractRawValues("monitoring.enabled_exporters", exporters), hasSize(0));
 
             final Map<String, Object> settings = new HashMap<>();
-            settings.put("xpack.monitoring.collection.interval", "3s");
+            settings.put("xpack.monitoring.collection.enabled", true);
+            settings.put("xpack.monitoring.collection.interval", "1s");
             settings.put("xpack.monitoring.exporters._local.enabled", true);
 
             awaitCallApi("cluster.put_settings", emptyMap(),
@@ -151,6 +152,7 @@ public class XPackRestIT extends ESClientYamlSuiteTestCase {
     private void disableMonitoring() throws Exception {
         if (isMonitoringTest()) {
             final Map<String, Object> settings = new HashMap<>();
+            settings.put("xpack.monitoring.collection.enabled", null);
             settings.put("xpack.monitoring.collection.interval", null);
             settings.put("xpack.monitoring.exporters._local.enabled", null);