Make env vars and sys props always win for rpConfiguration. (#1918)

* Make env vars and sys props always win for rpConfiguration.

* Env vars and sys props will always overlay rpConfigurations.
* Only impact connectionString and samplingRate.
* Add UT to cover this.

Signed-off-by: Pan Li <[email protected]>

* Fix test

* Spotless

* Remove String.format

* Minor updates

* Minor updates

* Minor updates

* Oops

* Annotation

* Fix UT.

Signed-off-by: Pan Li <[email protected]>

Co-authored-by: Trask Stalnaker <[email protected]>

Author: Incarnation-p-lee

agent/agent-tooling/src/main/java/com/microsoft/applicationinsights/agent/internal/configuration/ConfigurationBuilder.java

@@ -40,6 +40,7 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import org.checkerframework.checker.nullness.qual.Nullable;
 import org.slf4j.LoggerFactory;
 
 public class ConfigurationBuilder {
@@ -154,11 +155,15 @@ public static Configuration create(Path agentJarPath, RpConfiguration rpConfigur
               + " so no need to enable it under preview configuration");
     }
 
-    overlayEnvVars(config);
-    applySamplingPercentageRounding(config);
+    overlayFromEnv(config);
+    config.sampling.percentage = roundToNearest(config.sampling.percentage, true);
+    for (SamplingOverride override : config.preview.sampling.overrides) {
+      override.percentage = roundToNearest(override.percentage, true);
+    }
     // rp configuration should always be last (so it takes precedence)
     // currently applicationinsights-rp.json is only used by Azure Spring Cloud
     if (rpConfiguration != null) {
+      overlayFromEnv(rpConfiguration);
       overlayRpConfiguration(config, rpConfiguration);
     }
     // only set role instance to host name as a last resort
@@ -346,22 +351,8 @@ public static void logConfigurationWarnMessages() {
   }
 
   // visible for testing
-  static void overlayEnvVars(Configuration config) throws IOException {
-    config.connectionString =
-        overlayWithSysPropEnvVar(
-            APPLICATIONINSIGHTS_CONNECTION_STRING_SYS,
-            APPLICATIONINSIGHTS_CONNECTION_STRING_ENV,
-            config.connectionString);
-    if (config.connectionString == null) {
-      // this is for backwards compatibility only
-      String instrumentationKey = getEnvVar(APPINSIGHTS_INSTRUMENTATIONKEY);
-      if (instrumentationKey != null) {
-        configurationLogger.warn(
-            "APPINSIGHTS_INSTRUMENTATIONKEY is only supported for backwards compatibility,"
-                + " please consider using APPLICATIONINSIGHTS_CONNECTION_STRING instead");
-        config.connectionString = "InstrumentationKey=" + instrumentationKey;
-      }
-    }
+  static void overlayFromEnv(Configuration config) throws IOException {
+    config.connectionString = overlayConnectionStringFromEnv(config.connectionString);
 
     if (isTrimEmpty(config.role.name)) {
       // only use WEBSITE_SITE_NAME as a fallback
@@ -418,11 +409,34 @@ static void overlayEnvVars(Configuration config) throws IOException {
     overlayInstrumentationEnabledEnvVars(config);
   }
 
-  public static void applySamplingPercentageRounding(Configuration config) {
-    config.sampling.percentage = roundToNearest(config.sampling.percentage, true);
-    for (SamplingOverride override : config.preview.sampling.overrides) {
-      override.percentage = roundToNearest(override.percentage, true);
+  public static void overlayFromEnv(RpConfiguration config) {
+    config.connectionString = overlayConnectionStringFromEnv(config.connectionString);
+    config.sampling.percentage =
+        overlayWithEnvVar(APPLICATIONINSIGHTS_SAMPLING_PERCENTAGE, config.sampling.percentage);
+  }
+
+  @Nullable
+  private static String overlayConnectionStringFromEnv(String connectionString) {
+    String value =
+        overlayWithSysPropEnvVar(
+            APPLICATIONINSIGHTS_CONNECTION_STRING_SYS,
+            APPLICATIONINSIGHTS_CONNECTION_STRING_ENV,
+            connectionString);
+
+    if (value != null) {
+      return value;
+    }
+
+    // this is for backwards compatibility only
+    String instrumentationKey = getEnvVar(APPINSIGHTS_INSTRUMENTATIONKEY);
+    if (instrumentationKey != null) {
+      configurationLogger.warn(
+          "APPINSIGHTS_INSTRUMENTATIONKEY is only supported for backwards compatibility,"
+              + " please consider using APPLICATIONINSIGHTS_CONNECTION_STRING instead");
+      return "InstrumentationKey=" + instrumentationKey;
     }
+
+    return null;
   }
 
   // visible for testing

agent/agent-tooling/src/main/java/com/microsoft/applicationinsights/agent/internal/init/RpConfigurationPolling.java

@@ -95,6 +95,8 @@ public void run() {
         RpConfiguration newRpConfiguration =
             RpConfigurationBuilder.loadJsonConfigFile(rpConfiguration.configPath);
 
+        ConfigurationBuilder.overlayFromEnv(newRpConfiguration);
+
         if (!newRpConfiguration.connectionString.equals(rpConfiguration.connectionString)) {
           logger.debug(
               "Connection string from the JSON config file is overriding the previously configured connection string.");

agent/agent-tooling/src/test/java/com/microsoft/applicationinsights/agent/internal/configuration/ConfigurationBuilderTest.java

@@ -23,6 +23,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static uk.org.webcompere.systemstubs.SystemStubs.withEnvironmentVariable;
 
 import com.microsoft.applicationinsights.agent.internal.common.FriendlyException;
 import java.io.File;
@@ -98,4 +99,40 @@ void testGetJsonEncodingExceptionMessage() {
         .isEqualTo(
             "Application Insights Java agent's configuration file path/to/file has a malformed JSON\n");
   }
+
+  @Test
+  void testRpConfigurationOverlayWithEnvVarAndSysPropUnchanged() {
+    String testConnectionString = "test-connection-string";
+    float testSamplingPercentage = 10.0f;
+    RpConfiguration config = new RpConfiguration();
+
+    config.connectionString = testConnectionString;
+    config.sampling.percentage = testSamplingPercentage;
+
+    ConfigurationBuilder.overlayFromEnv(config);
+
+    assertThat(config.connectionString).isEqualTo(testConnectionString);
+    assertThat(config.sampling.percentage).isEqualTo(testSamplingPercentage);
+  }
+
+  @Test
+  void testRpConfigurationOverlayWithEnvVarAndSysPropPopulated() throws Exception {
+    String testConnectionString = "test-connection-string";
+    float testSamplingPercentage = 10.0f;
+
+    withEnvironmentVariable("APPLICATIONINSIGHTS_CONNECTION_STRING", testConnectionString)
+        .and("APPLICATIONINSIGHTS_SAMPLING_PERCENTAGE", String.valueOf(testSamplingPercentage))
+        .execute(
+            () -> {
+              RpConfiguration config = new RpConfiguration();
+
+              config.connectionString = String.format("original-%s", testConnectionString);
+              config.sampling.percentage = testSamplingPercentage + 1.0f;
+
+              ConfigurationBuilder.overlayFromEnv(config);
+
+              assertThat(config.connectionString).isEqualTo(testConnectionString);
+              assertThat(config.sampling.percentage).isEqualTo(testSamplingPercentage);
+            });
+  }
 }