Add check prefix config (#284)

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

@@ -19,6 +19,7 @@
 import org.datadog.jmxfetch.tasks.TaskStatusHandler;
 import org.datadog.jmxfetch.util.CustomLogger;
 import org.datadog.jmxfetch.util.FileHelper;
+import org.datadog.jmxfetch.util.ServiceCheckHelper;
 import org.slf4j.Marker;
 import org.slf4j.MarkerFactory;
 
@@ -812,10 +813,31 @@ private void sendServiceCheck(
             Reporter reporter, Instance instance, String message, String status) {
         String checkName = instance.getCheckName();
 
-        reporter.sendServiceCheck(checkName, status, message, instance.getServiceCheckTags());
+        if (instance.getServiceCheckPrefix() != null) {
+            this.sendCanConnectServiceCheck(reporter, checkName, instance.getServiceCheckPrefix(),
+                    status, message, instance.getServiceCheckTags());
+        } else {
+            this.sendCanConnectServiceCheck(reporter, checkName, checkName,
+                    status, message, instance.getServiceCheckTags());
+
+            // Service check with formatted name is kept for backward compatibility
+            String formattedCheckName = ServiceCheckHelper.formatServiceCheckPrefix(checkName);
+            if (!formattedCheckName.equals(checkName)) {
+                this.sendCanConnectServiceCheck(reporter, checkName, formattedCheckName,
+                        status, message, instance.getServiceCheckTags());
+            }
+        }
+
         reporter.resetServiceCheckCount(checkName);
     }
 
+    private void sendCanConnectServiceCheck(Reporter reporter, String checkName,
+        String serviceCheckPrefix, String status, String message, String[] tags) {
+        String serviceCheckName = String.format("%s.can_connect", serviceCheckPrefix);
+        reporter.sendServiceCheck(
+                checkName, serviceCheckName, status, message, tags);
+    }
+
     private Instance instantiate(
             Map<String, Object> instanceMap,
             Map<String, Object> initConfig,

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

@@ -90,6 +90,7 @@ public Yaml initialValue() {
     private String service;
     private Map<String, String> tags;
     private String checkName;
+    private String serviceCheckPrefix;
     private int maxReturnedMetrics;
     private boolean limitReached;
     private Connection connection;
@@ -186,6 +187,10 @@ public Instance(
             }
         }
 
+        if (initConfig != null) {
+            this.serviceCheckPrefix = (String) initConfig.get("service_check_prefix");
+        }
+
         // Alternative aliasing for CASSANDRA-4009 metrics
         // More information: https://issues.apache.org/jira/browse/CASSANDRA-4009
         this.cassandraAliasing = (Boolean) instanceMap.get("cassandra_aliasing");
@@ -706,6 +711,11 @@ public String getCheckName() {
         return this.checkName;
     }
 
+    /** Returns the check prefix. */
+    public String getServiceCheckPrefix() {
+        return this.serviceCheckPrefix;
+    }
+
     /** Returns the maximum number of metrics an instance may collect. */
     public int getMaxNumberOfMetrics() {
         return this.maxReturnedMetrics;

src/main/java/org/datadog/jmxfetch/reporter/Reporter.java

@@ -2,7 +2,6 @@
 
 import com.timgroup.statsd.ServiceCheck;
 import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.lang.StringUtils;
 
 import org.datadog.jmxfetch.App;
 import org.datadog.jmxfetch.Instance;
@@ -157,11 +156,9 @@ public void sendMetrics(List<Metric> metrics, String instanceName, boolean canon
     }
 
     /** Submits service check. */
-    public void sendServiceCheck(String checkName, String status, String message, String[] tags) {
+    public void sendServiceCheck(String checkName, String serviceCheckName,
+                                 String status, String message, String[] tags) {
         this.incrementServiceCheckCount(checkName);
-        String serviceCheckName = String.format(
-            "%s.can_connect", Reporter.formatServiceCheckPrefix(checkName));
-
         this.doSendServiceCheck(serviceCheckName, status, message, tags);
     }
 
@@ -184,13 +181,6 @@ protected Map<String, Integer> getServiceCheckCountMap() {
         return this.serviceCheckCount;
     }
 
-    /** Formats the service check prefix. */
-    public static String formatServiceCheckPrefix(String fullname) {
-        String[] chunks = fullname.split("\\.");
-        chunks[0] = chunks[0].replaceAll("[A-Z0-9:_\\-]", "");
-        return StringUtils.join(chunks, ".");
-    }
-
     protected ServiceCheck.Status statusToServiceCheckStatus(String status) {
         if (status == Status.STATUS_OK) {
             return ServiceCheck.Status.OK;

src/main/java/org/datadog/jmxfetch/util/ServiceCheckHelper.java

@@ -0,0 +1,22 @@
+package org.datadog.jmxfetch.util;
+
+import org.apache.commons.lang.StringUtils;
+
+public class ServiceCheckHelper {
+    /**
+     * Formats the service check prefix.
+     * First implemented here:
+     * https://github.com/DataDog/jmxfetch/commit/0428c41ebf7a14404ae50928e3ecfc229701c042
+     *
+     * <p>The formatted service check name is kept for backward compatibility only.
+     * Previously there were 2 JMXFetch integrations for activemq: one called activemq
+     * for older versions of activemq, the other called activemq_58 for v5.8+ of activemq,
+     * see https://github.com/DataDog/dd-agent/tree/5.10.x/conf.d
+     * And we still wanted both integrations to send the service check with the activemq prefix.
+     * */
+    public static String formatServiceCheckPrefix(String fullname) {
+        String[] chunks = fullname.split("\\.");
+        chunks[0] = chunks[0].replaceAll("[A-Z0-9:_\\-]", "");
+        return StringUtils.join(chunks, ".");
+    }
+}

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

@@ -72,7 +72,7 @@ public void testEmptyDefaultHostname() throws Exception {
         }
 
         List<Map<String, Object>> serviceChecks = getServiceChecks();
-        assertEquals(2, serviceChecks.size());
+        assertEquals(4, serviceChecks.size());
         for (Map<String, Object> sc : serviceChecks) {
             String[] tags = (String[]) sc.get("tags");
             this.assertHostnameTags(Arrays.asList(tags));
@@ -98,7 +98,7 @@ public void testServiceTagGlobal() throws Exception {
         }
 
         List<Map<String, Object>> serviceChecks = getServiceChecks();
-        assertEquals(2, serviceChecks.size());
+        assertEquals(4, serviceChecks.size());
         for (Map<String, Object> sc : serviceChecks) {
             String[] tags = (String[]) sc.get("tags");
             this.assertServiceTag(Arrays.asList(tags), "global");
@@ -119,7 +119,7 @@ public void testServiceTagInstanceOverride() throws Exception {
         }
 
         List<Map<String, Object>> serviceChecks = getServiceChecks();
-        assertEquals(2, serviceChecks.size());
+        assertEquals(4, serviceChecks.size());
         for (Map<String, Object> sc : serviceChecks) {
             String[] tags = (String[]) sc.get("tags");
             this.assertServiceTag(Arrays.asList(tags), "override");

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

@@ -7,7 +7,6 @@
 import static org.mockito.Mockito.when;
 
 import java.util.Arrays;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -42,7 +41,7 @@ public void testServiceCheckOK() throws Exception {
         String scStatus = (String) (sc.get("status"));
         String[] scTags = (String[]) (sc.get("tags"));
 
-        assertEquals(Reporter.formatServiceCheckPrefix("jmx") + ".can_connect", scName);
+        assertEquals("jmx.can_connect", scName);
         assertEquals(Status.STATUS_OK, scStatus);
         assertEquals(scTags.length, 3);
         assertTrue(Arrays.asList(scTags).contains("instance:jmx_test_instance"));
@@ -70,7 +69,7 @@ public void testServiceCheckWarning() throws Exception {
         List<Map<String, Object>> metrics = getMetrics();
         assertTrue(metrics.size() >= 350);
 
-        assertEquals(1, serviceChecks.size());
+        assertEquals(2, serviceChecks.size());
         Map<String, Object> sc = serviceChecks.get(0);
         assertNotNull(sc.get("name"));
         assertNotNull(sc.get("status"));
@@ -83,7 +82,7 @@ public void testServiceCheckWarning() throws Exception {
         String scStatus = (String) (sc.get("status"));
         String[] scTags = (String[]) (sc.get("tags"));
 
-        assertEquals(Reporter.formatServiceCheckPrefix("too_many_metrics") + ".can_connect", scName);
+        assertEquals("too_many_metrics.can_connect", scName);
         // We should have an OK service check status when too many metrics are getting sent
         assertEquals(Status.STATUS_OK, scStatus);
         assertEquals(scTags.length, 3);
@@ -102,7 +101,7 @@ public void testServiceCheckCRITICAL() throws Exception {
 
         // Test that a CRITICAL service check status is sent on initialization
         List<Map<String, Object>> serviceChecks = getServiceChecks();
-        assertEquals(1, serviceChecks.size());
+        assertEquals(2, serviceChecks.size());
 
         Map<String, Object> sc = serviceChecks.get(0);
         assertNotNull(sc.get("name"));
@@ -115,7 +114,7 @@ public void testServiceCheckCRITICAL() throws Exception {
         String scMessage = (String) (sc.get("message"));
         String[] scTags = (String[]) (sc.get("tags"));
 
-        assertEquals(Reporter.formatServiceCheckPrefix("non_running_process") + ".can_connect", scName);
+        assertEquals("non_running_process.can_connect", scName);
         assertEquals(Status.STATUS_ERROR, scStatus);
         assertEquals(
                 "Unable to instantiate or initialize instance process_regex: `.*non_running_process_test.*`. "
@@ -129,7 +128,7 @@ public void testServiceCheckCRITICAL() throws Exception {
         run();
 
         serviceChecks = getServiceChecks();
-        assertEquals(1, serviceChecks.size());
+        assertEquals(2, serviceChecks.size());
 
         sc = serviceChecks.get(0);
         assertNotNull(sc.get("name"));
@@ -142,7 +141,7 @@ public void testServiceCheckCRITICAL() throws Exception {
         scMessage = (String) (sc.get("message"));
         scTags = (String[]) (sc.get("tags"));
 
-        assertEquals(Reporter.formatServiceCheckPrefix("non_running_process") + ".can_connect", scName);
+        assertEquals("non_running_process.can_connect", scName);
         assertEquals(Status.STATUS_ERROR, scStatus);
         assertEquals(
                 "Unable to instantiate or initialize instance process_regex: `.*non_running_process_test.*`. "
@@ -168,7 +167,7 @@ public void testServiceCheckCounter() throws Exception {
         // Let's put a service check in the pipeline (we cannot call doIteration()
         // here unfortunately because it would call reportStatus which will flush
         // the count to the jmx_status.yaml file and reset the counter.
-        repo.sendServiceCheck("jmx", Status.STATUS_OK, "This is a test", null);
+        repo.sendServiceCheck("jmx", "jmx.can_connect", Status.STATUS_OK, "This is a test", null);
 
         // Let's check that the counter has been updated
         assertEquals(1, repo.getServiceCheckCount("jmx"));
@@ -180,16 +179,88 @@ public void testServiceCheckCounter() throws Exception {
     }
 
     @Test
-    public void testPrefixFormatter() throws Exception {
-        // Let's get a list of Strings to test (add real versionned check names
-        // here when you add  new versionned check)
-        String[][] data = {
-            {"activemq_58.foo.bar12", "activemq.foo.bar12"},
-            {"test_package-X86_64-VER1:0.weird.metric_name", "testpackage.weird.metric_name"}
-        };
-
-        // Let's test them all
-        for (int i = 0; i < data.length; ++i)
-            assertEquals(data[i][1], Reporter.formatServiceCheckPrefix(data[i][0]));
+    public void testServiceCheckPrefix() throws Exception {
+        // We expose a few metrics through JMX
+        registerMBean(new SimpleTestJavaApp(), "org.datadog.jmxfetch.test:type=ServiceCheckTest");
+
+        // We do a first collection
+        when(appConfig.isTargetDirectInstances()).thenReturn(true);
+        initApplication("jmx_check_prefix.yaml");
+
+        run();
+        List<Map<String, Object>> metrics = getMetrics();
+
+        // Test that the check prefix is used
+        List<Map<String, Object>> serviceChecks = getServiceChecks();
+
+        assertEquals(1, serviceChecks.size());
+        Map<String, Object> sc = serviceChecks.get(0);
+        assertNotNull(sc.get("name"));
+        assertNotNull(sc.get("status"));
+        assertNull(sc.get("message"));
+        assertNotNull(sc.get("tags"));
+
+        String scName = (String) (sc.get("name"));
+
+        assertEquals( "myprefix.can_connect", scName);
+    }
+
+    @Test
+    public void testServiceCheckNoPrefix() throws Exception {
+        // We expose a few metrics through JMX
+        registerMBean(new SimpleTestJavaApp(), "org.datadog.jmxfetch.test:type=ServiceCheckTest");
+
+        // We do a first collection
+        when(appConfig.isTargetDirectInstances()).thenReturn(true);
+        initApplication("jmx_check_no_prefix.yaml");
+
+        run();
+        List<Map<String, Object>> metrics = getMetrics();
+
+        // Test that the check prefix is used
+        List<Map<String, Object>> serviceChecks = getServiceChecks();
+
+        assertEquals(2, serviceChecks.size());
+        Map<String, Object> sc = serviceChecks.get(0);
+        assertNotNull(sc.get("name"));
+        assertNotNull(sc.get("status"));
+        assertNull(sc.get("message"));
+        assertNotNull(sc.get("tags"));
+
+        String scName = (String) (sc.get("name"));
+        assertEquals( "jmx_check_no_prefix.can_connect", scName);
+
+        Map<String, Object> sc2 = serviceChecks.get(1);
+        assertNotNull(sc2.get("name"));
+        assertNotNull(sc2.get("status"));
+        assertNull(sc2.get("message"));
+        assertNotNull(sc2.get("tags"));
+
+        String sc2Name = (String) (sc2.get("name"));
+        assertEquals( "jmxchecknoprefix.can_connect", sc2Name);
+    }
+
+    @Test
+    public void testServiceCheckOnceNoFormattingNeeded() throws Exception {
+        // We expose a few metrics through JMX
+        registerMBean(new SimpleTestJavaApp(), "org.datadog.jmxfetch.test:type=ServiceCheckTest");
+
+        // We do a first collection
+        when(appConfig.isTargetDirectInstances()).thenReturn(true);
+        initApplication("jmx.yaml");
+
+        run();
+
+        List<Map<String, Object>> serviceChecks = getServiceChecks();
+
+        // Only 1 service check is expected if the formatted service check prefix (`jmx`)
+        // is same as the unformatted one (`jmx`).
+        assertEquals(1, serviceChecks.size());
+
+        Map<String, Object> sc = serviceChecks.get(0);
+        String scName = (String) (sc.get("name"));
+
+        assertEquals("jmx.can_connect", scName);
     }
+
 }

src/test/java/org/datadog/jmxfetch/util/ServiceCheckHelperTest.java

@@ -0,0 +1,23 @@
+package org.datadog.jmxfetch.util;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+public class ServiceCheckHelperTest {
+
+    @Test
+    public void testFormatServiceCheckPrefix() {
+        // Let's get a list of Strings to test (add real versionned check names
+        // here when you add  new versionned check)
+        String[][] data = {
+                {"activemq_58.foo.bar12", "activemq.foo.bar12"},
+                {"test_package-X86_64-VER1:0.weird.metric_name", "testpackage.weird.metric_name"}
+        };
+
+        // Let's test them all
+        for (String[] datum : data) {
+            assertEquals(datum[1], ServiceCheckHelper.formatServiceCheckPrefix(datum[0]));
+        }
+    }
+}

src/test/resources/jmx_check_no_prefix.yaml

@@ -0,0 +1,9 @@
+init_config:
+  service_check_prefix: null
+
+instances:
+  -   jvm_direct: true
+      name: jmx_test_instance
+      conf:
+        - include:
+            domain: org.datadog.jmxfetch.test

src/test/resources/jmx_check_prefix.yaml

@@ -0,0 +1,9 @@
+init_config:
+  service_check_prefix: myprefix
+
+instances:
+  -   jvm_direct: true
+      name: jmx_test_instance
+      conf:
+        - include:
+            domain: org.datadog.jmxfetch.test