From 3cb51654d3bc3c0feb45b541ce28c6a65e6a79ff Mon Sep 17 00:00:00 2001 From: Carlos Roman Date: Fri, 31 May 2024 16:40:04 +0100 Subject: [PATCH 1/4] Fixing logger setup in TestCommon --- src/test/java/org/datadog/jmxfetch/TestCommon.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/datadog/jmxfetch/TestCommon.java b/src/test/java/org/datadog/jmxfetch/TestCommon.java index ff0bf445..84565872 100644 --- a/src/test/java/org/datadog/jmxfetch/TestCommon.java +++ b/src/test/java/org/datadog/jmxfetch/TestCommon.java @@ -73,7 +73,7 @@ public static void init() throws Exception { if (level == null) { level = "ALL"; } - CustomLogger.setup(LogLevel.ALL, "/tmp/jmxfetch_test.log", false); + CustomLogger.setup(LogLevel.fromString(level), "/tmp/jmxfetch_test.log", false); } /** From fb8f22fbd59e0147c53b72140da234e64c03cea4 Mon Sep 17 00:00:00 2001 From: Carlos Roman Date: Mon, 3 Jun 2024 09:46:23 +0100 Subject: [PATCH 2/4] Updating CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index abfe0559..a9eba045 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ Changelog ========= # 0.49.2-SNAPSHOT / TBC +* [BUGFIX] Telemetry is no longer initialized when telemetry is disabled [#522][] # 0.49.1 / 2024-04-09 * [FEATURE] Add ZGC Major and Minor Cycles and ZGC Major and Minor Pauses beans support out of the box (Generational ZGC support) [#509][] @@ -767,6 +768,7 @@ Changelog [#477]: https://github.com/DataDog/jmxfetch/issues/477 [#509]: https://github.com/DataDog/jmxfetch/issues/509 [#512]: https://github.com/DataDog/jmxfetch/pull/512 +[#522]: https://github.com/DataDog/jmxfetch/pull/522 [@alz]: https://github.com/alz [@aoking]: https://github.com/aoking [@arrawatia]: https://github.com/arrawatia From e29fb40399cb581c3f003a1c71ef3f621d6c389c Mon Sep 17 00:00:00 2001 From: Carlos Roman Date: Mon, 3 Jun 2024 09:46:43 +0100 Subject: [PATCH 3/4] Bumping Junit4 version to 4.13.2 --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index c82b9f0c..e93cd543 100644 --- a/pom.xml +++ b/pom.xml @@ -50,7 +50,7 @@ style.xml - 4.13.1 + 4.13.2 2.28.2 2.9 From 5ffabcc33bb6d6548f4819eb9dce7115b081e44d Mon Sep 17 00:00:00 2001 From: Carlos Roman Date: Mon, 3 Jun 2024 10:17:31 +0100 Subject: [PATCH 4/4] Skipping the registry of an MBean when telemetry not enabled --- src/main/java/org/datadog/jmxfetch/App.java | 29 ++++++++++++++- .../java/org/datadog/jmxfetch/AppConfig.java | 2 + .../java/org/datadog/jmxfetch/Instance.java | 30 ++++++++------- .../java/org/datadog/jmxfetch/TestApp.java | 37 ++++++------------- .../java/org/datadog/jmxfetch/TestCommon.java | 11 ++++-- .../datadog/jmxfetch/util/MetricsAssert.java | 6 +-- 6 files changed, 68 insertions(+), 47 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/App.java b/src/main/java/org/datadog/jmxfetch/App.java index 8c78e8ac..514610eb 100644 --- a/src/main/java/org/datadog/jmxfetch/App.java +++ b/src/main/java/org/datadog/jmxfetch/App.java @@ -133,7 +133,30 @@ public App(final AppConfig appConfig) { } this.configs = getConfigs(this.appConfig); - this.initTelemetryBean(); + this.appTelemetry = new AppTelemetry(); + + if (this.appConfig.getJmxfetchTelemetry()) { + log.info("Enabling JMX Fetch Telemetry"); + this.registerTelemetryBean(this.appTelemetry); + } + } + + private void registerTelemetryBean(AppTelemetry bean) { + MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); + ObjectName appTelemetryBeanName = getAppTelemetryBeanName(); + if (appTelemetryBeanName == null) { + return; + } + + try { + mbs.registerMBean(bean, appTelemetryBeanName); + log.debug("Succesfully registered app telemetry bean"); + } catch (InstanceAlreadyExistsException + | MBeanRegistrationException + | NotCompliantMBeanException e) { + log.warn("Could not register bean named '{}' for instance: ", + appTelemetryBeanName.toString(), e); + } } private ObjectName getAppTelemetryBeanName() { @@ -520,7 +543,9 @@ void start() { } void stop() { - this.teardownTelemetry(); + if (this.appConfig.getJmxfetchTelemetry()) { + this.teardownTelemetry(); + } this.collectionProcessor.stop(); this.recoveryProcessor.stop(); } diff --git a/src/main/java/org/datadog/jmxfetch/AppConfig.java b/src/main/java/org/datadog/jmxfetch/AppConfig.java index a01ff561..d8888efe 100644 --- a/src/main/java/org/datadog/jmxfetch/AppConfig.java +++ b/src/main/java/org/datadog/jmxfetch/AppConfig.java @@ -125,6 +125,8 @@ public class AppConfig { required = false) private boolean statsdTelemetry; + /* Do not default to true as this affects embedded mode where + customers can have custom MBean managers */ @Parameter( names = {"--jmxfetch_telemetry", "-jt"}, description = "Enable additional jmxfetch telemetry reporting", diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 9da8de75..dbb87152 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -240,7 +240,10 @@ public Instance( log.info("collect_default_jvm_metrics is false - not collecting default JVM metrics"); } - instanceTelemetryBean = createInstanceTelemetryBean(); + instanceTelemetryBean = new InstanceTelemetry(); + if (appConfig.getJmxfetchTelemetry()) { + registerTelemetryBean(instanceTelemetryBean); + } } private ObjectName getObjName(String domain,String instance) @@ -248,10 +251,9 @@ private ObjectName getObjName(String domain,String instance) return new ObjectName(domain + ":target_instance=" + ObjectName.quote(instance)); } - private InstanceTelemetry createInstanceTelemetryBean() { - mbs = ManagementFactory.getPlatformMBeanServer(); - InstanceTelemetry bean = new InstanceTelemetry(); - log.debug("Created jmx bean for instance: " + this.getCheckName()); + private InstanceTelemetry registerTelemetryBean(InstanceTelemetry bean) { + mbs = ManagementFactory.getPlatformMBeanServer(); + log.debug("Created jmx bean for instance: {}", this.getCheckName()); try { instanceTelemetryBeanName = getObjName(appConfig.getJmxfetchTelemetryDomain(), @@ -516,8 +518,8 @@ public List getMetrics() throws IOException { + " With beans fetched = " + instanceTelemetryBean.getBeansFetched() + " top attributes = " + instanceTelemetryBean.getTopLevelAttributeCount() + " metrics = " + instanceTelemetryBean.getMetricCount() - + " wildcard domain query count = " - + instanceTelemetryBean.getWildcardDomainQueryCount() + + " wildcard domain query count = " + + instanceTelemetryBean.getWildcardDomainQueryCount() + " bean match ratio = " + instanceTelemetryBean.getBeanMatchRatio()); } return metrics; @@ -710,7 +712,7 @@ private void getMatchingAttributes() throws IOException { reporter.displayNonMatchingAttributeName(jmxAttribute); } if (jmxAttribute.getMatchingConf() != null) { - attributeMatched = true; + attributeMatched = true; } } if (attributeMatched) { @@ -718,7 +720,7 @@ private void getMatchingAttributes() throws IOException { } } if (instanceTelemetryBean != null) { - instanceTelemetryBean.setBeanMatchRatio((double) + instanceTelemetryBean.setBeanMatchRatio((double) beansWithAttributeMatch / beans.size()); } log.info("Found {} matching attributes", matchingAttributes.size()); @@ -837,18 +839,21 @@ public boolean isLimitReached() { } private void cleanupTelemetryBean() { + if (!appConfig.getJmxfetchTelemetry()) { + // If telemetry is not enabled, no need to unregister the bean + return; + } try { mbs.unregisterMBean(instanceTelemetryBeanName); - log.debug("Successfully unregistered bean for instance: " + this.getCheckName()); + log.debug("Successfully unregistered bean for instance: {}", this.getCheckName()); } catch (MBeanRegistrationException | InstanceNotFoundException e) { - log.debug("Unable to unregister bean for instance: " + this.getCheckName()); + log.debug("Unable to unregister bean for instance: {}", this.getCheckName()); } } /** Clean up config and close connection. */ public void cleanUp() { cleanupTelemetryBean(); - this.appConfig = null; if (connection != null) { connection.closeConnector(); connection = null; @@ -859,7 +864,6 @@ public void cleanUp() { * Asynchronoush cleanup of instance, including connection. * */ public synchronized void cleanUpAsync() { - appConfig = null; cleanupTelemetryBean(); class AsyncCleaner implements Runnable { Connection conn; diff --git a/src/test/java/org/datadog/jmxfetch/TestApp.java b/src/test/java/org/datadog/jmxfetch/TestApp.java index 5fec8d25..2f39e584 100644 --- a/src/test/java/org/datadog/jmxfetch/TestApp.java +++ b/src/test/java/org/datadog/jmxfetch/TestApp.java @@ -61,10 +61,9 @@ public void testBeanTags() throws Exception { // Collecting metrics run(); - List> metrics = getMetrics(); // 14 = 13 metrics from java.lang + 1 metric explicitly defined in the yaml config file - assertEquals(14, metrics.size()); + assertEquals(14, getMetrics().size()); List tags = Arrays.asList( @@ -777,12 +776,10 @@ public void testAppCanonicalRate() throws Exception { initApplication("jmx_canonical.yaml"); run(); - List> metrics = getMetrics(); - // 29 = 13 metrics from java.lang + the 5 gauges we are explicitly collecting + 9 gauges // implicitly collected // + 2 multi-value, see jmx.yaml in the test/resources folder - assertEquals(29, metrics.size()); + assertEquals(29, getMetrics().size()); // We test for the presence and the value of the metrics we want to collect List commonTags = @@ -810,11 +807,10 @@ public void testAppCanonicalRate() throws Exception { // We run a second collection. The counter should now be present run(); - metrics = getMetrics(); // 30 = 13 metrics from java.lang + the 5 gauges we are explicitly collecting + 9 gauges // implicitly collected // + 2 multi-value + 2 counter, see jmx.yaml in the test/resources folder - assertEquals(31, metrics.size()); + assertEquals(31, getMetrics().size()); // We test for the same metrics but this time, the counter should be here // Previous metrics @@ -846,13 +842,11 @@ public void testAppCanonicalRate() throws Exception { testApp.decrementCounter(5); run(); - metrics = getMetrics(); - assertEquals(30, metrics.size()); + assertEquals(30, getMetrics().size()); // The metric should be back in the next cycle. run(); - metrics = getMetrics(); - assertEquals(31, metrics.size()); + assertEquals(31, getMetrics().size()); assertMetric("test.counter", 0.0, commonTags, 6); // Check that they are working again @@ -862,8 +856,7 @@ public void testAppCanonicalRate() throws Exception { testApp.populateTabularData(2); run(); - metrics = getMetrics(); - assertEquals(31, metrics.size()); + assertEquals(31, getMetrics().size()); // Previous metrics assertMetric("this.is.100", 100.0, commonTags, 9); @@ -903,20 +896,17 @@ public void testAppCount() throws Exception { // First collection should not contain our count run(); - metrics = getMetrics(); - assertEquals(13, metrics.size()); + assertEquals(13, getMetrics().size()); // Since our count is still equal to 0, we should report a delta equal to 0 run(); - metrics = getMetrics(); - assertEquals(14, metrics.size()); + assertEquals(14, getMetrics().size()); assertMetric("test.counter", 0, Collections.emptyList(), 4); // For the 3rd collection we increment the count to 5 so we should get a +5 delta testApp.incrementCounter(5); run(); - metrics = getMetrics(); - assertEquals(14, metrics.size()); + assertEquals(14, getMetrics().size()); assertMetric("test.counter", 5, Collections.emptyList(), 4); assertCoverage(); @@ -935,13 +925,11 @@ public void testAppCounterRate() throws Exception { // First collection should not contain our count run(); - metrics = getMetrics(); - assertEquals(26, metrics.size()); + assertEquals(26, getMetrics().size()); // Since our count is still equal to 0, we should report a delta equal to 0 run(); - metrics = getMetrics(); - assertEquals(28, metrics.size()); + assertEquals(28, getMetrics().size()); assertMetric("test.counter", 0, Collections.emptyList(), 4); assertMetric("test.rate", 0, Collections.emptyList(), 4); @@ -949,8 +937,7 @@ public void testAppCounterRate() throws Exception { // For the 3rd collection we increment the count to 5 so we should get a +5 delta testApp.incrementCounter(5); run(); - metrics = getMetrics(); - assertEquals(28, metrics.size()); + assertEquals(28, getMetrics().size()); assertMetric("test.counter", 0.95, 1, Collections.emptyList(), 4); assertMetric("test.rate", 0.95, 1, Collections.emptyList(), 4); diff --git a/src/test/java/org/datadog/jmxfetch/TestCommon.java b/src/test/java/org/datadog/jmxfetch/TestCommon.java index 84565872..4450c741 100644 --- a/src/test/java/org/datadog/jmxfetch/TestCommon.java +++ b/src/test/java/org/datadog/jmxfetch/TestCommon.java @@ -62,8 +62,8 @@ public class TestCommon { AppConfig appConfig = spy(AppConfig.builder().build()); App app; MBeanServer mbs; - List objectNames = new ArrayList(); - List> metrics; + List objectNames = new ArrayList<>(); + List> metrics = new ArrayList<>(); List> serviceChecks; /** Setup logger. */ @@ -104,6 +104,7 @@ public void unregisterMBeans() throws MBeanRegistrationException, InstanceNotFou for (ObjectName objectName : objectNames) { mbs.unregisterMBean(objectName); } + this.objectNames.clear(); } } @@ -112,6 +113,7 @@ public void unregisterMBeans() throws MBeanRegistrationException, InstanceNotFou */ @After public void teardown() { + this.metrics.clear(); if (app != null) { app.clearAllInstances(); app.stop(); @@ -214,8 +216,9 @@ protected void initApplicationWithYamlLines(String... yamlLines) /** Run a JMXFetch iteration. */ protected void run() { if (app != null) { + this.metrics.clear(); app.doIteration(); - metrics = ((ConsoleReporter) appConfig.getReporter()).getMetrics(); + this.metrics.addAll(((ConsoleReporter) appConfig.getReporter()).getMetrics()); } } @@ -382,7 +385,7 @@ public void assertCoverage() { } } - if (untestedMetrics.size() > 0) { + if (!untestedMetrics.isEmpty()) { String message = generateReport(untestedMetrics, totalMetrics); fail(message); } diff --git a/src/test/java/org/datadog/jmxfetch/util/MetricsAssert.java b/src/test/java/org/datadog/jmxfetch/util/MetricsAssert.java index 41ca0a8a..3302e5d6 100644 --- a/src/test/java/org/datadog/jmxfetch/util/MetricsAssert.java +++ b/src/test/java/org/datadog/jmxfetch/util/MetricsAssert.java @@ -48,14 +48,14 @@ public static void assertMetric( } if (countTags != -1) { - assertEquals(countTags, mTags.size()); + assertEquals("Tag count didn't match", countTags, mTags.size()); } for (String t : tags) { - assertThat(mTags, hasItem(t)); + assertThat("Did not contain tag", mTags, hasItem(t)); } if (metricType != null) { - assertEquals(metricType, m.get("type")); + assertEquals("Metric types not equal", metricType, m.get("type")); } // Brand the metric m.put("tested", true);