Merge pull request #225 from tylerbenson/tyler/appconfig-builder

Use Lombok Builder for AppConfig, Formalize jvm_direct Connection

src/main/java/org/datadog/jmxfetch/App.java

@@ -1,5 +1,7 @@
 package org.datadog.jmxfetch;
 
+import static org.datadog.jmxfetch.Instance.isDirectInstance;
+
 import com.google.common.primitives.Bytes;
 
 import com.beust.jcommander.JCommander;
@@ -110,7 +112,7 @@ public App(AppConfig appConfig) {
     public static void main(String[] args) {
 
         // Load the config from the args
-        AppConfig config = new AppConfig();
+        AppConfig config = AppConfig.builder().build();
         JCommander commander = null;
         try {
             // Try to parse the args using JCommander
@@ -827,8 +829,16 @@ public void init(boolean forceNewConnection) {
             }
 
             for (LinkedHashMap<String, Object> configInstance : configInstances) {
+                if (appConfig.isTargetDirectInstances() != isDirectInstance(configInstance)) {
+                    log.info("Skipping instance '{}'. targetDirectInstances={} != jvm_direct={}",
+                            name,
+                            appConfig.isTargetDirectInstances(),
+                            isDirectInstance(configInstance));
+                    continue;
+                }
+
                 // Create a new Instance object
-                log.info("Instantiating instance for: " + name);
+                log.info("Instantiating instance for: {}", name);
                 Instance instance =
                         instantiate(
                                 configInstance,

src/main/java/org/datadog/jmxfetch/AppConfig.java

@@ -1,16 +1,14 @@
 package org.datadog.jmxfetch;
 
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
-
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
 
+import lombok.Builder;
+import lombok.ToString;
 import org.datadog.jmxfetch.converter.ExitWatcherConverter;
 import org.datadog.jmxfetch.converter.ReporterConverter;
 import org.datadog.jmxfetch.reporter.ConsoleReporter;
 import org.datadog.jmxfetch.reporter.Reporter;
-import org.datadog.jmxfetch.reporter.ReporterFactory;
 import org.datadog.jmxfetch.validator.Log4JLevelValidator;
 import org.datadog.jmxfetch.validator.PositiveIntegerValidator;
 import org.datadog.jmxfetch.validator.ReporterValidator;
@@ -20,6 +18,8 @@
 import java.util.List;
 import java.util.Map;
 
+@Builder
+@ToString
 @Parameters(separators = "=")
 public class AppConfig {
     public static final String ACTION_COLLECT = "collect";
@@ -69,6 +69,7 @@ public class AppConfig {
             description = "Level of verbosity",
             validateWith = Log4JLevelValidator.class,
             required = false)
+    @Builder.Default
     private String logLevel = "INFO";
 
     @Parameter(
@@ -87,6 +88,7 @@ public class AppConfig {
             names = {"--tmp_directory", "-T"},
             description = "Absolute path to a temporary directory",
             required = false)
+    @Builder.Default
     private String tmpDirectory = "/tmp";
 
     @Parameter(
@@ -110,46 +112,53 @@ public class AppConfig {
             description = "Sleeping time during two iterations in ms",
             validateWith = PositiveIntegerValidator.class,
             required = false)
+    @Builder.Default
     private int checkPeriod = 15000;
 
     @Parameter(
             names = {"--thread_pool_size", "-t"},
             description = "The size of the thread pool",
             validateWith = PositiveIntegerValidator.class,
             required = false)
+    @Builder.Default
     private int threadPoolSize = DEFAULT_THREAD_POOL_SIZE;
 
     @Parameter(
             names = {"--reconnection_thread_pool_size", "-u"},
             description = "The size of the reconnection thread pool",
             validateWith = PositiveIntegerValidator.class,
             required = false)
+    @Builder.Default
     private int reconnectionThreadPoolSize = DEFAULT_THREAD_POOL_SIZE;
 
     @Parameter(
             names = {"--collection_timeout", "-x"},
             description = "The concurrent collection timeout in seconds",
             validateWith = PositiveIntegerValidator.class,
             required = false)
+    @Builder.Default
     private int collectionTimeout = DEFAULT_COLLECTION_TO_S;
 
     @Parameter(
             names = {"--reconnection_timeout", "-y"},
             description = "The reconnection timeout in seconds",
             validateWith = PositiveIntegerValidator.class,
             required = false)
+    @Builder.Default
     private int reconnectionTimeout = DEFAULT_RECONNECTION_TO_S;
 
     @Parameter(
             names = {"--ad_enabled", "--sd_enabled", "-w"},
             description = "Enable Auto Discovery.",
             required = false)
+    @Builder.Default
     private boolean adEnabled = false;
 
     @Parameter(
             names = {"--ad_pipe", "--sd_pipe", "-P"},
             description = "Auto Discovery pipe name.",
             required = false)
+    @Builder.Default
     private String adPipe = AD_PIPE_NAME;
 
     @Parameter(
@@ -166,6 +175,7 @@ public class AppConfig {
                             + "(default to null = no exit on file)",
             converter = ExitWatcherConverter.class,
             required = false)
+    @Builder.Default
     private ExitWatcher exitWatcher = new ExitWatcher();
 
     @Parameter(
@@ -174,7 +184,7 @@ public class AppConfig {
                     + "list_everything, list_collected_attributes, list_matching_attributes, "
                     + "list_not_matching_attributes, list_limited_attributes, list_jvms]",
             required = true)
-    private List<String> action = null;
+    private List<String> action;
 
     @Parameter(
             names = {"--ipc_host", "-H"},
@@ -187,8 +197,16 @@ public class AppConfig {
             description = "IPC port",
             validateWith = PositiveIntegerValidator.class,
             required = false)
+    @Builder.Default
     private int ipcPort = 0;
 
+    /**
+     * Boolean setting to determine whether to ignore jvm_direct instances.
+     * If set to true, other instances will be ignored.
+     */
+    @Builder.Default
+    private boolean targetDirectInstances = false;
+
     // This is used by things like APM agent to provide configuration from resources
     private List<String> instanceConfigResources;
     // This is used by things like APM agent to provide metric configuration from resources
@@ -200,6 +218,7 @@ public class AppConfig {
     // This is used by things like APM agent to provide tags that should be set with all metrics
     private Map<String, String> globalTags;
 
+    @Builder.Default
     private Status status = new Status();
 
     /** Updates the status and returns a boolean describing if the status was indeed updated.. */
@@ -323,6 +342,10 @@ public String getJmxLaunchFile() {
         return getTmpDirectory() + "/" + AD_LAUNCH_FILE;
     }
 
+    public boolean isTargetDirectInstances() {
+        return targetDirectInstances;
+    }
+
     public List<String> getInstanceConfigResources() {
         return instanceConfigResources;
     }
@@ -342,31 +365,4 @@ public Integer getRefreshBeansPeriod() {
     public Map<String, String> getGlobalTags() {
         return globalTags;
     }
-
-    /** Factory method used by dd-tracer-agent to run jmxfetch in the same process. */
-    public static AppConfig create(
-            List<String> instanceConfigResources,
-            List<String> metricConfigResources,
-            List<String> metricConfigFiles,
-            Integer checkPeriod,
-            Integer refreshBeansPeriod,
-            Map<String, String> globalTags,
-            String reporter,
-            String logLocation,
-            String logLevel) {
-        AppConfig config = new AppConfig();
-        config.action = ImmutableList.of(ACTION_COLLECT);
-        config.instanceConfigResources = ImmutableList.copyOf(instanceConfigResources);
-        config.metricConfigResources = ImmutableList.copyOf(metricConfigResources);
-        config.metricConfigFiles = ImmutableList.copyOf(metricConfigFiles);
-        if (checkPeriod != null) {
-            config.checkPeriod = checkPeriod;
-        }
-        config.refreshBeansPeriod = refreshBeansPeriod;
-        config.globalTags = ImmutableMap.copyOf(globalTags);
-        config.reporter = ReporterFactory.getReporter(reporter);
-        config.logLocation = logLocation;
-        config.logLevel = logLevel;
-        return config;
-    }
 }

src/main/java/org/datadog/jmxfetch/ConnectionFactory.java

@@ -1,5 +1,7 @@
 package org.datadog.jmxfetch;
 
+import static org.datadog.jmxfetch.Instance.isDirectInstance;
+
 import lombok.extern.slf4j.Slf4j;
 
 import java.io.IOException;
@@ -9,11 +11,17 @@
 @Slf4j
 public class ConnectionFactory {
     public static final String PROCESS_NAME_REGEX = "process_name_regex";
-    private static ConnectionFactory connectionFactory = null;
 
     /** Factory method to create connections, both remote and local to the target JVM. */
     public static Connection createConnection(LinkedHashMap<String, Object> connectionParams)
             throws IOException {
+        // This is used by dd-java-agent to enable directly connecting to the mbean server.
+        // This works since jmxfetch is being run as a library inside the process being monitored.
+        if (isDirectInstance(connectionParams)) {
+            log.info("Connecting to JMX directly on the JVM");
+            return new JvmDirectConnection();
+        }
+
         if (connectionParams.get(PROCESS_NAME_REGEX) != null) {
             try {
                 Class.forName("com.sun.tools.attach.AttachNotSupportedException");
@@ -26,13 +34,6 @@ public static Connection createConnection(LinkedHashMap<String, Object> connecti
             return new AttachApiConnection(connectionParams);
         }
 
-        // This is used by dd-java-agent to enable directly connecting to the mbean server.
-        // This works because jmxfetch is being run as a library inside the process.
-        if ("service:jmx:local:///".equals(connectionParams.get("jmx_url"))) {
-            log.info("Connecting using JMX Local");
-            return new LocalConnection();
-        }
-
         log.info("Connecting using JMX Remote");
         return new RemoteConnection(connectionParams);
     }

src/main/java/org/datadog/jmxfetch/Instance.java

@@ -56,6 +56,7 @@ public class Instance {
     private static final int MAX_RETURNED_METRICS = 350;
     private static final int DEFAULT_REFRESH_BEANS_PERIOD = 600;
     public static final String PROCESS_NAME_REGEX = "process_name_regex";
+    public static final String JVM_DIRECT = "jvm_direct";
     public static final String ATTRIBUTE = "Attribute: ";
 
     private static final ThreadLocal<Yaml> YAML =
@@ -209,6 +210,11 @@ public Instance(
         loadDefaultConfig(gcMetricConfig);
     }
 
+    public static boolean isDirectInstance(LinkedHashMap<String, Object> configInstance) {
+        Object directInstance = configInstance.get(JVM_DIRECT);
+        return directInstance instanceof Boolean && (Boolean) directInstance;
+    }
+
     private void loadDefaultConfig(String configResourcePath) {
         ArrayList<LinkedHashMap<String, Object>> defaultConf =
                 (ArrayList<LinkedHashMap<String, Object>>)
@@ -221,8 +227,11 @@ private void loadDefaultConfig(String configResourcePath) {
     @VisibleForTesting
     static void loadMetricConfigFiles(
             AppConfig appConfig, LinkedList<Configuration> configurationList) {
-        if (appConfig.getMetricConfigFiles() != null) {
-            for (String fileName : appConfig.getMetricConfigFiles()) {
+        List<String> metricConfigFiles = appConfig.getMetricConfigFiles();
+        if (metricConfigFiles != null && !metricConfigFiles.isEmpty()) {
+            log.warn("Loading files via metricConfigFiles setting is deprecated.  Please "
+                    + "migrate to using standard agent config files in the conf.d directory.");
+            for (String fileName : metricConfigFiles) {
                 String yamlPath = new File(fileName).getAbsolutePath();
                 FileInputStream yamlInputStream = null;
                 log.info("Reading metric config file " + yamlPath);
@@ -365,7 +374,9 @@ public void init(boolean forceNewConnection)
     /** Returns a string representation for the instance. */
     @Override
     public String toString() {
-        if (this.instanceMap.get(PROCESS_NAME_REGEX) != null) {
+        if (isDirectInstance(instanceMap)) {
+            return "jvm_direct";
+        } else if (this.instanceMap.get(PROCESS_NAME_REGEX) != null) {
             return "process_regex: `" + this.instanceMap.get(PROCESS_NAME_REGEX) + "`";
         } else if (this.instanceMap.get("jmx_url") != null) {
             return (String) this.instanceMap.get("jmx_url");

src/main/java/org/datadog/jmxfetch/LocalConnection.java → src/main/java/org/datadog/jmxfetch/JvmDirectConnection.java

@@ -6,9 +6,9 @@
 import java.lang.management.ManagementFactory;
 
 @Slf4j
-public class LocalConnection extends Connection {
+public class JvmDirectConnection extends Connection {
 
-    public LocalConnection() throws IOException {
+    public JvmDirectConnection() throws IOException {
         createConnection();
     }
 

src/test/java/org/datadog/jmxfetch/TestApp.java

@@ -3,6 +3,7 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.when;
 
 import java.io.File;
 import java.util.ArrayList;
@@ -490,6 +491,7 @@ public void testApp() throws Exception {
         registerMBean(testApp, "org.datadog.jmxfetch.test:type=SimpleTestJavaApp");
 
         // We do a first collection
+        when(appConfig.isTargetDirectInstances()).thenReturn(true);
         initApplication("jmx.yaml");
 
         run();

src/test/java/org/datadog/jmxfetch/TestCommon.java

@@ -32,7 +32,7 @@
 import org.junit.BeforeClass;
 
 public class TestCommon {
-    AppConfig appConfig = spy(new AppConfig());
+    AppConfig appConfig = spy(AppConfig.builder().build());
     App app;
     MBeanServer mbs;
     ArrayList<ObjectName> objectNames = new ArrayList<ObjectName>();

src/test/java/org/datadog/jmxfetch/TestParsingJCommander.java

@@ -28,7 +28,7 @@ public class TestParsingJCommander {
     private static final String IPC_PORT = "5001";
 
     private static AppConfig testCommand(String[] params) throws ParameterException {
-        AppConfig appConfig = new AppConfig();
+        AppConfig appConfig = AppConfig.builder().build();
         new JCommander(appConfig, params);
         return appConfig;
     }

src/test/java/org/datadog/jmxfetch/TestServiceChecks.java

@@ -4,6 +4,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.when;
 
 import java.util.Arrays;
 import java.util.HashMap;
@@ -19,6 +20,7 @@ public void testServiceCheckOK() throws Exception {
         registerMBean(new SimpleTestJavaApp(), "org.datadog.jmxfetch.test:type=ServiceCheckTest");
 
         // We do a first collection
+        when(appConfig.isTargetDirectInstances()).thenReturn(true);
         initApplication("jmx.yaml");
 
         run();
@@ -153,6 +155,7 @@ public void testServiceCheckCRITICAL() throws Exception {
 
     @Test
     public void testServiceCheckCounter() throws Exception {
+        when(appConfig.isTargetDirectInstances()).thenReturn(true);
         initApplication("jmx.yaml");
 
         Reporter repo = getReporter();

src/test/java/org/datadog/jmxfetch/tasks/TestTaskProcessor.java

@@ -24,8 +24,8 @@
 public class TestTaskProcessor {
     private static List<Instance> instances = null;
 
-	class TestSimpleTask extends InstanceTask<Boolean> {
-         TestSimpleTask(Instance instance) {
+    class TestSimpleTask extends InstanceTask<Boolean> {
+        TestSimpleTask(Instance instance) {
              super(instance);
         }
 

src/test/resources/jmx.yaml

@@ -1,7 +1,7 @@
 init_config:
 
 instances:
-    -   process_name_regex: .*surefire.*
+    -   jvm_direct: true
         refresh_beans: 4
         name: jmx_test_instance
         tags: