diff --git a/src/main/java/org/datadog/jmxfetch/App.java b/src/main/java/org/datadog/jmxfetch/App.java index 44c0699db..e7b855be4 100644 --- a/src/main/java/org/datadog/jmxfetch/App.java +++ b/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 instanceMap, Map initConfig, diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index ef39d5189..b924cb2a4 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -90,6 +90,7 @@ public Yaml initialValue() { private String service; private Map 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; diff --git a/src/main/java/org/datadog/jmxfetch/reporter/Reporter.java b/src/main/java/org/datadog/jmxfetch/reporter/Reporter.java index f536c38c0..cf0132d6a 100644 --- a/src/main/java/org/datadog/jmxfetch/reporter/Reporter.java +++ b/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 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 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; diff --git a/src/main/java/org/datadog/jmxfetch/util/ServiceCheckHelper.java b/src/main/java/org/datadog/jmxfetch/util/ServiceCheckHelper.java new file mode 100644 index 000000000..2f4d83143 --- /dev/null +++ b/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 + * + *

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, "."); + } +} diff --git a/src/test/java/org/datadog/jmxfetch/TestInstance.java b/src/test/java/org/datadog/jmxfetch/TestInstance.java index 34d4d6194..ab37ec592 100644 --- a/src/test/java/org/datadog/jmxfetch/TestInstance.java +++ b/src/test/java/org/datadog/jmxfetch/TestInstance.java @@ -72,7 +72,7 @@ public void testEmptyDefaultHostname() throws Exception { } List> serviceChecks = getServiceChecks(); - assertEquals(2, serviceChecks.size()); + assertEquals(4, serviceChecks.size()); for (Map sc : serviceChecks) { String[] tags = (String[]) sc.get("tags"); this.assertHostnameTags(Arrays.asList(tags)); @@ -98,7 +98,7 @@ public void testServiceTagGlobal() throws Exception { } List> serviceChecks = getServiceChecks(); - assertEquals(2, serviceChecks.size()); + assertEquals(4, serviceChecks.size()); for (Map sc : serviceChecks) { String[] tags = (String[]) sc.get("tags"); this.assertServiceTag(Arrays.asList(tags), "global"); @@ -119,7 +119,7 @@ public void testServiceTagInstanceOverride() throws Exception { } List> serviceChecks = getServiceChecks(); - assertEquals(2, serviceChecks.size()); + assertEquals(4, serviceChecks.size()); for (Map sc : serviceChecks) { String[] tags = (String[]) sc.get("tags"); this.assertServiceTag(Arrays.asList(tags), "override"); diff --git a/src/test/java/org/datadog/jmxfetch/TestServiceChecks.java b/src/test/java/org/datadog/jmxfetch/TestServiceChecks.java index 8f8a49f24..801b05d10 100644 --- a/src/test/java/org/datadog/jmxfetch/TestServiceChecks.java +++ b/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> metrics = getMetrics(); assertTrue(metrics.size() >= 350); - assertEquals(1, serviceChecks.size()); + assertEquals(2, serviceChecks.size()); Map 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> serviceChecks = getServiceChecks(); - assertEquals(1, serviceChecks.size()); + assertEquals(2, serviceChecks.size()); Map 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> metrics = getMetrics(); + + // Test that the check prefix is used + List> serviceChecks = getServiceChecks(); + + assertEquals(1, serviceChecks.size()); + Map 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> metrics = getMetrics(); + + // Test that the check prefix is used + List> serviceChecks = getServiceChecks(); + + assertEquals(2, serviceChecks.size()); + Map 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 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> 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 sc = serviceChecks.get(0); + String scName = (String) (sc.get("name")); + + assertEquals("jmx.can_connect", scName); } + } diff --git a/src/test/java/org/datadog/jmxfetch/util/ServiceCheckHelperTest.java b/src/test/java/org/datadog/jmxfetch/util/ServiceCheckHelperTest.java new file mode 100644 index 000000000..7363e5003 --- /dev/null +++ b/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])); + } + } +} diff --git a/src/test/resources/jmx_check_no_prefix.yaml b/src/test/resources/jmx_check_no_prefix.yaml new file mode 100644 index 000000000..b9251672c --- /dev/null +++ b/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 diff --git a/src/test/resources/jmx_check_prefix.yaml b/src/test/resources/jmx_check_prefix.yaml new file mode 100644 index 000000000..732133623 --- /dev/null +++ b/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