Fix enabling / disabling of APM agent "recording" in APMAgentSettings (#104324)

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/APM.java

@@ -70,8 +70,7 @@ public class APM extends Plugin implements NetworkPlugin, TelemetryPlugin {
         apmTracer.setNodeName(services.clusterService().getNodeName());
 
         final APMAgentSettings apmAgentSettings = new APMAgentSettings();
-        apmAgentSettings.syncAgentSystemProperties(settings);
-
+        apmAgentSettings.initAgentSystemProperties(settings);
         apmAgentSettings.addClusterSettingsListeners(services.clusterService(), telemetryProvider.get());
         logger.info("Sending apm metrics is {}", APMAgentSettings.TELEMETRY_METRICS_ENABLED_SETTING.get(settings) ? "enabled" : "disabled");
         logger.info("Sending apm tracing is {}", APMAgentSettings.TELEMETRY_TRACING_ENABLED_SETTING.get(settings) ? "enabled" : "disabled");

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/APMAgentSettings.java

@@ -45,12 +45,17 @@ public class APMAgentSettings {
         clusterSettings.addSettingsUpdateConsumer(TELEMETRY_TRACING_ENABLED_SETTING, enabled -> {
             apmTracer.setEnabled(enabled);
             this.setAgentSetting("instrument", Boolean.toString(enabled));
+            // The agent records data other than spans, e.g. JVM metrics, so we toggle this setting in order to
+            // minimise its impact to a running Elasticsearch.
+            boolean recording = enabled || clusterSettings.get(TELEMETRY_METRICS_ENABLED_SETTING);
+            this.setAgentSetting("recording", Boolean.toString(recording));
         });
         clusterSettings.addSettingsUpdateConsumer(TELEMETRY_METRICS_ENABLED_SETTING, enabled -> {
             apmMeterService.setEnabled(enabled);
             // The agent records data other than spans, e.g. JVM metrics, so we toggle this setting in order to
             // minimise its impact to a running Elasticsearch.
-            this.setAgentSetting("recording", Boolean.toString(enabled));
+            boolean recording = enabled || clusterSettings.get(TELEMETRY_TRACING_ENABLED_SETTING);
+            this.setAgentSetting("recording", Boolean.toString(recording));
         });
         clusterSettings.addSettingsUpdateConsumer(TELEMETRY_TRACING_NAMES_INCLUDE_SETTING, apmTracer::setIncludeNames);
         clusterSettings.addSettingsUpdateConsumer(TELEMETRY_TRACING_NAMES_EXCLUDE_SETTING, apmTracer::setExcludeNames);
@@ -59,11 +64,16 @@ public class APMAgentSettings {
     }
 
     /**
-     * Copies APM settings from the provided settings object into the corresponding system properties.
+     * Initialize APM settings from the provided settings object into the corresponding system properties.
+     * Later updates to these settings are synchronized using update consumers.
      * @param settings the settings to apply
      */
-    public void syncAgentSystemProperties(Settings settings) {
-        this.setAgentSetting("recording", Boolean.toString(TELEMETRY_TRACING_ENABLED_SETTING.get(settings)));
+    public void initAgentSystemProperties(Settings settings) {
+        boolean tracing = TELEMETRY_TRACING_ENABLED_SETTING.get(settings);
+        boolean metrics = TELEMETRY_METRICS_ENABLED_SETTING.get(settings);
+
+        this.setAgentSetting("recording", Boolean.toString(tracing || metrics));
+        this.setAgentSetting("instrument", Boolean.toString(tracing));
         // Apply values from the settings in the cluster state
         APM_AGENT_SETTINGS.getAsMap(settings).forEach(this::setAgentSetting);
     }

modules/apm/src/test/java/org/elasticsearch/telemetry/apm/internal/APMAgentSettingsTests.java

@@ -8,81 +8,190 @@
 
 package org.elasticsearch.telemetry.apm.internal;
 
+import org.elasticsearch.cluster.service.ClusterService;
+import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.MockSecureSettings;
 import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.test.ESTestCase;
+import org.mockito.Mockito;
 
 import java.util.List;
-
+import java.util.Set;
+
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.APM_AGENT_SETTINGS;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TELEMETRY_API_KEY_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TELEMETRY_METRICS_ENABLED_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TELEMETRY_SECRET_TOKEN_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TELEMETRY_TRACING_ENABLED_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TELEMETRY_TRACING_NAMES_EXCLUDE_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TELEMETRY_TRACING_NAMES_INCLUDE_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TELEMETRY_TRACING_SANITIZE_FIELD_NAMES;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TRACING_APM_API_KEY_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TRACING_APM_ENABLED_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TRACING_APM_NAMES_EXCLUDE_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TRACING_APM_NAMES_INCLUDE_SETTING;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TRACING_APM_SANITIZE_FIELD_NAMES;
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.TRACING_APM_SECRET_TOKEN_SETTING;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.hasItem;
+import static org.mockito.Mockito.clearInvocations;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 public class APMAgentSettingsTests extends ESTestCase {
+    APMAgentSettings apmAgentSettings = spy(new APMAgentSettings());
+    APMTelemetryProvider apmTelemetryProvider = mock(Mockito.RETURNS_DEEP_STUBS);
 
     /**
      * Check that when the tracer is enabled, it also sets the APM agent's recording system property to true.
      */
     public void testEnableTracing() {
-        APMAgentSettings apmAgentSettings = spy(new APMAgentSettings());
-        Settings settings = Settings.builder().put(APMAgentSettings.TELEMETRY_TRACING_ENABLED_SETTING.getKey(), true).build();
-        apmAgentSettings.syncAgentSystemProperties(settings);
-
-        verify(apmAgentSettings).setAgentSetting("recording", "true");
+        for (boolean metricsEnabled : List.of(true, false)) {
+            clearInvocations(apmAgentSettings, apmTelemetryProvider.getTracer());
+
+            Settings update = Settings.builder()
+                .put(TELEMETRY_TRACING_ENABLED_SETTING.getKey(), true)
+                .put(TELEMETRY_METRICS_ENABLED_SETTING.getKey(), metricsEnabled)
+                .build();
+            apmAgentSettings.initAgentSystemProperties(update);
+
+            verify(apmAgentSettings).setAgentSetting("recording", "true");
+            verify(apmAgentSettings).setAgentSetting("instrument", "true");
+            clearInvocations(apmAgentSettings);
+
+            Settings initial = Settings.builder().put(update).put(TELEMETRY_TRACING_ENABLED_SETTING.getKey(), false).build();
+            triggerUpdateConsumer(initial, update);
+            verify(apmAgentSettings).setAgentSetting("recording", "true");
+            verify(apmAgentSettings).setAgentSetting("instrument", "true");
+            verify(apmTelemetryProvider.getTracer()).setEnabled(true);
+        }
     }
 
     public void testEnableTracingUsingLegacySetting() {
-        APMAgentSettings apmAgentSettings = spy(new APMAgentSettings());
-        Settings settings = Settings.builder().put(APMAgentSettings.TRACING_APM_ENABLED_SETTING.getKey(), true).build();
-        apmAgentSettings.syncAgentSystemProperties(settings);
+        Settings settings = Settings.builder().put(TRACING_APM_ENABLED_SETTING.getKey(), true).build();
+        apmAgentSettings.initAgentSystemProperties(settings);
 
         verify(apmAgentSettings).setAgentSetting("recording", "true");
+        verify(apmAgentSettings).setAgentSetting("instrument", "true");
+    }
+
+    public void testEnableMetrics() {
+        for (boolean tracingEnabled : List.of(true, false)) {
+            clearInvocations(apmAgentSettings, apmTelemetryProvider.getMeterService());
+
+            Settings update = Settings.builder()
+                .put(TELEMETRY_METRICS_ENABLED_SETTING.getKey(), true)
+                .put(TELEMETRY_TRACING_ENABLED_SETTING.getKey(), tracingEnabled)
+                .build();
+            apmAgentSettings.initAgentSystemProperties(update);
+
+            verify(apmAgentSettings).setAgentSetting("recording", "true");
+            verify(apmAgentSettings).setAgentSetting("instrument", Boolean.toString(tracingEnabled));
+            clearInvocations(apmAgentSettings);
+
+            Settings initial = Settings.builder().put(update).put(TELEMETRY_METRICS_ENABLED_SETTING.getKey(), false).build();
+            triggerUpdateConsumer(initial, update);
+            verify(apmAgentSettings).setAgentSetting("recording", "true");
+            verify(apmTelemetryProvider.getMeterService()).setEnabled(true);
+        }
     }
 
     /**
-     * Check that when the tracer is disabled, it also sets the APM agent's recording system property to false.
+     * Check that when the tracer is disabled, it also sets the APM agent's recording system property to false unless metrics are enabled.
      */
     public void testDisableTracing() {
-        APMAgentSettings apmAgentSettings = spy(new APMAgentSettings());
-        Settings settings = Settings.builder().put(APMAgentSettings.TELEMETRY_TRACING_ENABLED_SETTING.getKey(), false).build();
-        apmAgentSettings.syncAgentSystemProperties(settings);
-
-        verify(apmAgentSettings).setAgentSetting("recording", "false");
+        for (boolean metricsEnabled : List.of(true, false)) {
+            clearInvocations(apmAgentSettings, apmTelemetryProvider.getTracer());
+
+            Settings update = Settings.builder()
+                .put(TELEMETRY_TRACING_ENABLED_SETTING.getKey(), false)
+                .put(TELEMETRY_METRICS_ENABLED_SETTING.getKey(), metricsEnabled)
+                .build();
+            apmAgentSettings.initAgentSystemProperties(update);
+
+            verify(apmAgentSettings).setAgentSetting("recording", Boolean.toString(metricsEnabled));
+            verify(apmAgentSettings).setAgentSetting("instrument", "false");
+            clearInvocations(apmAgentSettings);
+
+            Settings initial = Settings.builder().put(update).put(TELEMETRY_TRACING_ENABLED_SETTING.getKey(), true).build();
+            triggerUpdateConsumer(initial, update);
+            verify(apmAgentSettings).setAgentSetting("recording", Boolean.toString(metricsEnabled));
+            verify(apmAgentSettings).setAgentSetting("instrument", "false");
+            verify(apmTelemetryProvider.getTracer()).setEnabled(false);
+        }
     }
 
     public void testDisableTracingUsingLegacySetting() {
-        APMAgentSettings apmAgentSettings = spy(new APMAgentSettings());
-        Settings settings = Settings.builder().put(APMAgentSettings.TRACING_APM_ENABLED_SETTING.getKey(), false).build();
-        apmAgentSettings.syncAgentSystemProperties(settings);
+        Settings settings = Settings.builder().put(TRACING_APM_ENABLED_SETTING.getKey(), false).build();
+        apmAgentSettings.initAgentSystemProperties(settings);
 
         verify(apmAgentSettings).setAgentSetting("recording", "false");
+        verify(apmAgentSettings).setAgentSetting("instrument", "false");
+    }
+
+    public void testDisableMetrics() {
+        for (boolean tracingEnabled : List.of(true, false)) {
+            clearInvocations(apmAgentSettings, apmTelemetryProvider.getMeterService());
+
+            Settings update = Settings.builder()
+                .put(TELEMETRY_TRACING_ENABLED_SETTING.getKey(), tracingEnabled)
+                .put(TELEMETRY_METRICS_ENABLED_SETTING.getKey(), false)
+                .build();
+            apmAgentSettings.initAgentSystemProperties(update);
+
+            verify(apmAgentSettings).setAgentSetting("recording", Boolean.toString(tracingEnabled));
+            verify(apmAgentSettings).setAgentSetting("instrument", Boolean.toString(tracingEnabled));
+            clearInvocations(apmAgentSettings);
+
+            Settings initial = Settings.builder().put(update).put(TELEMETRY_METRICS_ENABLED_SETTING.getKey(), true).build();
+            triggerUpdateConsumer(initial, update);
+            verify(apmAgentSettings).setAgentSetting("recording", Boolean.toString(tracingEnabled));
+            verify(apmTelemetryProvider.getMeterService()).setEnabled(false);
+        }
+    }
+
+    private void triggerUpdateConsumer(Settings initial, Settings update) {
+        ClusterService clusterService = mock();
+        ClusterSettings clusterSettings = new ClusterSettings(
+            initial,
+            Set.of(
+                TELEMETRY_TRACING_ENABLED_SETTING,
+                TELEMETRY_METRICS_ENABLED_SETTING,
+                TELEMETRY_TRACING_NAMES_INCLUDE_SETTING,
+                TELEMETRY_TRACING_NAMES_EXCLUDE_SETTING,
+                TELEMETRY_TRACING_SANITIZE_FIELD_NAMES,
+                APM_AGENT_SETTINGS
+            )
+        );
+        when(clusterService.getClusterSettings()).thenReturn(clusterSettings);
+        apmAgentSettings.addClusterSettingsListeners(clusterService, apmTelemetryProvider);
+        clusterSettings.applySettings(update);
     }
 
     /**
      * Check that when cluster settings are synchronised with the system properties, agent settings are set.
      */
     public void testSetAgentSettings() {
-        APMAgentSettings apmAgentSettings = spy(new APMAgentSettings());
         Settings settings = Settings.builder()
-            .put(APMAgentSettings.TELEMETRY_TRACING_ENABLED_SETTING.getKey(), true)
-            .put(APMAgentSettings.APM_AGENT_SETTINGS.getKey() + "span_compression_enabled", "true")
+            .put(TELEMETRY_TRACING_ENABLED_SETTING.getKey(), true)
+            .put(APM_AGENT_SETTINGS.getKey() + "span_compression_enabled", "true")
             .build();
-        apmAgentSettings.syncAgentSystemProperties(settings);
+        apmAgentSettings.initAgentSystemProperties(settings);
 
         verify(apmAgentSettings).setAgentSetting("recording", "true");
         verify(apmAgentSettings).setAgentSetting("span_compression_enabled", "true");
     }
 
     public void testSetAgentsSettingsWithLegacyPrefix() {
-        APMAgentSettings apmAgentSettings = spy(new APMAgentSettings());
         Settings settings = Settings.builder()
-            .put(APMAgentSettings.TELEMETRY_TRACING_ENABLED_SETTING.getKey(), true)
+            .put(TELEMETRY_TRACING_ENABLED_SETTING.getKey(), true)
             .put("tracing.apm.agent.span_compression_enabled", "true")
             .build();
-        apmAgentSettings.syncAgentSystemProperties(settings);
+        apmAgentSettings.initAgentSystemProperties(settings);
 
         verify(apmAgentSettings).setAgentSetting("recording", "true");
         verify(apmAgentSettings).setAgentSetting("span_compression_enabled", "true");
@@ -92,57 +201,54 @@ public class APMAgentSettingsTests extends ESTestCase {
      * Check that invalid or forbidden APM agent settings are rejected.
      */
     public void testRejectForbiddenOrUnknownAgentSettings() {
-        List<String> prefixes = List.of(APMAgentSettings.APM_AGENT_SETTINGS.getKey(), "tracing.apm.agent.");
+        List<String> prefixes = List.of(APM_AGENT_SETTINGS.getKey(), "tracing.apm.agent.");
         for (String prefix : prefixes) {
             Settings settings = Settings.builder().put(prefix + "unknown", "true").build();
-            Exception exception = expectThrows(
-                IllegalArgumentException.class,
-                () -> APMAgentSettings.APM_AGENT_SETTINGS.getAsMap(settings)
-            );
+            Exception exception = expectThrows(IllegalArgumentException.class, () -> APM_AGENT_SETTINGS.getAsMap(settings));
             assertThat(exception.getMessage(), containsString("[" + prefix + "unknown]"));
         }
         // though, accept / ignore nested global_labels
         for (String prefix : prefixes) {
             Settings settings = Settings.builder().put(prefix + "global_labels." + randomAlphaOfLength(5), "123").build();
-            APMAgentSettings.APM_AGENT_SETTINGS.getAsMap(settings);
+            APM_AGENT_SETTINGS.getAsMap(settings);
         }
     }
 
     public void testTelemetryTracingNamesIncludeFallback() {
-        Settings settings = Settings.builder().put(APMAgentSettings.TRACING_APM_NAMES_INCLUDE_SETTING.getKey(), "abc,xyz").build();
+        Settings settings = Settings.builder().put(TRACING_APM_NAMES_INCLUDE_SETTING.getKey(), "abc,xyz").build();
 
-        List<String> included = APMAgentSettings.TELEMETRY_TRACING_NAMES_INCLUDE_SETTING.get(settings);
+        List<String> included = TELEMETRY_TRACING_NAMES_INCLUDE_SETTING.get(settings);
 
         assertThat(included, containsInAnyOrder("abc", "xyz"));
     }
 
     public void testTelemetryTracingNamesExcludeFallback() {
-        Settings settings = Settings.builder().put(APMAgentSettings.TRACING_APM_NAMES_EXCLUDE_SETTING.getKey(), "abc,xyz").build();
+        Settings settings = Settings.builder().put(TRACING_APM_NAMES_EXCLUDE_SETTING.getKey(), "abc,xyz").build();
 
-        List<String> included = APMAgentSettings.TELEMETRY_TRACING_NAMES_EXCLUDE_SETTING.get(settings);
+        List<String> included = TELEMETRY_TRACING_NAMES_EXCLUDE_SETTING.get(settings);
 
         assertThat(included, containsInAnyOrder("abc", "xyz"));
     }
 
     public void testTelemetryTracingSanitizeFieldNamesFallback() {
-        Settings settings = Settings.builder().put(APMAgentSettings.TRACING_APM_SANITIZE_FIELD_NAMES.getKey(), "abc,xyz").build();
+        Settings settings = Settings.builder().put(TRACING_APM_SANITIZE_FIELD_NAMES.getKey(), "abc,xyz").build();
 
-        List<String> included = APMAgentSettings.TELEMETRY_TRACING_SANITIZE_FIELD_NAMES.get(settings);
+        List<String> included = TELEMETRY_TRACING_SANITIZE_FIELD_NAMES.get(settings);
 
         assertThat(included, containsInAnyOrder("abc", "xyz"));
     }
 
     public void testTelemetryTracingSanitizeFieldNamesFallbackDefault() {
-        List<String> included = APMAgentSettings.TELEMETRY_TRACING_SANITIZE_FIELD_NAMES.get(Settings.EMPTY);
+        List<String> included = TELEMETRY_TRACING_SANITIZE_FIELD_NAMES.get(Settings.EMPTY);
         assertThat(included, hasItem("password")); // and more defaults
     }
 
     public void testTelemetrySecretTokenFallback() {
         MockSecureSettings secureSettings = new MockSecureSettings();
-        secureSettings.setString(APMAgentSettings.TRACING_APM_SECRET_TOKEN_SETTING.getKey(), "verysecret");
+        secureSettings.setString(TRACING_APM_SECRET_TOKEN_SETTING.getKey(), "verysecret");
         Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
 
-        try (SecureString secureString = APMAgentSettings.TELEMETRY_SECRET_TOKEN_SETTING.get(settings)) {
+        try (SecureString secureString = TELEMETRY_SECRET_TOKEN_SETTING.get(settings)) {
             assertEquals("verysecret", secureString.toString());
 
         }
@@ -150,12 +256,22 @@ public class APMAgentSettingsTests extends ESTestCase {
 
     public void testTelemetryApiKeyFallback() {
         MockSecureSettings secureSettings = new MockSecureSettings();
-        secureSettings.setString(APMAgentSettings.TRACING_APM_API_KEY_SETTING.getKey(), "abc");
+        secureSettings.setString(TRACING_APM_API_KEY_SETTING.getKey(), "abc");
         Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
 
-        try (SecureString secureString = APMAgentSettings.TELEMETRY_API_KEY_SETTING.get(settings)) {
+        try (SecureString secureString = TELEMETRY_API_KEY_SETTING.get(settings)) {
             assertEquals("abc", secureString.toString());
 
         }
     }
+
+    /**
+     * Check that invalid or forbidden APM agent settings are rejected if their last part resembles an allowed setting.
+     */
+    public void testRejectUnknownSettingResemblingAnAllowedOne() {
+        Settings settings = Settings.builder().put(APM_AGENT_SETTINGS.getKey() + "unknown.service_name", "true").build();
+
+        Exception exception = expectThrows(IllegalArgumentException.class, () -> APM_AGENT_SETTINGS.getAsMap(settings));
+        assertThat(exception.getMessage(), containsString("[telemetry.agent.unknown.service_name]"));
+    }
 }