Improve runtime attach configuration (#2283)

* code cleanup

* implement new config tests

* refactor & test properties config

* implement & fix test cases

* update changelog

* simplify things a bit

* minor cleanup

* make animal sniffer happy

* config sources: use separate factory class

* make animal sniffer happy again

* add missing file

* try to simplify config location

* rename test file

* fix wording

* don't try to attach twice in test app

* remove useless failing test + small refactor

* fix test

* use more concrete type for config to avoid confusion

* exclude duplicated cached-lookup-keys

* improve + test for duplicated classes

* use another way to exclude cached keys dep.

Author: SylvainJuge

CHANGELOG.asciidoc

@@ -28,7 +28,7 @@ endif::[]
 * Fixing missing Micrometer metrics in Spring boot due to premature initialization - {pull}2255[#2255]
 * Fixing hostname trimming of FQDN too aggressive - {pull}2286[#2286]
 * Fixing agent `unknown` version - {pull}2289[#2289]
-
+* Improve runtime attach configuration reliability - {pull}2283[#2283]
 
 [[release-notes-1.x]]
 === Java Agent version 1.x

apm-agent-attach-cli/src/test/java/co/elastic/apm/attach/ElasticApmAttacherTest.java

@@ -21,8 +21,8 @@
 import co.elastic.apm.agent.common.util.ResourceExtractionUtil;
 import net.bytebuddy.agent.ByteBuddyAgent;
 
+import javax.annotation.Nullable;
 import java.io.File;
-import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
@@ -55,7 +55,7 @@ public class ElasticApmAttacher {
      * @throws IllegalStateException if there was a problem while attaching the agent to this VM
      */
     public static void attach() {
-        attach(loadProperties("elasticapm.properties"));
+        attach(loadPropertiesFromClasspath("elasticapm.properties"));
     }
 
     /**
@@ -69,10 +69,10 @@ public static void attach() {
      * @since 1.11.0
      */
     public static void attach(String propertiesLocation) {
-        attach(loadProperties(propertiesLocation));
+        attach(loadPropertiesFromClasspath(propertiesLocation));
     }
 
-    private static Map<String, String> loadProperties(String propertiesLocation) {
+    private static Map<String, String> loadPropertiesFromClasspath(String propertiesLocation) {
         Map<String, String> propertyMap = new HashMap<>();
         final Properties props = new Properties();
         try (InputStream resourceStream = ElasticApmAttacher.class.getClassLoader().getResourceAsStream(propertiesLocation)) {
@@ -105,24 +105,21 @@ public static void attach(Map<String, String> configuration) {
         attach(ByteBuddyAgent.ProcessProvider.ForCurrentVm.INSTANCE.resolve(), configuration);
     }
 
-    static File createTempProperties(Map<String, String> configuration) {
+    /**
+     * Store configuration to a temporary file
+     *
+     * @param configuration agent configuration
+     * @param folder        temporary folder, use {@literal null} to use default
+     * @return created file if any, {@literal null} if none was created
+     */
+    static File createTempProperties(Map<String, String> configuration, @Nullable File folder) {
         File tempFile = null;
         if (!configuration.isEmpty()) {
             Properties properties = new Properties();
             properties.putAll(configuration);
 
-            // when an external configuration file is used, we have to load it last to give it higher priority
-            String externalConfig = configuration.get("config_file");
-            if (null != externalConfig) {
-                try (FileInputStream stream = new FileInputStream(externalConfig)) {
-                    properties.load(stream);
-                } catch (IOException e) {
-                    e.printStackTrace();
-                }
-            }
-
             try {
-                tempFile = File.createTempFile("elstcapm", ".tmp");
+                tempFile = File.createTempFile("elstcapm", ".tmp", folder);
                 try (FileOutputStream outputStream = new FileOutputStream(tempFile)) {
                     properties.store(outputStream, null);
                 }
@@ -148,10 +145,10 @@ public static void attach(String pid, Map<String, String> configuration) {
      *
      * @param pid           the PID of the JVM the agent should be attached on
      * @param configuration the agent configuration
-     * @param agentJarFile
+     * @param agentJarFile  the agent jar file
      */
     public static void attach(String pid, Map<String, String> configuration, File agentJarFile) {
-        File tempFile = createTempProperties(configuration);
+        File tempFile = createTempProperties(configuration, null);
         String agentArgs = tempFile == null ? null : TEMP_PROPERTIES_FILE_KEY + "=" + tempFile.getAbsolutePath();
 
         ByteBuddyAgent.attach(agentJarFile, pid, agentArgs, ElasticAttachmentProvider.get());

apm-agent-attach/src/main/java/co/elastic/apm/attach/ElasticApmAttacher.java

@@ -0,0 +1,67 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package co.elastic.apm.attach;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.FileReader;
+import java.io.IOException;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class ElasticApmAttacherTest {
+
+    @Test
+    void testCreateTempProperties(@TempDir File tmp) throws Exception {
+        File tempProperties = ElasticApmAttacher.createTempProperties(Map.of("foo", "bär"), tmp);
+        assertThat(tempProperties).isNotNull();
+
+        assertThat(tempProperties.getParentFile())
+            .describedAs("property files should be created at root of tmp folder")
+            .isEqualTo(tmp);
+
+        Properties properties = readProperties(tempProperties);
+        assertThat(properties)
+            .hasSize(1)
+            .containsEntry("foo", "bär");
+    }
+
+    @Test
+    void testCreateEmptyConfigDoesNotCreateFile(@TempDir File tmp) {
+        File tempProperties = ElasticApmAttacher.createTempProperties(Map.of(), tmp);
+        assertThat(tempProperties).isNull();
+        assertThat(tmp.listFiles())
+            .describedAs("no file should be created in temp folder")
+            .isEmpty();
+    }
+
+    private Properties readProperties(File propertyFile) throws IOException {
+        Properties properties = new Properties();
+        try (FileReader fileReader = new FileReader(propertyFile)) {
+            properties.load(fileReader);
+        }
+        return properties;
+    }
+
+
+}

apm-agent-attach/src/test/java/co/elastic/apm/attach/ElasticApmAttacherTest.java

@@ -134,7 +134,7 @@ public static void initialize(@Nullable final String agentArguments, final Instr
         ElasticApmAgent.agentJarFile = agentJarFile;
 
         // silently early abort when agent is disabled to minimize the number of loaded classes
-        List<ConfigurationSource> configSources = ElasticApmTracerBuilder.getConfigSources(agentArguments);
+        List<ConfigurationSource> configSources = ElasticApmTracerBuilder.getConfigSources(agentArguments, premain);
         for (ConfigurationSource configSource : configSources) {
             String enabled = configSource.getValue(CoreConfiguration.ENABLED_KEY);
             if (enabled != null && !Boolean.parseBoolean(enabled)) {

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/ElasticApmAgent.java

@@ -25,10 +25,10 @@
 import co.elastic.apm.agent.configuration.converter.TimeDurationValueConverter;
 import co.elastic.apm.agent.configuration.validation.RegexValidator;
 import co.elastic.apm.agent.impl.transaction.Span;
-import co.elastic.apm.agent.matcher.WildcardMatcher;
-import co.elastic.apm.agent.matcher.WildcardMatcherValueConverter;
 import co.elastic.apm.agent.matcher.MethodMatcher;
 import co.elastic.apm.agent.matcher.MethodMatcherValueConverter;
+import co.elastic.apm.agent.matcher.WildcardMatcher;
+import co.elastic.apm.agent.matcher.WildcardMatcherValueConverter;
 import org.stagemonitor.configuration.ConfigurationOption;
 import org.stagemonitor.configuration.ConfigurationOptionProvider;
 import org.stagemonitor.configuration.converter.MapValueConverter;
@@ -777,25 +777,24 @@ public TimeDuration getSpanMinDuration() {
      * Makes sure to not initialize ConfigurationOption, which would initialize the logger
      */
     @Nullable
-    public static String getConfigFileLocation(List<ConfigurationSource> configurationSources) {
-        String configFileLocation = DEFAULT_CONFIG_FILE;
-        for (ConfigurationSource configurationSource : configurationSources) {
-            String valueFromSource = configurationSource.getValue(CONFIG_FILE);
-            if (valueFromSource != null) {
-                configFileLocation = valueFromSource;
+    public static String getConfigFileLocation(List<ConfigurationSource> configurationSources, boolean premain) {
+        String configFileLocation = premain ? DEFAULT_CONFIG_FILE : null;
+        for (ConfigurationSource source : configurationSources) {
+            String value = source.getValue(CONFIG_FILE);
+            if (value != null) {
+                configFileLocation = value;
                 break;
             }
         }
-        if (configFileLocation.contains(AGENT_HOME_PLACEHOLDER)) {
+
+        if (configFileLocation != null) {
             String agentHome = ElasticApmAgent.getAgentHome();
             if (agentHome != null) {
-                return configFileLocation.replace(AGENT_HOME_PLACEHOLDER, agentHome);
-            } else {
-                return null;
+                configFileLocation = configFileLocation.replace(AGENT_HOME_PLACEHOLDER, agentHome);
             }
-        } else {
-            return configFileLocation;
         }
+
+        return configFileLocation;
     }
 
     @Nullable