From 4fc47e2c836cc8967feec7ccb69e10857f3dcd60 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Fri, 7 Apr 2023 21:09:33 +0000 Subject: [PATCH 01/41] Initial bean listening, adds attributes that match config --- .../org/datadog/jmxfetch/BeanListener.java | 8 + .../java/org/datadog/jmxfetch/Connection.java | 63 +++++++ .../java/org/datadog/jmxfetch/Instance.java | 155 +++++++++++++++++- 3 files changed, 225 insertions(+), 1 deletion(-) create mode 100644 src/main/java/org/datadog/jmxfetch/BeanListener.java diff --git a/src/main/java/org/datadog/jmxfetch/BeanListener.java b/src/main/java/org/datadog/jmxfetch/BeanListener.java new file mode 100644 index 000000000..c30ab789b --- /dev/null +++ b/src/main/java/org/datadog/jmxfetch/BeanListener.java @@ -0,0 +1,8 @@ +package org.datadog.jmxfetch; + +import javax.management.ObjectName; + +public interface BeanListener { + public void beanRegistered(ObjectName mBeanName); + public void beanUnregistered(ObjectName mBeanName); +} diff --git a/src/main/java/org/datadog/jmxfetch/Connection.java b/src/main/java/org/datadog/jmxfetch/Connection.java index cb60e48d4..f1032c3dc 100644 --- a/src/main/java/org/datadog/jmxfetch/Connection.java +++ b/src/main/java/org/datadog/jmxfetch/Connection.java @@ -7,7 +7,9 @@ import java.net.SocketTimeoutException; import java.util.HashMap; import java.util.Map; +import java.util.List; import java.util.Set; +import java.util.function.Consumer; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.BlockingQueue; import java.util.concurrent.ExecutorService; @@ -21,8 +23,16 @@ import javax.management.MBeanAttributeInfo; import javax.management.MBeanException; import javax.management.MBeanServerConnection; +import javax.management.MBeanServerDelegate; +import javax.management.relation.MBeanServerNotificationFilter; +import javax.management.MBeanServerNotification; +import javax.management.MalformedObjectNameException; +import javax.management.Notification; +import javax.management.NotificationFilter; +import javax.management.NotificationListener; import javax.management.ObjectName; import javax.management.ReflectionException; +import javax.management.remote.JMXConnectionNotification; import javax.management.remote.JMXConnector; import javax.management.remote.JMXConnectorFactory; import javax.management.remote.JMXServiceURL; @@ -36,6 +46,56 @@ public class Connection { protected Map env; protected JMXServiceURL address; + private static class MyConnectionNotificationListener implements NotificationListener { + public void handleNotification(Notification notification, Object handback) { + if (!(notification instanceof JMXConnectionNotification)) { + return; + } + if (!(handback instanceof Connection)) { + return; + } + + JMXConnectionNotification connNotif = (JMXConnectionNotification) notification; + Connection conn = (Connection) handback; + if (connNotif.getType() == JMXConnectionNotification.CLOSED + || connNotif.getType() == JMXConnectionNotification.FAILED) { + //conn.closeConnector(); + } + log.info("Received connection notification: " + connNotif.getType() + " Message: " + connNotif.getMessage()); + } + } + + private static class BeanNotificationListener implements NotificationListener { + private BeanListener bl; + + public BeanNotificationListener(BeanListener bl) { + this.bl = bl; + } + public void handleNotification(Notification notification, Object handback) { + if (!(notification instanceof MBeanServerNotification)) { + log.warn("Got unknown notification, expected MBeanServerNotification but got {}", notification.getClass()); + } + MBeanServerNotification mbs = (MBeanServerNotification) notification; + ObjectName mBeanName = mbs.getMBeanName(); + // TODO run this beanRegistered/unRegistered in a new thread or threadpool + if (mbs.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { + bl.beanRegistered(mBeanName); + } else if (mbs.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { + this.bl.beanUnregistered(mBeanName); + } + } + } + + public void subscribeToBeanScopes(List beanScopes, BeanListener bl) throws MalformedObjectNameException, IOException, InstanceNotFoundException{ + BeanNotificationListener listener = new BeanNotificationListener(bl); + for (String scope : beanScopes) { + ObjectName name = new ObjectName(scope); + MBeanServerNotificationFilter filter = new MBeanServerNotificationFilter(); + filter.enableObjectName(name); + } + mbs.addNotificationListener(MBeanServerDelegate.DELEGATE_NAME, listener, null, null); + } + /** Gets attributes for matching bean name. */ public MBeanAttributeInfo[] getAttributesForBean(ObjectName beanName) throws InstanceNotFoundException, IntrospectionException, ReflectionException, @@ -63,6 +123,9 @@ protected void createConnection() throws IOException { log.info("Connecting to: " + this.address); connector = JMXConnectorFactory.connect(this.address, this.env); mbs = connector.getMBeanServerConnection(); + + NotificationListener listener = new MyConnectionNotificationListener(); + connector.addConnectionNotificationListener(listener, null, this); } /** Gets attribute for matching bean and attribute name. */ diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 45ebfecdc..e995d80b0 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -25,7 +25,7 @@ import javax.security.auth.login.FailedLoginException; @Slf4j -public class Instance { +public class Instance implements BeanListener{ private static final List SIMPLE_TYPES = Arrays.asList( "long", @@ -102,6 +102,7 @@ public Yaml initialValue() { private AppConfig appConfig; private Boolean cassandraAliasing; private boolean emptyDefaultHostname; + private boolean enableBeanSubscription; /** Constructor, instantiates Instance based of a previous instance and appConfig. */ public Instance(Instance instance, AppConfig appConfig) { @@ -256,6 +257,8 @@ public Instance( } else { log.info("collect_default_jvm_metrics is false - not collecting default JVM metrics"); } + this.enableBeanSubscription = true; + } public static boolean isDirectInstance(Map configInstance) { @@ -416,6 +419,10 @@ public void init(boolean forceNewConnection) throws IOException, FailedLoginException, SecurityException { log.info("Trying to connect to JMX Server at " + this.toString()); connection = getConnection(instanceMap, forceNewConnection); + if (this.enableBeanSubscription) { + log.info("Subscribing for bean notifications before init"); + this.subscribeToBeans(); + } log.info( "Trying to collect bean list for the first time for JMX Server at " + this.toString()); @@ -506,6 +513,113 @@ public boolean timeToCollect() { } } + // TODO de-dup this with getMatchingAttributes which means taking 'action' into account as well + private void addMatchingAttributes(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos, int maxToAdd) throws IOException { + int newlyAddedAttributes = 0; + for (MBeanAttributeInfo attributeInfo : attributeInfos) { + if (newlyAddedAttributes > maxToAdd) { + limitReached = true; + break; + } + JmxAttribute jmxAttribute; + String attributeType = attributeInfo.getType(); + if (SIMPLE_TYPES.contains(attributeType)) { + log.debug( + ATTRIBUTE + + beanName + + " : " + + attributeInfo + + " has attributeInfo simple type"); + jmxAttribute = + new JmxSimpleAttribute( + attributeInfo, + beanName, + className, + instanceName, + checkName, + connection, + serviceNameProvider, + tags, + cassandraAliasing, + emptyDefaultHostname); + } else if (COMPOSED_TYPES.contains(attributeType)) { + log.debug( + ATTRIBUTE + + beanName + + " : " + + attributeInfo + + " has attributeInfo composite type"); + jmxAttribute = + new JmxComplexAttribute( + attributeInfo, + beanName, + className, + instanceName, + checkName, + connection, + serviceNameProvider, + tags, + emptyDefaultHostname); + } else if (MULTI_TYPES.contains(attributeType)) { + log.debug( + ATTRIBUTE + + beanName + + " : " + + attributeInfo + + " has attributeInfo tabular type"); + jmxAttribute = + new JmxTabularAttribute( + attributeInfo, + beanName, + className, + instanceName, + checkName, + connection, + serviceNameProvider, + tags, + emptyDefaultHostname); + } else { + try { + log.debug( + ATTRIBUTE + + beanName + + " : " + + attributeInfo + + " has an unsupported type: " + + attributeType); + } catch (NullPointerException e) { + log.warn("Caught unexpected NullPointerException"); + } + continue; + } + + // For each attribute we try it with each configuration to see if there is one that + // matches + // If so, we store the attribute so metrics will be collected from it. Otherwise we + // discard it. + for (Configuration conf : configurationList) { + try { + if (jmxAttribute.match(conf)) { + jmxAttribute.setMatchingConf(conf); + newlyAddedAttributes += jmxAttribute.getMetricsCount(); + log.info("Added attribute {} from bean {}.", jmxAttribute.toString(), beanName); + this.matchingAttributes.add(jmxAttribute); + + break; + } + } catch (Exception e) { + log.error( + "Error while trying to match attributeInfo configuration " + + "with the Attribute: " + + beanName + + " : " + + attributeInfo, + e); + } + } + } + } + private void getMatchingAttributes() throws IOException { limitReached = false; Reporter reporter = appConfig.getReporter(); @@ -678,6 +792,45 @@ private void getMatchingAttributes() throws IOException { log.info("Found " + matchingAttributes.size() + " matching attributes"); } + // TODO once this runs in a separate thread we need locks around the attribute collection + public void beanRegistered(ObjectName beanName) { + log.info("Bean registered. {}", beanName); + String className; + MBeanAttributeInfo[] attributeInfos; + try { + log.debug("Getting class name for bean: " + beanName); + className = connection.getClassNameForBean(beanName); + + // Get all the attributes for bean_name + log.debug("Getting attributes for bean: " + beanName); + attributeInfos = connection.getAttributesForBean(beanName); + + int currentMetricCount = this.matchingAttributes.size(); + this.addMatchingAttributes(beanName, className, attributeInfos, maxReturnedMetrics - currentMetricCount); + } catch (IOException e) { + // Nothing to do, connection issue + log.warn("Could not connect to get bean attributes or class name: " + e.getMessage()); + return; + } catch (Exception e) { + log.warn("Cannot get bean attributes or class name: " + e.getMessage()); + return; + } + } + public void beanUnregistered(ObjectName mBeanName) { + log.info("Bean unregistered. {}", mBeanName); + } + + private void subscribeToBeans() { + List beanScopes = this.getBeansScopes(); + try { + log.info("Subscribing to {} bean scopes", beanScopes.size()); + + connection.subscribeToBeanScopes(beanScopes, this); + } catch (Exception e) { + log.warn("Exception while subscribing to beans {}", e); + } + } + /** Returns a list of strings listing the bean scopes. */ public List getBeansScopes() { if (this.beanScopes == null) { From e104ed20ca343bf205bf4de0abbec28107df2c67 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 10 Apr 2023 21:44:07 +0000 Subject: [PATCH 02/41] Moves bean subscription into dedicated thread --- .../java/org/datadog/jmxfetch/Instance.java | 155 +++++++++++++----- 1 file changed, 113 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index e995d80b0..1e1bee241 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -6,6 +6,10 @@ import org.datadog.jmxfetch.service.ServiceNameProvider; import org.yaml.snakeyaml.Yaml; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.locks.ReentrantLock; +import static java.util.concurrent.TimeUnit.*; + import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -103,6 +107,7 @@ public Yaml initialValue() { private Boolean cassandraAliasing; private boolean emptyDefaultHostname; private boolean enableBeanSubscription; + private ReentrantLock beanAttributeLock; /** Constructor, instantiates Instance based of a previous instance and appConfig. */ public Instance(Instance instance, AppConfig appConfig) { @@ -257,8 +262,10 @@ public Instance( } else { log.info("collect_default_jvm_metrics is false - not collecting default JVM metrics"); } - this.enableBeanSubscription = true; + this.beanAttributeLock = new ReentrantLock(); + Boolean enableBeanSubscription = (Boolean) instanceMap.get("enable_bean_subscription"); + this.enableBeanSubscription = enableBeanSubscription != null && enableBeanSubscription; } public static boolean isDirectInstance(Map configInstance) { @@ -422,14 +429,21 @@ public void init(boolean forceNewConnection) if (this.enableBeanSubscription) { log.info("Subscribing for bean notifications before init"); this.subscribeToBeans(); + } else { + log.info("Bean subscription not enabled."); } log.info( "Trying to collect bean list for the first time for JMX Server at " + this.toString()); - this.refreshBeansList(); - this.initialRefreshTime = this.lastRefreshTime; - log.info("Connected to JMX Server at " + this.toString()); - this.getMatchingAttributes(); + this.beanAttributeLock.lock(); + try { + this.refreshBeansList(); + this.initialRefreshTime = this.lastRefreshTime; + log.info("Connected to JMX Server at " + this.toString()); + this.getMatchingAttributes(); + } finally { + this.beanAttributeLock.unlock(); + } log.info("Done initializing JMX Server at " + this.toString()); } @@ -459,38 +473,49 @@ public List getMetrics() throws IOException { Integer period = (this.initialRefreshTime == this.lastRefreshTime) ? this.initialRefreshBeansPeriod : this.refreshBeansPeriod; - if (isPeriodDue(this.lastRefreshTime, period)) { - log.info("Refreshing bean list"); - this.refreshBeansList(); - this.getMatchingAttributes(); - } - List metrics = new ArrayList(); - Iterator it = matchingAttributes.iterator(); + this.beanAttributeLock.lock(); + try { + if (isPeriodDue(this.lastRefreshTime, period)) { + log.info("Refreshing bean list"); + this.refreshBeansList(); + this.getMatchingAttributes(); + } - // increment the lastCollectionTime - this.lastCollectionTime = System.currentTimeMillis(); + Iterator it = matchingAttributes.iterator(); - while (it.hasNext()) { - JmxAttribute jmxAttr = it.next(); - try { - List jmxAttrMetrics = jmxAttr.getMetrics(); - metrics.addAll(jmxAttrMetrics); - this.failingAttributes.remove(jmxAttr); - } catch (IOException e) { - throw e; - } catch (Exception e) { - log.debug("Cannot get metrics for attribute: " + jmxAttr, e); - if (this.failingAttributes.contains(jmxAttr)) { - log.debug( - "Cannot generate metrics for attribute: " - + jmxAttr - + " twice in a row. Removing it from the attribute list"); - it.remove(); - } else { - this.failingAttributes.add(jmxAttr); + // increment the lastCollectionTime + this.lastCollectionTime = System.currentTimeMillis(); + + log.info("Starting Collection of " + matchingAttributes.size() + " attributes"); + while (it.hasNext()) { + JmxAttribute jmxAttr = it.next(); + try { + List jmxAttrMetrics = jmxAttr.getMetrics(); + metrics.addAll(jmxAttrMetrics); + this.failingAttributes.remove(jmxAttr); + } catch (IOException e) { + log.warn("Got IOException {}, rethrowing...", e); + this.beanAttributeLock.unlock(); + throw e; + } catch (Exception e) { + log.warn("Cannot get metrics for attribute: " + jmxAttr, e); + if (this.failingAttributes.contains(jmxAttr)) { + log.debug( + "Cannot generate metrics for attribute: " + + jmxAttr + + " twice in a row. Removing it from the attribute list"); + it.remove(); + } else { + this.failingAttributes.add(jmxAttr); + } } } + + long duration = System.currentTimeMillis() - this.lastCollectionTime; + log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); + } finally { + this.beanAttributeLock.unlock(); } return metrics; } @@ -514,7 +539,7 @@ public boolean timeToCollect() { } // TODO de-dup this with getMatchingAttributes which means taking 'action' into account as well - private void addMatchingAttributes(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos, int maxToAdd) throws IOException { + private void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos, int maxToAdd) throws IOException { int newlyAddedAttributes = 0; for (MBeanAttributeInfo attributeInfo : attributeInfos) { if (newlyAddedAttributes > maxToAdd) { @@ -602,9 +627,13 @@ private void addMatchingAttributes(ObjectName beanName, String className, MBeanA if (jmxAttribute.match(conf)) { jmxAttribute.setMatchingConf(conf); newlyAddedAttributes += jmxAttribute.getMetricsCount(); - log.info("Added attribute {} from bean {}.", jmxAttribute.toString(), beanName); - this.matchingAttributes.add(jmxAttribute); - + log.debug("Added attribute {} from bean {}.", jmxAttribute.toString(), beanName); + this.beanAttributeLock.lock(); + try { + this.matchingAttributes.add(jmxAttribute); + } finally { + this.beanAttributeLock.unlock(); + } break; } } catch (Exception e) { @@ -620,7 +649,12 @@ private void addMatchingAttributes(ObjectName beanName, String className, MBeanA } } + // must be called while this.beanAttributeLock is held private void getMatchingAttributes() throws IOException { + if (!this.beanAttributeLock.isHeldByCurrentThread()) { + log.error("BAD CONCURRENCY, lock not held {}", this.beanAttributeLock.toString()); + throw new IOException("BAD CONCURRENCY"); + } limitReached = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); @@ -794,7 +828,7 @@ private void getMatchingAttributes() throws IOException { // TODO once this runs in a separate thread we need locks around the attribute collection public void beanRegistered(ObjectName beanName) { - log.info("Bean registered. {}", beanName); + log.debug("Bean registered event. {}", beanName); String className; MBeanAttributeInfo[] attributeInfos; try { @@ -806,7 +840,13 @@ public void beanRegistered(ObjectName beanName) { attributeInfos = connection.getAttributesForBean(beanName); int currentMetricCount = this.matchingAttributes.size(); - this.addMatchingAttributes(beanName, className, attributeInfos, maxReturnedMetrics - currentMetricCount); + this.addMatchingAttributesForBean(beanName, className, attributeInfos, maxReturnedMetrics - currentMetricCount); + this.beanAttributeLock.lock(); + try { + this.beans.add(beanName); + } finally { + this.beanAttributeLock.unlock(); + } } catch (IOException e) { // Nothing to do, connection issue log.warn("Could not connect to get bean attributes or class name: " + e.getMessage()); @@ -817,15 +857,46 @@ public void beanRegistered(ObjectName beanName) { } } public void beanUnregistered(ObjectName mBeanName) { - log.info("Bean unregistered. {}", mBeanName); + log.info("Bean unregistered event. {}", mBeanName); + } + + private class BeanSubscriber implements Runnable { + private List beanScopes; + private Connection connection; + private BeanListener listener; + public CompletableFuture subscriptionSuccessful; + + BeanSubscriber(List beanScopes, Connection connection, BeanListener listener) { + this.beanScopes = beanScopes; + this.connection = connection; + this.listener = listener; + this.subscriptionSuccessful = new CompletableFuture(); + } + + public void run() { + try { + log.info("Subscribing to {} bean scopes", beanScopes.size()); + + connection.subscribeToBeanScopes(beanScopes, this.listener); + this.subscriptionSuccessful.complete(true); + + Thread.currentThread().join(); + } catch (Exception e) { + log.warn("Exception while subscribing to beans {}", e); + this.subscriptionSuccessful.complete(false); + } + } } private void subscribeToBeans() { List beanScopes = this.getBeansScopes(); - try { - log.info("Subscribing to {} bean scopes", beanScopes.size()); + BeanSubscriber subscriber = new BeanSubscriber(beanScopes, this.connection, this); - connection.subscribeToBeanScopes(beanScopes, this); + try { + new Thread(subscriber).start(); + if (subscriber.subscriptionSuccessful.get(1, SECONDS)) { + log.info("Subscribed successfully!"); + } } catch (Exception e) { log.warn("Exception while subscribing to beans {}", e); } From 9fee19d710776eedc338465c3d7b331cf745eff2 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 10 Apr 2023 21:46:23 +0000 Subject: [PATCH 03/41] Removes ConnectionListener --- .../java/org/datadog/jmxfetch/Connection.java | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Connection.java b/src/main/java/org/datadog/jmxfetch/Connection.java index f1032c3dc..cddf35f7d 100644 --- a/src/main/java/org/datadog/jmxfetch/Connection.java +++ b/src/main/java/org/datadog/jmxfetch/Connection.java @@ -46,25 +46,6 @@ public class Connection { protected Map env; protected JMXServiceURL address; - private static class MyConnectionNotificationListener implements NotificationListener { - public void handleNotification(Notification notification, Object handback) { - if (!(notification instanceof JMXConnectionNotification)) { - return; - } - if (!(handback instanceof Connection)) { - return; - } - - JMXConnectionNotification connNotif = (JMXConnectionNotification) notification; - Connection conn = (Connection) handback; - if (connNotif.getType() == JMXConnectionNotification.CLOSED - || connNotif.getType() == JMXConnectionNotification.FAILED) { - //conn.closeConnector(); - } - log.info("Received connection notification: " + connNotif.getType() + " Message: " + connNotif.getMessage()); - } - } - private static class BeanNotificationListener implements NotificationListener { private BeanListener bl; @@ -77,7 +58,6 @@ public void handleNotification(Notification notification, Object handback) { } MBeanServerNotification mbs = (MBeanServerNotification) notification; ObjectName mBeanName = mbs.getMBeanName(); - // TODO run this beanRegistered/unRegistered in a new thread or threadpool if (mbs.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { bl.beanRegistered(mBeanName); } else if (mbs.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { @@ -123,9 +103,6 @@ protected void createConnection() throws IOException { log.info("Connecting to: " + this.address); connector = JMXConnectorFactory.connect(this.address, this.env); mbs = connector.getMBeanServerConnection(); - - NotificationListener listener = new MyConnectionNotificationListener(); - connector.addConnectionNotificationListener(listener, null, this); } /** Gets attribute for matching bean and attribute name. */ From 0b996dfa1e7f987dc36434e5e70ca3524247dad1 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 12 Apr 2023 19:33:37 +0000 Subject: [PATCH 04/41] Adds test covering basic bean subscription functionality --- .../org/datadog/jmxfetch/TestInstance.java | 38 +++++++++++++++++++ src/test/resources/jmx_bean_subscription.yaml | 14 +++++++ 2 files changed, 52 insertions(+) create mode 100644 src/test/resources/jmx_bean_subscription.yaml diff --git a/src/test/java/org/datadog/jmxfetch/TestInstance.java b/src/test/java/org/datadog/jmxfetch/TestInstance.java index f2a2a9596..e07622dad 100644 --- a/src/test/java/org/datadog/jmxfetch/TestInstance.java +++ b/src/test/java/org/datadog/jmxfetch/TestInstance.java @@ -216,4 +216,42 @@ public void testRefreshBeans() throws Exception { // 17 = 13 metrics from java.lang + 2 iteration=one + 2 iteration=two assertEquals(17, metrics.size()); } + + /** Tests bean_subscription */ + @Test + public void testBeanSubscription() throws Exception { + SimpleTestJavaApp testApp = new SimpleTestJavaApp(); + initApplication("jmx_bean_subscription.yaml"); + + // We do a first collection + run(); + List> metrics = getMetrics(); + + // 13 metrics from java.lang + assertEquals(13, metrics.size()); + + // We register an additional mbean + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); + log.info("sleeping before the next collection"); + Thread.sleep(1500); + + // We run a second collection. refresh_beans_initial should be due. + run(); + metrics = getMetrics(); + + // 15 = 13 metrics from java.lang + 2 iteration=one + assertEquals(15, metrics.size()); + + // We register additional mbean + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=two"); + log.info("sleeping before the next collection"); + Thread.sleep(1500); + + // We run a third collection. + run(); + metrics = getMetrics(); + + // 17 = 13 metrics from java.lang + 2 iteration=one + 2 iteration=two + assertEquals(17, metrics.size()); + } } diff --git a/src/test/resources/jmx_bean_subscription.yaml b/src/test/resources/jmx_bean_subscription.yaml new file mode 100644 index 000000000..65158170c --- /dev/null +++ b/src/test/resources/jmx_bean_subscription.yaml @@ -0,0 +1,14 @@ +init_config: + +instances: + - process_name_regex: .*surefire.* + min_collection_interval: 1 + enable_bean_subscription: true + refresh_beans: 50 + name: jmx_test_instance + conf: + - include: + domain: org.datadog.jmxfetch.test + attribute: + - ShouldBe100 + - ShouldBe1000 \ No newline at end of file From 75a1bbcddf19af1eafb832261258b1889124fcf9 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 12 Apr 2023 20:11:51 +0000 Subject: [PATCH 05/41] Consolidate attribute matching and take 'ACTION' into account --- .../java/org/datadog/jmxfetch/Instance.java | 185 ++++-------------- 1 file changed, 39 insertions(+), 146 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 1e1bee241..5ce5eeedf 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -86,6 +86,7 @@ public Yaml initialValue() { private List beanScopes; private List configurationList = new ArrayList(); private List matchingAttributes; + private int metricCountForMatchingAttributes; private HashSet failingAttributes; private Integer initialRefreshBeansPeriod; private Integer refreshBeansPeriod; @@ -139,6 +140,7 @@ public Instance( this.tags = getTagsMap(instanceMap.get("tags"), appConfig); this.checkName = checkName; this.matchingAttributes = new ArrayList(); + this.metricCountForMatchingAttributes = 0; this.failingAttributes = new HashSet(); if (appConfig.getRefreshBeansPeriod() == null) { this.refreshBeansPeriod = (Integer) instanceMap.get("refresh_beans"); @@ -511,7 +513,7 @@ public List getMetrics() throws IOException { } } } - + long duration = System.currentTimeMillis() - this.lastCollectionTime; log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); } finally { @@ -538,13 +540,23 @@ public boolean timeToCollect() { } } - // TODO de-dup this with getMatchingAttributes which means taking 'action' into account as well - private void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos, int maxToAdd) throws IOException { - int newlyAddedAttributes = 0; + private void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos) throws IOException { + boolean metricReachedDisplayed = false; + Reporter reporter = appConfig.getReporter(); + String action = appConfig.getAction(); + for (MBeanAttributeInfo attributeInfo : attributeInfos) { - if (newlyAddedAttributes > maxToAdd) { - limitReached = true; - break; + if (this.metricCountForMatchingAttributes >= maxReturnedMetrics) { + this.limitReached = true; + if (action.equals(AppConfig.ACTION_COLLECT)) { + log.warn("Maximum number of metrics reached."); + break; + } else if (!metricReachedDisplayed + && !action.equals(AppConfig.ACTION_LIST_COLLECTED) + && !action.equals(AppConfig.ACTION_LIST_NOT_MATCHING)) { + reporter.displayMetricReached(); + metricReachedDisplayed = true; + } } JmxAttribute jmxAttribute; String attributeType = attributeInfo.getType(); @@ -626,7 +638,7 @@ private void addMatchingAttributesForBean(ObjectName beanName, String className, try { if (jmxAttribute.match(conf)) { jmxAttribute.setMatchingConf(conf); - newlyAddedAttributes += jmxAttribute.getMetricsCount(); + this.metricCountForMatchingAttributes += jmxAttribute.getMetricsCount(); log.debug("Added attribute {} from bean {}.", jmxAttribute.toString(), beanName); this.beanAttributeLock.lock(); try { @@ -634,6 +646,15 @@ private void addMatchingAttributesForBean(ObjectName beanName, String className, } finally { this.beanAttributeLock.unlock(); } + if (action.equals(AppConfig.ACTION_LIST_EVERYTHING) + || action.equals(AppConfig.ACTION_LIST_MATCHING) + || action.equals(AppConfig.ACTION_LIST_COLLECTED) + && !limitReached + || action.equals(AppConfig.ACTION_LIST_LIMITED) + && limitReached) { + reporter.displayMatchingAttributeName( + jmxAttribute, this.metricCountForMatchingAttributes, maxReturnedMetrics); + } break; } } catch (Exception e) { @@ -646,6 +667,12 @@ private void addMatchingAttributesForBean(ObjectName beanName, String className, e); } } + + if (jmxAttribute.getMatchingConf() == null + && (action.equals(AppConfig.ACTION_LIST_EVERYTHING) + || action.equals(AppConfig.ACTION_LIST_NOT_MATCHING))) { + reporter.displayNonMatchingAttributeName(jmxAttribute); + } } } @@ -658,11 +685,9 @@ private void getMatchingAttributes() throws IOException { limitReached = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); - boolean metricReachedDisplayed = false; this.matchingAttributes.clear(); this.failingAttributes.clear(); - int metricsCount = 0; if (!action.equals(AppConfig.ACTION_COLLECT)) { reporter.displayInstanceName(this); @@ -675,9 +700,6 @@ private void getMatchingAttributes() throws IOException { break; } } - if (!connection.isAlive()) { - throw new IOException("Connection to application died during bean refresh"); - } String className; MBeanAttributeInfo[] attributeInfos; try { @@ -688,145 +710,17 @@ private void getMatchingAttributes() throws IOException { log.debug("Getting attributes for bean: " + beanName); attributeInfos = connection.getAttributesForBean(beanName); } catch (IOException e) { - // we should not continue - log.warn("Cannot get bean attributes or class name: " + e.getMessage()); - if (e.getMessage().equals(connection.CLOSED_CLIENT_CAUSE)) { - throw e; - } - continue; + throw e; } catch (Exception e) { log.warn("Cannot get bean attributes or class name: " + e.getMessage()); continue; } - for (MBeanAttributeInfo attributeInfo : attributeInfos) { - if (metricsCount >= maxReturnedMetrics) { - limitReached = true; - if (action.equals(AppConfig.ACTION_COLLECT)) { - log.warn("Maximum number of metrics reached."); - break; - } else if (!metricReachedDisplayed - && !action.equals(AppConfig.ACTION_LIST_COLLECTED) - && !action.equals(AppConfig.ACTION_LIST_NOT_MATCHING)) { - reporter.displayMetricReached(); - metricReachedDisplayed = true; - } - } - JmxAttribute jmxAttribute; - String attributeType = attributeInfo.getType(); - if (SIMPLE_TYPES.contains(attributeType)) { - log.debug( - ATTRIBUTE - + beanName - + " : " - + attributeInfo - + " has attributeInfo simple type"); - jmxAttribute = - new JmxSimpleAttribute( - attributeInfo, - beanName, - className, - instanceName, - checkName, - connection, - serviceNameProvider, - tags, - cassandraAliasing, - emptyDefaultHostname); - } else if (COMPOSED_TYPES.contains(attributeType)) { - log.debug( - ATTRIBUTE - + beanName - + " : " - + attributeInfo - + " has attributeInfo composite type"); - jmxAttribute = - new JmxComplexAttribute( - attributeInfo, - beanName, - className, - instanceName, - checkName, - connection, - serviceNameProvider, - tags, - emptyDefaultHostname); - } else if (MULTI_TYPES.contains(attributeType)) { - log.debug( - ATTRIBUTE - + beanName - + " : " - + attributeInfo - + " has attributeInfo tabular type"); - jmxAttribute = - new JmxTabularAttribute( - attributeInfo, - beanName, - className, - instanceName, - checkName, - connection, - serviceNameProvider, - tags, - emptyDefaultHostname); - } else { - try { - log.debug( - ATTRIBUTE - + beanName - + " : " - + attributeInfo - + " has an unsupported type: " - + attributeType); - } catch (NullPointerException e) { - log.warn("Caught unexpected NullPointerException"); - } - continue; - } - - // For each attribute we try it with each configuration to see if there is one that - // matches - // If so, we store the attribute so metrics will be collected from it. Otherwise we - // discard it. - for (Configuration conf : configurationList) { - try { - if (jmxAttribute.match(conf)) { - jmxAttribute.setMatchingConf(conf); - metricsCount += jmxAttribute.getMetricsCount(); - this.matchingAttributes.add(jmxAttribute); - - if (action.equals(AppConfig.ACTION_LIST_EVERYTHING) - || action.equals(AppConfig.ACTION_LIST_MATCHING) - || action.equals(AppConfig.ACTION_LIST_COLLECTED) - && !limitReached - || action.equals(AppConfig.ACTION_LIST_LIMITED) - && limitReached) { - reporter.displayMatchingAttributeName( - jmxAttribute, metricsCount, maxReturnedMetrics); - } - break; - } - } catch (Exception e) { - log.error( - "Error while trying to match attributeInfo configuration " - + "with the Attribute: " - + beanName - + " : " - + attributeInfo, - e); - } - } - if (jmxAttribute.getMatchingConf() == null - && (action.equals(AppConfig.ACTION_LIST_EVERYTHING) - || action.equals(AppConfig.ACTION_LIST_NOT_MATCHING))) { - reporter.displayNonMatchingAttributeName(jmxAttribute); - } - } + addMatchingAttributesForBean(beanName, className, attributeInfos); } - log.info("Found " + matchingAttributes.size() + " matching attributes"); + log.info("Found {} matching attributes, collecting {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); } - // TODO once this runs in a separate thread we need locks around the attribute collection public void beanRegistered(ObjectName beanName) { log.debug("Bean registered event. {}", beanName); String className; @@ -839,8 +733,7 @@ public void beanRegistered(ObjectName beanName) { log.debug("Getting attributes for bean: " + beanName); attributeInfos = connection.getAttributesForBean(beanName); - int currentMetricCount = this.matchingAttributes.size(); - this.addMatchingAttributesForBean(beanName, className, attributeInfos, maxReturnedMetrics - currentMetricCount); + this.addMatchingAttributesForBean(beanName, className, attributeInfos); this.beanAttributeLock.lock(); try { this.beans.add(beanName); From 9e624cfcffde82678ff9bf9cd82b3af41833127b Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 12 Apr 2023 20:30:39 +0000 Subject: [PATCH 06/41] Utilize 'synchronized' keyword for simpler locking --- .../java/org/datadog/jmxfetch/Instance.java | 118 +++++++----------- 1 file changed, 44 insertions(+), 74 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 5ce5eeedf..dce0504d3 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -7,7 +7,6 @@ import org.yaml.snakeyaml.Yaml; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.locks.ReentrantLock; import static java.util.concurrent.TimeUnit.*; import java.io.File; @@ -108,7 +107,6 @@ public Yaml initialValue() { private Boolean cassandraAliasing; private boolean emptyDefaultHostname; private boolean enableBeanSubscription; - private ReentrantLock beanAttributeLock; /** Constructor, instantiates Instance based of a previous instance and appConfig. */ public Instance(Instance instance, AppConfig appConfig) { @@ -265,7 +263,6 @@ public Instance( log.info("collect_default_jvm_metrics is false - not collecting default JVM metrics"); } - this.beanAttributeLock = new ReentrantLock(); Boolean enableBeanSubscription = (Boolean) instanceMap.get("enable_bean_subscription"); this.enableBeanSubscription = enableBeanSubscription != null && enableBeanSubscription; } @@ -424,7 +421,7 @@ public Connection getConnection( } /** Initializes the instance. May force a new connection.. */ - public void init(boolean forceNewConnection) + public synchronized void init(boolean forceNewConnection) throws IOException, FailedLoginException, SecurityException { log.info("Trying to connect to JMX Server at " + this.toString()); connection = getConnection(instanceMap, forceNewConnection); @@ -437,15 +434,10 @@ public void init(boolean forceNewConnection) log.info( "Trying to collect bean list for the first time for JMX Server at " + this.toString()); - this.beanAttributeLock.lock(); - try { - this.refreshBeansList(); - this.initialRefreshTime = this.lastRefreshTime; - log.info("Connected to JMX Server at " + this.toString()); - this.getMatchingAttributes(); - } finally { - this.beanAttributeLock.unlock(); - } + this.refreshBeansList(); + this.initialRefreshTime = this.lastRefreshTime; + log.info("Connected to JMX Server at " + this.toString()); + this.getMatchingAttributes(); log.info("Done initializing JMX Server at " + this.toString()); } @@ -466,8 +458,7 @@ public String toString() { } /** Returns a map of metrics collected. */ - public List getMetrics() throws IOException { - + public synchronized List getMetrics() throws IOException { // In case of ephemeral beans, we can force to refresh the bean list x seconds // post initialization and every x seconds thereafter. // To enable this, a "refresh_beans_initial" and/or "refresh_beans" parameters must be @@ -476,49 +467,43 @@ public List getMetrics() throws IOException { ? this.initialRefreshBeansPeriod : this.refreshBeansPeriod; List metrics = new ArrayList(); - this.beanAttributeLock.lock(); - try { - if (isPeriodDue(this.lastRefreshTime, period)) { - log.info("Refreshing bean list"); - this.refreshBeansList(); - this.getMatchingAttributes(); - } + if (isPeriodDue(this.lastRefreshTime, period)) { + log.info("Refreshing bean list"); + this.refreshBeansList(); + this.getMatchingAttributes(); + } - Iterator it = matchingAttributes.iterator(); + Iterator it = matchingAttributes.iterator(); - // increment the lastCollectionTime - this.lastCollectionTime = System.currentTimeMillis(); + // increment the lastCollectionTime + this.lastCollectionTime = System.currentTimeMillis(); - log.info("Starting Collection of " + matchingAttributes.size() + " attributes"); - while (it.hasNext()) { - JmxAttribute jmxAttr = it.next(); - try { - List jmxAttrMetrics = jmxAttr.getMetrics(); - metrics.addAll(jmxAttrMetrics); - this.failingAttributes.remove(jmxAttr); - } catch (IOException e) { - log.warn("Got IOException {}, rethrowing...", e); - this.beanAttributeLock.unlock(); - throw e; - } catch (Exception e) { - log.warn("Cannot get metrics for attribute: " + jmxAttr, e); - if (this.failingAttributes.contains(jmxAttr)) { - log.debug( - "Cannot generate metrics for attribute: " - + jmxAttr - + " twice in a row. Removing it from the attribute list"); - it.remove(); - } else { - this.failingAttributes.add(jmxAttr); - } + log.info("Starting Collection of " + matchingAttributes.size() + " attributes"); + while (it.hasNext()) { + JmxAttribute jmxAttr = it.next(); + try { + List jmxAttrMetrics = jmxAttr.getMetrics(); + metrics.addAll(jmxAttrMetrics); + this.failingAttributes.remove(jmxAttr); + } catch (IOException e) { + log.warn("Got IOException {}, rethrowing...", e); + throw e; + } catch (Exception e) { + log.warn("Cannot get metrics for attribute: " + jmxAttr, e); + if (this.failingAttributes.contains(jmxAttr)) { + log.debug( + "Cannot generate metrics for attribute: " + + jmxAttr + + " twice in a row. Removing it from the attribute list"); + it.remove(); + } else { + this.failingAttributes.add(jmxAttr); } } - - long duration = System.currentTimeMillis() - this.lastCollectionTime; - log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); - } finally { - this.beanAttributeLock.unlock(); } + + long duration = System.currentTimeMillis() - this.lastCollectionTime; + log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); return metrics; } @@ -540,7 +525,7 @@ public boolean timeToCollect() { } } - private void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos) throws IOException { + private synchronized void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos) throws IOException { boolean metricReachedDisplayed = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); @@ -640,12 +625,7 @@ private void addMatchingAttributesForBean(ObjectName beanName, String className, jmxAttribute.setMatchingConf(conf); this.metricCountForMatchingAttributes += jmxAttribute.getMetricsCount(); log.debug("Added attribute {} from bean {}.", jmxAttribute.toString(), beanName); - this.beanAttributeLock.lock(); - try { - this.matchingAttributes.add(jmxAttribute); - } finally { - this.beanAttributeLock.unlock(); - } + this.matchingAttributes.add(jmxAttribute); if (action.equals(AppConfig.ACTION_LIST_EVERYTHING) || action.equals(AppConfig.ACTION_LIST_MATCHING) || action.equals(AppConfig.ACTION_LIST_COLLECTED) @@ -676,12 +656,7 @@ private void addMatchingAttributesForBean(ObjectName beanName, String className, } } - // must be called while this.beanAttributeLock is held - private void getMatchingAttributes() throws IOException { - if (!this.beanAttributeLock.isHeldByCurrentThread()) { - log.error("BAD CONCURRENCY, lock not held {}", this.beanAttributeLock.toString()); - throw new IOException("BAD CONCURRENCY"); - } + private synchronized void getMatchingAttributes() throws IOException { limitReached = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); @@ -721,7 +696,7 @@ private void getMatchingAttributes() throws IOException { log.info("Found {} matching attributes, collecting {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); } - public void beanRegistered(ObjectName beanName) { + public synchronized void beanRegistered(ObjectName beanName) { log.debug("Bean registered event. {}", beanName); String className; MBeanAttributeInfo[] attributeInfos; @@ -734,12 +709,7 @@ public void beanRegistered(ObjectName beanName) { attributeInfos = connection.getAttributesForBean(beanName); this.addMatchingAttributesForBean(beanName, className, attributeInfos); - this.beanAttributeLock.lock(); - try { - this.beans.add(beanName); - } finally { - this.beanAttributeLock.unlock(); - } + this.beans.add(beanName); } catch (IOException e) { // Nothing to do, connection issue log.warn("Could not connect to get bean attributes or class name: " + e.getMessage()); @@ -749,7 +719,7 @@ public void beanRegistered(ObjectName beanName) { return; } } - public void beanUnregistered(ObjectName mBeanName) { + public synchronized void beanUnregistered(ObjectName mBeanName) { log.info("Bean unregistered event. {}", mBeanName); } @@ -807,7 +777,7 @@ public List getBeansScopes() { * Query and refresh the instance's list of beans. Limit the query scope when possible on * certain actions, and fallback if necessary. */ - private void refreshBeansList() throws IOException { + private synchronized void refreshBeansList() throws IOException { this.beans = new HashSet(); String action = appConfig.getAction(); boolean limitQueryScopes = From 75d9296744fce285aaac69b1d0a1f2569257bccf Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 12 Apr 2023 21:12:51 +0000 Subject: [PATCH 07/41] Adds audit logging to bean refresh to catch any gaps in bean subscription --- .../org/datadog/jmxfetch/BeanSubscriber.java | 29 ++++++ .../java/org/datadog/jmxfetch/Instance.java | 88 ++++++++++--------- 2 files changed, 77 insertions(+), 40 deletions(-) create mode 100644 src/main/java/org/datadog/jmxfetch/BeanSubscriber.java diff --git a/src/main/java/org/datadog/jmxfetch/BeanSubscriber.java b/src/main/java/org/datadog/jmxfetch/BeanSubscriber.java new file mode 100644 index 000000000..63ae360ba --- /dev/null +++ b/src/main/java/org/datadog/jmxfetch/BeanSubscriber.java @@ -0,0 +1,29 @@ +package org.datadog.jmxfetch; + +import java.util.concurrent.CompletableFuture; +import java.util.List; + +public class BeanSubscriber implements Runnable { + private List beanScopes; + private Connection connection; + private BeanListener listener; + public CompletableFuture subscriptionSuccessful; + + BeanSubscriber(List beanScopes, Connection connection, BeanListener listener) { + this.beanScopes = beanScopes; + this.connection = connection; + this.listener = listener; + this.subscriptionSuccessful = new CompletableFuture(); + } + + public void run() { + try { + connection.subscribeToBeanScopes(beanScopes, this.listener); + this.subscriptionSuccessful.complete(true); + + Thread.currentThread().join(); + } catch (Exception e) { + this.subscriptionSuccessful.complete(false); + } + } +} \ No newline at end of file diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index dce0504d3..c837a8ee5 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -6,7 +6,6 @@ import org.datadog.jmxfetch.service.ServiceNameProvider; import org.yaml.snakeyaml.Yaml; -import java.util.concurrent.CompletableFuture; import static java.util.concurrent.TimeUnit.*; import java.io.File; @@ -107,6 +106,7 @@ public Yaml initialValue() { private Boolean cassandraAliasing; private boolean emptyDefaultHostname; private boolean enableBeanSubscription; + private boolean beanSubscriptionActive; /** Constructor, instantiates Instance based of a previous instance and appConfig. */ public Instance(Instance instance, AppConfig appConfig) { @@ -263,8 +263,10 @@ public Instance( log.info("collect_default_jvm_metrics is false - not collecting default JVM metrics"); } + this.beans = new HashSet<>(); Boolean enableBeanSubscription = (Boolean) instanceMap.get("enable_bean_subscription"); this.enableBeanSubscription = enableBeanSubscription != null && enableBeanSubscription; + this.beanSubscriptionActive = false; } public static boolean isDirectInstance(Map configInstance) { @@ -425,16 +427,16 @@ public synchronized void init(boolean forceNewConnection) throws IOException, FailedLoginException, SecurityException { log.info("Trying to connect to JMX Server at " + this.toString()); connection = getConnection(instanceMap, forceNewConnection); + log.info( + "Trying to collect bean list for the first time for JMX Server at " + + this.toString()); + this.refreshBeansList(); if (this.enableBeanSubscription) { - log.info("Subscribing for bean notifications before init"); + log.info("Subscribing for bean notifications"); this.subscribeToBeans(); } else { log.info("Bean subscription not enabled."); } - log.info( - "Trying to collect bean list for the first time for JMX Server at " - + this.toString()); - this.refreshBeansList(); this.initialRefreshTime = this.lastRefreshTime; log.info("Connected to JMX Server at " + this.toString()); this.getMatchingAttributes(); @@ -723,33 +725,6 @@ public synchronized void beanUnregistered(ObjectName mBeanName) { log.info("Bean unregistered event. {}", mBeanName); } - private class BeanSubscriber implements Runnable { - private List beanScopes; - private Connection connection; - private BeanListener listener; - public CompletableFuture subscriptionSuccessful; - - BeanSubscriber(List beanScopes, Connection connection, BeanListener listener) { - this.beanScopes = beanScopes; - this.connection = connection; - this.listener = listener; - this.subscriptionSuccessful = new CompletableFuture(); - } - - public void run() { - try { - log.info("Subscribing to {} bean scopes", beanScopes.size()); - - connection.subscribeToBeanScopes(beanScopes, this.listener); - this.subscriptionSuccessful.complete(true); - - Thread.currentThread().join(); - } catch (Exception e) { - log.warn("Exception while subscribing to beans {}", e); - this.subscriptionSuccessful.complete(false); - } - } - } private void subscribeToBeans() { List beanScopes = this.getBeansScopes(); @@ -757,11 +732,18 @@ private void subscribeToBeans() { try { new Thread(subscriber).start(); - if (subscriber.subscriptionSuccessful.get(1, SECONDS)) { - log.info("Subscribed successfully!"); - } + this.beanSubscriptionActive = subscriber.subscriptionSuccessful.get(1, SECONDS); } catch (Exception e) { - log.warn("Exception while subscribing to beans {}", e); + log.warn("Error retrieving bean subscription value, assuming subscription failed. Exception: {}", e); + this.beanSubscriptionActive = false; + } + + if (this.beanSubscriptionActive) { + log.info("Subscribed to {} bean scopes successfully!", beanScopes.size()); + } else { + log.warn("Bean subscription failed! Will rely on bean_refresh, ensure it is set " + + " to an appropriate value (currently {} seconds)", + this.refreshBeansPeriod); } } @@ -778,27 +760,53 @@ public List getBeansScopes() { * certain actions, and fallback if necessary. */ private synchronized void refreshBeansList() throws IOException { - this.beans = new HashSet(); + Set newBeans = new HashSet<>(); String action = appConfig.getAction(); boolean limitQueryScopes = !action.equals(AppConfig.ACTION_LIST_EVERYTHING) && !action.equals(AppConfig.ACTION_LIST_NOT_MATCHING); + boolean fullBeanQueryNeeded = false; if (limitQueryScopes) { try { List beanScopes = getBeansScopes(); for (String scope : beanScopes) { ObjectName name = new ObjectName(scope); - this.beans.addAll(connection.queryNames(name)); + newBeans.addAll(connection.queryNames(name)); } } catch (Exception e) { + fullBeanQueryNeeded = true; log.error( "Unable to compute a common bean scope, querying all beans as a fallback", e); } } - this.beans = (this.beans.isEmpty()) ? connection.queryNames(null) : this.beans; + if (fullBeanQueryNeeded) { + newBeans = connection.queryNames(null); + } + if (this.beanSubscriptionActive && !fullBeanQueryNeeded) { + // This code exists to validate the bean-subscription is working properly + // If every new bean and de-registered bean properly fires an event, then + // this.beans (current set that has been updated via subscription) should + // always equal the new set of beans queried (unless it was a full bean query) + Set beansNotSeen = new HashSet<>(); + if (!this.beans.containsAll(newBeans)) { + beansNotSeen.addAll(newBeans); + beansNotSeen.removeAll(this.beans); + log.error("Bean refresh found {} new beans that were not already known via subscription", beansNotSeen.size()); + } + if (!newBeans.containsAll(this.beans)){ + beansNotSeen.addAll(this.beans); + beansNotSeen.removeAll(newBeans); + log.error("Bean refresh found {} FEWER beans than already known via subscription", beansNotSeen.size()); + } + + for (ObjectName b : beansNotSeen) { + log.error("Bean {} is one that has never been seen before, see why subscription did not detect this bean.", b.toString()); + } + } + this.beans = newBeans; this.lastRefreshTime = System.currentTimeMillis(); } From e4b2e6d6cad8524184bfbd6b55d8581e3fef6d11 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 17 Apr 2023 19:54:05 +0000 Subject: [PATCH 08/41] Shortens test timeframes for subscription to be processed --- src/test/java/org/datadog/jmxfetch/TestInstance.java | 10 +++++++--- src/test/resources/jmx_bean_subscription.yaml | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/datadog/jmxfetch/TestInstance.java b/src/test/java/org/datadog/jmxfetch/TestInstance.java index e07622dad..bb1b9678e 100644 --- a/src/test/java/org/datadog/jmxfetch/TestInstance.java +++ b/src/test/java/org/datadog/jmxfetch/TestInstance.java @@ -220,7 +220,12 @@ public void testRefreshBeans() throws Exception { /** Tests bean_subscription */ @Test public void testBeanSubscription() throws Exception { + // This delay is for the subscription thread to recieve the MBeanNotification + // and call into `Instance`. Conceptually this is just a Thread.yield() + int subscriptionDelay = 10; SimpleTestJavaApp testApp = new SimpleTestJavaApp(); + // Bean-refresh interval is to to 50s, so the only bean refresh will be + // initial fetch initApplication("jmx_bean_subscription.yaml"); // We do a first collection @@ -233,9 +238,8 @@ public void testBeanSubscription() throws Exception { // We register an additional mbean registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); log.info("sleeping before the next collection"); - Thread.sleep(1500); + Thread.sleep(subscriptionDelay); - // We run a second collection. refresh_beans_initial should be due. run(); metrics = getMetrics(); @@ -245,7 +249,7 @@ public void testBeanSubscription() throws Exception { // We register additional mbean registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=two"); log.info("sleeping before the next collection"); - Thread.sleep(1500); + Thread.sleep(subscriptionDelay); // We run a third collection. run(); diff --git a/src/test/resources/jmx_bean_subscription.yaml b/src/test/resources/jmx_bean_subscription.yaml index 65158170c..d0f883ba0 100644 --- a/src/test/resources/jmx_bean_subscription.yaml +++ b/src/test/resources/jmx_bean_subscription.yaml @@ -2,7 +2,7 @@ init_config: instances: - process_name_regex: .*surefire.* - min_collection_interval: 1 + min_collection_interval: null enable_bean_subscription: true refresh_beans: 50 name: jmx_test_instance From ff1596792cfe7366d1b30b23ebc28413526128a9 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 17 Apr 2023 19:59:50 +0000 Subject: [PATCH 09/41] Back to explicit locking --- .../java/org/datadog/jmxfetch/Instance.java | 320 ++++++++++++------ 1 file changed, 209 insertions(+), 111 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index c837a8ee5..5bf2a6ee1 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -26,6 +26,8 @@ import javax.management.ObjectName; import javax.security.auth.login.FailedLoginException; +import java.util.concurrent.locks.ReentrantLock; + @Slf4j public class Instance implements BeanListener{ private static final List SIMPLE_TYPES = @@ -107,6 +109,7 @@ public Yaml initialValue() { private boolean emptyDefaultHostname; private boolean enableBeanSubscription; private boolean beanSubscriptionActive; + private DebugReentrantLock beanAndAttributeLock; /** Constructor, instantiates Instance based of a previous instance and appConfig. */ public Instance(Instance instance, AppConfig appConfig) { @@ -267,6 +270,7 @@ public Instance( Boolean enableBeanSubscription = (Boolean) instanceMap.get("enable_bean_subscription"); this.enableBeanSubscription = enableBeanSubscription != null && enableBeanSubscription; this.beanSubscriptionActive = false; + this.beanAndAttributeLock = new DebugReentrantLock(false); } public static boolean isDirectInstance(Map configInstance) { @@ -422,25 +426,86 @@ public Connection getConnection( return connection; } + private class LockHolder implements AutoCloseable { + private final ReentrantLock lock; + + public LockHolder(ReentrantLock lock) { + this.lock = lock; + + lock.lock(); + } + + @Override + public void close() { + /* + log.info("Releasing lock from thread {}, lockCount is {}", Thread.currentThread().toString(), lock.getHoldCount()); + for (StackTraceElement elem : Thread.currentThread().getStackTrace()) { + log.info("\t{}", elem.toString()); + } */ + lock.unlock(); + //log.info("Lock released, lockCount is {}", lock.getHoldCount()); + } + } + + private class DebugReentrantLock extends ReentrantLock { + private boolean verbose; + public DebugReentrantLock(boolean verbose) { + this.verbose = verbose; + } + @Override + public void lock() { + if (this.verbose) { + log.info("About to acquire lock from thread {}, lockCount is {}", Thread.currentThread().toString(), this.getHoldCount()); + for (StackTraceElement elem : Thread.currentThread().getStackTrace()) { + log.info("\t{}", elem.toString()); + } + } + super.lock(); + if (this.verbose) { + log.info("Lock acquired, lockCount is {}", this.getHoldCount()); + } + } + @Override + public void unlock() { + if (this.verbose) { + log.info("Releasing lock from thread {}, lockCount is {}", Thread.currentThread().toString(), this.getHoldCount()); + for (StackTraceElement elem : Thread.currentThread().getStackTrace()) { + log.info("\t{}", elem.toString()); + } + } + super.unlock(); + + if (this.verbose) { + log.info("Lock released, lockCount is {}", this.getHoldCount()); + } + } + public Thread getOwningThread() { + return getOwner(); + } + } + /** Initializes the instance. May force a new connection.. */ - public synchronized void init(boolean forceNewConnection) + public void init(boolean forceNewConnection) throws IOException, FailedLoginException, SecurityException { log.info("Trying to connect to JMX Server at " + this.toString()); connection = getConnection(instanceMap, forceNewConnection); - log.info( - "Trying to collect bean list for the first time for JMX Server at " - + this.toString()); - this.refreshBeansList(); if (this.enableBeanSubscription) { log.info("Subscribing for bean notifications"); this.subscribeToBeans(); } else { log.info("Bean subscription not enabled."); } - this.initialRefreshTime = this.lastRefreshTime; - log.info("Connected to JMX Server at " + this.toString()); - this.getMatchingAttributes(); - log.info("Done initializing JMX Server at " + this.toString()); + log.info( + "Trying to collect bean list for the first time for JMX Server at " + + this.toString()); + + try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + this.refreshBeansList(true); + this.initialRefreshTime = this.lastRefreshTime; + log.info("Connected to JMX Server at " + this.toString()); + this.getMatchingAttributes(); + log.info("Done initializing JMX Server at " + this.toString()); + } } /** Returns a string representation for the instance. */ @@ -460,52 +525,73 @@ public String toString() { } /** Returns a map of metrics collected. */ - public synchronized List getMetrics() throws IOException { + public List getMetrics() throws IOException { // In case of ephemeral beans, we can force to refresh the bean list x seconds // post initialization and every x seconds thereafter. // To enable this, a "refresh_beans_initial" and/or "refresh_beans" parameters must be // specified in the yaml/json config + log.debug("getMetrics invoked"); Integer period = (this.initialRefreshTime == this.lastRefreshTime) ? this.initialRefreshBeansPeriod : this.refreshBeansPeriod; List metrics = new ArrayList(); if (isPeriodDue(this.lastRefreshTime, period)) { log.info("Refreshing bean list"); - this.refreshBeansList(); - this.getMatchingAttributes(); + try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + this.refreshBeansList(false); + this.getMatchingAttributes(); + } } - Iterator it = matchingAttributes.iterator(); - - // increment the lastCollectionTime - this.lastCollectionTime = System.currentTimeMillis(); - - log.info("Starting Collection of " + matchingAttributes.size() + " attributes"); - while (it.hasNext()) { - JmxAttribute jmxAttr = it.next(); - try { - List jmxAttrMetrics = jmxAttr.getMetrics(); - metrics.addAll(jmxAttrMetrics); - this.failingAttributes.remove(jmxAttr); - } catch (IOException e) { - log.warn("Got IOException {}, rethrowing...", e); - throw e; - } catch (Exception e) { - log.warn("Cannot get metrics for attribute: " + jmxAttr, e); - if (this.failingAttributes.contains(jmxAttr)) { - log.debug( - "Cannot generate metrics for attribute: " - + jmxAttr - + " twice in a row. Removing it from the attribute list"); - it.remove(); - } else { - this.failingAttributes.add(jmxAttr); + if (this.beanAndAttributeLock.isLocked() && !this.beanAndAttributeLock.isHeldByCurrentThread()) { + log.debug("Waiting for lock, isLocked {} isHeldByCurrentThread {} holdCount {}", this.beanAndAttributeLock.isLocked(), this.beanAndAttributeLock.isHeldByCurrentThread(), this.beanAndAttributeLock.getHoldCount()); + Thread owner = this.beanAndAttributeLock.getOwningThread(); + if (owner != null) { + log.debug("Owning thread is {} ", owner.getName()); + } + Map allStacks = Thread.getAllStackTraces(); + for (Map.Entry entry : allStacks.entrySet()) { + log.info("Thread: {} {}", entry.getKey().toString(), (entry.getKey() == owner ? "THIS IS THE LOCK" : "")); + + for (StackTraceElement elem : entry.getValue()) { + log.info("\t{}", elem.toString()); } } } + try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + log.debug("Got the lock"); + Iterator it = matchingAttributes.iterator(); + + // increment the lastCollectionTime + this.lastCollectionTime = System.currentTimeMillis(); - long duration = System.currentTimeMillis() - this.lastCollectionTime; - log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); + log.info("Starting Collection of " + matchingAttributes.size() + " attributes"); + while (it.hasNext()) { + JmxAttribute jmxAttr = it.next(); + try { + List jmxAttrMetrics = jmxAttr.getMetrics(); + metrics.addAll(jmxAttrMetrics); + this.failingAttributes.remove(jmxAttr); + } catch (IOException e) { + log.warn("Got IOException {}, rethrowing...", e); + throw e; + } catch (Exception e) { + log.warn("Cannot get metrics for attribute: " + jmxAttr, e); + if (this.failingAttributes.contains(jmxAttr)) { + log.debug( + "Cannot generate metrics for attribute: " + + jmxAttr + + " twice in a row. Removing it from the attribute list"); + it.remove(); + } else { + this.failingAttributes.add(jmxAttr); + } + } + } + + long duration = System.currentTimeMillis() - this.lastCollectionTime; + log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); + } return metrics; } @@ -527,7 +613,11 @@ public boolean timeToCollect() { } } - private synchronized void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos) throws IOException { + private void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos) throws IOException { + if (!this.beanAndAttributeLock.isHeldByCurrentThread()) { + log.error("BAD CONCURRENCY - lock should have been held but was not"); + throw new IOException("BAD CONC"); + } boolean metricReachedDisplayed = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); @@ -658,47 +748,50 @@ private synchronized void addMatchingAttributesForBean(ObjectName beanName, Stri } } - private synchronized void getMatchingAttributes() throws IOException { + private void getMatchingAttributes() throws IOException { limitReached = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); - this.matchingAttributes.clear(); - this.failingAttributes.clear(); + try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + this.matchingAttributes.clear(); + this.failingAttributes.clear(); - if (!action.equals(AppConfig.ACTION_COLLECT)) { - reporter.displayInstanceName(this); - } + if (!action.equals(AppConfig.ACTION_COLLECT)) { + reporter.displayInstanceName(this); + } - for (ObjectName beanName : beans) { - if (limitReached) { - log.debug("Limit reached"); - if (action.equals(AppConfig.ACTION_COLLECT)) { - break; + for (ObjectName beanName : beans) { + if (limitReached) { + log.debug("Limit reached"); + if (action.equals(AppConfig.ACTION_COLLECT)) { + break; + } + } + String className; + MBeanAttributeInfo[] attributeInfos; + try { + log.debug("Getting class name for bean: " + beanName); + className = connection.getClassNameForBean(beanName); + + // Get all the attributes for bean_name + log.debug("Getting attributes for bean: " + beanName); + attributeInfos = connection.getAttributesForBean(beanName); + } catch (IOException e) { + throw e; + } catch (Exception e) { + log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); + continue; } - } - String className; - MBeanAttributeInfo[] attributeInfos; - try { - log.debug("Getting class name for bean: " + beanName); - className = connection.getClassNameForBean(beanName); - - // Get all the attributes for bean_name - log.debug("Getting attributes for bean: " + beanName); - attributeInfos = connection.getAttributesForBean(beanName); - } catch (IOException e) { - throw e; - } catch (Exception e) { - log.warn("Cannot get bean attributes or class name: " + e.getMessage()); - continue; - } - addMatchingAttributesForBean(beanName, className, attributeInfos); + addMatchingAttributesForBean(beanName, className, attributeInfos); + } + log.info("Found {} matching attributes, collecting {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); } - log.info("Found {} matching attributes, collecting {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); } - public synchronized void beanRegistered(ObjectName beanName) { + public void beanRegistered(ObjectName beanName) { + this.beanAndAttributeLock.lock(); log.debug("Bean registered event. {}", beanName); String className; MBeanAttributeInfo[] attributeInfos; @@ -715,14 +808,16 @@ public synchronized void beanRegistered(ObjectName beanName) { } catch (IOException e) { // Nothing to do, connection issue log.warn("Could not connect to get bean attributes or class name: " + e.getMessage()); - return; } catch (Exception e) { - log.warn("Cannot get bean attributes or class name: " + e.getMessage()); - return; + log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); + } finally { + this.beanAndAttributeLock.unlock(); } } - public synchronized void beanUnregistered(ObjectName mBeanName) { - log.info("Bean unregistered event. {}", mBeanName); + public void beanUnregistered(ObjectName mBeanName) { + try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + log.info("Bean unregistered event. {}", mBeanName); + } } @@ -759,55 +854,58 @@ public List getBeansScopes() { * Query and refresh the instance's list of beans. Limit the query scope when possible on * certain actions, and fallback if necessary. */ - private synchronized void refreshBeansList() throws IOException { + private void refreshBeansList(boolean isInitialQuery) throws IOException { Set newBeans = new HashSet<>(); String action = appConfig.getAction(); boolean limitQueryScopes = !action.equals(AppConfig.ACTION_LIST_EVERYTHING) && !action.equals(AppConfig.ACTION_LIST_NOT_MATCHING); - boolean fullBeanQueryNeeded = false; - if (limitQueryScopes) { - try { - List beanScopes = getBeansScopes(); - for (String scope : beanScopes) { - ObjectName name = new ObjectName(scope); - newBeans.addAll(connection.queryNames(name)); + try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + + boolean fullBeanQueryNeeded = false; + if (limitQueryScopes) { + try { + List beanScopes = getBeansScopes(); + for (String scope : beanScopes) { + ObjectName name = new ObjectName(scope); + newBeans.addAll(connection.queryNames(name)); + } + } catch (Exception e) { + fullBeanQueryNeeded = true; + log.error( + "Unable to compute a common bean scope, querying all beans as a fallback", + e); } - } catch (Exception e) { - fullBeanQueryNeeded = true; - log.error( - "Unable to compute a common bean scope, querying all beans as a fallback", - e); } - } - if (fullBeanQueryNeeded) { - newBeans = connection.queryNames(null); - } - if (this.beanSubscriptionActive && !fullBeanQueryNeeded) { - // This code exists to validate the bean-subscription is working properly - // If every new bean and de-registered bean properly fires an event, then - // this.beans (current set that has been updated via subscription) should - // always equal the new set of beans queried (unless it was a full bean query) - Set beansNotSeen = new HashSet<>(); - if (!this.beans.containsAll(newBeans)) { - beansNotSeen.addAll(newBeans); - beansNotSeen.removeAll(this.beans); - log.error("Bean refresh found {} new beans that were not already known via subscription", beansNotSeen.size()); - } - if (!newBeans.containsAll(this.beans)){ - beansNotSeen.addAll(this.beans); - beansNotSeen.removeAll(newBeans); - log.error("Bean refresh found {} FEWER beans than already known via subscription", beansNotSeen.size()); + if (fullBeanQueryNeeded) { + newBeans = connection.queryNames(null); } + if (this.beanSubscriptionActive && !fullBeanQueryNeeded && !isInitialQuery) { + // This code exists to validate the bean-subscription is working properly + // If every new bean and de-registered bean properly fires an event, then + // this.beans (current set that has been updated via subscription) should + // always equal the new set of beans queried (unless it was a full bean query) + Set beansNotSeen = new HashSet<>(); + if (!this.beans.containsAll(newBeans)) { + beansNotSeen.addAll(newBeans); + beansNotSeen.removeAll(this.beans); + log.error("Bean refresh found {} new beans that were not already known via subscription", beansNotSeen.size()); + } + if (!newBeans.containsAll(this.beans)){ + beansNotSeen.addAll(this.beans); + beansNotSeen.removeAll(newBeans); + log.error("Bean refresh found {} FEWER beans than already known via subscription", beansNotSeen.size()); + } - for (ObjectName b : beansNotSeen) { - log.error("Bean {} is one that has never been seen before, see why subscription did not detect this bean.", b.toString()); + for (ObjectName b : beansNotSeen) { + log.error("Bean {} is one that has never been seen before, see why subscription did not detect this bean.", b.toString()); + } } + this.beans = newBeans; + this.lastRefreshTime = System.currentTimeMillis(); } - this.beans = newBeans; - this.lastRefreshTime = System.currentTimeMillis(); } /** Returns a string array listing the service check tags. */ From e0600d30b98175d32129fd85d3dda9f0736c0cbf Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 17 Apr 2023 20:59:54 +0000 Subject: [PATCH 10/41] Removes un-necessary lockholder class --- .../java/org/datadog/jmxfetch/Instance.java | 70 +++++++++---------- 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 5bf2a6ee1..c9478ac13 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -426,27 +426,6 @@ public Connection getConnection( return connection; } - private class LockHolder implements AutoCloseable { - private final ReentrantLock lock; - - public LockHolder(ReentrantLock lock) { - this.lock = lock; - - lock.lock(); - } - - @Override - public void close() { - /* - log.info("Releasing lock from thread {}, lockCount is {}", Thread.currentThread().toString(), lock.getHoldCount()); - for (StackTraceElement elem : Thread.currentThread().getStackTrace()) { - log.info("\t{}", elem.toString()); - } */ - lock.unlock(); - //log.info("Lock released, lockCount is {}", lock.getHoldCount()); - } - } - private class DebugReentrantLock extends ReentrantLock { private boolean verbose; public DebugReentrantLock(boolean verbose) { @@ -499,12 +478,15 @@ public void init(boolean forceNewConnection) "Trying to collect bean list for the first time for JMX Server at " + this.toString()); - try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + this.beanAndAttributeLock.lock(); + try { this.refreshBeansList(true); this.initialRefreshTime = this.lastRefreshTime; log.info("Connected to JMX Server at " + this.toString()); this.getMatchingAttributes(); log.info("Done initializing JMX Server at " + this.toString()); + } finally { + this.beanAndAttributeLock.unlock(); } } @@ -531,18 +513,6 @@ public List getMetrics() throws IOException { // To enable this, a "refresh_beans_initial" and/or "refresh_beans" parameters must be // specified in the yaml/json config log.debug("getMetrics invoked"); - Integer period = (this.initialRefreshTime == this.lastRefreshTime) - ? this.initialRefreshBeansPeriod : this.refreshBeansPeriod; - - List metrics = new ArrayList(); - if (isPeriodDue(this.lastRefreshTime, period)) { - log.info("Refreshing bean list"); - try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { - this.refreshBeansList(false); - this.getMatchingAttributes(); - } - } - if (this.beanAndAttributeLock.isLocked() && !this.beanAndAttributeLock.isHeldByCurrentThread()) { log.debug("Waiting for lock, isLocked {} isHeldByCurrentThread {} holdCount {}", this.beanAndAttributeLock.isLocked(), this.beanAndAttributeLock.isHeldByCurrentThread(), this.beanAndAttributeLock.getHoldCount()); Thread owner = this.beanAndAttributeLock.getOwningThread(); @@ -558,8 +528,21 @@ public List getMetrics() throws IOException { } } } - try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + + Integer period = (this.initialRefreshTime == this.lastRefreshTime) + ? this.initialRefreshBeansPeriod : this.refreshBeansPeriod; + + List metrics = new ArrayList(); + this.beanAndAttributeLock.lock(); + try { log.debug("Got the lock"); + + if (isPeriodDue(this.lastRefreshTime, period)) { + log.info("Refreshing bean list"); + this.refreshBeansList(false); + this.getMatchingAttributes(); + } + Iterator it = matchingAttributes.iterator(); // increment the lastCollectionTime @@ -591,6 +574,8 @@ public List getMetrics() throws IOException { long duration = System.currentTimeMillis() - this.lastCollectionTime; log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); + } finally { + this.beanAndAttributeLock.unlock(); } return metrics; } @@ -753,7 +738,8 @@ private void getMatchingAttributes() throws IOException { Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); - try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + this.beanAndAttributeLock.lock(); + try { this.matchingAttributes.clear(); this.failingAttributes.clear(); @@ -787,6 +773,8 @@ private void getMatchingAttributes() throws IOException { addMatchingAttributesForBean(beanName, className, attributeInfos); } log.info("Found {} matching attributes, collecting {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); + } finally { + this.beanAndAttributeLock.unlock(); } } @@ -815,8 +803,11 @@ public void beanRegistered(ObjectName beanName) { } } public void beanUnregistered(ObjectName mBeanName) { - try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + this.beanAndAttributeLock.lock(); + try { log.info("Bean unregistered event. {}", mBeanName); + } finally { + this.beanAndAttributeLock.unlock(); } } @@ -861,7 +852,8 @@ private void refreshBeansList(boolean isInitialQuery) throws IOException { !action.equals(AppConfig.ACTION_LIST_EVERYTHING) && !action.equals(AppConfig.ACTION_LIST_NOT_MATCHING); - try (LockHolder ignored = new LockHolder(this.beanAndAttributeLock)) { + this.beanAndAttributeLock.lock(); + try { boolean fullBeanQueryNeeded = false; if (limitQueryScopes) { @@ -905,6 +897,8 @@ private void refreshBeansList(boolean isInitialQuery) throws IOException { } this.beans = newBeans; this.lastRefreshTime = System.currentTimeMillis(); + } finally { + this.beanAndAttributeLock.unlock(); } } From 64d843811722243e07de9e56190914d9d9073b70 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 17 Apr 2023 22:00:56 +0000 Subject: [PATCH 11/41] implements bean unsubscription --- .../java/org/datadog/jmxfetch/Connection.java | 2 + .../java/org/datadog/jmxfetch/Instance.java | 22 ++++++++- .../org/datadog/jmxfetch/JmxAttribute.java | 18 ++++++- .../datadog/jmxfetch/JmxComplexAttribute.java | 2 +- .../datadog/jmxfetch/JmxSimpleAttribute.java | 2 +- .../datadog/jmxfetch/JmxTabularAttribute.java | 2 +- .../java/org/datadog/jmxfetch/TestCommon.java | 22 ++++++++- .../org/datadog/jmxfetch/TestInstance.java | 47 +++++++++++++++++++ 8 files changed, 111 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Connection.java b/src/main/java/org/datadog/jmxfetch/Connection.java index cddf35f7d..3886024db 100644 --- a/src/main/java/org/datadog/jmxfetch/Connection.java +++ b/src/main/java/org/datadog/jmxfetch/Connection.java @@ -55,8 +55,10 @@ public BeanNotificationListener(BeanListener bl) { public void handleNotification(Notification notification, Object handback) { if (!(notification instanceof MBeanServerNotification)) { log.warn("Got unknown notification, expected MBeanServerNotification but got {}", notification.getClass()); + return; } MBeanServerNotification mbs = (MBeanServerNotification) notification; + log.debug("MBeanNotification: ts {} seqNum: {} msg: '{}' userData: {} ", mbs.getTimeStamp(), mbs.getSequenceNumber(), mbs.getMessage(), mbs.getUserData()); ObjectName mBeanName = mbs.getMBeanName(); if (mbs.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { bl.beanRegistered(mBeanName); diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index c9478ac13..34ce2af48 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -566,6 +566,8 @@ public List getMetrics() throws IOException { + jmxAttr + " twice in a row. Removing it from the attribute list"); it.remove(); + this.failingAttributes.remove(jmxAttr); + this.metricCountForMatchingAttributes -= jmxAttr.getLastMetricsCount(); } else { this.failingAttributes.add(jmxAttr); } @@ -799,14 +801,27 @@ public void beanRegistered(ObjectName beanName) { } catch (Exception e) { log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); } finally { + log.debug("Bean registration processed. {}", beanName); this.beanAndAttributeLock.unlock(); } } public void beanUnregistered(ObjectName mBeanName) { this.beanAndAttributeLock.lock(); try { - log.info("Bean unregistered event. {}", mBeanName); + int removedMetrics = 0; + int removedAttributes = 0; + for (Iterator it = this.matchingAttributes.iterator(); it.hasNext();) { + JmxAttribute current = it.next(); + if (current.getBeanName().compareTo(mBeanName) == 0) { + it.remove(); + removedMetrics += current.getLastMetricsCount(); + removedAttributes++; + } + } + log.info("Bean unregistered event, removed {} attributes ({} metrics) matching bean {}", removedAttributes, removedMetrics, mBeanName); + this.metricCountForMatchingAttributes -= removedMetrics; } finally { + log.debug("Bean unregistration processed. {}", mBeanName); this.beanAndAttributeLock.unlock(); } } @@ -961,6 +976,11 @@ public int getMaxNumberOfMetrics() { return this.maxReturnedMetrics; } + /** Returns the current number of metrics this instance will collect. */ + public int getCurrentNumberOfMetrics() { + return this.metricCountForMatchingAttributes; + } + /** Returns whether or not the instance has reached the maximum bean collection limit. */ public boolean isLimitReached() { return this.limitReached; diff --git a/src/main/java/org/datadog/jmxfetch/JmxAttribute.java b/src/main/java/org/datadog/jmxfetch/JmxAttribute.java index 68661d82c..76729f144 100644 --- a/src/main/java/org/datadog/jmxfetch/JmxAttribute.java +++ b/src/main/java/org/datadog/jmxfetch/JmxAttribute.java @@ -62,6 +62,7 @@ public abstract class JmxAttribute { private List defaultTagsList; private boolean cassandraAliasing; protected String checkName; + private int lastMetricSize; JmxAttribute( MBeanAttributeInfo attribute, @@ -84,6 +85,7 @@ public abstract class JmxAttribute { this.cassandraAliasing = cassandraAliasing; this.checkName = checkName; this.serviceNameProvider = serviceNameProvider; + this.lastMetricSize = 0; // A bean name is formatted like that: // org.apache.cassandra.db:type=Caches,keyspace=system,cache=HintsColumnFamilyKeyCache @@ -254,10 +256,18 @@ public String toString() { + attribute.getType(); } - public abstract List getMetrics() + protected abstract List getMetricsImpl() throws AttributeNotFoundException, InstanceNotFoundException, MBeanException, ReflectionException, IOException; + final public List getMetrics() + throws AttributeNotFoundException, InstanceNotFoundException, MBeanException, + ReflectionException, IOException { + List metrics = this.getMetricsImpl(); + this.lastMetricSize = metrics.size(); + return metrics; + } + /** * An abstract function implemented in the inherited classes JmxSimpleAttribute and * JmxComplexAttribute. @@ -279,6 +289,12 @@ public int getMetricsCount() { } } + /** Gets the most recent collection's metric count */ + public int getLastMetricsCount() { + return this.lastMetricSize; + } + + /** Gets the JMX Attribute info value. Makes a call through the connection */ Object getJmxValue() throws AttributeNotFoundException, InstanceNotFoundException, MBeanException, diff --git a/src/main/java/org/datadog/jmxfetch/JmxComplexAttribute.java b/src/main/java/org/datadog/jmxfetch/JmxComplexAttribute.java index 70ca4cb47..38a6553da 100644 --- a/src/main/java/org/datadog/jmxfetch/JmxComplexAttribute.java +++ b/src/main/java/org/datadog/jmxfetch/JmxComplexAttribute.java @@ -61,7 +61,7 @@ private void populateSubAttributeList(Object attributeValue) { } @Override - public List getMetrics() throws AttributeNotFoundException, MBeanException, + public List getMetricsImpl() throws AttributeNotFoundException, MBeanException, ReflectionException, InstanceNotFoundException, IOException { List metrics = new ArrayList(subAttributeList.size()); for (String subAttribute : subAttributeList) { diff --git a/src/main/java/org/datadog/jmxfetch/JmxSimpleAttribute.java b/src/main/java/org/datadog/jmxfetch/JmxSimpleAttribute.java index c57f16e67..a3ffb074b 100644 --- a/src/main/java/org/datadog/jmxfetch/JmxSimpleAttribute.java +++ b/src/main/java/org/datadog/jmxfetch/JmxSimpleAttribute.java @@ -43,7 +43,7 @@ public JmxSimpleAttribute( } @Override - public List getMetrics() + public List getMetricsImpl() throws AttributeNotFoundException, InstanceNotFoundException, MBeanException, ReflectionException, IOException { if (cachedMetric == null) { diff --git a/src/main/java/org/datadog/jmxfetch/JmxTabularAttribute.java b/src/main/java/org/datadog/jmxfetch/JmxTabularAttribute.java index d36369f6c..1a72f3af4 100644 --- a/src/main/java/org/datadog/jmxfetch/JmxTabularAttribute.java +++ b/src/main/java/org/datadog/jmxfetch/JmxTabularAttribute.java @@ -134,7 +134,7 @@ protected String[] getTags(String key, String subAttribute) } @Override - public List getMetrics() + public List getMetricsImpl() throws AttributeNotFoundException, InstanceNotFoundException, MBeanException, ReflectionException, IOException { Map> subMetrics = new HashMap>(); diff --git a/src/test/java/org/datadog/jmxfetch/TestCommon.java b/src/test/java/org/datadog/jmxfetch/TestCommon.java index ab9416ac9..50840e7c4 100644 --- a/src/test/java/org/datadog/jmxfetch/TestCommon.java +++ b/src/test/java/org/datadog/jmxfetch/TestCommon.java @@ -14,6 +14,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -73,7 +74,7 @@ protected void registerMBean(Object application, String objectStringName) * @throws MBeanRegistrationException */ @After - public void unregisterMBean() throws MBeanRegistrationException, InstanceNotFoundException { + public void unregisterAllMBeans() throws MBeanRegistrationException, InstanceNotFoundException { if (mbs != null) { for (ObjectName objectName : objectNames) { mbs.unregisterMBean(objectName); @@ -81,6 +82,25 @@ public void unregisterMBean() throws MBeanRegistrationException, InstanceNotFoun } } + /** + * Unregister a specific MBean. + * + * @throws InstanceNotFoundException + * @throws MBeanRegistrationException + */ + protected void unregisterMBean(Object application, String objectStringName) throws MBeanRegistrationException, InstanceNotFoundException, MalformedObjectNameException { + if (mbs != null) { + ObjectName objectName = new ObjectName(objectStringName); + for (Iterator it = objectNames.iterator(); it.hasNext();) { + ObjectName elem = it.next(); + if (elem.compareTo(objectName) == 0) { + it.remove(); + } + } + mbs.unregisterMBean(objectName); + } + } + /** Init JMXFetch with the given YAML configuration file. */ protected void initApplication(String yamlFileName, String autoDiscoveryPipeFile) throws FileNotFoundException, IOException { diff --git a/src/test/java/org/datadog/jmxfetch/TestInstance.java b/src/test/java/org/datadog/jmxfetch/TestInstance.java index bb1b9678e..14a76c353 100644 --- a/src/test/java/org/datadog/jmxfetch/TestInstance.java +++ b/src/test/java/org/datadog/jmxfetch/TestInstance.java @@ -258,4 +258,51 @@ public void testBeanSubscription() throws Exception { // 17 = 13 metrics from java.lang + 2 iteration=one + 2 iteration=two assertEquals(17, metrics.size()); } + + @Test + public void testBeanUnsubscription() throws Exception { + // This delay is for the subscription thread to recieve the MBeanNotification + // and call into `Instance`. Conceptually this is just a Thread.yield() + int subscriptionDelay = 200; + SimpleTestJavaApp testApp = new SimpleTestJavaApp(); + // Bean-refresh interval is to to 50s, so the only bean refresh will be + // initial fetch + initApplication("jmx_bean_subscription.yaml"); + + // We do a first collection + run(); + List> metrics = getMetrics(); + + // 13 metrics from java.lang + assertEquals(13, metrics.size()); + + // We register an additional mbean + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); + log.info("sleeping before the next collection"); + Thread.sleep(subscriptionDelay); + + run(); + metrics = getMetrics(); + + // 15 = 13 metrics from java.lang + 2 iteration=one + assertEquals(15, metrics.size()); + + // We UN-register first mbean + unregisterMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); + log.info("sleeping before the next collection"); + Thread.sleep(subscriptionDelay); + + // We run a third collection. + run(); + metrics = getMetrics(); + + // 13 metrics from java.lang only + // Note that the collected metrics size is correct even without any special work for bean + // unregistration as the `iteration=one` metric will fail to be found and simply not be included + assertEquals(13, metrics.size()); + // Which is why this second check exists, it reflects the running total of metrics + // collected which is used to determine if new attributes can be added + Instance i = app.getInstances().get(0); + assertEquals(13, i.getCurrentNumberOfMetrics()); + } } From 7f7c1a6eb1b11cce0fef8920f9947ea6d634656d Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Fri, 19 May 2023 21:01:25 +0000 Subject: [PATCH 12/41] Adds extra logging instructions for misbehaving jmx server --- tools/misbehaving-jmx-server/README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tools/misbehaving-jmx-server/README.md b/tools/misbehaving-jmx-server/README.md index ad8f3a110..a351fbc1a 100644 --- a/tools/misbehaving-jmx-server/README.md +++ b/tools/misbehaving-jmx-server/README.md @@ -20,6 +20,15 @@ as well as an HTTP control interface to allow injection of network errors. - `RMI_HOST` - hostname for JMX to listen on (default localhost) - `CONTROL_PORT` - HTTP control port (default 8080) +To enable debug logging, set the system property `-Dorg.slf4j.simpleLogger.defaultLogLevel=debug`. +This can be done in the Dockerfile `CMD` if running in a container + +For extra debug logging around RMI systems, try any/all of the following: +- `-Djava.rmi.server.logCalls=true` +- `-Dsun.rmi.server.exceptionTrace=true` +- `-Dsun.rmi.transport.logLevel=verbose` +- `-Dsun.rmi.transport.tcp.logLevel=verbose` + ## HTTP Control Actions - POST `/cutNetwork` - Denies any requests to create a new socket (ie, no more connections will be 'accept'ed) and then closes existing TCP sockets - POST `/restoreNetwork` - Allows new sockets to be created From 1bd15f083427d109ae3efb6eb5f77c7264193373 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Fri, 19 May 2023 21:02:07 +0000 Subject: [PATCH 13/41] Adds dedicated remote container bean subscription tests --- .../jmxfetch/TestBeanSubscription.java | 195 ++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java diff --git a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java new file mode 100644 index 000000000..cb1d91fbf --- /dev/null +++ b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java @@ -0,0 +1,195 @@ +package org.datadog.jmxfetch; + +import static org.junit.Assert.assertEquals; + +import java.io.IOException; +import java.nio.file.Paths; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Arrays; + +import lombok.extern.slf4j.Slf4j; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import org.testcontainers.images.builder.ImageFromDockerfile; + +import org.datadog.jmxfetch.reporter.ConsoleReporter; + +@Slf4j +public class TestBeanSubscription extends TestCommon { + private static final int rmiPort = 9090; + private static final int controlPort = 9091; + private JMXServerControlClient controlClient; + private static Slf4jLogConsumer logConsumer = new Slf4jLogConsumer(log); + + private static ImageFromDockerfile img = new ImageFromDockerfile() + .withFileFromPath(".", Paths.get("./tools/misbehaving-jmx-server/")); + + @Before + public void setup() { + this.controlClient = new JMXServerControlClient(cont.getHost(), cont.getMappedPort(controlPort)); + } + + @Rule + public GenericContainer cont = new GenericContainer<>(img) + .withExposedPorts(rmiPort, controlPort) + .withEnv(Collections.singletonMap("RMI_PORT", "" + rmiPort)) + .withEnv(Collections.singletonMap("CONTROL_PORT", "" + controlPort)) + .waitingFor(Wait.forLogMessage(".*IAMREADY.*", 1)); + + @Test + public void testJMXFetchBasic() throws IOException, InterruptedException { + String testDomain = "test-domain"; + this.initApplicationWithYamlLines( + "init_config:", + " is_jmx: true", + "", + "instances:", + " - name: jmxint_container", + " host: " + cont.getHost(), + " port: " + cont.getMappedPort(rmiPort), + " min_collection_interval: null", // allow collections at arbitrary intervals since we trigger them manually in the tests + " enable_bean_subscription: true", + " refresh_beans: 5000", // effectively disable bean refresh + " collect_default_jvm_metrics: false", + " max_returned_metrics: 300000", + " conf:", + " - include:", + " domain: " + testDomain, + " attribute:", + " - DoubleValue", + " - NumberValue", + " - FloatValue", + " - BooleanValue" + ); + + log.info("hello"); + this.app.doIteration(); + List> metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + assertEquals(0, metrics.size()); + + int numBeans = 2; + int numAttributesPerBean = 4; + + this.controlClient.createMBeans(testDomain, numBeans); + + // Allow time for subscriptions to come through and be registered + Thread.sleep(100); + + this.app.doIteration(); + metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + assertEquals(numBeans * numAttributesPerBean, metrics.size()); + } + + @Test + public void testJMXFetchManyBeans() throws IOException, InterruptedException { + cont.followOutput(logConsumer); + String testDomain = "test-domain"; + this.initApplicationWithYamlLines( + "init_config:", + " is_jmx: true", + "", + "instances:", + " - name: jmxint_container", + " host: " + cont.getHost(), + " port: " + cont.getMappedPort(rmiPort), + " min_collection_interval: null", + " enable_bean_subscription: true", + " refresh_beans: 5000", // effectively disable bean refresh + " collect_default_jvm_metrics: false", + " max_returned_metrics: 300000", + " conf:", + " - include:", + " domain: " + testDomain, + " attribute:", + " - DoubleValue", + " - NumberValue", + " - FloatValue", + " - BooleanValue" + ); + + this.app.doIteration(); + List> metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + assertEquals(0, metrics.size()); + + int numBeans = 200; + int numAttributesPerBean = 4; + + this.controlClient.createMBeans(testDomain, numBeans); + + // Time for subscriptions to come through and be registered + Thread.sleep(2000); + + this.app.doIteration(); + metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + assertEquals(numBeans * numAttributesPerBean, metrics.size()); + } + + @Test + public void testConcurrentCollectionWithSubscriptionUpdates() throws IOException, InterruptedException { + String testDomain = "test-domain"; + cont.followOutput(logConsumer); + this.initApplicationWithYamlLines( + "init_config:", + " is_jmx: true", + "", + "instances:", + " - name: jmxint_container", + " host: " + cont.getHost(), + " port: " + cont.getMappedPort(rmiPort), + " min_collection_interval: null", + " enable_bean_subscription: true", + " refresh_beans: 5000", // effectively disable bean refresh + " collect_default_jvm_metrics: false", + " max_returned_metrics: 300000", + " conf:", + " - include:", + " domain: " + testDomain, + " attribute:", + " - DoubleValue", + " - NumberValue", + " - FloatValue", + " - BooleanValue" + ); + + this.app.doIteration(); + List> metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + // Sanity check, no beans exist, none should be found + assertEquals(0, metrics.size()); + + int numBeans = 200; + int numAttributesPerBean = 4; + int expectedMetrics = numBeans * numAttributesPerBean; + + // This call blocks until the beans actually exist in the remote application + this.controlClient.createMBeans(testDomain, numBeans); + + // Intentionally leaving no time for subscriptions to be processed to test how + // a collection behaves when interleaved with bean subscription traffic + + this.app.doIteration(); + // Iteration is done, don't care how many metrics were collected + // (almost certainly less than numBeans * numAttributesPerBean) + metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + + // Sleep to ensure bean subscriptions get processed (ie, attribute matching correctly takes place) + Thread.sleep(2000); + + // Now, without doing an iteration, we should see the correct number for + // how many metrics are about to be collected + // This is effectively testing "Did the attribute matching execute correctly for all bean notifications" + assertEquals("getCurrentNumberOfMetrics returns the correct value _before_ running a collection", expectedMetrics, this.app.getInstances().get(0).getCurrentNumberOfMetrics()); + + // Do an actual collection to ensure validate the metrics collected + this.app.doIteration(); + metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + + assertEquals("actual metrics collected matches expectedMetrics", expectedMetrics, metrics.size()); + } +} From 4fd1b09c0b5e2a5e77ddb033f0857074dae9fed8 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 22 May 2023 17:06:29 +0000 Subject: [PATCH 14/41] Removes old mutex debugging code --- src/main/java/org/datadog/jmxfetch/Instance.java | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 7a2492498..4ac466681 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -513,21 +513,6 @@ public List getMetrics() throws IOException { // To enable this, a "refresh_beans_initial" and/or "refresh_beans" parameters must be // specified in the yaml/json config log.debug("getMetrics invoked"); - if (this.beanAndAttributeLock.isLocked() && !this.beanAndAttributeLock.isHeldByCurrentThread()) { - log.debug("Waiting for lock, isLocked {} isHeldByCurrentThread {} holdCount {}", this.beanAndAttributeLock.isLocked(), this.beanAndAttributeLock.isHeldByCurrentThread(), this.beanAndAttributeLock.getHoldCount()); - Thread owner = this.beanAndAttributeLock.getOwningThread(); - if (owner != null) { - log.debug("Owning thread is {} ", owner.getName()); - } - Map allStacks = Thread.getAllStackTraces(); - for (Map.Entry entry : allStacks.entrySet()) { - log.info("Thread: {} {}", entry.getKey().toString(), (entry.getKey() == owner ? "THIS IS THE LOCK" : "")); - - for (StackTraceElement elem : entry.getValue()) { - log.info("\t{}", elem.toString()); - } - } - } Integer period = (this.initialRefreshTime == this.lastRefreshTime) ? this.initialRefreshBeansPeriod : this.refreshBeansPeriod; From 57f197008c0e03c325322dbde8c7e675eee7d816 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 22 May 2023 17:07:24 +0000 Subject: [PATCH 15/41] Adds bean unsubscription test and (failing) bean subscription w/ network cut test --- .../jmxfetch/TestBeanSubscription.java | 116 ++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java index cb1d91fbf..69aaf87e1 100644 --- a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java +++ b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java @@ -192,4 +192,120 @@ public void testConcurrentCollectionWithSubscriptionUpdates() throws IOException assertEquals("actual metrics collected matches expectedMetrics", expectedMetrics, metrics.size()); } + + @Test + public void testBeanRemoval() throws IOException, InterruptedException { + String testDomain = "test-domain"; + cont.followOutput(logConsumer); + this.initApplicationWithYamlLines( + "init_config:", + " is_jmx: true", + "", + "instances:", + " - name: jmxint_container", + " host: " + cont.getHost(), + " port: " + cont.getMappedPort(rmiPort), + " min_collection_interval: null", + " enable_bean_subscription: true", + " refresh_beans: 5000", // effectively disable bean refresh + " collect_default_jvm_metrics: false", + " max_returned_metrics: 300000", + " conf:", + " - include:", + " domain: " + testDomain, + " attribute:", + " - DoubleValue", + " - NumberValue", + " - FloatValue", + " - BooleanValue" + ); + + this.app.doIteration(); + List> metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + assertEquals("Sanity check, no beans/metrics exist at beginning of test", 0, metrics.size()); + + int numBeans = 20; + int numAttributesPerBean = 4; + int expectedMetrics = numBeans * numAttributesPerBean; + + this.controlClient.createMBeans(testDomain, numBeans); + + Thread.sleep(500); + + this.app.doIteration(); + metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + + assertEquals("After creating beans, correct metrics collected", expectedMetrics, metrics.size()); + + numBeans = 10; + expectedMetrics = numBeans * numAttributesPerBean; + + this.controlClient.createMBeans(testDomain, numBeans); + + Thread.sleep(500); + + assertEquals("Number of metrics to be collected properly updated", expectedMetrics, this.app.getInstances().get(0).getCurrentNumberOfMetrics()); + + this.app.doIteration(); + metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + + assertEquals("After removing beans, correct metrics collected", expectedMetrics, metrics.size()); + } + + @Test + public void testDisconnectDuringBeanCreation() throws IOException, InterruptedException { + String testDomain = "test-domain"; + cont.followOutput(logConsumer); + this.initApplicationWithYamlLines( + "init_config:", + " is_jmx: true", + "", + "instances:", + " - name: jmxint_container", + " host: " + cont.getHost(), + " port: " + cont.getMappedPort(rmiPort), + " min_collection_interval: null", + " enable_bean_subscription: true", + " refresh_beans: 5000", // effectively disable bean refresh + " collect_default_jvm_metrics: false", + " max_returned_metrics: 300000", + " conf:", + " - include:", + " domain: " + testDomain, + " attribute:", + " - DoubleValue", + " - NumberValue", + " - FloatValue", + " - BooleanValue" + ); + + this.app.doIteration(); + List> metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + assertEquals("Sanity check, no beans/metrics exist at beginning of test", 0, metrics.size()); + + + // Cut network + this.controlClient.jmxCutNetwork(); + + + int numBeans = 20; + int numAttributesPerBean = 4; + int expectedMetrics = numBeans * numAttributesPerBean; + + this.controlClient.createMBeans(testDomain, numBeans); + // once beans created, restore network + + this.controlClient.jmxRestoreNetwork(); + + // Allow connection restoration and bean refresh + Thread.sleep(500); + + this.app.doIteration(); + metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + + assertEquals("Number of metrics to be collected properly updated", expectedMetrics, this.app.getInstances().get(0).getCurrentNumberOfMetrics()); + + assertEquals("Network recovered, did we collect correct metrics?", expectedMetrics, metrics.size()); + + } } From 6670056595c8839255c03957cdf7b95656918492 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 22 May 2023 21:17:31 +0000 Subject: [PATCH 16/41] Adds JMX connection listener to mark instance as broken on connection failure --- src/main/java/org/datadog/jmxfetch/App.java | 3 + .../java/org/datadog/jmxfetch/Connection.java | 41 +++++++++- .../java/org/datadog/jmxfetch/Instance.java | 13 ++- .../datadog/jmxfetch/JvmDirectConnection.java | 4 + .../jmxfetch/TestBeanSubscription.java | 80 +++++++++++++++++-- .../java/org/datadog/jmxfetch/TestCommon.java | 3 + 6 files changed, 132 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/App.java b/src/main/java/org/datadog/jmxfetch/App.java index a536392c9..9077bd885 100644 --- a/src/main/java/org/datadog/jmxfetch/App.java +++ b/src/main/java/org/datadog/jmxfetch/App.java @@ -434,6 +434,9 @@ void start() { } void stop() { + for (Instance in : this.getInstances()) { + in.cleanUp(); + } this.collectionProcessor.stop(); this.recoveryProcessor.stop(); } diff --git a/src/main/java/org/datadog/jmxfetch/Connection.java b/src/main/java/org/datadog/jmxfetch/Connection.java index e119e07ad..7ceae6d28 100644 --- a/src/main/java/org/datadog/jmxfetch/Connection.java +++ b/src/main/java/org/datadog/jmxfetch/Connection.java @@ -20,6 +20,7 @@ import javax.management.AttributeNotFoundException; import javax.management.InstanceNotFoundException; import javax.management.IntrospectionException; +import javax.management.ListenerNotFoundException; import javax.management.MBeanAttributeInfo; import javax.management.MBeanException; import javax.management.MBeanInfo; @@ -45,6 +46,31 @@ public class Connection { protected MBeanServerConnection mbs; protected Map env; protected JMXServiceURL address; + private NotificationListener connectionNotificationListener; + private boolean seenConnectionIssues; + + private static class ConnectionNotificationListener implements NotificationListener { + public void handleNotification(Notification notification, Object handback) { + if (!(notification instanceof JMXConnectionNotification)) { + return; + } + if (!(handback instanceof Connection)) { + return; + } + + JMXConnectionNotification connNotif = (JMXConnectionNotification) notification; + Connection conn = (Connection) handback; + + if (connNotif.getType() == JMXConnectionNotification.CLOSED + || connNotif.getType() == JMXConnectionNotification.FAILED + || connNotif.getType() == JMXConnectionNotification.NOTIFS_LOST) { + log.warn("Connection is potentially in a bad state, marking connection issues. {} - {}", connNotif.getType(), connNotif.getMessage()); + conn.seenConnectionIssues = true; + } + log.debug("Received connection notification: {} Message: {}", + connNotif.getType(), connNotif.getMessage()); + } + } private static class BeanNotificationListener implements NotificationListener { private BeanListener bl; @@ -54,11 +80,10 @@ public BeanNotificationListener(BeanListener bl) { } public void handleNotification(Notification notification, Object handback) { if (!(notification instanceof MBeanServerNotification)) { - log.warn("Got unknown notification, expected MBeanServerNotification but got {}", notification.getClass()); return; } MBeanServerNotification mbs = (MBeanServerNotification) notification; - log.debug("MBeanNotification: ts {} seqNum: {} msg: '{}' userData: {} ", mbs.getTimeStamp(), mbs.getSequenceNumber(), mbs.getMessage(), mbs.getUserData()); + log.debug("MBeanNotification: ts {} seqNum: {} msg: '{}'", mbs.getTimeStamp(), mbs.getSequenceNumber(), mbs.getMessage()); ObjectName mBeanName = mbs.getMBeanName(); if (mbs.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { bl.beanRegistered(mBeanName); @@ -98,6 +123,9 @@ protected void createConnection() throws IOException { log.info("Connecting to: " + this.address); connector = JMXConnectorFactory.connect(this.address, this.env); mbs = connector.getMBeanServerConnection(); + + this.connectionNotificationListener = new ConnectionNotificationListener(); + connector.addConnectionNotificationListener(this.connectionNotificationListener, null, this); } /** Gets attribute for matching bean and attribute name. */ @@ -115,13 +143,20 @@ public Object getAttribute(ObjectName objectName, String attributeName) public void closeConnector() { if (connector != null) { try { + this.connector.removeConnectionNotificationListener(this.connectionNotificationListener); connector.close(); - } catch (IOException e) { + connector = null; + } catch (IOException | ListenerNotFoundException e) { // ignore } } } + /** True if connection has been notified of failure/lost notifications */ + public boolean hasSeenConnectionIssues() { + return this.seenConnectionIssues; + } + /** Returns a boolean describing if the connection is still alive. */ public boolean isAlive() { if (connector == null) { diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 4ac466681..88cda07fd 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -508,6 +508,10 @@ public String toString() { /** Returns a map of metrics collected. */ public List getMetrics() throws IOException { + if (!this.isInstanceHealthy()) { + this.connection.closeConnector(); + throw new IOException("Instance is in a bad state", null); + } // In case of ephemeral beans, we can force to refresh the bean list x seconds // post initialization and every x seconds thereafter. // To enable this, a "refresh_beans_initial" and/or "refresh_beans" parameters must be @@ -760,7 +764,7 @@ private void getMatchingAttributes() throws IOException { addMatchingAttributesForBean(beanName, className, attributeInfos); } - log.info("Found {} matching attributes, collecting {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); + log.info("Found {} matching attributes with {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); } finally { this.beanAndAttributeLock.unlock(); } @@ -904,6 +908,13 @@ private void refreshBeansList(boolean isInitialQuery) throws IOException { } } + /** True if instance is in a good state to collect metrics */ + private boolean isInstanceHealthy() { + // If we have subscription mode on and the connection has either failed or notifications + // have been lost, then we must consider this instance unhealthy and re-init. + return !(this.beanSubscriptionActive && connection.hasSeenConnectionIssues()); + } + /** Returns a string array listing the service check tags. */ public String[] getServiceCheckTags() { List tags = new ArrayList(); diff --git a/src/main/java/org/datadog/jmxfetch/JvmDirectConnection.java b/src/main/java/org/datadog/jmxfetch/JvmDirectConnection.java index 08c85c770..99c4b27d7 100644 --- a/src/main/java/org/datadog/jmxfetch/JvmDirectConnection.java +++ b/src/main/java/org/datadog/jmxfetch/JvmDirectConnection.java @@ -20,6 +20,10 @@ public void closeConnector() { // ignore } + public boolean isConnectorClosed() { + return false; + } + public boolean isAlive() { return true; } diff --git a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java index 69aaf87e1..5f396d713 100644 --- a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java +++ b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java @@ -253,8 +253,8 @@ public void testBeanRemoval() throws IOException, InterruptedException { } @Test - public void testDisconnectDuringBeanCreation() throws IOException, InterruptedException { - String testDomain = "test-domain"; + public void testNetworkFailure() throws IOException, InterruptedException { + String testDomain = "test-domain-nwkfail"; cont.followOutput(logConsumer); this.initApplicationWithYamlLines( "init_config:", @@ -283,28 +283,92 @@ public void testDisconnectDuringBeanCreation() throws IOException, InterruptedEx List> metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); assertEquals("Sanity check, no beans/metrics exist at beginning of test", 0, metrics.size()); - - // Cut network this.controlClient.jmxCutNetwork(); + // In testing, this needs a slight delay for the connection to "fail" + // via JMXConnectionNotification. + // Artificially sleep to allow time for this since that is the point of this test + Thread.sleep(50); + this.controlClient.jmxRestoreNetwork(); + + int numBeans = 20; + int numAttributesPerBean = 4; + int expectedMetrics = numBeans * numAttributesPerBean; + this.controlClient.createMBeans(testDomain, numBeans); + + // One iteration to recover instance, no metrics are actually collected + this.app.doIteration(); + // Second iteration should collect metrics + this.app.doIteration(); + metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + + assertEquals("Number of metrics to be collected properly updated", expectedMetrics, this.app.getInstances().get(0).getCurrentNumberOfMetrics()); + + assertEquals("Network recovered, did we collect correct metrics?", expectedMetrics, metrics.size()); + } + + @Test + public void testDisconnectDuringBeanCreation() throws IOException, InterruptedException { + String testDomain = "test-domain-dsc-bn-creat"; + cont.followOutput(logConsumer); + this.initApplicationWithYamlLines( + "init_config:", + " is_jmx: true", + "", + "instances:", + " - name: jmxint_container", + " host: " + cont.getHost(), + " port: " + cont.getMappedPort(rmiPort), + " min_collection_interval: null", + " enable_bean_subscription: true", + " refresh_beans: 5000", // effectively disable bean refresh + " collect_default_jvm_metrics: false", + " max_returned_metrics: 300000", + " conf:", + " - include:", + " domain: " + testDomain, + " attribute:", + " - DoubleValue", + " - NumberValue", + " - FloatValue", + " - BooleanValue" + ); + + this.app.doIteration(); + List> metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); + assertEquals("Sanity check, no beans/metrics exist at beginning of test", 0, metrics.size()); + + this.controlClient.jmxCutNetwork(); int numBeans = 20; int numAttributesPerBean = 4; int expectedMetrics = numBeans * numAttributesPerBean; this.controlClient.createMBeans(testDomain, numBeans); - // once beans created, restore network + // once beans created, restore network this.controlClient.jmxRestoreNetwork(); - // Allow connection restoration and bean refresh + // When attempting to collect metrics, instance is marked as broken due to an unhealthy network + // _and_ it is added to brokenInstances so the broken instance is recovered in the _same_ iteration + // Note in other "reconnection" tests (see TestReconnection) there are two iterations required + // The first marks it as broken and the second recovers it. This test only needs one since getMetrics + // fails so quickly. So this is technically a race and if this test ever becomes flakey this is why. + this.app.doIteration(); + + // Now create more beans which triggers subscription updates + numBeans = 22; + expectedMetrics = numBeans * numAttributesPerBean; + this.controlClient.createMBeans(testDomain, numBeans); + + // Allow subscription updates to be processed Thread.sleep(500); + assertEquals("Number of metrics to be collected properly updated", expectedMetrics, this.app.getInstances().get(0).getCurrentNumberOfMetrics()); + this.app.doIteration(); metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); - assertEquals("Number of metrics to be collected properly updated", expectedMetrics, this.app.getInstances().get(0).getCurrentNumberOfMetrics()); - assertEquals("Network recovered, did we collect correct metrics?", expectedMetrics, metrics.size()); } diff --git a/src/test/java/org/datadog/jmxfetch/TestCommon.java b/src/test/java/org/datadog/jmxfetch/TestCommon.java index 3c99bc94a..635f52c27 100644 --- a/src/test/java/org/datadog/jmxfetch/TestCommon.java +++ b/src/test/java/org/datadog/jmxfetch/TestCommon.java @@ -109,6 +109,9 @@ public void unregisterAllMBeans() throws MBeanRegistrationException, InstanceNot mbs.unregisterMBean(objectName); } } + if (this.app != null) { + this.app.stop(); + } } /** From d74319a898092ad66c232b4307137c94c79f1f04 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Tue, 23 May 2023 19:19:16 +0000 Subject: [PATCH 17/41] Removes un-needed bean subscriber thread --- .../org/datadog/jmxfetch/BeanSubscriber.java | 29 ------------------- .../java/org/datadog/jmxfetch/Instance.java | 24 +++++++-------- 2 files changed, 12 insertions(+), 41 deletions(-) delete mode 100644 src/main/java/org/datadog/jmxfetch/BeanSubscriber.java diff --git a/src/main/java/org/datadog/jmxfetch/BeanSubscriber.java b/src/main/java/org/datadog/jmxfetch/BeanSubscriber.java deleted file mode 100644 index 63ae360ba..000000000 --- a/src/main/java/org/datadog/jmxfetch/BeanSubscriber.java +++ /dev/null @@ -1,29 +0,0 @@ -package org.datadog.jmxfetch; - -import java.util.concurrent.CompletableFuture; -import java.util.List; - -public class BeanSubscriber implements Runnable { - private List beanScopes; - private Connection connection; - private BeanListener listener; - public CompletableFuture subscriptionSuccessful; - - BeanSubscriber(List beanScopes, Connection connection, BeanListener listener) { - this.beanScopes = beanScopes; - this.connection = connection; - this.listener = listener; - this.subscriptionSuccessful = new CompletableFuture(); - } - - public void run() { - try { - connection.subscribeToBeanScopes(beanScopes, this.listener); - this.subscriptionSuccessful.complete(true); - - Thread.currentThread().join(); - } catch (Exception e) { - this.subscriptionSuccessful.complete(false); - } - } -} \ No newline at end of file diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 88cda07fd..3ef74ea4d 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -22,8 +22,11 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; + +import javax.management.InstanceNotFoundException; import javax.management.MBeanAttributeInfo; import javax.management.MBeanInfo; +import javax.management.MalformedObjectNameException; import javax.management.ObjectName; import javax.security.auth.login.FailedLoginException; @@ -820,22 +823,19 @@ public void beanUnregistered(ObjectName mBeanName) { private void subscribeToBeans() { List beanScopes = this.getBeansScopes(); - BeanSubscriber subscriber = new BeanSubscriber(beanScopes, this.connection, this); try { - new Thread(subscriber).start(); - this.beanSubscriptionActive = subscriber.subscriptionSuccessful.get(1, SECONDS); - } catch (Exception e) { - log.warn("Error retrieving bean subscription value, assuming subscription failed. Exception: {}", e); - this.beanSubscriptionActive = false; - } - - if (this.beanSubscriptionActive) { + // TODO - Processing a bean subscription event could be slow, which could hold up other subscriptions + // Would be better to queue subscription events up and process that queue on a dedicated thread. + connection.subscribeToBeanScopes(beanScopes, this); + this.beanSubscriptionActive = true; log.info("Subscribed to {} bean scopes successfully!", beanScopes.size()); - } else { + } catch (MalformedObjectNameException | InstanceNotFoundException | IOException e) { + e.printStackTrace(); log.warn("Bean subscription failed! Will rely on bean_refresh, ensure it is set " - + " to an appropriate value (currently {} seconds)", - this.refreshBeansPeriod); + + " to an appropriate value (currently {} seconds). Exception: ", + this.refreshBeansPeriod, e); + this.beanSubscriptionActive = false; } } From fe7705c3a58f4397fbfd53d27f4e9af0eb93a15c Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Tue, 23 May 2023 20:15:40 +0000 Subject: [PATCH 18/41] Clarifies bean-audit logging --- .../java/org/datadog/jmxfetch/Instance.java | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 3ef74ea4d..425f80a07 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -860,7 +860,6 @@ private void refreshBeansList(boolean isInitialQuery) throws IOException { this.beanAndAttributeLock.lock(); try { - boolean fullBeanQueryNeeded = false; if (limitQueryScopes) { try { @@ -885,20 +884,29 @@ private void refreshBeansList(boolean isInitialQuery) throws IOException { // If every new bean and de-registered bean properly fires an event, then // this.beans (current set that has been updated via subscription) should // always equal the new set of beans queried (unless it was a full bean query) - Set beansNotSeen = new HashSet<>(); if (!this.beans.containsAll(newBeans)) { + // Newly queried set of beans contains beans not actively tracked + // ie, maybe a registeredBean subscription msg did not get processed correctly + Set beansNotSeen = new HashSet<>(); beansNotSeen.addAll(newBeans); beansNotSeen.removeAll(this.beans); - log.error("Bean refresh found {} new beans that were not already known via subscription", beansNotSeen.size()); + + log.error("Bean refresh found {} new beans that were not already tracked via subscription", beansNotSeen.size()); + for (ObjectName b : beansNotSeen) { + log.error("New not-tracked bean {}", b); + } } if (!newBeans.containsAll(this.beans)){ - beansNotSeen.addAll(this.beans); - beansNotSeen.removeAll(newBeans); - log.error("Bean refresh found {} FEWER beans than already known via subscription", beansNotSeen.size()); - } - - for (ObjectName b : beansNotSeen) { - log.error("Bean {} is one that has never been seen before, see why subscription did not detect this bean.", b.toString()); + // Newly queried set of beans is missing beans that are actively tracked + // ie, maybe a deregisteredBean subscription msg did not get processed correctly + Set incorrectlyTrackedBeans = new HashSet<>(); + incorrectlyTrackedBeans.addAll(this.beans); + incorrectlyTrackedBeans.removeAll(newBeans); + + log.error("Bean refresh found {} FEWER beans than already tracked via subscription", incorrectlyTrackedBeans.size()); + for (ObjectName b : incorrectlyTrackedBeans) { + log.error("Currently tracked bean that not returned from fresh query: {}", b); + } } } this.beans = newBeans; From c26e91b4d6b12c06caccf3bb78f72d75b98469f3 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Tue, 23 May 2023 21:01:34 +0000 Subject: [PATCH 19/41] Updates in-proc bean subscription tests with more attribute counting testing --- .../org/datadog/jmxfetch/TestInstance.java | 100 +++++++++++++----- src/test/resources/jmx_bean_subscription.yaml | 4 +- 2 files changed, 74 insertions(+), 30 deletions(-) diff --git a/src/test/java/org/datadog/jmxfetch/TestInstance.java b/src/test/java/org/datadog/jmxfetch/TestInstance.java index 14a76c353..1de5309de 100644 --- a/src/test/java/org/datadog/jmxfetch/TestInstance.java +++ b/src/test/java/org/datadog/jmxfetch/TestInstance.java @@ -19,6 +19,10 @@ public class TestInstance extends TestCommon { private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger("Test Instance"); + // This delay exists for the subscription thread to recieve the MBeanNotification + // and call into `Instance`. Conceptually this is just a Thread.yield() + private static int subscriptionDelayMs = 50; + @Test public void testMinCollectionInterval() throws Exception { registerMBean(new SimpleTestJavaApp(), "org.datadog.jmxfetch.test:foo=Bar,qux=Baz"); @@ -220,89 +224,127 @@ public void testRefreshBeans() throws Exception { /** Tests bean_subscription */ @Test public void testBeanSubscription() throws Exception { - // This delay is for the subscription thread to recieve the MBeanNotification - // and call into `Instance`. Conceptually this is just a Thread.yield() - int subscriptionDelay = 10; SimpleTestJavaApp testApp = new SimpleTestJavaApp(); - // Bean-refresh interval is to to 50s, so the only bean refresh will be // initial fetch initApplication("jmx_bean_subscription.yaml"); // We do a first collection run(); List> metrics = getMetrics(); + int numRegisteredAttributes = 0; - // 13 metrics from java.lang - assertEquals(13, metrics.size()); + assertEquals(numRegisteredAttributes, metrics.size()); - // We register an additional mbean + // We register first mbean with 2 matching attributes registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); - log.info("sleeping before the next collection"); - Thread.sleep(subscriptionDelay); + numRegisteredAttributes += 2; + + Thread.sleep(subscriptionDelayMs); run(); metrics = getMetrics(); - // 15 = 13 metrics from java.lang + 2 iteration=one - assertEquals(15, metrics.size()); + assertEquals(numRegisteredAttributes, metrics.size()); // We register additional mbean registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=two"); - log.info("sleeping before the next collection"); - Thread.sleep(subscriptionDelay); + numRegisteredAttributes += 2; + Thread.sleep(subscriptionDelayMs); // We run a third collection. run(); metrics = getMetrics(); - // 17 = 13 metrics from java.lang + 2 iteration=one + 2 iteration=two - assertEquals(17, metrics.size()); + assertEquals(numRegisteredAttributes, metrics.size()); } @Test public void testBeanUnsubscription() throws Exception { - // This delay is for the subscription thread to recieve the MBeanNotification - // and call into `Instance`. Conceptually this is just a Thread.yield() - int subscriptionDelay = 200; SimpleTestJavaApp testApp = new SimpleTestJavaApp(); // Bean-refresh interval is to to 50s, so the only bean refresh will be // initial fetch initApplication("jmx_bean_subscription.yaml"); + int numRegisteredAttributes = 0; // We do a first collection run(); List> metrics = getMetrics(); - // 13 metrics from java.lang - assertEquals(13, metrics.size()); + // Sanity check -- no beans exist yet + assertEquals(numRegisteredAttributes, metrics.size()); // We register an additional mbean registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); - log.info("sleeping before the next collection"); - Thread.sleep(subscriptionDelay); + numRegisteredAttributes += 2; + + Thread.sleep(subscriptionDelayMs); run(); metrics = getMetrics(); - // 15 = 13 metrics from java.lang + 2 iteration=one - assertEquals(15, metrics.size()); + assertEquals(numRegisteredAttributes, metrics.size()); // We UN-register first mbean unregisterMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); - log.info("sleeping before the next collection"); - Thread.sleep(subscriptionDelay); + numRegisteredAttributes -= 2; + Thread.sleep(subscriptionDelayMs); // We run a third collection. run(); metrics = getMetrics(); - // 13 metrics from java.lang only // Note that the collected metrics size is correct even without any special work for bean // unregistration as the `iteration=one` metric will fail to be found and simply not be included - assertEquals(13, metrics.size()); + assertEquals(numRegisteredAttributes, metrics.size()); // Which is why this second check exists, it reflects the running total of metrics // collected which is used to determine if new attributes can be added Instance i = app.getInstances().get(0); - assertEquals(13, i.getCurrentNumberOfMetrics()); + assertEquals(numRegisteredAttributes, i.getCurrentNumberOfMetrics()); + } + + @Test + public void testBeanSubscriptionAttributeCounting() throws Exception { + // This test only looks at the instance's getCurrentNumberOfMetrics as this + // should be updated on bean registration/deregistration + + SimpleTestJavaApp testApp = new SimpleTestJavaApp(); + initApplication("jmx_bean_subscription.yaml"); // max returned metrics is 10 + + int numRegisteredAttributes = 0; + assertEquals(numRegisteredAttributes, app.getInstances().get(0).getCurrentNumberOfMetrics()); + + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); + numRegisteredAttributes += 2; + Thread.sleep(subscriptionDelayMs); + assertEquals(numRegisteredAttributes, app.getInstances().get(0).getCurrentNumberOfMetrics()); + + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=two"); + numRegisteredAttributes += 2; + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=three"); + numRegisteredAttributes += 2; + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=four"); + numRegisteredAttributes += 2; + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=five"); + numRegisteredAttributes += 2; + + Thread.sleep(subscriptionDelayMs * 2); // wait longer bc more beans + assertEquals(numRegisteredAttributes, app.getInstances().get(0).getCurrentNumberOfMetrics()); + + + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=six"); + // no change to numRegisteredAttributes as this registration should FAIL due to max number of metrics + + Thread.sleep(subscriptionDelayMs); + assertEquals(numRegisteredAttributes, app.getInstances().get(0).getCurrentNumberOfMetrics()); + + unregisterMBean(testApp, "org.datadog.jmxfetch.test:iteration=one"); + numRegisteredAttributes -= 2; + Thread.sleep(subscriptionDelayMs); + assertEquals(numRegisteredAttributes, app.getInstances().get(0).getCurrentNumberOfMetrics()); + + registerMBean(testApp, "org.datadog.jmxfetch.test:iteration=seven"); + numRegisteredAttributes += 2; + Thread.sleep(subscriptionDelayMs); + assertEquals(numRegisteredAttributes, app.getInstances().get(0).getCurrentNumberOfMetrics()); } } diff --git a/src/test/resources/jmx_bean_subscription.yaml b/src/test/resources/jmx_bean_subscription.yaml index d0f883ba0..c7814acce 100644 --- a/src/test/resources/jmx_bean_subscription.yaml +++ b/src/test/resources/jmx_bean_subscription.yaml @@ -4,7 +4,9 @@ instances: - process_name_regex: .*surefire.* min_collection_interval: null enable_bean_subscription: true - refresh_beans: 50 + collect_default_jvm_metrics: false + refresh_beans: 50000 + max_returned_metrics: 10 name: jmx_test_instance conf: - include: From d746e4030f4f02ebcb0c055a96629bf45d41198c Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Tue, 23 May 2023 21:09:40 +0000 Subject: [PATCH 20/41] Removes explicit locking in favor of 'synchronized' --- .../java/org/datadog/jmxfetch/Instance.java | 335 +++++++----------- 1 file changed, 131 insertions(+), 204 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 425f80a07..51023868a 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -113,7 +113,6 @@ public Yaml initialValue() { private boolean emptyDefaultHostname; private boolean enableBeanSubscription; private boolean beanSubscriptionActive; - private DebugReentrantLock beanAndAttributeLock; /** Constructor, instantiates Instance based of a previous instance and appConfig. */ public Instance(Instance instance, AppConfig appConfig) { @@ -274,7 +273,6 @@ public Instance( Boolean enableBeanSubscription = (Boolean) instanceMap.get("enable_bean_subscription"); this.enableBeanSubscription = enableBeanSubscription != null && enableBeanSubscription; this.beanSubscriptionActive = false; - this.beanAndAttributeLock = new DebugReentrantLock(false); } public static boolean isDirectInstance(Map configInstance) { @@ -430,45 +428,8 @@ public Connection getConnection( return connection; } - private class DebugReentrantLock extends ReentrantLock { - private boolean verbose; - public DebugReentrantLock(boolean verbose) { - this.verbose = verbose; - } - @Override - public void lock() { - if (this.verbose) { - log.info("About to acquire lock from thread {}, lockCount is {}", Thread.currentThread().toString(), this.getHoldCount()); - for (StackTraceElement elem : Thread.currentThread().getStackTrace()) { - log.info("\t{}", elem.toString()); - } - } - super.lock(); - if (this.verbose) { - log.info("Lock acquired, lockCount is {}", this.getHoldCount()); - } - } - @Override - public void unlock() { - if (this.verbose) { - log.info("Releasing lock from thread {}, lockCount is {}", Thread.currentThread().toString(), this.getHoldCount()); - for (StackTraceElement elem : Thread.currentThread().getStackTrace()) { - log.info("\t{}", elem.toString()); - } - } - super.unlock(); - - if (this.verbose) { - log.info("Lock released, lockCount is {}", this.getHoldCount()); - } - } - public Thread getOwningThread() { - return getOwner(); - } - } - /** Initializes the instance. May force a new connection.. */ - public void init(boolean forceNewConnection) + public synchronized void init(boolean forceNewConnection) throws IOException, FailedLoginException, SecurityException { log.info("Trying to connect to JMX Server at " + this.toString()); connection = getConnection(instanceMap, forceNewConnection); @@ -481,16 +442,11 @@ public void init(boolean forceNewConnection) log.info( "Trying to collect bean list for the first time for JMX Server at {}", this); - this.beanAndAttributeLock.lock(); - try { - this.refreshBeansList(true); - this.initialRefreshTime = this.lastRefreshTime; - log.info("Connected to JMX Server at {} with {} beans", this, this.beans.size()); - this.getMatchingAttributes(); - log.info("Done initializing JMX Server at {}", this); - } finally { - this.beanAndAttributeLock.unlock(); - } + this.refreshBeansList(true); + this.initialRefreshTime = this.lastRefreshTime; + log.info("Connected to JMX Server at {} with {} beans", this, this.beans.size()); + this.getMatchingAttributes(); + log.info("Done initializing JMX Server at {}", this); } /** Returns a string representation for the instance. */ @@ -510,7 +466,7 @@ public String toString() { } /** Returns a map of metrics collected. */ - public List getMetrics() throws IOException { + public synchronized List getMetrics() throws IOException { if (!this.isInstanceHealthy()) { this.connection.closeConnector(); throw new IOException("Instance is in a bad state", null); @@ -525,52 +481,45 @@ public List getMetrics() throws IOException { ? this.initialRefreshBeansPeriod : this.refreshBeansPeriod; List metrics = new ArrayList(); - this.beanAndAttributeLock.lock(); - try { - log.debug("Got the lock"); - - if (isPeriodDue(this.lastRefreshTime, period)) { - log.info("Refreshing bean list"); - this.refreshBeansList(false); - this.getMatchingAttributes(); - } - - Iterator it = matchingAttributes.iterator(); - - // increment the lastCollectionTime - this.lastCollectionTime = System.currentTimeMillis(); + if (isPeriodDue(this.lastRefreshTime, period)) { + log.info("Refreshing bean list"); + this.refreshBeansList(false); + this.getMatchingAttributes(); + } - log.info("Starting Collection of " + matchingAttributes.size() + " attributes"); - while (it.hasNext()) { - JmxAttribute jmxAttr = it.next(); - try { - List jmxAttrMetrics = jmxAttr.getMetrics(); - metrics.addAll(jmxAttrMetrics); + Iterator it = matchingAttributes.iterator(); + + // increment the lastCollectionTime + this.lastCollectionTime = System.currentTimeMillis(); + + log.info("Starting Collection of " + matchingAttributes.size() + " attributes"); + while (it.hasNext()) { + JmxAttribute jmxAttr = it.next(); + try { + List jmxAttrMetrics = jmxAttr.getMetrics(); + metrics.addAll(jmxAttrMetrics); + this.failingAttributes.remove(jmxAttr); + } catch (IOException e) { + log.warn("Got IOException {}, rethrowing...", e); + throw e; + } catch (Exception e) { + log.warn("Cannot get metrics for attribute: " + jmxAttr, e); + if (this.failingAttributes.contains(jmxAttr)) { + log.debug( + "Cannot generate metrics for attribute: " + + jmxAttr + + " twice in a row. Removing it from the attribute list"); + it.remove(); this.failingAttributes.remove(jmxAttr); - } catch (IOException e) { - log.warn("Got IOException {}, rethrowing...", e); - throw e; - } catch (Exception e) { - log.warn("Cannot get metrics for attribute: " + jmxAttr, e); - if (this.failingAttributes.contains(jmxAttr)) { - log.debug( - "Cannot generate metrics for attribute: " - + jmxAttr - + " twice in a row. Removing it from the attribute list"); - it.remove(); - this.failingAttributes.remove(jmxAttr); - this.metricCountForMatchingAttributes -= jmxAttr.getLastMetricsCount(); - } else { - this.failingAttributes.add(jmxAttr); - } + this.metricCountForMatchingAttributes -= jmxAttr.getLastMetricsCount(); + } else { + this.failingAttributes.add(jmxAttr); } } - - long duration = System.currentTimeMillis() - this.lastCollectionTime; - log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); - } finally { - this.beanAndAttributeLock.unlock(); } + + long duration = System.currentTimeMillis() - this.lastCollectionTime; + log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); return metrics; } @@ -592,11 +541,7 @@ public boolean timeToCollect() { } } - private void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos) throws IOException { - if (!this.beanAndAttributeLock.isHeldByCurrentThread()) { - log.error("BAD CONCURRENCY - lock should have been held but was not"); - throw new IOException("BAD CONC"); - } + private synchronized void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos) throws IOException { boolean metricReachedDisplayed = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); @@ -732,49 +677,43 @@ private void getMatchingAttributes() throws IOException { Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); - this.beanAndAttributeLock.lock(); - try { - this.matchingAttributes.clear(); - this.failingAttributes.clear(); + this.matchingAttributes.clear(); + this.failingAttributes.clear(); - if (!action.equals(AppConfig.ACTION_COLLECT)) { - reporter.displayInstanceName(this); - } + if (!action.equals(AppConfig.ACTION_COLLECT)) { + reporter.displayInstanceName(this); + } - for (ObjectName beanName : beans) { - if (limitReached) { - log.debug("Limit reached"); - if (action.equals(AppConfig.ACTION_COLLECT)) { - break; - } - } - String className; - MBeanAttributeInfo[] attributeInfos; - try { - log.debug("Getting bean info for bean: {}", beanName); - MBeanInfo info = connection.getMBeanInfo(beanName); - - log.debug("Getting class name for bean: {}", beanName); - className = info.getClassName(); - log.debug("Getting attributes for bean: {}", beanName); - attributeInfos = info.getAttributes(); - } catch (IOException e) { - throw e; - } catch (Exception e) { - log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); - continue; + for (ObjectName beanName : beans) { + if (limitReached) { + log.debug("Limit reached"); + if (action.equals(AppConfig.ACTION_COLLECT)) { + break; } - - addMatchingAttributesForBean(beanName, className, attributeInfos); } - log.info("Found {} matching attributes with {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); - } finally { - this.beanAndAttributeLock.unlock(); + String className; + MBeanAttributeInfo[] attributeInfos; + try { + log.debug("Getting bean info for bean: {}", beanName); + MBeanInfo info = connection.getMBeanInfo(beanName); + + log.debug("Getting class name for bean: {}", beanName); + className = info.getClassName(); + log.debug("Getting attributes for bean: {}", beanName); + attributeInfos = info.getAttributes(); + } catch (IOException e) { + throw e; + } catch (Exception e) { + log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); + continue; + } + + addMatchingAttributesForBean(beanName, className, attributeInfos); } + log.info("Found {} matching attributes with {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); } - public void beanRegistered(ObjectName beanName) { - this.beanAndAttributeLock.lock(); + public synchronized void beanRegistered(ObjectName beanName) { log.debug("Bean registered event. {}", beanName); String className; MBeanAttributeInfo[] attributeInfos; @@ -794,30 +733,23 @@ public void beanRegistered(ObjectName beanName) { log.warn("Could not connect to get bean attributes or class name: " + e.getMessage()); } catch (Exception e) { log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); - } finally { - log.debug("Bean registration processed. {}", beanName); - this.beanAndAttributeLock.unlock(); } - } - public void beanUnregistered(ObjectName mBeanName) { - this.beanAndAttributeLock.lock(); - try { - int removedMetrics = 0; - int removedAttributes = 0; - for (Iterator it = this.matchingAttributes.iterator(); it.hasNext();) { - JmxAttribute current = it.next(); - if (current.getBeanName().compareTo(mBeanName) == 0) { - it.remove(); - removedMetrics += current.getLastMetricsCount(); - removedAttributes++; - } + log.debug("Bean registration processed. {}", beanName); + } + public synchronized void beanUnregistered(ObjectName mBeanName) { + int removedMetrics = 0; + int removedAttributes = 0; + for (Iterator it = this.matchingAttributes.iterator(); it.hasNext();) { + JmxAttribute current = it.next(); + if (current.getBeanName().compareTo(mBeanName) == 0) { + it.remove(); + removedMetrics += current.getLastMetricsCount(); + removedAttributes++; } - log.info("Bean unregistered event, removed {} attributes ({} metrics) matching bean {}", removedAttributes, removedMetrics, mBeanName); - this.metricCountForMatchingAttributes -= removedMetrics; - } finally { - log.debug("Bean unregistration processed. {}", mBeanName); - this.beanAndAttributeLock.unlock(); } + log.info("Bean unregistered event, removed {} attributes ({} metrics) matching bean {}", removedAttributes, removedMetrics, mBeanName); + this.metricCountForMatchingAttributes -= removedMetrics; + log.debug("Bean unregistration processed. {}", mBeanName); } @@ -851,69 +783,64 @@ public List getBeansScopes() { * Query and refresh the instance's list of beans. Limit the query scope when possible on * certain actions, and fallback if necessary. */ - private void refreshBeansList(boolean isInitialQuery) throws IOException { + private synchronized void refreshBeansList(boolean isInitialQuery) throws IOException { Set newBeans = new HashSet<>(); String action = appConfig.getAction(); boolean limitQueryScopes = !action.equals(AppConfig.ACTION_LIST_EVERYTHING) && !action.equals(AppConfig.ACTION_LIST_NOT_MATCHING); - this.beanAndAttributeLock.lock(); - try { - boolean fullBeanQueryNeeded = false; - if (limitQueryScopes) { - try { - List beanScopes = getBeansScopes(); - for (String scope : beanScopes) { - ObjectName name = new ObjectName(scope); - newBeans.addAll(connection.queryNames(name)); - } - } catch (Exception e) { - fullBeanQueryNeeded = true; - log.error( - "Unable to compute a common bean scope, querying all beans as a fallback", - e); + boolean fullBeanQueryNeeded = false; + if (limitQueryScopes) { + try { + List beanScopes = getBeansScopes(); + for (String scope : beanScopes) { + ObjectName name = new ObjectName(scope); + newBeans.addAll(connection.queryNames(name)); } + } catch (Exception e) { + fullBeanQueryNeeded = true; + log.error( + "Unable to compute a common bean scope, querying all beans as a fallback", + e); } + } - if (fullBeanQueryNeeded) { - newBeans = connection.queryNames(null); - } - if (this.beanSubscriptionActive && !fullBeanQueryNeeded && !isInitialQuery) { - // This code exists to validate the bean-subscription is working properly - // If every new bean and de-registered bean properly fires an event, then - // this.beans (current set that has been updated via subscription) should - // always equal the new set of beans queried (unless it was a full bean query) - if (!this.beans.containsAll(newBeans)) { - // Newly queried set of beans contains beans not actively tracked - // ie, maybe a registeredBean subscription msg did not get processed correctly - Set beansNotSeen = new HashSet<>(); - beansNotSeen.addAll(newBeans); - beansNotSeen.removeAll(this.beans); - - log.error("Bean refresh found {} new beans that were not already tracked via subscription", beansNotSeen.size()); - for (ObjectName b : beansNotSeen) { - log.error("New not-tracked bean {}", b); - } + if (fullBeanQueryNeeded) { + newBeans = connection.queryNames(null); + } + if (this.beanSubscriptionActive && !fullBeanQueryNeeded && !isInitialQuery) { + // This code exists to validate the bean-subscription is working properly + // If every new bean and de-registered bean properly fires an event, then + // this.beans (current set that has been updated via subscription) should + // always equal the new set of beans queried (unless it was a full bean query) + if (!this.beans.containsAll(newBeans)) { + // Newly queried set of beans contains beans not actively tracked + // ie, maybe a registeredBean subscription msg did not get processed correctly + Set beansNotSeen = new HashSet<>(); + beansNotSeen.addAll(newBeans); + beansNotSeen.removeAll(this.beans); + + log.error("Bean refresh found {} new beans that were not already tracked via subscription", beansNotSeen.size()); + for (ObjectName b : beansNotSeen) { + log.error("New not-tracked bean {}", b); } - if (!newBeans.containsAll(this.beans)){ - // Newly queried set of beans is missing beans that are actively tracked - // ie, maybe a deregisteredBean subscription msg did not get processed correctly - Set incorrectlyTrackedBeans = new HashSet<>(); - incorrectlyTrackedBeans.addAll(this.beans); - incorrectlyTrackedBeans.removeAll(newBeans); - - log.error("Bean refresh found {} FEWER beans than already tracked via subscription", incorrectlyTrackedBeans.size()); - for (ObjectName b : incorrectlyTrackedBeans) { - log.error("Currently tracked bean that not returned from fresh query: {}", b); - } + } + if (!newBeans.containsAll(this.beans)){ + // Newly queried set of beans is missing beans that are actively tracked + // ie, maybe a deregisteredBean subscription msg did not get processed correctly + Set incorrectlyTrackedBeans = new HashSet<>(); + incorrectlyTrackedBeans.addAll(this.beans); + incorrectlyTrackedBeans.removeAll(newBeans); + + log.error("Bean refresh found {} FEWER beans than already tracked via subscription", incorrectlyTrackedBeans.size()); + for (ObjectName b : incorrectlyTrackedBeans) { + log.error("Currently tracked bean that not returned from fresh query: {}", b); } } - this.beans = newBeans; - this.lastRefreshTime = System.currentTimeMillis(); - } finally { - this.beanAndAttributeLock.unlock(); } + this.beans = newBeans; + this.lastRefreshTime = System.currentTimeMillis(); } /** True if instance is in a good state to collect metrics */ From 090f0dc8e02ef9439d501812d219df7f17ffd223 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Tue, 23 May 2023 21:36:19 +0000 Subject: [PATCH 21/41] Adds owned thread to process mbean subscription events --- .../jmxfetch/BeanNotificationListener.java | 56 +++++++++++++++++++ .../java/org/datadog/jmxfetch/Connection.java | 23 -------- .../java/org/datadog/jmxfetch/Instance.java | 2 - 3 files changed, 56 insertions(+), 25 deletions(-) create mode 100644 src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java diff --git a/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java b/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java new file mode 100644 index 000000000..594957f6a --- /dev/null +++ b/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java @@ -0,0 +1,56 @@ +package org.datadog.jmxfetch; + +import lombok.extern.slf4j.Slf4j; + +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.LinkedBlockingQueue; + +import javax.management.MBeanServerNotification; +import javax.management.Notification; +import javax.management.NotificationListener; +import javax.management.ObjectName; + + +@Slf4j +class BeanNotificationListener implements NotificationListener { + private final BlockingQueue queue; + private final BeanListener beanListener; + + public BeanNotificationListener(final BeanListener bl) { + this.beanListener = bl; + this.queue = new LinkedBlockingQueue<>(); + new Thread(new Runnable() { + @Override + public void run() { + while (true) { + MBeanServerNotification mbs; + try { + mbs = queue.take(); + processMBeanServerNotification(mbs); + } catch (InterruptedException e) { + // ignore + } + } + } + }).start(); + } + + @Override + public void handleNotification(Notification notification, Object handback) { + if (!(notification instanceof MBeanServerNotification)) { + return; + } + MBeanServerNotification mbs = (MBeanServerNotification) notification; + queue.offer(mbs); + } + + private void processMBeanServerNotification(MBeanServerNotification mbs) { + log.debug("MBeanNotification: ts {} seqNum: {} msg: '{}'", mbs.getTimeStamp(), mbs.getSequenceNumber(), mbs.getMessage()); + ObjectName mBeanName = mbs.getMBeanName(); + if (mbs.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { + beanListener.beanRegistered(mBeanName); + } else if (mbs.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { + beanListener.beanUnregistered(mBeanName); + } + } +} \ No newline at end of file diff --git a/src/main/java/org/datadog/jmxfetch/Connection.java b/src/main/java/org/datadog/jmxfetch/Connection.java index 7ceae6d28..715ca5e45 100644 --- a/src/main/java/org/datadog/jmxfetch/Connection.java +++ b/src/main/java/org/datadog/jmxfetch/Connection.java @@ -11,7 +11,6 @@ import java.util.Set; import java.util.function.Consumer; import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.BlockingQueue; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.ThreadFactory; @@ -27,7 +26,6 @@ import javax.management.MBeanServerConnection; import javax.management.MBeanServerDelegate; import javax.management.relation.MBeanServerNotificationFilter; -import javax.management.MBeanServerNotification; import javax.management.MalformedObjectNameException; import javax.management.Notification; import javax.management.NotificationFilter; @@ -72,27 +70,6 @@ public void handleNotification(Notification notification, Object handback) { } } - private static class BeanNotificationListener implements NotificationListener { - private BeanListener bl; - - public BeanNotificationListener(BeanListener bl) { - this.bl = bl; - } - public void handleNotification(Notification notification, Object handback) { - if (!(notification instanceof MBeanServerNotification)) { - return; - } - MBeanServerNotification mbs = (MBeanServerNotification) notification; - log.debug("MBeanNotification: ts {} seqNum: {} msg: '{}'", mbs.getTimeStamp(), mbs.getSequenceNumber(), mbs.getMessage()); - ObjectName mBeanName = mbs.getMBeanName(); - if (mbs.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { - bl.beanRegistered(mBeanName); - } else if (mbs.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { - this.bl.beanUnregistered(mBeanName); - } - } - } - public void subscribeToBeanScopes(List beanScopes, BeanListener bl) throws MalformedObjectNameException, IOException, InstanceNotFoundException{ BeanNotificationListener listener = new BeanNotificationListener(bl); for (String scope : beanScopes) { diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 51023868a..0b9a5f11d 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -757,8 +757,6 @@ private void subscribeToBeans() { List beanScopes = this.getBeansScopes(); try { - // TODO - Processing a bean subscription event could be slow, which could hold up other subscriptions - // Would be better to queue subscription events up and process that queue on a dedicated thread. connection.subscribeToBeanScopes(beanScopes, this); this.beanSubscriptionActive = true; log.info("Subscribed to {} bean scopes successfully!", beanScopes.size()); From c3fbfe6b439b771ae99ecddbb7c170040ac70e8e Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Tue, 23 May 2023 21:50:01 +0000 Subject: [PATCH 22/41] Remove some unused logs and accidental regressions --- src/main/java/org/datadog/jmxfetch/Instance.java | 15 +++++++-------- .../datadog/jmxfetch/TestBeanSubscription.java | 1 - 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 0b9a5f11d..477abb5e3 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -475,7 +475,6 @@ public synchronized List getMetrics() throws IOException { // post initialization and every x seconds thereafter. // To enable this, a "refresh_beans_initial" and/or "refresh_beans" parameters must be // specified in the yaml/json config - log.debug("getMetrics invoked"); Integer period = (this.initialRefreshTime == this.lastRefreshTime) ? this.initialRefreshBeansPeriod : this.refreshBeansPeriod; @@ -500,10 +499,9 @@ public synchronized List getMetrics() throws IOException { metrics.addAll(jmxAttrMetrics); this.failingAttributes.remove(jmxAttr); } catch (IOException e) { - log.warn("Got IOException {}, rethrowing...", e); throw e; } catch (Exception e) { - log.warn("Cannot get metrics for attribute: " + jmxAttr, e); + log.debug("Cannot get metrics for attribute: " + jmxAttr, e); if (this.failingAttributes.contains(jmxAttr)) { log.debug( "Cannot generate metrics for attribute: " @@ -684,7 +682,7 @@ private void getMatchingAttributes() throws IOException { reporter.displayInstanceName(this); } - for (ObjectName beanName : beans) { + for (ObjectName beanName : this.beans) { if (limitReached) { log.debug("Limit reached"); if (action.equals(AppConfig.ACTION_COLLECT)) { @@ -704,7 +702,7 @@ private void getMatchingAttributes() throws IOException { } catch (IOException e) { throw e; } catch (Exception e) { - log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); + log.warn("Cannot get bean attributes or class name: {}", e.getMessage()); continue; } @@ -743,13 +741,15 @@ public synchronized void beanUnregistered(ObjectName mBeanName) { JmxAttribute current = it.next(); if (current.getBeanName().compareTo(mBeanName) == 0) { it.remove(); + // `getLastMetricsCount` used here instead of `getMetricsCount` because + // the bean no longer exists by the time we get this message. + // `getMetricsCount` does a live query and therefore will fail removedMetrics += current.getLastMetricsCount(); removedAttributes++; } } - log.info("Bean unregistered event, removed {} attributes ({} metrics) matching bean {}", removedAttributes, removedMetrics, mBeanName); + log.debug("Bean unregistered, removed {} attributes ({} metrics) matching bean {}", removedAttributes, removedMetrics, mBeanName); this.metricCountForMatchingAttributes -= removedMetrics; - log.debug("Bean unregistration processed. {}", mBeanName); } @@ -761,7 +761,6 @@ private void subscribeToBeans() { this.beanSubscriptionActive = true; log.info("Subscribed to {} bean scopes successfully!", beanScopes.size()); } catch (MalformedObjectNameException | InstanceNotFoundException | IOException e) { - e.printStackTrace(); log.warn("Bean subscription failed! Will rely on bean_refresh, ensure it is set " + " to an appropriate value (currently {} seconds). Exception: ", this.refreshBeansPeriod, e); diff --git a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java index 5f396d713..4500cfa2a 100644 --- a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java +++ b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java @@ -69,7 +69,6 @@ public void testJMXFetchBasic() throws IOException, InterruptedException { " - BooleanValue" ); - log.info("hello"); this.app.doIteration(); List> metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); assertEquals(0, metrics.size()); From addb80f501d389bdbe2d9578ed079e4527d79ed4 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 24 May 2023 20:41:17 +0000 Subject: [PATCH 23/41] Addresses linting errors --- .../org/datadog/jmxfetch/BeanListener.java | 5 +- .../jmxfetch/BeanNotificationListener.java | 17 ++++--- .../java/org/datadog/jmxfetch/Connection.java | 30 +++++------- .../java/org/datadog/jmxfetch/Instance.java | 49 ++++++++++++------- .../org/datadog/jmxfetch/JmxAttribute.java | 5 +- 5 files changed, 58 insertions(+), 48 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/BeanListener.java b/src/main/java/org/datadog/jmxfetch/BeanListener.java index c30ab789b..9af948988 100644 --- a/src/main/java/org/datadog/jmxfetch/BeanListener.java +++ b/src/main/java/org/datadog/jmxfetch/BeanListener.java @@ -3,6 +3,7 @@ import javax.management.ObjectName; public interface BeanListener { - public void beanRegistered(ObjectName mBeanName); - public void beanUnregistered(ObjectName mBeanName); + public void beanRegistered(ObjectName beanName); + + public void beanUnregistered(ObjectName beanName); } diff --git a/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java b/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java index 594957f6a..8640f872a 100644 --- a/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java +++ b/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java @@ -44,13 +44,16 @@ public void handleNotification(Notification notification, Object handback) { queue.offer(mbs); } - private void processMBeanServerNotification(MBeanServerNotification mbs) { - log.debug("MBeanNotification: ts {} seqNum: {} msg: '{}'", mbs.getTimeStamp(), mbs.getSequenceNumber(), mbs.getMessage()); - ObjectName mBeanName = mbs.getMBeanName(); - if (mbs.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { - beanListener.beanRegistered(mBeanName); - } else if (mbs.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { - beanListener.beanUnregistered(mBeanName); + private void processMBeanServerNotification(MBeanServerNotification notif) { + log.debug("MBeanNotification: ts {} seqNum: {} msg: '{}'", + notif.getTimeStamp(), + notif.getSequenceNumber(), + notif.getMessage()); + ObjectName beanName = notif.getMBeanName(); + if (notif.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { + beanListener.beanRegistered(beanName); + } else if (notif.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { + beanListener.beanUnregistered(beanName); } } } \ No newline at end of file diff --git a/src/main/java/org/datadog/jmxfetch/Connection.java b/src/main/java/org/datadog/jmxfetch/Connection.java index 715ca5e45..605018388 100644 --- a/src/main/java/org/datadog/jmxfetch/Connection.java +++ b/src/main/java/org/datadog/jmxfetch/Connection.java @@ -3,35 +3,24 @@ import lombok.extern.slf4j.Slf4j; import java.io.IOException; -import java.io.InterruptedIOException; -import java.net.SocketTimeoutException; -import java.util.HashMap; -import java.util.Map; import java.util.List; +import java.util.Map; import java.util.Set; -import java.util.function.Consumer; -import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.ThreadFactory; -import java.util.concurrent.TimeUnit; import javax.management.Attribute; import javax.management.AttributeNotFoundException; import javax.management.InstanceNotFoundException; import javax.management.IntrospectionException; import javax.management.ListenerNotFoundException; -import javax.management.MBeanAttributeInfo; import javax.management.MBeanException; import javax.management.MBeanInfo; import javax.management.MBeanServerConnection; import javax.management.MBeanServerDelegate; -import javax.management.relation.MBeanServerNotificationFilter; import javax.management.MalformedObjectNameException; import javax.management.Notification; -import javax.management.NotificationFilter; import javax.management.NotificationListener; import javax.management.ObjectName; import javax.management.ReflectionException; +import javax.management.relation.MBeanServerNotificationFilter; import javax.management.remote.JMXConnectionNotification; import javax.management.remote.JMXConnector; import javax.management.remote.JMXConnectorFactory; @@ -62,7 +51,8 @@ public void handleNotification(Notification notification, Object handback) { if (connNotif.getType() == JMXConnectionNotification.CLOSED || connNotif.getType() == JMXConnectionNotification.FAILED || connNotif.getType() == JMXConnectionNotification.NOTIFS_LOST) { - log.warn("Connection is potentially in a bad state, marking connection issues. {} - {}", connNotif.getType(), connNotif.getMessage()); + log.warn("Marking connection issues due to {} - {}", + connNotif.getType(), connNotif.getMessage()); conn.seenConnectionIssues = true; } log.debug("Received connection notification: {} Message: {}", @@ -70,7 +60,9 @@ public void handleNotification(Notification notification, Object handback) { } } - public void subscribeToBeanScopes(List beanScopes, BeanListener bl) throws MalformedObjectNameException, IOException, InstanceNotFoundException{ + /** Subscribes for bean registration/deregistration events under the specified bean scopes. */ + public void subscribeToBeanScopes(List beanScopes, BeanListener bl) + throws MalformedObjectNameException, IOException, InstanceNotFoundException { BeanNotificationListener listener = new BeanNotificationListener(bl); for (String scope : beanScopes) { ObjectName name = new ObjectName(scope); @@ -102,7 +94,8 @@ protected void createConnection() throws IOException { mbs = connector.getMBeanServerConnection(); this.connectionNotificationListener = new ConnectionNotificationListener(); - connector.addConnectionNotificationListener(this.connectionNotificationListener, null, this); + connector.addConnectionNotificationListener( + this.connectionNotificationListener, null, this); } /** Gets attribute for matching bean and attribute name. */ @@ -120,7 +113,8 @@ public Object getAttribute(ObjectName objectName, String attributeName) public void closeConnector() { if (connector != null) { try { - this.connector.removeConnectionNotificationListener(this.connectionNotificationListener); + this.connector.removeConnectionNotificationListener( + this.connectionNotificationListener); connector.close(); connector = null; } catch (IOException | ListenerNotFoundException e) { @@ -129,7 +123,7 @@ public void closeConnector() { } } - /** True if connection has been notified of failure/lost notifications */ + /** True if connection has been notified of failure/lost notifications. */ public boolean hasSeenConnectionIssues() { return this.seenConnectionIssues; } diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 477abb5e3..d3a08a23c 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -6,8 +6,6 @@ import org.datadog.jmxfetch.service.ServiceNameProvider; import org.yaml.snakeyaml.Yaml; -import static java.util.concurrent.TimeUnit.*; - import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -30,10 +28,8 @@ import javax.management.ObjectName; import javax.security.auth.login.FailedLoginException; -import java.util.concurrent.locks.ReentrantLock; - @Slf4j -public class Instance implements BeanListener{ +public class Instance implements BeanListener { private static final List SIMPLE_TYPES = Arrays.asList( "long", @@ -517,7 +513,8 @@ public synchronized List getMetrics() throws IOException { } long duration = System.currentTimeMillis() - this.lastCollectionTime; - log.info("Collection " + matchingAttributes.size() + " matching attributes finished in " + duration + "ms."); + log.info("Collection of {} matching attributes finished in {}ms", + matchingAttributes.size(), duration); return metrics; } @@ -539,7 +536,11 @@ public boolean timeToCollect() { } } - private synchronized void addMatchingAttributesForBean(ObjectName beanName, String className, MBeanAttributeInfo[] attributeInfos) throws IOException { + private synchronized void addMatchingAttributesForBean( + ObjectName beanName, + String className, + MBeanAttributeInfo[] attributeInfos + ) throws IOException { boolean metricReachedDisplayed = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); @@ -638,7 +639,7 @@ private synchronized void addMatchingAttributesForBean(ObjectName beanName, Stri if (jmxAttribute.match(conf)) { jmxAttribute.setMatchingConf(conf); this.metricCountForMatchingAttributes += jmxAttribute.getMetricsCount(); - log.debug("Added attribute {} from bean {}.", jmxAttribute.toString(), beanName); + log.debug("Added attribute {} from bean {}.", jmxAttribute, beanName); this.matchingAttributes.add(jmxAttribute); if (action.equals(AppConfig.ACTION_LIST_EVERYTHING) || action.equals(AppConfig.ACTION_LIST_MATCHING) @@ -647,7 +648,9 @@ private synchronized void addMatchingAttributesForBean(ObjectName beanName, Stri || action.equals(AppConfig.ACTION_LIST_LIMITED) && limitReached) { reporter.displayMatchingAttributeName( - jmxAttribute, this.metricCountForMatchingAttributes, maxReturnedMetrics); + jmxAttribute, + this.metricCountForMatchingAttributes, + maxReturnedMetrics); } break; } @@ -708,9 +711,12 @@ private void getMatchingAttributes() throws IOException { addMatchingAttributesForBean(beanName, className, attributeInfos); } - log.info("Found {} matching attributes with {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); + log.info("Found {} matching attributes with {} metrics total", + matchingAttributes.size(), + this.metricCountForMatchingAttributes); } + /** Adds any matching attributes from the specified bean. */ public synchronized void beanRegistered(ObjectName beanName) { log.debug("Bean registered event. {}", beanName); String className; @@ -734,12 +740,14 @@ public synchronized void beanRegistered(ObjectName beanName) { } log.debug("Bean registration processed. {}", beanName); } - public synchronized void beanUnregistered(ObjectName mBeanName) { + + /** Removes any matching attributes from the specified bean. */ + public synchronized void beanUnregistered(ObjectName beanName) { int removedMetrics = 0; int removedAttributes = 0; for (Iterator it = this.matchingAttributes.iterator(); it.hasNext();) { JmxAttribute current = it.next(); - if (current.getBeanName().compareTo(mBeanName) == 0) { + if (current.getBeanName().compareTo(beanName) == 0) { it.remove(); // `getLastMetricsCount` used here instead of `getMetricsCount` because // the bean no longer exists by the time we get this message. @@ -748,11 +756,13 @@ public synchronized void beanUnregistered(ObjectName mBeanName) { removedAttributes++; } } - log.debug("Bean unregistered, removed {} attributes ({} metrics) matching bean {}", removedAttributes, removedMetrics, mBeanName); + log.debug("Bean unregistered, removed {} attributes ({} metrics) matching bean {}", + removedAttributes, + removedMetrics, + beanName); this.metricCountForMatchingAttributes -= removedMetrics; } - private void subscribeToBeans() { List beanScopes = this.getBeansScopes(); @@ -818,21 +828,22 @@ private synchronized void refreshBeansList(boolean isInitialQuery) throws IOExce beansNotSeen.addAll(newBeans); beansNotSeen.removeAll(this.beans); - log.error("Bean refresh found {} new beans that were not already tracked via subscription", beansNotSeen.size()); + log.error("Bean refresh found {} previously untracked beans", beansNotSeen.size()); for (ObjectName b : beansNotSeen) { log.error("New not-tracked bean {}", b); } } - if (!newBeans.containsAll(this.beans)){ + if (!newBeans.containsAll(this.beans)) { // Newly queried set of beans is missing beans that are actively tracked // ie, maybe a deregisteredBean subscription msg did not get processed correctly Set incorrectlyTrackedBeans = new HashSet<>(); incorrectlyTrackedBeans.addAll(this.beans); incorrectlyTrackedBeans.removeAll(newBeans); - log.error("Bean refresh found {} FEWER beans than already tracked via subscription", incorrectlyTrackedBeans.size()); + log.error("Bean refresh found {} fewer beans than expected", + incorrectlyTrackedBeans.size()); for (ObjectName b : incorrectlyTrackedBeans) { - log.error("Currently tracked bean that not returned from fresh query: {}", b); + log.error("Currently tracked bean not returned from fresh query: {}", b); } } } @@ -840,7 +851,7 @@ private synchronized void refreshBeansList(boolean isInitialQuery) throws IOExce this.lastRefreshTime = System.currentTimeMillis(); } - /** True if instance is in a good state to collect metrics */ + /** True if instance is in a good state to collect metrics. */ private boolean isInstanceHealthy() { // If we have subscription mode on and the connection has either failed or notifications // have been lost, then we must consider this instance unhealthy and re-init. diff --git a/src/main/java/org/datadog/jmxfetch/JmxAttribute.java b/src/main/java/org/datadog/jmxfetch/JmxAttribute.java index 76729f144..ddde933a4 100644 --- a/src/main/java/org/datadog/jmxfetch/JmxAttribute.java +++ b/src/main/java/org/datadog/jmxfetch/JmxAttribute.java @@ -260,7 +260,8 @@ protected abstract List getMetricsImpl() throws AttributeNotFoundException, InstanceNotFoundException, MBeanException, ReflectionException, IOException; - final public List getMetrics() + /** Returns all metrics tracked by this attribute. */ + public final List getMetrics() throws AttributeNotFoundException, InstanceNotFoundException, MBeanException, ReflectionException, IOException { List metrics = this.getMetricsImpl(); @@ -289,7 +290,7 @@ public int getMetricsCount() { } } - /** Gets the most recent collection's metric count */ + /** Gets the most recent collection's metric count. */ public int getLastMetricsCount() { return this.lastMetricSize; } From 08cee1bca242fbef43979d831d87af2bd61bff72 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 24 May 2023 20:45:12 +0000 Subject: [PATCH 24/41] Renames ambiguous interface from BeanListener -> BeanTracker --- src/main/java/org/datadog/jmxfetch/BeanListener.java | 9 --------- .../org/datadog/jmxfetch/BeanNotificationListener.java | 8 ++++---- src/main/java/org/datadog/jmxfetch/BeanTracker.java | 9 +++++++++ src/main/java/org/datadog/jmxfetch/Connection.java | 2 +- src/main/java/org/datadog/jmxfetch/Instance.java | 6 +++--- 5 files changed, 17 insertions(+), 17 deletions(-) delete mode 100644 src/main/java/org/datadog/jmxfetch/BeanListener.java create mode 100644 src/main/java/org/datadog/jmxfetch/BeanTracker.java diff --git a/src/main/java/org/datadog/jmxfetch/BeanListener.java b/src/main/java/org/datadog/jmxfetch/BeanListener.java deleted file mode 100644 index 9af948988..000000000 --- a/src/main/java/org/datadog/jmxfetch/BeanListener.java +++ /dev/null @@ -1,9 +0,0 @@ -package org.datadog.jmxfetch; - -import javax.management.ObjectName; - -public interface BeanListener { - public void beanRegistered(ObjectName beanName); - - public void beanUnregistered(ObjectName beanName); -} diff --git a/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java b/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java index 8640f872a..89c137a8a 100644 --- a/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java +++ b/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java @@ -14,9 +14,9 @@ @Slf4j class BeanNotificationListener implements NotificationListener { private final BlockingQueue queue; - private final BeanListener beanListener; + private final BeanTracker beanListener; - public BeanNotificationListener(final BeanListener bl) { + public BeanNotificationListener(final BeanTracker bl) { this.beanListener = bl; this.queue = new LinkedBlockingQueue<>(); new Thread(new Runnable() { @@ -51,9 +51,9 @@ private void processMBeanServerNotification(MBeanServerNotification notif) { notif.getMessage()); ObjectName beanName = notif.getMBeanName(); if (notif.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { - beanListener.beanRegistered(beanName); + beanListener.trackBean(beanName); } else if (notif.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { - beanListener.beanUnregistered(beanName); + beanListener.untrackBean(beanName); } } } \ No newline at end of file diff --git a/src/main/java/org/datadog/jmxfetch/BeanTracker.java b/src/main/java/org/datadog/jmxfetch/BeanTracker.java new file mode 100644 index 000000000..81ff9b1cb --- /dev/null +++ b/src/main/java/org/datadog/jmxfetch/BeanTracker.java @@ -0,0 +1,9 @@ +package org.datadog.jmxfetch; + +import javax.management.ObjectName; + +public interface BeanTracker { + public void trackBean(ObjectName beanName); + + public void untrackBean(ObjectName beanName); +} diff --git a/src/main/java/org/datadog/jmxfetch/Connection.java b/src/main/java/org/datadog/jmxfetch/Connection.java index 605018388..9d7484c6c 100644 --- a/src/main/java/org/datadog/jmxfetch/Connection.java +++ b/src/main/java/org/datadog/jmxfetch/Connection.java @@ -61,7 +61,7 @@ public void handleNotification(Notification notification, Object handback) { } /** Subscribes for bean registration/deregistration events under the specified bean scopes. */ - public void subscribeToBeanScopes(List beanScopes, BeanListener bl) + public void subscribeToBeanScopes(List beanScopes, BeanTracker bl) throws MalformedObjectNameException, IOException, InstanceNotFoundException { BeanNotificationListener listener = new BeanNotificationListener(bl); for (String scope : beanScopes) { diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index d3a08a23c..91c3ead85 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -29,7 +29,7 @@ import javax.security.auth.login.FailedLoginException; @Slf4j -public class Instance implements BeanListener { +public class Instance implements BeanTracker { private static final List SIMPLE_TYPES = Arrays.asList( "long", @@ -717,7 +717,7 @@ private void getMatchingAttributes() throws IOException { } /** Adds any matching attributes from the specified bean. */ - public synchronized void beanRegistered(ObjectName beanName) { + public synchronized void trackBean(ObjectName beanName) { log.debug("Bean registered event. {}", beanName); String className; MBeanAttributeInfo[] attributeInfos; @@ -742,7 +742,7 @@ public synchronized void beanRegistered(ObjectName beanName) { } /** Removes any matching attributes from the specified bean. */ - public synchronized void beanUnregistered(ObjectName beanName) { + public synchronized void untrackBean(ObjectName beanName) { int removedMetrics = 0; int removedAttributes = 0; for (Iterator it = this.matchingAttributes.iterator(); it.hasNext();) { From 27dc0570d25e5b0d3d0c614c71960aaa682604f1 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 24 May 2023 21:02:23 +0000 Subject: [PATCH 25/41] Fixes bug where metricReached would display multiple times incorrectly --- .../java/org/datadog/jmxfetch/Instance.java | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 91c3ead85..8cce3adb3 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -487,7 +487,6 @@ public synchronized List getMetrics() throws IOException { // increment the lastCollectionTime this.lastCollectionTime = System.currentTimeMillis(); - log.info("Starting Collection of " + matchingAttributes.size() + " attributes"); while (it.hasNext()) { JmxAttribute jmxAttr = it.next(); try { @@ -539,9 +538,9 @@ public boolean timeToCollect() { private synchronized void addMatchingAttributesForBean( ObjectName beanName, String className, - MBeanAttributeInfo[] attributeInfos + MBeanAttributeInfo[] attributeInfos, + boolean metricReachedPreviouslyDisplayed ) throws IOException { - boolean metricReachedDisplayed = false; Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); @@ -551,11 +550,10 @@ private synchronized void addMatchingAttributesForBean( if (action.equals(AppConfig.ACTION_COLLECT)) { log.warn("Maximum number of metrics reached."); break; - } else if (!metricReachedDisplayed + } else if (!metricReachedPreviouslyDisplayed && !action.equals(AppConfig.ACTION_LIST_COLLECTED) && !action.equals(AppConfig.ACTION_LIST_NOT_MATCHING)) { reporter.displayMetricReached(); - metricReachedDisplayed = true; } } JmxAttribute jmxAttribute; @@ -644,9 +642,9 @@ private synchronized void addMatchingAttributesForBean( if (action.equals(AppConfig.ACTION_LIST_EVERYTHING) || action.equals(AppConfig.ACTION_LIST_MATCHING) || action.equals(AppConfig.ACTION_LIST_COLLECTED) - && !limitReached + && !this.limitReached || action.equals(AppConfig.ACTION_LIST_LIMITED) - && limitReached) { + && this.limitReached) { reporter.displayMatchingAttributeName( jmxAttribute, this.metricCountForMatchingAttributes, @@ -674,19 +672,19 @@ private synchronized void addMatchingAttributesForBean( } private void getMatchingAttributes() throws IOException { - limitReached = false; - Reporter reporter = appConfig.getReporter(); - String action = appConfig.getAction(); - this.matchingAttributes.clear(); this.failingAttributes.clear(); + this.limitReached = false; + + Reporter reporter = appConfig.getReporter(); + String action = appConfig.getAction(); if (!action.equals(AppConfig.ACTION_COLLECT)) { reporter.displayInstanceName(this); } for (ObjectName beanName : this.beans) { - if (limitReached) { + if (this.limitReached) { log.debug("Limit reached"); if (action.equals(AppConfig.ACTION_COLLECT)) { break; @@ -709,7 +707,7 @@ private void getMatchingAttributes() throws IOException { continue; } - addMatchingAttributesForBean(beanName, className, attributeInfos); + addMatchingAttributesForBean(beanName, className, attributeInfos, limitReached); } log.info("Found {} matching attributes with {} metrics total", matchingAttributes.size(), @@ -730,7 +728,7 @@ public synchronized void trackBean(ObjectName beanName) { log.debug("Getting attributes for bean: {}", beanName); attributeInfos = info.getAttributes(); - this.addMatchingAttributesForBean(beanName, className, attributeInfos); + this.addMatchingAttributesForBean(beanName, className, attributeInfos, false); this.beans.add(beanName); } catch (IOException e) { // Nothing to do, connection issue From 642ee12141a23e42244812b6e495df52be635dd1 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 31 May 2023 16:53:27 +0000 Subject: [PATCH 26/41] Corrects old name and var decl --- .../datadog/jmxfetch/BeanNotificationListener.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java b/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java index 89c137a8a..026ca39f9 100644 --- a/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java +++ b/src/main/java/org/datadog/jmxfetch/BeanNotificationListener.java @@ -14,18 +14,17 @@ @Slf4j class BeanNotificationListener implements NotificationListener { private final BlockingQueue queue; - private final BeanTracker beanListener; + private final BeanTracker beanTracker; - public BeanNotificationListener(final BeanTracker bl) { - this.beanListener = bl; + public BeanNotificationListener(final BeanTracker bt) { + this.beanTracker = bt; this.queue = new LinkedBlockingQueue<>(); new Thread(new Runnable() { @Override public void run() { while (true) { - MBeanServerNotification mbs; try { - mbs = queue.take(); + MBeanServerNotification mbs = queue.take(); processMBeanServerNotification(mbs); } catch (InterruptedException e) { // ignore @@ -51,9 +50,9 @@ private void processMBeanServerNotification(MBeanServerNotification notif) { notif.getMessage()); ObjectName beanName = notif.getMBeanName(); if (notif.getType().equals(MBeanServerNotification.REGISTRATION_NOTIFICATION)) { - beanListener.trackBean(beanName); + beanTracker.trackBean(beanName); } else if (notif.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { - beanListener.untrackBean(beanName); + beanTracker.untrackBean(beanName); } } } \ No newline at end of file From e8c2b68573739aec60d0773735822b00d0fdab0b Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Mon, 5 Jun 2023 17:35:55 +0000 Subject: [PATCH 27/41] Adds toggle to turn on bean subscription globally --- src/main/java/org/datadog/jmxfetch/Instance.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 8cce3adb3..b5bece7c1 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -268,6 +268,12 @@ public Instance( this.beans = new HashSet<>(); Boolean enableBeanSubscription = (Boolean) instanceMap.get("enable_bean_subscription"); this.enableBeanSubscription = enableBeanSubscription != null && enableBeanSubscription; + + // Global override for enabling bean subscription on all instances + String enableSubscriptionOverride = System.getenv("DD_JMXFETCH_ENABLE_BEAN_SUBSCRIPTION"); + if (enableSubscriptionOverride != null && enableSubscriptionOverride.equalsIgnoreCase("true")) { + this.enableBeanSubscription = true; + } this.beanSubscriptionActive = false; } From 79ce04758bac9db369ca9b87c3061211018e5787 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 5 Jul 2023 20:28:57 +0000 Subject: [PATCH 28/41] Adds env vars to enable subscription globally for validation --- .../java/org/datadog/jmxfetch/Instance.java | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index b5bece7c1..330cda814 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -19,6 +19,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Random; import java.util.Set; import javax.management.InstanceNotFoundException; @@ -270,10 +271,18 @@ public Instance( this.enableBeanSubscription = enableBeanSubscription != null && enableBeanSubscription; // Global override for enabling bean subscription on all instances - String enableSubscriptionOverride = System.getenv("DD_JMXFETCH_ENABLE_BEAN_SUBSCRIPTION"); - if (enableSubscriptionOverride != null && enableSubscriptionOverride.equalsIgnoreCase("true")) { + String enableSubscriptionOverride = System.getenv("DD_JMXFETCH_SUBSCRIPTION_ENABLED"); + if (enableSubscriptionOverride != null + && enableSubscriptionOverride.equalsIgnoreCase("true")) { this.enableBeanSubscription = true; } + String subscriptionCoinFlip = System.getenv("DD_JMXFETCH_SUBSCRIPTION_FLIPCOIN"); + if (subscriptionCoinFlip != null && subscriptionCoinFlip.equalsIgnoreCase("true")) { + Random rd = new Random(); + boolean enabled = rd.nextBoolean(); + this.enableBeanSubscription = enabled; + } + log.info("JMXFetch Subscription mode enabled={}", this.enableBeanSubscription); this.beanSubscriptionActive = false; } @@ -518,13 +527,16 @@ public synchronized List getMetrics() throws IOException { } long duration = System.currentTimeMillis() - this.lastCollectionTime; - log.info("Collection of {} matching attributes finished in {}ms", - matchingAttributes.size(), duration); + log.info("Collection finished in {}ms. MatchingAttributes={} CollectedMetrics={}", + duration, matchingAttributes.size(), metrics.size()); return metrics; } /** Returns whather or not the given period has elapsed since reference time. */ public boolean isPeriodDue(long refTime, Integer refPeriod) { + if (this.beanSubscriptionActive) { + return false; + } if ((System.currentTimeMillis() - refTime) / 1000 < refPeriod) { return false; } else { From dcf963cebfc55496be08d6e9ae654b6e7c4c681a Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Thu, 6 Jul 2023 20:48:23 +0000 Subject: [PATCH 29/41] Updates TestBeanSubscription to use container IP directly similar to ReconnectContainer --- .../jmxfetch/TestBeanSubscription.java | 72 +++++++++++++------ .../jmxfetch/TestReconnectContainer.java | 1 - 2 files changed, 50 insertions(+), 23 deletions(-) diff --git a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java index 4500cfa2a..c30994578 100644 --- a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java +++ b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java @@ -11,9 +11,11 @@ import lombok.extern.slf4j.Slf4j; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TestRule; +import org.junit.runners.model.Statement; +import org.junit.runner.Description; import org.testcontainers.containers.GenericContainer; import org.testcontainers.containers.output.Slf4jLogConsumer; import org.testcontainers.containers.wait.strategy.Wait; @@ -25,35 +27,56 @@ public class TestBeanSubscription extends TestCommon { private static final int rmiPort = 9090; private static final int controlPort = 9091; + private static final int supervisorPort = 9092; private JMXServerControlClient controlClient; + private JMXServerSupervisorClient supervisorClient; private static Slf4jLogConsumer logConsumer = new Slf4jLogConsumer(log); private static ImageFromDockerfile img = new ImageFromDockerfile() .withFileFromPath(".", Paths.get("./tools/misbehaving-jmx-server/")); - @Before - public void setup() { - this.controlClient = new JMXServerControlClient(cont.getHost(), cont.getMappedPort(controlPort)); - } - - @Rule + @Rule(order = 0) public GenericContainer cont = new GenericContainer<>(img) - .withExposedPorts(rmiPort, controlPort) .withEnv(Collections.singletonMap("RMI_PORT", "" + rmiPort)) .withEnv(Collections.singletonMap("CONTROL_PORT", "" + controlPort)) - .waitingFor(Wait.forLogMessage(".*IAMREADY.*", 1)); + .withEnv(Collections.singletonMap("SUPERVISOR_PORT", "" + supervisorPort)) + .waitingFor(Wait.forLogMessage(".*Supervisor HTTP Server Started. Waiting for initialization payload POST to /init.*", 1)); + + @Rule(order = 1) + public TestRule setupRule = new TestRule() { + @Override + public Statement apply(final Statement base, Description description) { + return new Statement() { + @Override + public void evaluate() throws Throwable { + String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); + controlClient = new JMXServerControlClient(ipAddress, controlPort); + supervisorClient = new JMXServerSupervisorClient(ipAddress, supervisorPort); + cont.followOutput(logConsumer); + try { + log.info("Initializing JMX Server with RMI hostname {}", ipAddress); + supervisorClient.initializeJMXServer(ipAddress); + } catch (IOException e) { + log.warn("Supervisor call to set rmi hostname failed, tests may fail in some environments, e: ", e); + } + base.evaluate(); + } + }; + } + }; @Test public void testJMXFetchBasic() throws IOException, InterruptedException { String testDomain = "test-domain"; + String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", "", "instances:", " - name: jmxint_container", - " host: " + cont.getHost(), - " port: " + cont.getMappedPort(rmiPort), + " host: " + ipAddress, + " port: " + rmiPort, " min_collection_interval: null", // allow collections at arbitrary intervals since we trigger them manually in the tests " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh @@ -85,19 +108,20 @@ public void testJMXFetchBasic() throws IOException, InterruptedException { metrics = ((ConsoleReporter) this.appConfig.getReporter()).getMetrics(); assertEquals(numBeans * numAttributesPerBean, metrics.size()); } - + @Test public void testJMXFetchManyBeans() throws IOException, InterruptedException { cont.followOutput(logConsumer); String testDomain = "test-domain"; + String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", "", "instances:", " - name: jmxint_container", - " host: " + cont.getHost(), - " port: " + cont.getMappedPort(rmiPort), + " host: " + ipAddress, + " port: " + rmiPort, " min_collection_interval: null", " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh @@ -134,14 +158,15 @@ public void testJMXFetchManyBeans() throws IOException, InterruptedException { public void testConcurrentCollectionWithSubscriptionUpdates() throws IOException, InterruptedException { String testDomain = "test-domain"; cont.followOutput(logConsumer); + String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", "", "instances:", " - name: jmxint_container", - " host: " + cont.getHost(), - " port: " + cont.getMappedPort(rmiPort), + " host: " + ipAddress, + " port: " + rmiPort, " min_collection_interval: null", " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh @@ -196,14 +221,15 @@ public void testConcurrentCollectionWithSubscriptionUpdates() throws IOException public void testBeanRemoval() throws IOException, InterruptedException { String testDomain = "test-domain"; cont.followOutput(logConsumer); + String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", "", "instances:", " - name: jmxint_container", - " host: " + cont.getHost(), - " port: " + cont.getMappedPort(rmiPort), + " host: " + ipAddress, + " port: " + rmiPort, " min_collection_interval: null", " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh @@ -255,14 +281,15 @@ public void testBeanRemoval() throws IOException, InterruptedException { public void testNetworkFailure() throws IOException, InterruptedException { String testDomain = "test-domain-nwkfail"; cont.followOutput(logConsumer); + String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", "", "instances:", " - name: jmxint_container", - " host: " + cont.getHost(), - " port: " + cont.getMappedPort(rmiPort), + " host: " + ipAddress, + " port: " + rmiPort, " min_collection_interval: null", " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh @@ -310,14 +337,15 @@ public void testNetworkFailure() throws IOException, InterruptedException { public void testDisconnectDuringBeanCreation() throws IOException, InterruptedException { String testDomain = "test-domain-dsc-bn-creat"; cont.followOutput(logConsumer); + String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", "", "instances:", " - name: jmxint_container", - " host: " + cont.getHost(), - " port: " + cont.getMappedPort(rmiPort), + " host: " + ipAddress, + " port: " + rmiPort, " min_collection_interval: null", " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh diff --git a/src/test/java/org/datadog/jmxfetch/TestReconnectContainer.java b/src/test/java/org/datadog/jmxfetch/TestReconnectContainer.java index 10e886cb2..e761c5ca1 100644 --- a/src/test/java/org/datadog/jmxfetch/TestReconnectContainer.java +++ b/src/test/java/org/datadog/jmxfetch/TestReconnectContainer.java @@ -12,7 +12,6 @@ import javax.management.remote.JMXConnectorFactory; import javax.management.remote.JMXServiceURL; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TestRule; From 905532add1e20d91b2b6d56d2465d548dd6b0acb Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 27 Dec 2023 16:12:47 -0500 Subject: [PATCH 30/41] Re-land changes from #481 --- .../java/org/datadog/jmxfetch/Instance.java | 45 ++----------------- 1 file changed, 3 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index fec90d75d..2ca6a1d66 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -16,7 +16,6 @@ import java.io.InputStream; import java.lang.management.ManagementFactory; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -39,44 +38,6 @@ @Slf4j public class Instance implements BeanTracker { - private static final List SIMPLE_TYPES = - Arrays.asList( - "long", - "java.lang.String", - "int", - "float", - "double", - "java.lang.Double", - "java.lang.Float", - "java.lang.Integer", - "java.lang.Long", - "java.util.concurrent.atomic.AtomicInteger", - "java.util.concurrent.atomic.AtomicLong", - "java.lang.Object", - "java.lang.Boolean", - "boolean", - "java.lang.Number", - //Workaround for jasperserver, which returns attribute types as `class ` - "class java.lang.String", - "class java.lang.Double", - "class java.lang.Float", - "class java.lang.Integer", - "class java.lang.Long", - "class java.util.concurrent.atomic.AtomicInteger", - "class java.util.concurrent.atomic.AtomicLong", - "class java.lang.Object", - "class java.lang.Boolean", - "class java.lang.Number"); - private static final List COMPOSED_TYPES = - Arrays.asList( - "javax.management.openmbean.CompositeData", - "java.util.HashMap", - "java.util.Map"); - private static final List MULTI_TYPES = - Arrays.asList( - "javax.management.openmbean.TabularData", - //Adding TabularDataSupport as it implements TabularData - "javax.management.openmbean.TabularDataSupport"); 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"; @@ -647,7 +608,7 @@ private synchronized void addMatchingAttributesForBean( } JmxAttribute jmxAttribute; String attributeType = attributeInfo.getType(); - if (SIMPLE_TYPES.contains(attributeType)) { + if (JmxSimpleAttribute.matchAttributeType(attributeType)) { log.debug( ATTRIBUTE + beanName @@ -667,7 +628,7 @@ private synchronized void addMatchingAttributesForBean( cassandraAliasing, emptyDefaultHostname, normalizeBeanParamTags); - } else if (COMPOSED_TYPES.contains(attributeType)) { + } else if (JmxComplexAttribute.matchAttributeType(attributeType)) { log.debug( ATTRIBUTE + beanName @@ -686,7 +647,7 @@ private synchronized void addMatchingAttributesForBean( tags, emptyDefaultHostname, normalizeBeanParamTags); - } else if (MULTI_TYPES.contains(attributeType)) { + } else if (JmxTabularAttribute.matchAttributeType(attributeType)) { log.debug( ATTRIBUTE + beanName From bad14d0afc1a3dec7d35edae57b32d68c738f1bc Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 27 Dec 2023 16:15:25 -0500 Subject: [PATCH 31/41] Removes random idea for deployment validation --- src/main/java/org/datadog/jmxfetch/Instance.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 2ca6a1d66..b319e5938 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -22,7 +22,6 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; -import java.util.Random; import java.util.Set; import javax.management.InstanceNotFoundException; @@ -255,12 +254,6 @@ public Instance( && enableSubscriptionOverride.equalsIgnoreCase("true")) { this.enableBeanSubscription = true; } - String subscriptionCoinFlip = System.getenv("DD_JMXFETCH_SUBSCRIPTION_FLIPCOIN"); - if (subscriptionCoinFlip != null && subscriptionCoinFlip.equalsIgnoreCase("true")) { - Random rd = new Random(); - boolean enabled = rd.nextBoolean(); - this.enableBeanSubscription = enabled; - } log.info("JMXFetch Subscription mode enabled={}", this.enableBeanSubscription); this.beanSubscriptionActive = false; instanceTelemetryBean = createInstanceTelemetryBean(); From be3c9b467bf44220c24259da6f5e81da1f3ca9bb Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 27 Dec 2023 16:34:29 -0500 Subject: [PATCH 32/41] Relands bean match ratio from #487 --- .../java/org/datadog/jmxfetch/Instance.java | 27 ++++++++++++++----- .../jmxfetch/util/InstanceTelemetry.java | 9 +++++++ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index b319e5938..81a645e71 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -586,6 +586,7 @@ private synchronized void addMatchingAttributesForBean( ) throws IOException { Reporter reporter = appConfig.getReporter(); String action = appConfig.getAction(); + int matchedAttributesForBean = 0; for (MBeanAttributeInfo attributeInfo : info.getAttributes()) { if (this.metricCountForMatchingAttributes >= maxReturnedMetrics) { @@ -683,6 +684,7 @@ private synchronized void addMatchingAttributesForBean( if (jmxAttribute.match(conf)) { jmxAttribute.setMatchingConf(conf); this.metricCountForMatchingAttributes += jmxAttribute.getMetricsCount(); + matchedAttributesForBean++; log.debug("Added attribute {} from bean {}.", jmxAttribute, beanName); this.matchingAttributes.add(jmxAttribute); if (action.equals(AppConfig.ACTION_LIST_EVERYTHING) @@ -718,6 +720,8 @@ private synchronized void addMatchingAttributesForBean( reporter.displayNonMatchingAttributeName(jmxAttribute); } } + + return matchedAttributesForBean; } private void getMatchingAttributes() throws IOException { @@ -753,8 +757,16 @@ private void getMatchingAttributes() throws IOException { continue; } - addMatchingAttributesForBean(beanName, info, limitReached); + int numMatchedAttributes = addMatchingAttributesForBean(beanName, info, limitReached); + if (numMatchedAttributes > 0) { + beansWithAttributeMatch++; + } + } + + if (instanceTelemetryBean != null) { + instanceTelemetryBean.setBeanMatchRatio(beansWithAttributeMatch / beans.size()); } + log.info("Found {} matching attributes with {} metrics total", matchingAttributes.size(), this.metricCountForMatchingAttributes); @@ -765,11 +777,12 @@ public synchronized void trackBean(ObjectName beanName) { log.debug("Bean registered event. {}", beanName); String className; MBeanAttributeInfo[] attributeInfos; + int matchedAttributesForBean; try { log.debug("Getting bean info for bean: {}", beanName); MBeanInfo info = connection.getMBeanInfo(beanName); - this.addMatchingAttributesForBean(beanName, info, false); + matchedAttributesForBean = this.addMatchingAttributesForBean(beanName, info, false); this.beans.add(beanName); } catch (IOException e) { // Nothing to do, connection issue @@ -777,7 +790,7 @@ public synchronized void trackBean(ObjectName beanName) { } catch (Exception e) { log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); } - log.debug("Bean registration processed. {}", beanName); + log.debug("Bean registration processed. '{}'. Found {} matching attributes.", beanName, matchedAttributesForBean); } /** Removes any matching attributes from the specified bean. */ @@ -873,9 +886,9 @@ private synchronized void refreshBeansList(boolean isInitialQuery) throws IOExce beansNotSeen.addAll(newBeans); beansNotSeen.removeAll(this.beans); - log.error("Bean refresh found {} previously untracked beans", beansNotSeen.size()); + log.error("[beansub-audit] Bean refresh found {} previously untracked beans", beansNotSeen.size()); for (ObjectName b : beansNotSeen) { - log.error("New not-tracked bean {}", b); + log.error("[beansub-audit] New not-tracked bean {}", b); } } if (!newBeans.containsAll(this.beans)) { @@ -885,10 +898,10 @@ private synchronized void refreshBeansList(boolean isInitialQuery) throws IOExce incorrectlyTrackedBeans.addAll(this.beans); incorrectlyTrackedBeans.removeAll(newBeans); - log.error("Bean refresh found {} fewer beans than expected", + log.error("[beansub-audit] Bean refresh found {} fewer beans than expected", incorrectlyTrackedBeans.size()); for (ObjectName b : incorrectlyTrackedBeans) { - log.error("Currently tracked bean not returned from fresh query: {}", b); + log.error("[beansub-audit] Currently tracked bean not returned from fresh query: {}", b); } } } diff --git a/src/main/java/org/datadog/jmxfetch/util/InstanceTelemetry.java b/src/main/java/org/datadog/jmxfetch/util/InstanceTelemetry.java index 3cbb938fd..745885284 100644 --- a/src/main/java/org/datadog/jmxfetch/util/InstanceTelemetry.java +++ b/src/main/java/org/datadog/jmxfetch/util/InstanceTelemetry.java @@ -16,6 +16,15 @@ public InstanceTelemetry() { topLevelAttributeCount = 0; metricCount = 0; wildcardDomainQueryCount = 0; + // This needs to be re-thought a bit + // it makes sense in a bean-refresh-loop world + // but in a subscription-world + // it's not clear what this should be + // current thought is to split this + // into two fields: + // - numBeansWithMatchingAttributes + // - numBeansWithoutMatchingAttributes + // a bit wordy though. beanMatchRatio = 0.0; } From c118cc22db3c076d5a253a3587668040169df6ed Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 27 Dec 2023 16:37:35 -0500 Subject: [PATCH 33/41] Fixes whoopsie --- src/main/java/org/datadog/jmxfetch/Instance.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 81a645e71..e5f3327b5 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -579,7 +579,7 @@ public boolean timeToCollect() { } } - private synchronized void addMatchingAttributesForBean( + private synchronized int addMatchingAttributesForBean( ObjectName beanName, MBeanInfo info, boolean metricReachedPreviouslyDisplayed @@ -777,7 +777,7 @@ public synchronized void trackBean(ObjectName beanName) { log.debug("Bean registered event. {}", beanName); String className; MBeanAttributeInfo[] attributeInfos; - int matchedAttributesForBean; + int matchedAttributesForBean = 0; try { log.debug("Getting bean info for bean: {}", beanName); MBeanInfo info = connection.getMBeanInfo(beanName); From ea0bf5bc3e3b82c9f128c3f2493a6cd1db425dbe Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 27 Dec 2023 16:55:37 -0500 Subject: [PATCH 34/41] Fixes divide by zero bug introduced in previous commit --- src/main/java/org/datadog/jmxfetch/Instance.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index e5f3327b5..002afed57 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -710,9 +710,6 @@ private synchronized int addMatchingAttributesForBean( e); } } - // TODO if there was a matching conf, then we must return - // true so that the instanceTelemetryBeanRatio can be kept correctly - // ref: jmxfetch pr 487 if (jmxAttribute.getMatchingConf() == null && (action.equals(AppConfig.ACTION_LIST_EVERYTHING) @@ -763,7 +760,7 @@ private void getMatchingAttributes() throws IOException { } } - if (instanceTelemetryBean != null) { + if (instanceTelemetryBean != null && beans.size() > 0) { instanceTelemetryBean.setBeanMatchRatio(beansWithAttributeMatch / beans.size()); } From 82bbc9425097126771312cbabcb3357016fb3797 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 27 Dec 2023 16:56:06 -0500 Subject: [PATCH 35/41] Adds tlm for beans registered and unregistered --- .../java/org/datadog/jmxfetch/Instance.java | 10 ++++++++++ .../jmxfetch/util/InstanceTelemetry.java | 20 +++++++++++++++++++ .../org/datadog/jmxfetch/TestInstance.java | 13 +++++++++++- 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 002afed57..4b1f567ea 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -788,6 +788,11 @@ public synchronized void trackBean(ObjectName beanName) { log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); } log.debug("Bean registration processed. '{}'. Found {} matching attributes.", beanName, matchedAttributesForBean); + + if (instanceTelemetryBean != null) { + int beanRegisterationsHandled = instanceTelemetryBean.getBeanRegistrationsHandled(); + instanceTelemetryBean.setBeanRegistrationsHandled(beanRegisterationsHandled + 1); + } } /** Removes any matching attributes from the specified bean. */ @@ -811,6 +816,11 @@ public synchronized void untrackBean(ObjectName beanName) { removedMetrics, beanName); this.metricCountForMatchingAttributes -= removedMetrics; + + if (instanceTelemetryBean != null) { + int beanUnRegistrationsHandled = instanceTelemetryBean.getBeanUnregistrationsHandled(); + instanceTelemetryBean.setBeanUnregistrationsHandled(beanUnRegistrationsHandled + 1); + } } private void subscribeToBeans() { diff --git a/src/main/java/org/datadog/jmxfetch/util/InstanceTelemetry.java b/src/main/java/org/datadog/jmxfetch/util/InstanceTelemetry.java index 745885284..a943bc6d9 100644 --- a/src/main/java/org/datadog/jmxfetch/util/InstanceTelemetry.java +++ b/src/main/java/org/datadog/jmxfetch/util/InstanceTelemetry.java @@ -9,6 +9,8 @@ public class InstanceTelemetry implements InstanceTelemetryMBean { private int metricCount; private int wildcardDomainQueryCount; private double beanMatchRatio; + private int beanRegistrationsHandled; + private int beanUnregistrationsHandled; /** Jmxfetch telemetry bean constructor. */ public InstanceTelemetry() { @@ -26,6 +28,8 @@ public InstanceTelemetry() { // - numBeansWithoutMatchingAttributes // a bit wordy though. beanMatchRatio = 0.0; + beanRegistrationsHandled = 0; + beanUnregistrationsHandled = 0; } public int getBeansFetched() { @@ -48,6 +52,14 @@ public double getBeanMatchRatio() { return beanMatchRatio; } + public int getBeanRegistrationsHandled() { + return beanRegistrationsHandled; + } + + public int getBeanUnregistrationsHandled() { + return beanUnregistrationsHandled; + } + public void setBeansFetched(int count) { beansFetched = count; } @@ -68,4 +80,12 @@ public void setBeanMatchRatio(double ratio) { beanMatchRatio = ratio; } + public void setBeanRegistrationsHandled(int count) { + beanRegistrationsHandled = count; + } + + public void setBeanUnregistrationsHandled(int count) { + beanUnregistrationsHandled = count; + } + } diff --git a/src/test/java/org/datadog/jmxfetch/TestInstance.java b/src/test/java/org/datadog/jmxfetch/TestInstance.java index daaf64e43..0486e9f20 100644 --- a/src/test/java/org/datadog/jmxfetch/TestInstance.java +++ b/src/test/java/org/datadog/jmxfetch/TestInstance.java @@ -277,6 +277,12 @@ public void testBeanSubscription() throws Exception { metrics = getMetrics(); assertEquals(numRegisteredAttributes, metrics.size()); + + List instances = getInstances(); + assertEquals(1, instances.size()); + Instance i = instances.get(0); + assertEquals(2, i.getInstanceTelemetryBean().getBeanRegistrationsHandled()); + assertEquals(0, i.getInstanceTelemetryBean().getBeanUnregistrationsHandled()); } @Test @@ -319,8 +325,13 @@ public void testBeanUnsubscription() throws Exception { assertEquals(numRegisteredAttributes, metrics.size()); // Which is why this second check exists, it reflects the running total of metrics // collected which is used to determine if new attributes can be added - Instance i = app.getInstances().get(0); + List instances = getInstances(); + assertEquals(1, instances.size()); + Instance i = instances.get(0); assertEquals(numRegisteredAttributes, i.getCurrentNumberOfMetrics()); + + assertEquals(1, i.getInstanceTelemetryBean().getBeanRegistrationsHandled()); + assertEquals(1, i.getInstanceTelemetryBean().getBeanUnregistrationsHandled()); } @Test From b40c5d646b3ef9bcc46a68aac42a315f2d1d5556 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Thu, 28 Dec 2023 21:33:52 +0100 Subject: [PATCH 36/41] Update tools/misbehaving-jmx-server/README.md Co-authored-by: Kaylyn --- tools/misbehaving-jmx-server/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/misbehaving-jmx-server/README.md b/tools/misbehaving-jmx-server/README.md index e64a70b29..9c251fb1c 100644 --- a/tools/misbehaving-jmx-server/README.md +++ b/tools/misbehaving-jmx-server/README.md @@ -30,7 +30,7 @@ a secondary `init` payload that contains the correct RMI Hostname. It is designe To enable debug logging, set the system property `-Dorg.slf4j.simpleLogger.defaultLogLevel=debug`. This can be done in the Dockerfile `CMD` if running in a container -For extra debug logging around RMI systems, try any/all of the following: +For extra debug logging around RMI systems, try the following: - `-Djava.rmi.server.logCalls=true` - `-Dsun.rmi.server.exceptionTrace=true` - `-Dsun.rmi.transport.logLevel=verbose` From 34290a17bcbde76ef16d2ab9ae65a353714c7365 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Thu, 28 Dec 2023 16:53:53 -0500 Subject: [PATCH 37/41] Fixes linting errors --- .../java/org/datadog/jmxfetch/Instance.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index 4b1f567ea..a28eff418 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -8,7 +8,6 @@ import org.yaml.snakeyaml.Yaml; - import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -24,14 +23,14 @@ import java.util.Map.Entry; import java.util.Set; -import javax.management.InstanceNotFoundException; import javax.management.InstanceAlreadyExistsException; -import javax.management.NotCompliantMBeanException; -import javax.management.MBeanRegistrationException; +import javax.management.InstanceNotFoundException; import javax.management.MBeanAttributeInfo; -import javax.management.MBeanServer; import javax.management.MBeanInfo; +import javax.management.MBeanRegistrationException; +import javax.management.MBeanServer; import javax.management.MalformedObjectNameException; +import javax.management.NotCompliantMBeanException; import javax.management.ObjectName; import javax.security.auth.login.FailedLoginException; @@ -787,7 +786,8 @@ public synchronized void trackBean(ObjectName beanName) { } catch (Exception e) { log.warn("Cannot get bean attributes or class name: " + e.getMessage(), e); } - log.debug("Bean registration processed. '{}'. Found {} matching attributes.", beanName, matchedAttributesForBean); + log.debug("Bean registration processed. '{}'. Found {} matching attributes.", + beanName, matchedAttributesForBean); if (instanceTelemetryBean != null) { int beanRegisterationsHandled = instanceTelemetryBean.getBeanRegistrationsHandled(); @@ -893,7 +893,8 @@ private synchronized void refreshBeansList(boolean isInitialQuery) throws IOExce beansNotSeen.addAll(newBeans); beansNotSeen.removeAll(this.beans); - log.error("[beansub-audit] Bean refresh found {} previously untracked beans", beansNotSeen.size()); + log.error("[beansub-audit] Bean refresh found {} previously untracked beans", + beansNotSeen.size()); for (ObjectName b : beansNotSeen) { log.error("[beansub-audit] New not-tracked bean {}", b); } @@ -908,7 +909,8 @@ private synchronized void refreshBeansList(boolean isInitialQuery) throws IOExce log.error("[beansub-audit] Bean refresh found {} fewer beans than expected", incorrectlyTrackedBeans.size()); for (ObjectName b : incorrectlyTrackedBeans) { - log.error("[beansub-audit] Currently tracked bean not returned from fresh query: {}", b); + log.error("[beansub-audit] Currently tracked bean not returned from fresh" + + " query: {}", b); } } } From 01e760c884e9ebea5db4b1314b62cfdd88fa1a61 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Wed, 17 Jan 2024 16:32:08 -0500 Subject: [PATCH 38/41] Removes per-instance bean-subscription enablement in favor of application-level config --- src/main/java/org/datadog/jmxfetch/AppConfig.java | 11 +++++++++++ src/main/java/org/datadog/jmxfetch/Instance.java | 10 +--------- .../org/datadog/jmxfetch/TestBeanSubscription.java | 13 +++++++------ .../java/org/datadog/jmxfetch/TestInstance.java | 6 ++++++ src/test/resources/jmx_bean_subscription.yaml | 5 ++--- 5 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/datadog/jmxfetch/AppConfig.java b/src/main/java/org/datadog/jmxfetch/AppConfig.java index a01ff561d..7956961d4 100644 --- a/src/main/java/org/datadog/jmxfetch/AppConfig.java +++ b/src/main/java/org/datadog/jmxfetch/AppConfig.java @@ -230,6 +230,13 @@ public class AppConfig { required = false) private String statusLocation; + @Parameter( + names = {"--enable_bean_subscription", "-B"}, + description = + "EXPERIMENTAL: If true, JMX beans will be discovered via subscription rather than poll-based. Obsoletes 'initialBeanRefreshPeriod' and 'beanRefreshPeriod'.", + required = false) + private boolean enableBeanSubscription; + @Parameter( names = {"--exit_file_location", "-e"}, description = @@ -518,4 +525,8 @@ public int getSocketTimeout() { public String getVersion() { return MetadataHelper.getVersion(); } + + public boolean getEnableBeanSubscription() { + return enableBeanSubscription; + } } diff --git a/src/main/java/org/datadog/jmxfetch/Instance.java b/src/main/java/org/datadog/jmxfetch/Instance.java index a28eff418..01ae63f4b 100644 --- a/src/main/java/org/datadog/jmxfetch/Instance.java +++ b/src/main/java/org/datadog/jmxfetch/Instance.java @@ -244,15 +244,7 @@ public Instance( } this.beans = new HashSet<>(); - Boolean enableBeanSubscription = (Boolean) instanceMap.get("enable_bean_subscription"); - this.enableBeanSubscription = enableBeanSubscription != null && enableBeanSubscription; - - // Global override for enabling bean subscription on all instances - String enableSubscriptionOverride = System.getenv("DD_JMXFETCH_SUBSCRIPTION_ENABLED"); - if (enableSubscriptionOverride != null - && enableSubscriptionOverride.equalsIgnoreCase("true")) { - this.enableBeanSubscription = true; - } + this.enableBeanSubscription = appConfig.getEnableBeanSubscription(); log.info("JMXFetch Subscription mode enabled={}", this.enableBeanSubscription); this.beanSubscriptionActive = false; instanceTelemetryBean = createInstanceTelemetryBean(); diff --git a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java index ccb94f7a2..d04b313a6 100644 --- a/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java +++ b/src/test/java/org/datadog/jmxfetch/TestBeanSubscription.java @@ -1,6 +1,7 @@ package org.datadog.jmxfetch; import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.when; import java.io.IOException; import java.nio.file.Paths; @@ -69,6 +70,7 @@ public void evaluate() throws Throwable { public void testJMXFetchBasic() throws IOException, InterruptedException { String testDomain = "test-domain"; String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); + when(appConfig.getEnableBeanSubscription()).thenReturn(true); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", @@ -78,7 +80,6 @@ public void testJMXFetchBasic() throws IOException, InterruptedException { " host: " + ipAddress, " port: " + rmiPort, " min_collection_interval: null", // allow collections at arbitrary intervals since we trigger them manually in the tests - " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh " collect_default_jvm_metrics: false", " max_returned_metrics: 300000", @@ -109,6 +110,7 @@ public void testJMXFetchManyBeans() throws IOException, InterruptedException { cont.followOutput(logConsumer); String testDomain = "test-domain"; String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); + when(appConfig.getEnableBeanSubscription()).thenReturn(true); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", @@ -118,7 +120,6 @@ public void testJMXFetchManyBeans() throws IOException, InterruptedException { " host: " + ipAddress, " port: " + rmiPort, " min_collection_interval: null", - " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh " collect_default_jvm_metrics: false", " max_returned_metrics: 300000", @@ -149,6 +150,7 @@ public void testConcurrentCollectionWithSubscriptionUpdates() throws IOException String testDomain = "test-domain"; cont.followOutput(logConsumer); String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); + when(appConfig.getEnableBeanSubscription()).thenReturn(true); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", @@ -158,7 +160,6 @@ public void testConcurrentCollectionWithSubscriptionUpdates() throws IOException " host: " + ipAddress, " port: " + rmiPort, " min_collection_interval: null", - " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh " collect_default_jvm_metrics: false", " max_returned_metrics: 300000", @@ -207,6 +208,7 @@ public void testBeanRemoval() throws IOException, InterruptedException { String testDomain = "test-domain"; cont.followOutput(logConsumer); String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); + when(appConfig.getEnableBeanSubscription()).thenReturn(true); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", @@ -216,7 +218,6 @@ public void testBeanRemoval() throws IOException, InterruptedException { " host: " + ipAddress, " port: " + rmiPort, " min_collection_interval: null", - " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh " collect_default_jvm_metrics: false", " max_returned_metrics: 300000", @@ -262,6 +263,7 @@ public void testNetworkFailure() throws IOException, InterruptedException { String testDomain = "test-domain-nwkfail"; cont.followOutput(logConsumer); String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); + when(appConfig.getEnableBeanSubscription()).thenReturn(true); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", @@ -271,7 +273,6 @@ public void testNetworkFailure() throws IOException, InterruptedException { " host: " + ipAddress, " port: " + rmiPort, " min_collection_interval: null", - " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh " collect_default_jvm_metrics: false", " max_returned_metrics: 300000", @@ -313,6 +314,7 @@ public void testDisconnectDuringBeanCreation() throws IOException, InterruptedEx String testDomain = "test-domain-dsc-bn-creat"; cont.followOutput(logConsumer); String ipAddress = cont.getContainerInfo().getNetworkSettings().getIpAddress(); + when(appConfig.getEnableBeanSubscription()).thenReturn(true); this.initApplicationWithYamlLines( "init_config:", " is_jmx: true", @@ -322,7 +324,6 @@ public void testDisconnectDuringBeanCreation() throws IOException, InterruptedEx " host: " + ipAddress, " port: " + rmiPort, " min_collection_interval: null", - " enable_bean_subscription: true", " refresh_beans: 5000", // effectively disable bean refresh " collect_default_jvm_metrics: false", " max_returned_metrics: 300000", diff --git a/src/test/java/org/datadog/jmxfetch/TestInstance.java b/src/test/java/org/datadog/jmxfetch/TestInstance.java index 0486e9f20..deaa0d33e 100644 --- a/src/test/java/org/datadog/jmxfetch/TestInstance.java +++ b/src/test/java/org/datadog/jmxfetch/TestInstance.java @@ -246,6 +246,8 @@ public void testRefreshBeans() throws Exception { @Test public void testBeanSubscription() throws Exception { SimpleTestJavaApp testApp = new SimpleTestJavaApp(); + AppConfig config = mock(AppConfig.class); + when(config.getEnableBeanSubscription()).thenReturn(true); // initial fetch initApplication("jmx_bean_subscription.yaml"); @@ -288,6 +290,8 @@ public void testBeanSubscription() throws Exception { @Test public void testBeanUnsubscription() throws Exception { SimpleTestJavaApp testApp = new SimpleTestJavaApp(); + AppConfig config = mock(AppConfig.class); + when(config.getEnableBeanSubscription()).thenReturn(true); // Bean-refresh interval is to to 50s, so the only bean refresh will be // initial fetch initApplication("jmx_bean_subscription.yaml"); @@ -340,6 +344,8 @@ public void testBeanSubscriptionAttributeCounting() throws Exception { // should be updated on bean registration/deregistration SimpleTestJavaApp testApp = new SimpleTestJavaApp(); + AppConfig config = mock(AppConfig.class); + when(config.getEnableBeanSubscription()).thenReturn(true); initApplication("jmx_bean_subscription.yaml"); // max returned metrics is 10 int numRegisteredAttributes = 0; diff --git a/src/test/resources/jmx_bean_subscription.yaml b/src/test/resources/jmx_bean_subscription.yaml index c7814acce..01a75f4f4 100644 --- a/src/test/resources/jmx_bean_subscription.yaml +++ b/src/test/resources/jmx_bean_subscription.yaml @@ -3,7 +3,6 @@ init_config: instances: - process_name_regex: .*surefire.* min_collection_interval: null - enable_bean_subscription: true collect_default_jvm_metrics: false refresh_beans: 50000 max_returned_metrics: 10 @@ -11,6 +10,6 @@ instances: conf: - include: domain: org.datadog.jmxfetch.test - attribute: + attribute: - ShouldBe100 - - ShouldBe1000 \ No newline at end of file + - ShouldBe1000 From 95820e91d6d6d584813453d28c102ed1b76f68b1 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Thu, 25 Jan 2024 21:21:12 +0000 Subject: [PATCH 39/41] Fixes linter warning --- src/main/java/org/datadog/jmxfetch/AppConfig.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/datadog/jmxfetch/AppConfig.java b/src/main/java/org/datadog/jmxfetch/AppConfig.java index 7956961d4..0260fcec0 100644 --- a/src/main/java/org/datadog/jmxfetch/AppConfig.java +++ b/src/main/java/org/datadog/jmxfetch/AppConfig.java @@ -233,7 +233,9 @@ public class AppConfig { @Parameter( names = {"--enable_bean_subscription", "-B"}, description = - "EXPERIMENTAL: If true, JMX beans will be discovered via subscription rather than poll-based. Obsoletes 'initialBeanRefreshPeriod' and 'beanRefreshPeriod'.", + "EXPERIMENTAL: If true, JMX beans will be discovered via subscription rather" + + " than poll-based. Obsoletes 'initialBeanRefreshPeriod' and" + + " 'beanRefreshPeriod'.", required = false) private boolean enableBeanSubscription; From f9167457e3acd5f718ae3ef49cf8160451d6e906 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Thu, 25 Jan 2024 22:28:12 +0000 Subject: [PATCH 40/41] Fixes spy usage in bean sub tests --- .../java/org/datadog/jmxfetch/TestCommon.java | 18 ++++++++++++++++-- .../org/datadog/jmxfetch/TestInstance.java | 14 ++++++++------ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/test/java/org/datadog/jmxfetch/TestCommon.java b/src/test/java/org/datadog/jmxfetch/TestCommon.java index bcdcb4c52..14b61f8d5 100644 --- a/src/test/java/org/datadog/jmxfetch/TestCommon.java +++ b/src/test/java/org/datadog/jmxfetch/TestCommon.java @@ -146,8 +146,14 @@ public void teardown() { } /** Init JMXFetch with the given YAML configuration file. */ - protected void initApplication(String yamlFileName, String autoDiscoveryPipeFile) + protected void initApplication(String yamlFileName, String autoDiscoveryPipeFile, AppConfig passedAppConfig) throws FileNotFoundException, IOException { + AppConfig appConfig = this.appConfig; + if (passedAppConfig != null) { + appConfig = passedAppConfig; + } + this.appConfig = appConfig; + // We do a first collection // We initialize the main app that will collect these metrics using JMX String confdDirectory = @@ -199,7 +205,15 @@ protected void initApplication(String yamlFileName, String autoDiscoveryPipeFile } protected void initApplication(String yamlFileName) throws FileNotFoundException, IOException { - initApplication(yamlFileName, ""); + initApplication(yamlFileName, "", null); + } + + protected void initApplication(String yamlFileName, AppConfig appConfig) throws FileNotFoundException, IOException { + initApplication(yamlFileName, "", appConfig); + } + + protected void initApplication(String yamlFileName, String autoDiscoveryPipeFile) throws FileNotFoundException, IOException { + initApplication(yamlFileName, autoDiscoveryPipeFile, null); } /* diff --git a/src/test/java/org/datadog/jmxfetch/TestInstance.java b/src/test/java/org/datadog/jmxfetch/TestInstance.java index deaa0d33e..9b00601bb 100644 --- a/src/test/java/org/datadog/jmxfetch/TestInstance.java +++ b/src/test/java/org/datadog/jmxfetch/TestInstance.java @@ -6,6 +6,7 @@ import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.spy; import java.net.URL; import java.util.ArrayList; @@ -246,10 +247,11 @@ public void testRefreshBeans() throws Exception { @Test public void testBeanSubscription() throws Exception { SimpleTestJavaApp testApp = new SimpleTestJavaApp(); - AppConfig config = mock(AppConfig.class); + AppConfig config = spy(AppConfig.builder().build()); when(config.getEnableBeanSubscription()).thenReturn(true); + // initial fetch - initApplication("jmx_bean_subscription.yaml"); + initApplication("jmx_bean_subscription.yaml", config); // We do a first collection run(); @@ -290,11 +292,11 @@ public void testBeanSubscription() throws Exception { @Test public void testBeanUnsubscription() throws Exception { SimpleTestJavaApp testApp = new SimpleTestJavaApp(); - AppConfig config = mock(AppConfig.class); + AppConfig config = spy(AppConfig.builder().build()); when(config.getEnableBeanSubscription()).thenReturn(true); // Bean-refresh interval is to to 50s, so the only bean refresh will be // initial fetch - initApplication("jmx_bean_subscription.yaml"); + initApplication("jmx_bean_subscription.yaml", config); int numRegisteredAttributes = 0; // We do a first collection @@ -344,9 +346,9 @@ public void testBeanSubscriptionAttributeCounting() throws Exception { // should be updated on bean registration/deregistration SimpleTestJavaApp testApp = new SimpleTestJavaApp(); - AppConfig config = mock(AppConfig.class); + AppConfig config = spy(AppConfig.builder().build()); when(config.getEnableBeanSubscription()).thenReturn(true); - initApplication("jmx_bean_subscription.yaml"); // max returned metrics is 10 + initApplication("jmx_bean_subscription.yaml", config); // max returned metrics is 10 int numRegisteredAttributes = 0; assertEquals(numRegisteredAttributes, app.getInstances().get(0).getCurrentNumberOfMetrics()); From ff5e64f9f0f4885594eb968f882f32a45944af97 Mon Sep 17 00:00:00 2001 From: Scott Opell Date: Thu, 8 Feb 2024 20:03:39 +0000 Subject: [PATCH 41/41] Adds env var enablement for backwards compatibility during rollout --- src/main/java/org/datadog/jmxfetch/AppConfig.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/datadog/jmxfetch/AppConfig.java b/src/main/java/org/datadog/jmxfetch/AppConfig.java index 0260fcec0..42b5e9cbb 100644 --- a/src/main/java/org/datadog/jmxfetch/AppConfig.java +++ b/src/main/java/org/datadog/jmxfetch/AppConfig.java @@ -529,6 +529,10 @@ public String getVersion() { } public boolean getEnableBeanSubscription() { - return enableBeanSubscription; + // As noted in `pkg/jmxfetch/jmxfetch.go` in the agent, using an env var + // for enablement is a temporary measure until the stable JMXFetch is upgraded + // to a version supporting this CLI arg. + boolean isEnvEnabled = System.getenv("DD_JMX_BEAN_SUBSCRIPTION_ENABLED") != null; + return isEnvEnabled || enableBeanSubscription; } }