Migrate Config system to environment component (#9191)

* feat(env): Migrate ConfigProvider to environment component
* feat(env): Migrate agent bootstrap to environment component
* feat(env): Improve null values handling

Signed-off-by: Nayeem Kamal <[email protected]>

Author: PerfectSlayer

components/environment/src/main/java/datadog/environment/EnvironmentVariables.java

@@ -1,6 +1,5 @@
 package datadog.environment;
 
-import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
 /**
@@ -32,7 +31,7 @@ private EnvironmentVariables() {}
    * @return The environment variable value, {@code defaultValue} if missing, can't be retrieved or
    *     the environment variable name is {@code null}.
    */
-  public static String getOrDefault(@Nonnull String name, String defaultValue) {
+  public static String getOrDefault(String name, String defaultValue) {
     if (name == null) {
       return defaultValue;
     }

components/environment/src/main/java/datadog/environment/SystemProperties.java

@@ -1,6 +1,6 @@
 package datadog.environment;
 
-import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 
 /**
  * Safely queries system properties against security manager.
@@ -18,7 +18,7 @@ private SystemProperties() {}
    * @return The system property value, {@code null} if missing, can't be retrieved, or the system
    *     property name is {@code null}.
    */
-  public static String get(String property) {
+  public static @Nullable String get(String property) {
     return getOrDefault(property, null);
   }
 
@@ -31,7 +31,7 @@ public static String get(String property) {
    * @return The system property value, {@code defaultValue} if missing, can't be retrieved, or the
    *     system property name is {@code null}.
    */
-  public static String getOrDefault(@Nonnull String property, String defaultValue) {
+  public static String getOrDefault(String property, String defaultValue) {
     if (property == null) {
       return defaultValue;
     }
@@ -50,11 +50,32 @@ public static String getOrDefault(@Nonnull String property, String defaultValue)
    * @return {@code true} if the system property was successfully set, {@code} false otherwise.
    */
   public static boolean set(String property, String value) {
+    if (property == null || value == null) {
+      return false;
+    }
     try {
       System.setProperty(property, value);
       return true;
     } catch (SecurityException ignored) {
       return false;
     }
   }
+
+  /**
+   * Clears a system property.
+   *
+   * @param property The system property name to clear.
+   * @return The previous value of the system property, {@code null} if there was no prior property
+   *     and property can't be cleared.
+   */
+  public static @Nullable String clear(String property) {
+    if (property == null) {
+      return null;
+    }
+    try {
+      return System.clearProperty(property);
+    } catch (SecurityException ignored) {
+      return null;
+    }
+  }
 }

components/environment/src/main/java/datadog/environment/package-info.java

@@ -0,0 +1,4 @@
+@ParametersAreNonnullByDefault
+package datadog.environment;
+
+import javax.annotation.ParametersAreNonnullByDefault;

components/environment/src/test/java/datadog/environment/SystemPropertiesTest.java

@@ -38,11 +38,29 @@ void testGetOrDefault() {
 
   @Test
   void testSet() {
-    String testProperty = "test.property";
-    String testValue = "test-value";
-    assumeTrue(SystemProperties.get(testProperty) == null);
-
+    String testProperty = "test.set.property";
+    String testValue = "test.set.value";
+    assertNull(SystemProperties.get(testProperty));
     assertTrue(SystemProperties.set(testProperty, testValue));
     assertEquals(testValue, SystemProperties.get(testProperty));
+    // Null values
+    assertDoesNotThrow(() -> SystemProperties.set(testProperty, null));
+    assertFalse(SystemProperties.set(testProperty, null));
+    assertDoesNotThrow(() -> SystemProperties.set(null, testValue));
+    assertFalse(SystemProperties.set(null, testValue));
+  }
+
+  @Test
+  void testClear() {
+    String testProperty = "test.clear.property";
+    String testValue = "test.clear.value";
+    assertNull(SystemProperties.get(testProperty));
+    assertNull(SystemProperties.clear(testProperty));
+    assumeTrue(SystemProperties.set(testProperty, testValue));
+    assertEquals(testValue, SystemProperties.clear(testProperty));
+    assertNull(SystemProperties.clear(testProperty));
+    // Null values
+    assertDoesNotThrow(() -> SystemProperties.clear(null));
+    assertNull(SystemProperties.clear(null));
   }
 }

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/benchmark/StaticEventLogger.java

@@ -1,5 +1,6 @@
 package datadog.trace.bootstrap.benchmark;
 
+import datadog.environment.SystemProperties;
 import java.io.*;
 import java.nio.charset.StandardCharsets;
 import java.util.Objects;
@@ -18,8 +19,8 @@ public class StaticEventLogger {
   static {
     BufferedWriter writer = null;
 
-    if ("true".equalsIgnoreCase(System.getProperty("dd.benchmark.enabled"))) {
-      String dir = System.getProperty("dd.benchmark.output.dir");
+    if ("true".equalsIgnoreCase(SystemProperties.get("dd.benchmark.enabled"))) {
+      String dir = SystemProperties.get("dd.benchmark.output.dir");
       dir = (dir != null ? dir + File.separator : "");
       String fileName = dir + "startup_" + System.currentTimeMillis() + ".csv";
 

dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java

@@ -5,6 +5,7 @@
 import static datadog.trace.agent.tooling.bytebuddy.matcher.GlobalIgnoresMatcher.globalIgnoresMatcher;
 import static net.bytebuddy.matcher.ElementMatchers.isDefaultFinalizer;
 
+import datadog.environment.SystemProperties;
 import datadog.trace.agent.tooling.bytebuddy.SharedTypePools;
 import datadog.trace.agent.tooling.bytebuddy.iast.TaintableRedefinitionStrategyListener;
 import datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers;
@@ -318,18 +319,17 @@ public static Set<InstrumenterModule.TargetSystem> getEnabledSystems() {
   }
 
   private static void addByteBuddyRawSetting() {
-    final String savedPropertyValue = System.getProperty(TypeDefinition.RAW_TYPES_PROPERTY);
-    try {
-      System.setProperty(TypeDefinition.RAW_TYPES_PROPERTY, "true");
-      final boolean rawTypes = TypeDescription.AbstractBase.RAW_TYPES;
-      if (!rawTypes && DEBUG) {
-        log.debug("Too late to enable {}", TypeDefinition.RAW_TYPES_PROPERTY);
-      }
-    } finally {
+    final String savedPropertyValue = SystemProperties.get(TypeDefinition.RAW_TYPES_PROPERTY);
+    final boolean overridden = SystemProperties.set(TypeDefinition.RAW_TYPES_PROPERTY, "true");
+    final boolean rawTypes = TypeDescription.AbstractBase.RAW_TYPES;
+    if (!rawTypes && DEBUG) {
+      log.debug("Too late to enable {}", TypeDefinition.RAW_TYPES_PROPERTY);
+    }
+    if (overridden) {
       if (savedPropertyValue == null) {
-        System.clearProperty(TypeDefinition.RAW_TYPES_PROPERTY);
+        SystemProperties.clear(TypeDefinition.RAW_TYPES_PROPERTY);
       } else {
-        System.setProperty(TypeDefinition.RAW_TYPES_PROPERTY, savedPropertyValue);
+        SystemProperties.set(TypeDefinition.RAW_TYPES_PROPERTY, savedPropertyValue);
       }
     }
   }

dd-java-agent/src/test/groovy/datadog/trace/agent/InitializationTelemetryTest.groovy

@@ -58,15 +58,15 @@ class InitializationTelemetryTest extends Specification {
   })
   def "incomplete agent start-up"() {
     // In this case, the SecurityManager blocks a custom permission that is checked by bytebuddy causing
-    // agent initialization to fail.  However, we should catch the exception allowing the application
+    // agent initialization to fail. However, we should catch the exception allowing the application
     // to run normally.
     when:
     def result = InitializationTelemetryCheck.runTestJvm(InitializationTelemetryCheck.BlockByteBuddy)
 
-    then:
+    then: 'should complete successfully and catch error'
     result.exitCode == 0
     !result.telemetryJson.contains('library_entrypoint.complete')
-    result.telemetryJson.contains('error_type:java.lang.IllegalStateException')
+    result.telemetryJson.contains('error_type:')
   }
 
   @IgnoreIf(reason = "SecurityManager is permanently disabled as of JDK 24", value = {

internal-api/src/main/java/datadog/trace/api/Config.java

@@ -632,8 +632,10 @@
 import static datadog.trace.util.CollectionUtils.tryMakeImmutableSet;
 import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName;
 
+import datadog.environment.EnvironmentVariables;
 import datadog.environment.JavaVirtualMachine;
 import datadog.environment.OperatingSystem;
+import datadog.environment.SystemProperties;
 import datadog.trace.api.civisibility.CiVisibilityWellKnownTags;
 import datadog.trace.api.config.GeneralConfig;
 import datadog.trace.api.config.ProfilingConfig;
@@ -1239,7 +1241,7 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins
     configFileStatus = configProvider.getConfigFileStatus();
     runtimeIdEnabled =
         configProvider.getBoolean(RUNTIME_ID_ENABLED, true, RUNTIME_METRICS_RUNTIME_ID_ENABLED);
-    runtimeVersion = System.getProperty("java.version", "unknown");
+    runtimeVersion = SystemProperties.getOrDefault("java.version", "unknown");
 
     // Note: We do not want APiKey to be loaded from property for security reasons
     // Note: we do not use defined default here
@@ -3409,7 +3411,7 @@ public static boolean isDatadogProfilerSafeInCurrentEnvironment() {
     // don't want to put this logic (which will evolve) in the public ProfilingConfig, and can't
     // access Platform there
     if (!JavaVirtualMachine.isJ9() && isJavaVersion(8)) {
-      String arch = System.getProperty("os.arch");
+      String arch = SystemProperties.get("os.arch");
       if ("aarch64".equalsIgnoreCase(arch) || "arm64".equalsIgnoreCase(arch)) {
         return false;
       }
@@ -4453,12 +4455,12 @@ public CiVisibilityWellKnownTags getCiVisibilityWellKnownTags() {
         getRuntimeId(),
         getEnv(),
         LANGUAGE_TAG_VALUE,
-        System.getProperty("java.runtime.name"),
-        System.getProperty("java.version"),
-        System.getProperty("java.vendor"),
-        System.getProperty("os.arch"),
-        System.getProperty("os.name"),
-        System.getProperty("os.version"),
+        SystemProperties.get("java.runtime.name"),
+        SystemProperties.get("java.version"),
+        SystemProperties.get("java.vendor"),
+        SystemProperties.get("os.arch"),
+        SystemProperties.get("os.name"),
+        SystemProperties.get("os.version"),
         isServiceNameSetByUser() ? "true" : "false");
   }
 
@@ -5197,7 +5199,7 @@ private static boolean isWindowsOS() {
   }
 
   private static String getEnv(String name) {
-    String value = System.getenv(name);
+    String value = EnvironmentVariables.get(name);
     if (value != null) {
       ConfigCollector.get().put(name, value, ConfigOrigin.ENV);
     }
@@ -5220,7 +5222,7 @@ private static String getProp(String name) {
   }
 
   private static String getProp(String name, String def) {
-    String value = System.getProperty(name, def);
+    String value = SystemProperties.getOrDefault(name, def);
     if (value != null) {
       ConfigCollector.get().put(name, value, ConfigOrigin.JVM_PROP);
     }

internal-api/src/main/java/datadog/trace/api/Platform.java

@@ -5,6 +5,8 @@
 import static datadog.environment.JavaVirtualMachine.isJavaVersionAtLeast;
 import static datadog.environment.JavaVirtualMachine.isOracleJDK8;
 
+import datadog.environment.SystemProperties;
+
 /**
  * This class is used early on during premain; it must not touch features like JMX or JUL in case
  * they trigger early loading/binding.
@@ -51,7 +53,7 @@ private static boolean checkForJfr() {
 
   private static boolean checkForNativeImageBuilder() {
     try {
-      return "org.graalvm.nativeimage.builder".equals(System.getProperty("jdk.module.main"));
+      return "org.graalvm.nativeimage.builder".equals(SystemProperties.get("jdk.module.main"));
     } catch (Throwable e) {
       return false;
     }

internal-api/src/main/java/datadog/trace/api/env/CapturedEnvironment.java

@@ -1,5 +1,6 @@
 package datadog.trace.api.env;
 
+import datadog.environment.EnvironmentVariables;
 import datadog.environment.JavaVirtualMachine;
 import datadog.trace.api.config.GeneralConfig;
 import java.io.File;
@@ -77,8 +78,8 @@ static void useFixedProcessInfo(final ProcessInfo processInfo) {
    * autodetection will return either the JAR filename or the java main class.
    */
   private String autodetectServiceName() {
-    String inAas = System.getenv("DD_AZURE_APP_SERVICES");
-    String siteName = System.getenv("WEBSITE_SITE_NAME");
+    String inAas = EnvironmentVariables.get("DD_AZURE_APP_SERVICES");
+    String siteName = EnvironmentVariables.get("WEBSITE_SITE_NAME");
 
     if (("true".equalsIgnoreCase(inAas) || "1".equals(inAas)) && siteName != null) {
       return siteName;