From 978789437339647d3ce3d02def880408573683b2 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Thu, 16 Jul 2020 16:02:19 +0100 Subject: [PATCH 01/19] Allow caching regular expression matching in rules This improves performance for some use cases. The exporter is configured with a set of rules to match regular expressions against bean names. Regular expressions can be CPU-intensive, and matching the same bean names over the same patterns is time-consuming when there are tens of thousands of bean names (e.g: exposed by Kafka). Using a single `ConcurrentHashMap` to store the result of pattern matching here. The `MatchedRule` class was introduced here to "store" this result. Each rule in the configuration leads to a `MatchedRule` which helps build the set of metrics to expose to Prometheus. The mapping between a bean name + attributes to a `MatchedRule` is done once. Some (in Kafka, most) bean names may not match any rule, so caching of `MatchedRule.unmatched()` helps prevent re-computation of bean names to non-existent rules. The logic to map a bean name to a Prometheus MetricFamilySample is unchanged from the upstream, the `MatchedRule` was introduced only to be able to cache the results. Caching is disabled by default (behaviour unchanged), and can be enabled by setting `cacheRules: true` in the configuration. Signed-off-by: Flavien Raynaud --- README.md | 14 +- .../java/io/prometheus/jmx/JmxCollector.java | 204 +++++++++++------- .../java/io/prometheus/jmx/MatchedRule.java | 62 ++++++ .../io/prometheus/jmx/JmxCollectorTest.java | 31 ++- 4 files changed, 222 insertions(+), 89 deletions(-) create mode 100644 collector/src/main/java/io/prometheus/jmx/MatchedRule.java diff --git a/README.md b/README.md index c634ccbe..936fa4f7 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,7 @@ lowercaseOutputName: false lowercaseOutputLabelNames: false whitelistObjectNames: ["org.apache.cassandra.metrics:*"] blacklistObjectNames: ["org.apache.cassandra.metrics:type=ColumnFamily,*"] +cacheRules: false rules: - pattern: 'org.apache.cassandra.metrics<>Value: (\d+)' name: cassandra_$1_$2 @@ -57,16 +58,17 @@ rules: Name | Description ---------|------------ startDelaySeconds | start delay before serving requests. Any requests within the delay period will result in an empty metrics set. -hostPort | The host and port to connect to via remote JMX. If neither this nor jmxUrl is specified, will talk to the local JVM. -username | The username to be used in remote JMX password authentication. -password | The password to be used in remote JMX password authentication. -jmxUrl | A full JMX URL to connect to. Should not be specified if hostPort is. -ssl | Whether JMX connection should be done over SSL. To configure certificates you have to set following system properties:
`-Djavax.net.ssl.keyStore=/home/user/.keystore`
`-Djavax.net.ssl.keyStorePassword=changeit`
`-Djavax.net.ssl.trustStore=/home/user/.truststore`
`-Djavax.net.ssl.trustStorePassword=changeit` +hostPort | The host and port to connect to via remote JMX. If neither this nor jmxUrl is specified, will talk to the local JVM. +username | The username to be used in remote JMX password authentication. +password | The password to be used in remote JMX password authentication. +jmxUrl | A full JMX URL to connect to. Should not be specified if hostPort is. +ssl | Whether JMX connection should be done over SSL. To configure certificates you have to set following system properties:
`-Djavax.net.ssl.keyStore=/home/user/.keystore`
`-Djavax.net.ssl.keyStorePassword=changeit`
`-Djavax.net.ssl.trustStore=/home/user/.truststore`
`-Djavax.net.ssl.trustStorePassword=changeit` lowercaseOutputName | Lowercase the output metric name. Applies to default format and `name`. Defaults to false. lowercaseOutputLabelNames | Lowercase the output metric label names. Applies to default format and `labels`. Defaults to false. whitelistObjectNames | A list of [ObjectNames](http://docs.oracle.com/javase/6/docs/api/javax/management/ObjectName.html) to query. Defaults to all mBeans. blacklistObjectNames | A list of [ObjectNames](http://docs.oracle.com/javase/6/docs/api/javax/management/ObjectName.html) to not query. Takes precedence over `whitelistObjectNames`. Defaults to none. -rules | A list of rules to apply in order, processing stops at the first matching rule. Attributes that aren't matched aren't collected. If not specified, defaults to collecting everything in the default format. +cacheRules | Whether to cach bean to rule computation. If true, bean values are ommitted from patterns when matching. +rules | A list of rules to apply in order, processing stops at the first matching rule. Attributes that aren't matched aren't collected. If not specified, defaults to collecting everything in the default format. pattern | Regex pattern to match against each bean attribute. The pattern is not anchored. Capture groups can be used in other options. Defaults to matching everything. attrNameSnakeCase | Converts the attribute name to snake case. This is seen in the names matched by the pattern and the default format. For example, anAttrName to an\_attr\_name. Defaults to false. name | The metric name to set. Capture groups from the `pattern` can be used. If not specified, the default format will be used. If it evaluates to empty, processing of this attribute stops with no output. diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 6cb9b1a6..d9b43ab7 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -4,8 +4,6 @@ import io.prometheus.client.Counter; import org.yaml.snakeyaml.Yaml; -import javax.management.MalformedObjectNameException; -import javax.management.ObjectName; import java.io.File; import java.io.FileReader; import java.io.IOException; @@ -21,9 +19,13 @@ import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.management.MalformedObjectNameException; +import javax.management.ObjectName; import static java.lang.String.format; @@ -60,6 +62,7 @@ private static class Config { boolean lowercaseOutputLabelNames; List whitelistObjectNames = new ArrayList(); List blacklistObjectNames = new ArrayList(); + boolean cacheRules = false; List rules = new ArrayList(); long lastUpdate = 0L; } @@ -69,6 +72,7 @@ private static class Config { private long createTimeNanoSecs = System.nanoTime(); private final JmxMBeanPropertyCache jmxMBeanPropertyCache = new JmxMBeanPropertyCache(); + private final ConcurrentMap cachedRules = new ConcurrentHashMap(); public JmxCollector(File in) throws IOException, MalformedObjectNameException { configFile = in; @@ -165,7 +169,11 @@ private Config loadConfig(Map yamlConfig) throws MalformedObject } } - if (yamlConfig.containsKey("rules")) { + if (yamlConfig.containsKey("cacheRules")) { + cfg.cacheRules = (Boolean)yamlConfig.get("cacheRules"); + } + + if (yamlConfig.containsKey("rules")) { List> configRules = (List>) yamlConfig.get("rules"); for (Map ruleObject : configRules) { Map yamlRule = ruleObject; @@ -287,9 +295,13 @@ class Receiver implements JmxScraper.MBeanReceiver { Map metricFamilySamplesMap = new HashMap(); - private static final char SEP = '_'; + ConcurrentMap cachedRules; + private static final char SEP = '_'; + Receiver(ConcurrentMap cachedRules) { + this.cachedRules = cachedRules; + } // [] and () are special in regexes, so swtich to <>. private String angleBrackets(String s) { @@ -307,13 +319,14 @@ void addSample(MetricFamilySamples.Sample sample, Type type, String help) { mfs.samples.add(sample); } - private void defaultExport( + private MatchedRule defaultExport( String domain, LinkedHashMap beanProperties, LinkedList attrKeys, String attrName, String help, - Object value, + Double value, + double valueFactor, Type type) { StringBuilder name = new StringBuilder(); name.append(domain); @@ -350,8 +363,7 @@ private void defaultExport( } } - addSample(new MetricFamilySamples.Sample(fullname, labelNames, labelValues, ((Number)value).doubleValue()), - type, help); + return new MatchedRule(fullname, type, help, labelNames, labelValues, value, valueFactor); } public void recordBean( @@ -368,99 +380,134 @@ public void recordBean( String help = attrDescription + " (" + beanName + attrName + ")"; String attrNameSnakeCase = toSnakeAndLowerCase(attrName); - for (Rule rule : config.rules) { - Matcher matcher = null; - String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName); - if (rule.pattern != null) { - matcher = rule.pattern.matcher(matchName + ": " + beanValue); - if (!matcher.matches()) { - continue; + MatchedRule matchedRule = null; + String cacheName = beanName + attrName; + if (config.cacheRules) { + matchedRule = cachedRules.get(cacheName); + } + + if (matchedRule == null) { + for (Rule rule : config.rules) { + Matcher matcher = null; + String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName); + + // Using bean value in caching is not possible (caching is only done on bean name, values can change) + if (!config.cacheRules) { + matchName = matchName + ": " + beanValue; } - } - Number value; - if (rule.value != null && !rule.value.isEmpty()) { - String val = matcher.replaceAll(rule.value); + if (rule.pattern != null) { + matcher = rule.pattern.matcher(matchName); + if (!matcher.matches()) { + continue; + } + } - try { - beanValue = Double.valueOf(val); - } catch (NumberFormatException e) { - LOGGER.fine("Unable to parse configured value '" + val + "' to number for bean: " + beanName + attrName + ": " + beanValue); - return; + Double value = null; + if (rule.value != null && !rule.value.isEmpty()) { + String val = matcher.replaceAll(rule.value); + try { + value = Double.valueOf(val); + } catch (NumberFormatException e) { + LOGGER.fine("Unable to parse configured value '" + val + "' to number for bean: " + beanName + attrName + ": " + beanValue); + return; + } } - } - if (beanValue instanceof Number) { - value = ((Number)beanValue).doubleValue() * rule.valueFactor; - } else if (beanValue instanceof Boolean) { - value = (Boolean)beanValue ? 1 : 0; - } else { - LOGGER.fine("Ignoring unsupported bean: " + beanName + attrName + ": " + beanValue); - return; - } - // If there's no name provided, use default export format. - if (rule.name == null) { - defaultExport(domain, beanProperties, attrKeys, rule.attrNameSnakeCase ? attrNameSnakeCase : attrName, help, value, rule.type); - return; - } + // If there's no name provided, use default export format. + if (rule.name == null) { + matchedRule = defaultExport(domain, beanProperties, attrKeys, rule.attrNameSnakeCase ? attrNameSnakeCase : attrName, help, value, rule.valueFactor, rule.type); + break; + } - // Matcher is set below here due to validation in the constructor. - String name = safeName(matcher.replaceAll(rule.name)); - if (name.isEmpty()) { - return; - } - if (config.lowercaseOutputName) { - name = name.toLowerCase(); - } + // Matcher is set below here due to validation in the constructor. + String name = safeName(matcher.replaceAll(rule.name)); + if (name.isEmpty()) { + return; + } + if (config.lowercaseOutputName) { + name = name.toLowerCase(); + } - // Set the help. - if (rule.help != null) { - help = matcher.replaceAll(rule.help); - } + // Set the help. + if (rule.help != null) { + help = matcher.replaceAll(rule.help); + } - // Set the labels. - ArrayList labelNames = new ArrayList(); - ArrayList labelValues = new ArrayList(); - if (rule.labelNames != null) { - for (int i = 0; i < rule.labelNames.size(); i++) { - final String unsafeLabelName = rule.labelNames.get(i); - final String labelValReplacement = rule.labelValues.get(i); - try { - String labelName = safeName(matcher.replaceAll(unsafeLabelName)); - String labelValue = matcher.replaceAll(labelValReplacement); - if (config.lowercaseOutputLabelNames) { - labelName = labelName.toLowerCase(); - } - if (!labelName.isEmpty() && !labelValue.isEmpty()) { - labelNames.add(labelName); - labelValues.add(labelValue); + // Set the labels. + ArrayList labelNames = new ArrayList(); + ArrayList labelValues = new ArrayList(); + if (rule.labelNames != null) { + for (int i = 0; i < rule.labelNames.size(); i++) { + final String unsafeLabelName = rule.labelNames.get(i); + final String labelValReplacement = rule.labelValues.get(i); + try { + String labelName = safeName(matcher.replaceAll(unsafeLabelName)); + String labelValue = matcher.replaceAll(labelValReplacement); + if (config.lowercaseOutputLabelNames) { + labelName = labelName.toLowerCase(); + } + if (!labelName.isEmpty() && !labelValue.isEmpty()) { + labelNames.add(labelName); + labelValues.add(labelValue); + } + } catch (Exception e) { + throw new RuntimeException( + format("Matcher '%s' unable to use: '%s' value: '%s'", matcher, unsafeLabelName, labelValReplacement), e); } - } catch (Exception e) { - throw new RuntimeException( - format("Matcher '%s' unable to use: '%s' value: '%s'", matcher, unsafeLabelName, labelValReplacement), e); } } + + matchedRule = new MatchedRule(name, rule.type, help, labelNames, labelValues, value, rule.valueFactor); + break; + } + + // No rule matches the mbean, cache it as an invalid rule to get cache hits in the future + if (matchedRule == null) { + matchedRule = MatchedRule.unmatched(); } - // Add to samples. - LOGGER.fine("add metric sample: " + name + " " + labelNames + " " + labelValues + " " + value.doubleValue()); - addSample(new MetricFamilySamples.Sample(name, labelNames, labelValues, value.doubleValue()), rule.type, help); + if (config.cacheRules) { + cachedRules.put(cacheName, matchedRule); + } + } + + if (matchedRule.isUnmatched()) { return; } + + Number value; + if (matchedRule.value != null) { + beanValue = matchedRule.value; + } + + if (beanValue instanceof Number) { + value = ((Number) beanValue).doubleValue() * matchedRule.valueFactor; + } else if (beanValue instanceof Boolean) { + value = (Boolean) beanValue ? 1 : 0; + } else { + LOGGER.fine("Ignoring unsupported bean: " + beanName + attrName + ": " + beanValue); + return; + } + + // Add to samples. + LOGGER.fine("add metric sample: " + matchedRule.name + " " + matchedRule.labelNames + " " + matchedRule.labelValues + " " + value.doubleValue()); + addSample(new MetricFamilySamples.Sample(matchedRule.name, matchedRule.labelNames, matchedRule.labelValues, value.doubleValue()), matchedRule.type, help); } } - public List collect() { - if (configFile != null) { + public List collect() { + if (configFile != null) { long mtime = configFile.lastModified(); if (mtime > config.lastUpdate) { LOGGER.fine("Configuration file changed, reloading..."); reloadConfig(); + cachedRules.clear(); // rules may have changed with the configuration, clear the rule cache } } - Receiver receiver = new Receiver(); + Receiver receiver = new Receiver(cachedRules); JmxScraper scraper = new JmxScraper(config.jmxUrl, config.username, config.password, config.ssl, config.whitelistObjectNames, config.blacklistObjectNames, receiver, jmxMBeanPropertyCache); long start = System.nanoTime(); @@ -488,6 +535,12 @@ public List collect() { samples.add(new MetricFamilySamples.Sample( "jmx_scrape_error", new ArrayList(), new ArrayList(), error)); mfsList.add(new MetricFamilySamples("jmx_scrape_error", Type.GAUGE, "Non-zero if this scrape failed.", samples)); + if (config.cacheRules) { + samples = new ArrayList(); + samples.add(new MetricFamilySamples.Sample( + "jmx_scrape_cached_beans", new ArrayList(), new ArrayList(), cachedRules.size())); + mfsList.add(new MetricFamilySamples("jmx_scrape_cached_beans", Type.GAUGE, "Number of beans with their matching rule cached", samples)); + } return mfsList; } @@ -495,6 +548,7 @@ public List describe() { List sampleFamilies = new ArrayList(); sampleFamilies.add(new MetricFamilySamples("jmx_scrape_duration_seconds", Type.GAUGE, "Time this JMX scrape took, in seconds.", new ArrayList())); sampleFamilies.add(new MetricFamilySamples("jmx_scrape_error", Type.GAUGE, "Non-zero if this scrape failed.", new ArrayList())); + sampleFamilies.add(new MetricFamilySamples("jmx_scrape_cached_beans", Type.GAUGE, "Number of beans with their matching rule cached", new ArrayList())); return sampleFamilies; } diff --git a/collector/src/main/java/io/prometheus/jmx/MatchedRule.java b/collector/src/main/java/io/prometheus/jmx/MatchedRule.java new file mode 100644 index 00000000..c9288a28 --- /dev/null +++ b/collector/src/main/java/io/prometheus/jmx/MatchedRule.java @@ -0,0 +1,62 @@ +package io.prometheus.jmx; + +import io.prometheus.client.Collector.Type; + +import java.util.List; + +/** + * MatchedRule is the result of matching a JMX bean against the rules present in the configuration file. + * As rules are matched using regular expressions, caching helps prevent having to match the same beans to the same list + * of regular expressions. + */ +public class MatchedRule { + final String name; + final Type type; + final String help; + final List labelNames; + final List labelValues; + final Double value; + final double valueFactor; + + private static final MatchedRule _unmatched = new MatchedRule(); + + private MatchedRule() { + this.name = null; + this.type = null; + this.help = null; + this.labelNames = null; + this.labelValues = null; + this.value = null; + this.valueFactor = 1.0; + } + + public MatchedRule( + final String name, + final Type type, + final String help, + final List labelNames, + final List labelValues, + final Double value, + double valueFactor) { + this.name = name; + this.type = type; + this.help = help; + this.labelNames = labelNames; + this.labelValues = labelValues; + this.value = value; + this.valueFactor = valueFactor; + } + + /** + * A unmatched MatchedRule, used when no rule matching a JMX bean has been found in the configuration. + * Cached unmatched rules are still a cache hit, that will not produce any metric/value. + * @return the invalid rule + */ + public static MatchedRule unmatched() { + return _unmatched; + } + + public boolean isUnmatched() { + return this == _unmatched; + } +} diff --git a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java index 05334e9a..7d29a5a4 100644 --- a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java +++ b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java @@ -1,18 +1,19 @@ package io.prometheus.jmx; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.fail; - import io.prometheus.client.Collector; import io.prometheus.client.CollectorRegistry; -import java.lang.management.ManagementFactory; -import javax.management.MBeanServer; -import org.junit.Test; import org.junit.Before; import org.junit.BeforeClass; +import org.junit.Test; + +import java.lang.management.ManagementFactory; +import javax.management.MBeanServer; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; public class JmxCollectorTest { @@ -269,4 +270,18 @@ public void testCamelLastExchangFailureTimestamp() throws Exception{ Double actual = registry.getSampleValue("org_apache_camel_LastExchangeFailureTimestamp", new String[]{"context", "route", "type"}, new String[]{"my-camel-context", "my-route-name", "routes"}); assertEquals(Camel.EXPECTED_SECONDS, actual, 0); } + + @Test + public void testCachedBeansDisabled() throws Exception { + JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4".replace('`','"')).register(registry); + assertNull(registry.getSampleValue("jmx_scrape_cached_beans", new String[]{}, new String[]{})); + assertEquals(4.0, registry.getSampleValue("foo", new String[]{}, new String[]{}), .001); + } + + @Test + public void testCachedBeansEnabled() throws Exception { + JmxCollector jc = new JmxCollector("\n---\ncacheRules: true\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4".replace('`','"')).register(registry); + assertTrue(registry.getSampleValue("jmx_scrape_cached_beans", new String[]{}, new String[]{}) > 0); + assertEquals(4.0, registry.getSampleValue("foo", new String[]{}, new String[]{}), .001); + } } From 6129ff8b3356581b70332b0b3e58083f82094c43 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Wed, 12 Aug 2020 18:52:09 +0100 Subject: [PATCH 02/19] Remove stale cached rules Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index d9b43ab7..59be70f8 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -12,6 +12,7 @@ import java.io.StringWriter; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedList; @@ -296,11 +297,13 @@ class Receiver implements JmxScraper.MBeanReceiver { new HashMap(); ConcurrentMap cachedRules; + Set lastCachedRules; private static final char SEP = '_'; - Receiver(ConcurrentMap cachedRules) { + Receiver(ConcurrentMap cachedRules, Set lastCachedRules) { this.cachedRules = cachedRules; + this.lastCachedRules = lastCachedRules; } // [] and () are special in regexes, so swtich to <>. @@ -384,6 +387,7 @@ public void recordBean( String cacheName = beanName + attrName; if (config.cacheRules) { matchedRule = cachedRules.get(cacheName); + lastCachedRules.add(cacheName); } if (matchedRule == null) { @@ -497,6 +501,15 @@ public void recordBean( } + // Remove stale rules (in the cache but not collected in the last run of the collector) + private void evictStaleCachedRules(Set lastCachedRules) { + for (String cacheName : cachedRules.keySet()) { + if (!lastCachedRules.contains(cacheName)) { + cachedRules.remove(cacheName); + } + } + } + public List collect() { if (configFile != null) { long mtime = configFile.lastModified(); @@ -507,7 +520,8 @@ public List collect() { } } - Receiver receiver = new Receiver(cachedRules); + Set lastCachedRules = new HashSet(); + Receiver receiver = new Receiver(cachedRules, lastCachedRules); JmxScraper scraper = new JmxScraper(config.jmxUrl, config.username, config.password, config.ssl, config.whitelistObjectNames, config.blacklistObjectNames, receiver, jmxMBeanPropertyCache); long start = System.nanoTime(); @@ -524,6 +538,8 @@ public List collect() { e.printStackTrace(new PrintWriter(sw)); LOGGER.severe("JMX scrape failed: " + sw.toString()); } + evictStaleCachedRules(lastCachedRules); + List mfsList = new ArrayList(); mfsList.addAll(receiver.metricFamilySamplesMap.values()); List samples = new ArrayList(); From a05c606fa19344e5ffd9eb8294bbd62a0f9de42f Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Thu, 13 Aug 2020 16:41:42 +0100 Subject: [PATCH 03/19] Cache regular expression matching per rule Instead of having a global `cacheRules` flag, have a per-rule `cache` flag (default: false). Rules that do not have the `cache` flag set will have the default behaviour. In order to allow caching of rules with values that change over time (e.g: gauges), a new per-rule `matchBeanValue` flag has also been added (default: true). Disabling the flag will not add the bean value to the expression when matching against the list of rules (slightly different behaviour than before, but toggleable per rule) and allow caching of just the bean name (no matter the value). Signed-off-by: Flavien Raynaud --- README.md | 20 +- .../java/io/prometheus/jmx/JmxCollector.java | 211 +++++++++--------- .../java/io/prometheus/jmx/MatchedRule.java | 4 + .../io/prometheus/jmx/MatchedRulesCache.java | 93 ++++++++ .../io/prometheus/jmx/JmxCollectorTest.java | 18 +- 5 files changed, 231 insertions(+), 115 deletions(-) create mode 100644 collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java diff --git a/README.md b/README.md index 936fa4f7..d6b4014f 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,6 @@ lowercaseOutputName: false lowercaseOutputLabelNames: false whitelistObjectNames: ["org.apache.cassandra.metrics:*"] blacklistObjectNames: ["org.apache.cassandra.metrics:type=ColumnFamily,*"] -cacheRules: false rules: - pattern: 'org.apache.cassandra.metrics<>Value: (\d+)' name: cassandra_$1_$2 @@ -52,6 +51,8 @@ rules: valueFactor: 0.001 labels: {} help: "Cassandra metric $1 $2" + matchBeanValue: true + cache: false type: GAUGE attrNameSnakeCase: false ``` @@ -67,16 +68,17 @@ lowercaseOutputName | Lowercase the output metric name. Applies to default forma lowercaseOutputLabelNames | Lowercase the output metric label names. Applies to default format and `labels`. Defaults to false. whitelistObjectNames | A list of [ObjectNames](http://docs.oracle.com/javase/6/docs/api/javax/management/ObjectName.html) to query. Defaults to all mBeans. blacklistObjectNames | A list of [ObjectNames](http://docs.oracle.com/javase/6/docs/api/javax/management/ObjectName.html) to not query. Takes precedence over `whitelistObjectNames`. Defaults to none. -cacheRules | Whether to cach bean to rule computation. If true, bean values are ommitted from patterns when matching. rules | A list of rules to apply in order, processing stops at the first matching rule. Attributes that aren't matched aren't collected. If not specified, defaults to collecting everything in the default format. -pattern | Regex pattern to match against each bean attribute. The pattern is not anchored. Capture groups can be used in other options. Defaults to matching everything. +pattern | Regex pattern to match against each bean attribute. The pattern is not anchored. Capture groups can be used in other options. Defaults to matching everything. attrNameSnakeCase | Converts the attribute name to snake case. This is seen in the names matched by the pattern and the default format. For example, anAttrName to an\_attr\_name. Defaults to false. -name | The metric name to set. Capture groups from the `pattern` can be used. If not specified, the default format will be used. If it evaluates to empty, processing of this attribute stops with no output. -value | Value for the metric. Static values and capture groups from the `pattern` can be used. If not specified the scraped mBean value will be used. -valueFactor | Optional number that `value` (or the scraped mBean value if `value` is not specified) is multiplied by, mainly used to convert mBean values from milliseconds to seconds. -labels | A map of label name to label value pairs. Capture groups from `pattern` can be used in each. `name` must be set to use this. Empty names and values are ignored. If not specified and the default format is not being used, no labels are set. -help | Help text for the metric. Capture groups from `pattern` can be used. `name` must be set to use this. Defaults to the mBean attribute description and the full name of the attribute. -type | The type of the metric, can be `GAUGE`, `COUNTER` or `UNTYPED`. `name` must be set to use this. Defaults to `UNTYPED`. +name | The metric name to set. Capture groups from the `pattern` can be used. If not specified, the default format will be used. If it evaluates to empty, processing of this attribute stops with no output. +value | Value for the metric. Static values and capture groups from the `pattern` can be used. If not specified the scraped mBean value will be used. +valueFactor | Optional number that `value` (or the scraped mBean value if `value` is not specified) is multiplied by, mainly used to convert mBean values from milliseconds to seconds. +labels | A map of label name to label value pairs. Capture groups from `pattern` can be used in each. `name` must be set to use this. Empty names and values are ignored. If not specified and the default format is not being used, no labels are set. +help | Help text for the metric. Capture groups from `pattern` can be used. `name` must be set to use this. Defaults to the mBean attribute description and the full name of the attribute. +matchBeanValue | Whether to append the bean value in the expression to match patterns against for this rule. (e.g: "beanName: "). Defaults to `true`. +cache | Whether to cache bean name expressions to rule computation (match and mismatch). This can increase performance when collecting a lot of mbeans. Defaults to `false`. +type | The type of the metric, can be `GAUGE`, `COUNTER` or `UNTYPED`. `name` must be set to use this. Defaults to `UNTYPED`. Metric names and label names are sanitized. All characters other than `[a-zA-Z0-9:_]` are replaced with underscores, and adjacent underscores are collapsed. There's no limitations on label values or the help text. diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 59be70f8..02426ec2 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -20,8 +20,6 @@ import java.util.Map; import java.util.Set; import java.util.TreeMap; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -41,13 +39,15 @@ public class JmxCollector extends Collector implements Collector.Describable { private static final Logger LOGGER = Logger.getLogger(JmxCollector.class.getName()); - private static class Rule { + static class Rule { Pattern pattern; String name; String value; Double valueFactor = 1.0; String help; boolean attrNameSnakeCase; + boolean matchBeanValue = true; + boolean cache = false; Type type = Type.UNTYPED; ArrayList labelNames; ArrayList labelValues; @@ -63,7 +63,6 @@ private static class Config { boolean lowercaseOutputLabelNames; List whitelistObjectNames = new ArrayList(); List blacklistObjectNames = new ArrayList(); - boolean cacheRules = false; List rules = new ArrayList(); long lastUpdate = 0L; } @@ -73,7 +72,7 @@ private static class Config { private long createTimeNanoSecs = System.nanoTime(); private final JmxMBeanPropertyCache jmxMBeanPropertyCache = new JmxMBeanPropertyCache(); - private final ConcurrentMap cachedRules = new ConcurrentHashMap(); + private final MatchedRulesCache cachedRules = new MatchedRulesCache(); public JmxCollector(File in) throws IOException, MalformedObjectNameException { configFile = in; @@ -82,7 +81,7 @@ public JmxCollector(File in) throws IOException, MalformedObjectNameException { } public JmxCollector(String yamlConfig) throws MalformedObjectNameException { - config = loadConfig((Map)new Yaml().load(yamlConfig)); + config = loadConfig((Map)new Yaml().load(yamlConfig)); } public JmxCollector(InputStream inputStream) throws MalformedObjectNameException { @@ -170,10 +169,6 @@ private Config loadConfig(Map yamlConfig) throws MalformedObject } } - if (yamlConfig.containsKey("cacheRules")) { - cfg.cacheRules = (Boolean)yamlConfig.get("cacheRules"); - } - if (yamlConfig.containsKey("rules")) { List> configRules = (List>) yamlConfig.get("rules"); for (Map ruleObject : configRules) { @@ -200,6 +195,12 @@ private Config loadConfig(Map yamlConfig) throws MalformedObject if (yamlRule.containsKey("attrNameSnakeCase")) { rule.attrNameSnakeCase = (Boolean)yamlRule.get("attrNameSnakeCase"); } + if (yamlRule.containsKey("matchBeanValue")) { + rule.matchBeanValue = (Boolean)yamlRule.get("matchBeanValue"); + } + if (yamlRule.containsKey("cache")) { + rule.cache = (Boolean)yamlRule.get("cache"); + } if (yamlRule.containsKey("type")) { rule.type = Type.valueOf((String)yamlRule.get("type")); } @@ -296,12 +297,12 @@ class Receiver implements JmxScraper.MBeanReceiver { Map metricFamilySamplesMap = new HashMap(); - ConcurrentMap cachedRules; - Set lastCachedRules; + MatchedRulesCache cachedRules; + Set lastCachedRules; private static final char SEP = '_'; - Receiver(ConcurrentMap cachedRules, Set lastCachedRules) { + Receiver(MatchedRulesCache cachedRules, Set lastCachedRules) { this.cachedRules = cachedRules; this.lastCachedRules = lastCachedRules; } @@ -323,6 +324,7 @@ void addSample(MetricFamilySamples.Sample sample, Type type, String help) { } private MatchedRule defaultExport( + String matchName, String domain, LinkedHashMap beanProperties, LinkedList attrKeys, @@ -366,7 +368,7 @@ private MatchedRule defaultExport( } } - return new MatchedRule(fullname, type, help, labelNames, labelValues, value, valueFactor); + return new MatchedRule(fullname, matchName, type, help, labelNames, labelValues, value, valueFactor); } public void recordBean( @@ -383,97 +385,106 @@ public void recordBean( String help = attrDescription + " (" + beanName + attrName + ")"; String attrNameSnakeCase = toSnakeAndLowerCase(attrName); - MatchedRule matchedRule = null; - String cacheName = beanName + attrName; - if (config.cacheRules) { - matchedRule = cachedRules.get(cacheName); - lastCachedRules.add(cacheName); - } + MatchedRule matchedRule = MatchedRule.unmatched(); - if (matchedRule == null) { - for (Rule rule : config.rules) { - Matcher matcher = null; - String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName); + for (Rule rule : config.rules) { + String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName); - // Using bean value in caching is not possible (caching is only done on bean name, values can change) - if (!config.cacheRules) { - matchName = matchName + ": " + beanValue; - } + if (rule.matchBeanValue) { + matchName = matchName + ": " + beanValue; + } - if (rule.pattern != null) { - matcher = rule.pattern.matcher(matchName); - if (!matcher.matches()) { - continue; + if (rule.cache) { + MatchedRule cachedRule = cachedRules.get(rule, matchName); + if (cachedRule != null) { + lastCachedRules.add(new MatchedRulesCache.Entry(rule, matchName)); + if (!cachedRule.isUnmatched()) { + matchedRule = cachedRule; + break; } - } - Double value = null; - if (rule.value != null && !rule.value.isEmpty()) { - String val = matcher.replaceAll(rule.value); - try { - value = Double.valueOf(val); - } catch (NumberFormatException e) { - LOGGER.fine("Unable to parse configured value '" + val + "' to number for bean: " + beanName + attrName + ": " + beanValue); - return; - } + // The bean was cached earlier, but did not match the current rule. + // Skip it to avoid matching against the same pattern again + continue; } + } - // If there's no name provided, use default export format. - if (rule.name == null) { - matchedRule = defaultExport(domain, beanProperties, attrKeys, rule.attrNameSnakeCase ? attrNameSnakeCase : attrName, help, value, rule.valueFactor, rule.type); - break; + Matcher matcher = null; + if (rule.pattern != null) { + matcher = rule.pattern.matcher(matchName); + if (!matcher.matches()) { + if (rule.cache) { + cachedRules.put(rule, matchName, MatchedRule.unmatched()); + lastCachedRules.add(new MatchedRulesCache.Entry(rule, matchName)); + } + continue; } + } - // Matcher is set below here due to validation in the constructor. - String name = safeName(matcher.replaceAll(rule.name)); - if (name.isEmpty()) { + Double value = null; + if (rule.value != null && !rule.value.isEmpty()) { + String val = matcher.replaceAll(rule.value); + try { + value = Double.valueOf(val); + } catch (NumberFormatException e) { + LOGGER.fine("Unable to parse configured value '" + val + "' to number for bean: " + beanName + attrName + ": " + beanValue); return; } - if (config.lowercaseOutputName) { - name = name.toLowerCase(); - } + } - // Set the help. - if (rule.help != null) { - help = matcher.replaceAll(rule.help); + // If there's no name provided, use default export format. + if (rule.name == null) { + matchedRule = defaultExport(matchName, domain, beanProperties, attrKeys, rule.attrNameSnakeCase ? attrNameSnakeCase : attrName, help, value, rule.valueFactor, rule.type); + if (rule.cache) { + lastCachedRules.add(new MatchedRulesCache.Entry(rule, matchName)); } + break; + } - // Set the labels. - ArrayList labelNames = new ArrayList(); - ArrayList labelValues = new ArrayList(); - if (rule.labelNames != null) { - for (int i = 0; i < rule.labelNames.size(); i++) { - final String unsafeLabelName = rule.labelNames.get(i); - final String labelValReplacement = rule.labelValues.get(i); - try { - String labelName = safeName(matcher.replaceAll(unsafeLabelName)); - String labelValue = matcher.replaceAll(labelValReplacement); - if (config.lowercaseOutputLabelNames) { - labelName = labelName.toLowerCase(); - } - if (!labelName.isEmpty() && !labelValue.isEmpty()) { - labelNames.add(labelName); - labelValues.add(labelValue); - } - } catch (Exception e) { - throw new RuntimeException( - format("Matcher '%s' unable to use: '%s' value: '%s'", matcher, unsafeLabelName, labelValReplacement), e); - } - } - } + // Matcher is set below here due to validation in the constructor. + String name = safeName(matcher.replaceAll(rule.name)); + if (name.isEmpty()) { + return; + } + if (config.lowercaseOutputName) { + name = name.toLowerCase(); + } - matchedRule = new MatchedRule(name, rule.type, help, labelNames, labelValues, value, rule.valueFactor); - break; + // Set the help. + if (rule.help != null) { + help = matcher.replaceAll(rule.help); } - // No rule matches the mbean, cache it as an invalid rule to get cache hits in the future - if (matchedRule == null) { - matchedRule = MatchedRule.unmatched(); + // Set the labels. + ArrayList labelNames = new ArrayList(); + ArrayList labelValues = new ArrayList(); + if (rule.labelNames != null) { + for (int i = 0; i < rule.labelNames.size(); i++) { + final String unsafeLabelName = rule.labelNames.get(i); + final String labelValReplacement = rule.labelValues.get(i); + try { + String labelName = safeName(matcher.replaceAll(unsafeLabelName)); + String labelValue = matcher.replaceAll(labelValReplacement); + if (config.lowercaseOutputLabelNames) { + labelName = labelName.toLowerCase(); + } + if (!labelName.isEmpty() && !labelValue.isEmpty()) { + labelNames.add(labelName); + labelValues.add(labelValue); + } + } catch (Exception e) { + throw new RuntimeException( + format("Matcher '%s' unable to use: '%s' value: '%s'", matcher, unsafeLabelName, labelValReplacement), e); + } + } } - if (config.cacheRules) { - cachedRules.put(cacheName, matchedRule); + matchedRule = new MatchedRule(name, matchName, rule.type, help, labelNames, labelValues, value, rule.valueFactor); + if (rule.cache) { + cachedRules.put(rule, matchName, matchedRule); + lastCachedRules.add(new MatchedRulesCache.Entry(rule, matchName)); } + break; } if (matchedRule.isUnmatched()) { @@ -501,15 +512,6 @@ public void recordBean( } - // Remove stale rules (in the cache but not collected in the last run of the collector) - private void evictStaleCachedRules(Set lastCachedRules) { - for (String cacheName : cachedRules.keySet()) { - if (!lastCachedRules.contains(cacheName)) { - cachedRules.remove(cacheName); - } - } - } - public List collect() { if (configFile != null) { long mtime = configFile.lastModified(); @@ -520,7 +522,7 @@ public List collect() { } } - Set lastCachedRules = new HashSet(); + Set lastCachedRules = new HashSet(); Receiver receiver = new Receiver(cachedRules, lastCachedRules); JmxScraper scraper = new JmxScraper(config.jmxUrl, config.username, config.password, config.ssl, config.whitelistObjectNames, config.blacklistObjectNames, receiver, jmxMBeanPropertyCache); @@ -538,7 +540,7 @@ public List collect() { e.printStackTrace(new PrintWriter(sw)); LOGGER.severe("JMX scrape failed: " + sw.toString()); } - evictStaleCachedRules(lastCachedRules); + cachedRules.evictStaleEntries(lastCachedRules); List mfsList = new ArrayList(); mfsList.addAll(receiver.metricFamilySamplesMap.values()); @@ -551,12 +553,14 @@ public List collect() { samples.add(new MetricFamilySamples.Sample( "jmx_scrape_error", new ArrayList(), new ArrayList(), error)); mfsList.add(new MetricFamilySamples("jmx_scrape_error", Type.GAUGE, "Non-zero if this scrape failed.", samples)); - if (config.cacheRules) { - samples = new ArrayList(); - samples.add(new MetricFamilySamples.Sample( - "jmx_scrape_cached_beans", new ArrayList(), new ArrayList(), cachedRules.size())); - mfsList.add(new MetricFamilySamples("jmx_scrape_cached_beans", Type.GAUGE, "Number of beans with their matching rule cached", samples)); - } + samples = new ArrayList(); + samples.add(new MetricFamilySamples.Sample( + "jmx_scrape_cache_matched_beans", new ArrayList(), new ArrayList(), cachedRules.matchedCount())); + mfsList.add(new MetricFamilySamples("jmx_scrape_cache_matched_beans", Type.GAUGE, "Number of beans with their matching rule cached", samples)); + samples = new ArrayList(); + samples.add(new MetricFamilySamples.Sample( + "jmx_scrape_cache_unmatched_beans", new ArrayList(), new ArrayList(), cachedRules.unmatchedCount())); + mfsList.add(new MetricFamilySamples("jmx_scrape_cache_unmatched_beans", Type.GAUGE, "Number of beans cached without a matching rule", samples)); return mfsList; } @@ -564,7 +568,8 @@ public List describe() { List sampleFamilies = new ArrayList(); sampleFamilies.add(new MetricFamilySamples("jmx_scrape_duration_seconds", Type.GAUGE, "Time this JMX scrape took, in seconds.", new ArrayList())); sampleFamilies.add(new MetricFamilySamples("jmx_scrape_error", Type.GAUGE, "Non-zero if this scrape failed.", new ArrayList())); - sampleFamilies.add(new MetricFamilySamples("jmx_scrape_cached_beans", Type.GAUGE, "Number of beans with their matching rule cached", new ArrayList())); + sampleFamilies.add(new MetricFamilySamples("jmx_scrape_cache_matched_beans", Type.GAUGE, "Number of beans with their matching rule cached", new ArrayList())); + sampleFamilies.add(new MetricFamilySamples("jmx_scrape_cache_unmatched_beans", Type.GAUGE, "Number of beans cached without a matching rule", new ArrayList())); return sampleFamilies; } diff --git a/collector/src/main/java/io/prometheus/jmx/MatchedRule.java b/collector/src/main/java/io/prometheus/jmx/MatchedRule.java index c9288a28..99a27899 100644 --- a/collector/src/main/java/io/prometheus/jmx/MatchedRule.java +++ b/collector/src/main/java/io/prometheus/jmx/MatchedRule.java @@ -11,6 +11,7 @@ */ public class MatchedRule { final String name; + final String matchName; final Type type; final String help; final List labelNames; @@ -22,6 +23,7 @@ public class MatchedRule { private MatchedRule() { this.name = null; + this.matchName = null; this.type = null; this.help = null; this.labelNames = null; @@ -32,6 +34,7 @@ private MatchedRule() { public MatchedRule( final String name, + final String matchName, final Type type, final String help, final List labelNames, @@ -39,6 +42,7 @@ public MatchedRule( final Double value, double valueFactor) { this.name = name; + this.matchName = matchName; this.type = type; this.help = help; this.labelNames = labelNames; diff --git a/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java b/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java new file mode 100644 index 00000000..c76e9906 --- /dev/null +++ b/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java @@ -0,0 +1,93 @@ +package io.prometheus.jmx; + +import java.util.Collection; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +/** + * MatchedRulesCache is a cache for bean name to configured rule mapping (See JmxCollector.Receiver). + * The cache also retains unmatched entries (a bean name not matching a rule pattern) to avoid + * matching against the same pattern in later bean collections. + */ +public class MatchedRulesCache { + private final Map cachedRules; + + public MatchedRulesCache() { + this.cachedRules = new ConcurrentHashMap(); + } + + public void clear() { + cachedRules.clear(); + } + + public void put(final JmxCollector.Rule rule, final String name, final MatchedRule matchedRule) { + final Entry entry = new Entry(rule, name); + cachedRules.put(entry, matchedRule); + } + + public MatchedRule get(final JmxCollector.Rule rule, final String name) { + final Entry entry = new Entry(rule, name); + return cachedRules.get(entry); + } + + // Remove stale rules (in the cache but not collected in the last run of the collector) + public void evictStaleEntries(Collection lastCachedEntries) { + for (Entry entry : cachedRules.keySet()) { + if (!lastCachedEntries.contains(entry)) { + cachedRules.remove(entry); + } + } + } + + public long matchedCount() { + long count = 0; + for (MatchedRule matchedRule : cachedRules.values()) { + if (!matchedRule.isUnmatched()) { + count++; + } + } + return count; + } + + public long unmatchedCount() { + long count = 0; + for (MatchedRule matchedRule : cachedRules.values()) { + if (matchedRule.isUnmatched()) { + count++; + } + } + return count; + } + + public static class Entry { + public final JmxCollector.Rule rule; + public final String name; + + public Entry(final JmxCollector.Rule rule, final String name) { + this.rule = rule; + this.name = name; + } + + @Override + public int hashCode() { + int result = 17; + result = 31 * result + rule.hashCode(); + result = 31 * result + name.hashCode(); + return result; + } + + @Override + public boolean equals(Object obj) { + if (obj == this) { + return true; + } + + if (!(obj instanceof Entry)) { + return false; + } + + Entry entry = (Entry) obj; + return rule.equals(entry.rule) && name.equals(entry.name); + } + } +} \ No newline at end of file diff --git a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java index 7d29a5a4..4a95d25c 100644 --- a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java +++ b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java @@ -271,17 +271,29 @@ public void testCamelLastExchangFailureTimestamp() throws Exception{ assertEquals(Camel.EXPECTED_SECONDS, actual, 0); } + @Test + public void testMatchBeanValueDisabled() throws Exception { + JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*:`\n name: foo\n matchBeanValue: false".replace('`','"')).register(registry); + assertNull(registry.getSampleValue("foo", new String[]{}, new String[]{})); + } + + @Test + public void testMatchBeanValueEnabled() throws Exception { + JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*:`\n name: foo\n matchBeanValue: true".replace('`','"')).register(registry); + assertNotNull(registry.getSampleValue("foo", new String[]{}, new String[]{})); + } + @Test public void testCachedBeansDisabled() throws Exception { JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4".replace('`','"')).register(registry); - assertNull(registry.getSampleValue("jmx_scrape_cached_beans", new String[]{}, new String[]{})); + assertEquals(0.0, registry.getSampleValue("jmx_scrape_cache_matched_beans", new String[]{}, new String[]{}), .001); assertEquals(4.0, registry.getSampleValue("foo", new String[]{}, new String[]{}), .001); } @Test public void testCachedBeansEnabled() throws Exception { - JmxCollector jc = new JmxCollector("\n---\ncacheRules: true\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4".replace('`','"')).register(registry); - assertTrue(registry.getSampleValue("jmx_scrape_cached_beans", new String[]{}, new String[]{}) > 0); + JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4\n cache: true\n matchBeanValue: true".replace('`','"')).register(registry); + assertTrue(registry.getSampleValue("jmx_scrape_cache_matched_beans", new String[]{}, new String[]{}) > 0); assertEquals(4.0, registry.getSampleValue("foo", new String[]{}, new String[]{}), .001); } } From 10b9f5b04f1c5682b1018781f1a7865bba086d51 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Fri, 14 Aug 2020 11:40:26 +0100 Subject: [PATCH 04/19] Remove usage of matchBeanValue Always match on bean value, but cache names without the bean values. Signed-off-by: Flavien Raynaud --- README.md | 1 - .../java/io/prometheus/jmx/JmxCollector.java | 26 +++++++------------ .../io/prometheus/jmx/JmxCollectorTest.java | 12 --------- 3 files changed, 10 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index d6b4014f..05a19d40 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,6 @@ value | Value for the metric. Static values and capture groups from valueFactor | Optional number that `value` (or the scraped mBean value if `value` is not specified) is multiplied by, mainly used to convert mBean values from milliseconds to seconds. labels | A map of label name to label value pairs. Capture groups from `pattern` can be used in each. `name` must be set to use this. Empty names and values are ignored. If not specified and the default format is not being used, no labels are set. help | Help text for the metric. Capture groups from `pattern` can be used. `name` must be set to use this. Defaults to the mBean attribute description and the full name of the attribute. -matchBeanValue | Whether to append the bean value in the expression to match patterns against for this rule. (e.g: "beanName: "). Defaults to `true`. cache | Whether to cache bean name expressions to rule computation (match and mismatch). This can increase performance when collecting a lot of mbeans. Defaults to `false`. type | The type of the metric, can be `GAUGE`, `COUNTER` or `UNTYPED`. `name` must be set to use this. Defaults to `UNTYPED`. diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 02426ec2..37a9f2a1 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -46,7 +46,6 @@ static class Rule { Double valueFactor = 1.0; String help; boolean attrNameSnakeCase; - boolean matchBeanValue = true; boolean cache = false; Type type = Type.UNTYPED; ArrayList labelNames; @@ -195,9 +194,6 @@ private Config loadConfig(Map yamlConfig) throws MalformedObject if (yamlRule.containsKey("attrNameSnakeCase")) { rule.attrNameSnakeCase = (Boolean)yamlRule.get("attrNameSnakeCase"); } - if (yamlRule.containsKey("matchBeanValue")) { - rule.matchBeanValue = (Boolean)yamlRule.get("matchBeanValue"); - } if (yamlRule.containsKey("cache")) { rule.cache = (Boolean)yamlRule.get("cache"); } @@ -388,16 +384,13 @@ public void recordBean( MatchedRule matchedRule = MatchedRule.unmatched(); for (Rule rule : config.rules) { - String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName); - - if (rule.matchBeanValue) { - matchName = matchName + ": " + beanValue; - } + String cacheName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName); + String matchName = cacheName + ": " + beanValue; if (rule.cache) { - MatchedRule cachedRule = cachedRules.get(rule, matchName); + MatchedRule cachedRule = cachedRules.get(rule, cacheName); if (cachedRule != null) { - lastCachedRules.add(new MatchedRulesCache.Entry(rule, matchName)); + lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); if (!cachedRule.isUnmatched()) { matchedRule = cachedRule; break; @@ -414,8 +407,8 @@ public void recordBean( matcher = rule.pattern.matcher(matchName); if (!matcher.matches()) { if (rule.cache) { - cachedRules.put(rule, matchName, MatchedRule.unmatched()); - lastCachedRules.add(new MatchedRulesCache.Entry(rule, matchName)); + cachedRules.put(rule, cacheName, MatchedRule.unmatched()); + lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); } continue; } @@ -436,7 +429,8 @@ public void recordBean( if (rule.name == null) { matchedRule = defaultExport(matchName, domain, beanProperties, attrKeys, rule.attrNameSnakeCase ? attrNameSnakeCase : attrName, help, value, rule.valueFactor, rule.type); if (rule.cache) { - lastCachedRules.add(new MatchedRulesCache.Entry(rule, matchName)); + cachedRules.put(rule, cacheName, matchedRule); + lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); } break; } @@ -481,8 +475,8 @@ public void recordBean( matchedRule = new MatchedRule(name, matchName, rule.type, help, labelNames, labelValues, value, rule.valueFactor); if (rule.cache) { - cachedRules.put(rule, matchName, matchedRule); - lastCachedRules.add(new MatchedRulesCache.Entry(rule, matchName)); + cachedRules.put(rule, cacheName, matchedRule); + lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); } break; } diff --git a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java index 4a95d25c..42af021a 100644 --- a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java +++ b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java @@ -271,18 +271,6 @@ public void testCamelLastExchangFailureTimestamp() throws Exception{ assertEquals(Camel.EXPECTED_SECONDS, actual, 0); } - @Test - public void testMatchBeanValueDisabled() throws Exception { - JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*:`\n name: foo\n matchBeanValue: false".replace('`','"')).register(registry); - assertNull(registry.getSampleValue("foo", new String[]{}, new String[]{})); - } - - @Test - public void testMatchBeanValueEnabled() throws Exception { - JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*:`\n name: foo\n matchBeanValue: true".replace('`','"')).register(registry); - assertNotNull(registry.getSampleValue("foo", new String[]{}, new String[]{})); - } - @Test public void testCachedBeansDisabled() throws Exception { JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4".replace('`','"')).register(registry); From 8a18cd8bc8e3d6aedcb1fcf87fd3f5ceabe2926b Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Fri, 14 Aug 2020 11:47:45 +0100 Subject: [PATCH 05/19] Add helper `addToCache` method in Receiver It adds the result of the rule matching to the cache of matched rules (if enabled). Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 37a9f2a1..5d57ef35 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -319,6 +319,15 @@ void addSample(MetricFamilySamples.Sample sample, Type type, String help) { mfs.samples.add(sample); } + // Add the matched rule to the cached rules and tag it as not stale (lastCachedRules), + // if the rule is configured to be cached + private void addToCache(final Rule rule, final String cacheName, final MatchedRule matchedRule) { + if (rule.cache) { + cachedRules.put(rule, cacheName, matchedRule); + lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); + } + } + private MatchedRule defaultExport( String matchName, String domain, @@ -390,7 +399,7 @@ public void recordBean( if (rule.cache) { MatchedRule cachedRule = cachedRules.get(rule, cacheName); if (cachedRule != null) { - lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); + addToCache(rule, cacheName, cachedRule); if (!cachedRule.isUnmatched()) { matchedRule = cachedRule; break; @@ -406,10 +415,7 @@ public void recordBean( if (rule.pattern != null) { matcher = rule.pattern.matcher(matchName); if (!matcher.matches()) { - if (rule.cache) { - cachedRules.put(rule, cacheName, MatchedRule.unmatched()); - lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); - } + addToCache(rule, cacheName, MatchedRule.unmatched()); continue; } } @@ -428,10 +434,7 @@ public void recordBean( // If there's no name provided, use default export format. if (rule.name == null) { matchedRule = defaultExport(matchName, domain, beanProperties, attrKeys, rule.attrNameSnakeCase ? attrNameSnakeCase : attrName, help, value, rule.valueFactor, rule.type); - if (rule.cache) { - cachedRules.put(rule, cacheName, matchedRule); - lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); - } + addToCache(rule, cacheName, matchedRule); break; } @@ -474,10 +477,7 @@ public void recordBean( } matchedRule = new MatchedRule(name, matchName, rule.type, help, labelNames, labelValues, value, rule.valueFactor); - if (rule.cache) { - cachedRules.put(rule, cacheName, matchedRule); - lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); - } + addToCache(rule, cacheName, matchedRule); break; } From 8845803192d49b2f2de8ffdf5208118a4981c6fc Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Fri, 14 Aug 2020 11:49:44 +0100 Subject: [PATCH 06/19] Remove stale mentions of `matchBeanValue` Signed-off-by: Flavien Raynaud --- README.md | 1 - collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/README.md b/README.md index 05a19d40..18e670da 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,6 @@ rules: valueFactor: 0.001 labels: {} help: "Cassandra metric $1 $2" - matchBeanValue: true cache: false type: GAUGE attrNameSnakeCase: false diff --git a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java index 42af021a..838c1006 100644 --- a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java +++ b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java @@ -280,7 +280,7 @@ public void testCachedBeansDisabled() throws Exception { @Test public void testCachedBeansEnabled() throws Exception { - JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4\n cache: true\n matchBeanValue: true".replace('`','"')).register(registry); + JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4\n cache: true".replace('`','"')).register(registry); assertTrue(registry.getSampleValue("jmx_scrape_cache_matched_beans", new String[]{}, new String[]{}) > 0); assertEquals(4.0, registry.getSampleValue("foo", new String[]{}, new String[]{}), .001); } From c701cf2ae30006e33b95dbf7e48484f57a997de2 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Tue, 18 Aug 2020 16:30:47 +0100 Subject: [PATCH 07/19] Rename cacheName -> cacheKey Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 5d57ef35..b3783d10 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -71,7 +71,7 @@ private static class Config { private long createTimeNanoSecs = System.nanoTime(); private final JmxMBeanPropertyCache jmxMBeanPropertyCache = new JmxMBeanPropertyCache(); - private final MatchedRulesCache cachedRules = new MatchedRulesCache(); + private MatchedRulesCache cachedRules = new MatchedRulesCache(); public JmxCollector(File in) throws IOException, MalformedObjectNameException { configFile = in; @@ -321,10 +321,10 @@ void addSample(MetricFamilySamples.Sample sample, Type type, String help) { // Add the matched rule to the cached rules and tag it as not stale (lastCachedRules), // if the rule is configured to be cached - private void addToCache(final Rule rule, final String cacheName, final MatchedRule matchedRule) { + private void addToCache(final Rule rule, final String cacheKey, final MatchedRule matchedRule) { if (rule.cache) { - cachedRules.put(rule, cacheName, matchedRule); - lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheName)); + cachedRules.put(rule, cacheKey, matchedRule); + lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheKey)); } } @@ -393,13 +393,13 @@ public void recordBean( MatchedRule matchedRule = MatchedRule.unmatched(); for (Rule rule : config.rules) { - String cacheName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName); - String matchName = cacheName + ": " + beanValue; + String cacheKey = beanName + attrName; + String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName) + ": " + beanValue; if (rule.cache) { - MatchedRule cachedRule = cachedRules.get(rule, cacheName); + MatchedRule cachedRule = cachedRules.get(rule, cacheKey); if (cachedRule != null) { - addToCache(rule, cacheName, cachedRule); + addToCache(rule, cacheKey, cachedRule); if (!cachedRule.isUnmatched()) { matchedRule = cachedRule; break; @@ -415,7 +415,7 @@ public void recordBean( if (rule.pattern != null) { matcher = rule.pattern.matcher(matchName); if (!matcher.matches()) { - addToCache(rule, cacheName, MatchedRule.unmatched()); + addToCache(rule, cacheKey, MatchedRule.unmatched()); continue; } } @@ -434,7 +434,7 @@ public void recordBean( // If there's no name provided, use default export format. if (rule.name == null) { matchedRule = defaultExport(matchName, domain, beanProperties, attrKeys, rule.attrNameSnakeCase ? attrNameSnakeCase : attrName, help, value, rule.valueFactor, rule.type); - addToCache(rule, cacheName, matchedRule); + addToCache(rule, cacheKey, matchedRule); break; } @@ -477,7 +477,7 @@ public void recordBean( } matchedRule = new MatchedRule(name, matchName, rule.type, help, labelNames, labelValues, value, rule.valueFactor); - addToCache(rule, cacheName, matchedRule); + addToCache(rule, cacheKey, matchedRule); break; } From fe7bf661f5dd05d1c9185be1e63712923e71fccc Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Tue, 18 Aug 2020 16:33:24 +0100 Subject: [PATCH 08/19] Add MatchedRule::isMatched method Signed-off-by: Flavien Raynaud --- collector/src/main/java/io/prometheus/jmx/JmxCollector.java | 3 ++- collector/src/main/java/io/prometheus/jmx/MatchedRule.java | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index b3783d10..2b465bb3 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -399,8 +399,9 @@ public void recordBean( if (rule.cache) { MatchedRule cachedRule = cachedRules.get(rule, cacheKey); if (cachedRule != null) { + // add to the cache to signal that this cacheKey is not stale and should stay in the cache addToCache(rule, cacheKey, cachedRule); - if (!cachedRule.isUnmatched()) { + if (cachedRule.isMatched()) { matchedRule = cachedRule; break; } diff --git a/collector/src/main/java/io/prometheus/jmx/MatchedRule.java b/collector/src/main/java/io/prometheus/jmx/MatchedRule.java index 99a27899..e2deb468 100644 --- a/collector/src/main/java/io/prometheus/jmx/MatchedRule.java +++ b/collector/src/main/java/io/prometheus/jmx/MatchedRule.java @@ -63,4 +63,8 @@ public static MatchedRule unmatched() { public boolean isUnmatched() { return this == _unmatched; } + + public boolean isMatched() { + return !isUnmatched(); + } } From 5b7893a398685eec84ae45e8d8783b54bd437ba2 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Tue, 18 Aug 2020 18:36:12 +0100 Subject: [PATCH 09/19] Replace MatchedRulesCache.Entry with a Map Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 12 +-- .../io/prometheus/jmx/MatchedRulesCache.java | 100 ++++++++++-------- 2 files changed, 61 insertions(+), 51 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 2b465bb3..dad00d95 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -12,7 +12,6 @@ import java.io.StringWriter; import java.util.ArrayList; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedList; @@ -294,11 +293,11 @@ class Receiver implements JmxScraper.MBeanReceiver { new HashMap(); MatchedRulesCache cachedRules; - Set lastCachedRules; + MatchedRulesCache.LastEntries lastCachedRules; private static final char SEP = '_'; - Receiver(MatchedRulesCache cachedRules, Set lastCachedRules) { + Receiver(MatchedRulesCache cachedRules, MatchedRulesCache.LastEntries lastCachedRules) { this.cachedRules = cachedRules; this.lastCachedRules = lastCachedRules; } @@ -324,7 +323,7 @@ void addSample(MetricFamilySamples.Sample sample, Type type, String help) { private void addToCache(final Rule rule, final String cacheKey, final MatchedRule matchedRule) { if (rule.cache) { cachedRules.put(rule, cacheKey, matchedRule); - lastCachedRules.add(new MatchedRulesCache.Entry(rule, cacheKey)); + lastCachedRules.add(rule, cacheKey); } } @@ -399,8 +398,7 @@ public void recordBean( if (rule.cache) { MatchedRule cachedRule = cachedRules.get(rule, cacheKey); if (cachedRule != null) { - // add to the cache to signal that this cacheKey is not stale and should stay in the cache - addToCache(rule, cacheKey, cachedRule); + lastCachedRules.add(rule, cacheKey); if (cachedRule.isMatched()) { matchedRule = cachedRule; break; @@ -517,7 +515,7 @@ public List collect() { } } - Set lastCachedRules = new HashSet(); + MatchedRulesCache.LastEntries lastCachedRules = new MatchedRulesCache.LastEntries(); Receiver receiver = new Receiver(cachedRules, lastCachedRules); JmxScraper scraper = new JmxScraper(config.jmxUrl, config.username, config.password, config.ssl, config.whitelistObjectNames, config.blacklistObjectNames, receiver, jmxMBeanPropertyCache); diff --git a/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java b/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java index c76e9906..c84af404 100644 --- a/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java +++ b/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java @@ -1,7 +1,9 @@ package io.prometheus.jmx; -import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; /** @@ -10,40 +12,61 @@ * matching against the same pattern in later bean collections. */ public class MatchedRulesCache { - private final Map cachedRules; + private final Map> cachedRules; public MatchedRulesCache() { - this.cachedRules = new ConcurrentHashMap(); + this.cachedRules = new ConcurrentHashMap>(); } public void clear() { cachedRules.clear(); } - public void put(final JmxCollector.Rule rule, final String name, final MatchedRule matchedRule) { - final Entry entry = new Entry(rule, name); - cachedRules.put(entry, matchedRule); + public void put(final JmxCollector.Rule rule, final String cacheKey, final MatchedRule matchedRule) { + Map cachedRulesForRule = cachedRules.get(rule); + if (cachedRulesForRule == null) { + synchronized (cachedRules) { + cachedRulesForRule = cachedRules.get(rule); + if (cachedRulesForRule == null) { + cachedRulesForRule = new ConcurrentHashMap(); + cachedRules.put(rule, cachedRulesForRule); + } + } + } + + cachedRulesForRule.put(cacheKey, matchedRule); } - public MatchedRule get(final JmxCollector.Rule rule, final String name) { - final Entry entry = new Entry(rule, name); - return cachedRules.get(entry); + public MatchedRule get(final JmxCollector.Rule rule, final String cacheKey) { + Map cachedRulesForRule = cachedRules.get(rule); + return (cachedRulesForRule == null) ? null : cachedRulesForRule.get(cacheKey); } // Remove stale rules (in the cache but not collected in the last run of the collector) - public void evictStaleEntries(Collection lastCachedEntries) { - for (Entry entry : cachedRules.keySet()) { - if (!lastCachedEntries.contains(entry)) { - cachedRules.remove(entry); + public void evictStaleEntries(final LastEntries lastEntries) { + for (Map.Entry> entry : cachedRules.entrySet()) { + JmxCollector.Rule rule = entry.getKey(); + Map cachedRulesForRule = entry.getValue(); + + for (String cacheKey : cachedRulesForRule.keySet()) { + if (!lastEntries.contains(rule, cacheKey)) { + cachedRulesForRule.remove(cacheKey); + } + } + + if (cachedRulesForRule.size() == 0) { + cachedRules.remove(rule); } } } public long matchedCount() { long count = 0; - for (MatchedRule matchedRule : cachedRules.values()) { - if (!matchedRule.isUnmatched()) { - count++; + for (Map matchedRules : cachedRules.values()) { + for (MatchedRule matchedRule : matchedRules.values()) { + if (matchedRule.isUnmatched()) { + count++; + } } } return count; @@ -51,43 +74,32 @@ public long matchedCount() { public long unmatchedCount() { long count = 0; - for (MatchedRule matchedRule : cachedRules.values()) { - if (matchedRule.isUnmatched()) { - count++; + for (Map matchedRules : cachedRules.values()) { + for (MatchedRule matchedRule : matchedRules.values()) { + if (matchedRule.isUnmatched()) { + count++; + } } } return count; } - public static class Entry { - public final JmxCollector.Rule rule; - public final String name; + public static class LastEntries { + final Map> lastCachedEntries = new HashMap>(); - public Entry(final JmxCollector.Rule rule, final String name) { - this.rule = rule; - this.name = name; - } - - @Override - public int hashCode() { - int result = 17; - result = 31 * result + rule.hashCode(); - result = 31 * result + name.hashCode(); - return result; - } - - @Override - public boolean equals(Object obj) { - if (obj == this) { - return true; + public void add(final JmxCollector.Rule rule, final String cacheKey) { + Set lastCachedEntriesForRule = lastCachedEntries.get(rule); + if (lastCachedEntriesForRule == null) { + lastCachedEntriesForRule = new HashSet(); + lastCachedEntries.put(rule, lastCachedEntriesForRule); } - if (!(obj instanceof Entry)) { - return false; - } + lastCachedEntriesForRule.add(cacheKey); + } - Entry entry = (Entry) obj; - return rule.equals(entry.rule) && name.equals(entry.name); + public boolean contains(final JmxCollector.Rule rule, final String cacheKey) { + Set lastCachedEntriesForRule = lastCachedEntries.get(rule); + return (lastCachedEntriesForRule != null) && lastCachedEntriesForRule.contains(cacheKey); } } } \ No newline at end of file From 8b284c38b1cdb606d935c63ba07c7ef12975d33f Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Wed, 19 Aug 2020 13:38:21 +0100 Subject: [PATCH 10/19] Rename LastEntries -> StalenessTracker Record count of cached beans by relying on the StalenessTracker to avoid race conditions between scrapes. Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 29 ++++++-------- .../io/prometheus/jmx/MatchedRulesCache.java | 40 ++++++------------- .../io/prometheus/jmx/JmxCollectorTest.java | 4 +- 3 files changed, 26 insertions(+), 47 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index dad00d95..5610ca90 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -293,13 +293,13 @@ class Receiver implements JmxScraper.MBeanReceiver { new HashMap(); MatchedRulesCache cachedRules; - MatchedRulesCache.LastEntries lastCachedRules; + MatchedRulesCache.StalenessTracker stalenessTracker; private static final char SEP = '_'; - Receiver(MatchedRulesCache cachedRules, MatchedRulesCache.LastEntries lastCachedRules) { + Receiver(MatchedRulesCache cachedRules, MatchedRulesCache.StalenessTracker stalenessTracker) { this.cachedRules = cachedRules; - this.lastCachedRules = lastCachedRules; + this.stalenessTracker = stalenessTracker; } // [] and () are special in regexes, so swtich to <>. @@ -318,12 +318,12 @@ void addSample(MetricFamilySamples.Sample sample, Type type, String help) { mfs.samples.add(sample); } - // Add the matched rule to the cached rules and tag it as not stale (lastCachedRules), + // Add the matched rule to the cached rules and tag it as not stale // if the rule is configured to be cached private void addToCache(final Rule rule, final String cacheKey, final MatchedRule matchedRule) { if (rule.cache) { cachedRules.put(rule, cacheKey, matchedRule); - lastCachedRules.add(rule, cacheKey); + stalenessTracker.add(rule, cacheKey); } } @@ -398,7 +398,7 @@ public void recordBean( if (rule.cache) { MatchedRule cachedRule = cachedRules.get(rule, cacheKey); if (cachedRule != null) { - lastCachedRules.add(rule, cacheKey); + stalenessTracker.add(rule, cacheKey); if (cachedRule.isMatched()) { matchedRule = cachedRule; break; @@ -515,8 +515,8 @@ public List collect() { } } - MatchedRulesCache.LastEntries lastCachedRules = new MatchedRulesCache.LastEntries(); - Receiver receiver = new Receiver(cachedRules, lastCachedRules); + MatchedRulesCache.StalenessTracker stalenessTracker = new MatchedRulesCache.StalenessTracker(); + Receiver receiver = new Receiver(cachedRules, stalenessTracker); JmxScraper scraper = new JmxScraper(config.jmxUrl, config.username, config.password, config.ssl, config.whitelistObjectNames, config.blacklistObjectNames, receiver, jmxMBeanPropertyCache); long start = System.nanoTime(); @@ -533,7 +533,7 @@ public List collect() { e.printStackTrace(new PrintWriter(sw)); LOGGER.severe("JMX scrape failed: " + sw.toString()); } - cachedRules.evictStaleEntries(lastCachedRules); + cachedRules.evictStaleEntries(stalenessTracker); List mfsList = new ArrayList(); mfsList.addAll(receiver.metricFamilySamplesMap.values()); @@ -548,12 +548,8 @@ public List collect() { mfsList.add(new MetricFamilySamples("jmx_scrape_error", Type.GAUGE, "Non-zero if this scrape failed.", samples)); samples = new ArrayList(); samples.add(new MetricFamilySamples.Sample( - "jmx_scrape_cache_matched_beans", new ArrayList(), new ArrayList(), cachedRules.matchedCount())); - mfsList.add(new MetricFamilySamples("jmx_scrape_cache_matched_beans", Type.GAUGE, "Number of beans with their matching rule cached", samples)); - samples = new ArrayList(); - samples.add(new MetricFamilySamples.Sample( - "jmx_scrape_cache_unmatched_beans", new ArrayList(), new ArrayList(), cachedRules.unmatchedCount())); - mfsList.add(new MetricFamilySamples("jmx_scrape_cache_unmatched_beans", Type.GAUGE, "Number of beans cached without a matching rule", samples)); + "jmx_scrape_cached_beans", new ArrayList(), new ArrayList(), stalenessTracker.cachedCount())); + mfsList.add(new MetricFamilySamples("jmx_scrape_cached_beans", Type.GAUGE, "Number of beans with their matching rule cached", samples)); return mfsList; } @@ -561,8 +557,7 @@ public List describe() { List sampleFamilies = new ArrayList(); sampleFamilies.add(new MetricFamilySamples("jmx_scrape_duration_seconds", Type.GAUGE, "Time this JMX scrape took, in seconds.", new ArrayList())); sampleFamilies.add(new MetricFamilySamples("jmx_scrape_error", Type.GAUGE, "Non-zero if this scrape failed.", new ArrayList())); - sampleFamilies.add(new MetricFamilySamples("jmx_scrape_cache_matched_beans", Type.GAUGE, "Number of beans with their matching rule cached", new ArrayList())); - sampleFamilies.add(new MetricFamilySamples("jmx_scrape_cache_unmatched_beans", Type.GAUGE, "Number of beans cached without a matching rule", new ArrayList())); + sampleFamilies.add(new MetricFamilySamples("jmx_scrape_cached_beans", Type.GAUGE, "Number of beans with their matching rule cached", new ArrayList())); return sampleFamilies; } diff --git a/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java b/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java index c84af404..2608b108 100644 --- a/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java +++ b/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java @@ -43,13 +43,13 @@ public MatchedRule get(final JmxCollector.Rule rule, final String cacheKey) { } // Remove stale rules (in the cache but not collected in the last run of the collector) - public void evictStaleEntries(final LastEntries lastEntries) { + public void evictStaleEntries(final StalenessTracker stalenessTracker) { for (Map.Entry> entry : cachedRules.entrySet()) { JmxCollector.Rule rule = entry.getKey(); Map cachedRulesForRule = entry.getValue(); for (String cacheKey : cachedRulesForRule.keySet()) { - if (!lastEntries.contains(rule, cacheKey)) { + if (!stalenessTracker.contains(rule, cacheKey)) { cachedRulesForRule.remove(cacheKey); } } @@ -60,32 +60,8 @@ public void evictStaleEntries(final LastEntries lastEntries) { } } - public long matchedCount() { - long count = 0; - for (Map matchedRules : cachedRules.values()) { - for (MatchedRule matchedRule : matchedRules.values()) { - if (matchedRule.isUnmatched()) { - count++; - } - } - } - return count; - } - - public long unmatchedCount() { - long count = 0; - for (Map matchedRules : cachedRules.values()) { - for (MatchedRule matchedRule : matchedRules.values()) { - if (matchedRule.isUnmatched()) { - count++; - } - } - } - return count; - } - - public static class LastEntries { - final Map> lastCachedEntries = new HashMap>(); + public static class StalenessTracker { + private final Map> lastCachedEntries = new HashMap>(); public void add(final JmxCollector.Rule rule, final String cacheKey) { Set lastCachedEntriesForRule = lastCachedEntries.get(rule); @@ -101,5 +77,13 @@ public boolean contains(final JmxCollector.Rule rule, final String cacheKey) { Set lastCachedEntriesForRule = lastCachedEntries.get(rule); return (lastCachedEntriesForRule != null) && lastCachedEntriesForRule.contains(cacheKey); } + + public long cachedCount() { + long count = 0; + for (Set cacheKeys : lastCachedEntries.values()) { + count += cacheKeys.size(); + } + return count; + } } } \ No newline at end of file diff --git a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java index 838c1006..7952fd55 100644 --- a/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java +++ b/collector/src/test/java/io/prometheus/jmx/JmxCollectorTest.java @@ -274,14 +274,14 @@ public void testCamelLastExchangFailureTimestamp() throws Exception{ @Test public void testCachedBeansDisabled() throws Exception { JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4".replace('`','"')).register(registry); - assertEquals(0.0, registry.getSampleValue("jmx_scrape_cache_matched_beans", new String[]{}, new String[]{}), .001); + assertEquals(0.0, registry.getSampleValue("jmx_scrape_cached_beans", new String[]{}, new String[]{}), .001); assertEquals(4.0, registry.getSampleValue("foo", new String[]{}, new String[]{}), .001); } @Test public void testCachedBeansEnabled() throws Exception { JmxCollector jc = new JmxCollector("\n---\nrules:\n- pattern: `.*`\n name: foo\n value: 1\n valueFactor: 4\n cache: true".replace('`','"')).register(registry); - assertTrue(registry.getSampleValue("jmx_scrape_cache_matched_beans", new String[]{}, new String[]{}) > 0); + assertTrue(registry.getSampleValue("jmx_scrape_cached_beans", new String[]{}, new String[]{}) > 0); assertEquals(4.0, registry.getSampleValue("foo", new String[]{}, new String[]{}), .001); } } From b727c463ad7e67e6a9be27d190184a80e2faaad1 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Wed, 19 Aug 2020 14:43:22 +0100 Subject: [PATCH 11/19] Make rule cache reloading thread-safe Signed-off-by: Flavien Raynaud --- .../src/main/java/io/prometheus/jmx/JmxCollector.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 5610ca90..eb27a531 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -506,12 +506,21 @@ public void recordBean( } public List collect() { + MatchedRulesCache cachedRules = this.cachedRules; + if (configFile != null) { long mtime = configFile.lastModified(); if (mtime > config.lastUpdate) { LOGGER.fine("Configuration file changed, reloading..."); reloadConfig(); - cachedRules.clear(); // rules may have changed with the configuration, clear the rule cache + synchronized (this) { + // rules may have changed with the configuration, clear the cache + // another thread may have changed the cache already, clear only if it hasn't been changed + if (cachedRules == this.cachedRules) { + this.cachedRules = new MatchedRulesCache(); + } + cachedRules = this.cachedRules; + } } } From 52bfec557375c68cc6c24c3b9968e55584ee17be Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Wed, 19 Aug 2020 14:48:22 +0100 Subject: [PATCH 12/19] Update README Signed-off-by: Flavien Raynaud --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 18e670da..3a6d1a2f 100644 --- a/README.md +++ b/README.md @@ -75,7 +75,7 @@ value | Value for the metric. Static values and capture groups from valueFactor | Optional number that `value` (or the scraped mBean value if `value` is not specified) is multiplied by, mainly used to convert mBean values from milliseconds to seconds. labels | A map of label name to label value pairs. Capture groups from `pattern` can be used in each. `name` must be set to use this. Empty names and values are ignored. If not specified and the default format is not being used, no labels are set. help | Help text for the metric. Capture groups from `pattern` can be used. `name` must be set to use this. Defaults to the mBean attribute description and the full name of the attribute. -cache | Whether to cache bean name expressions to rule computation (match and mismatch). This can increase performance when collecting a lot of mbeans. Defaults to `false`. +cache | Whether to cache bean name expressions to rule computation (match and mismatch). Not recommended for rules matching on bean value only the first value will be cached and re-used. This can increase performance when collecting a lot of mbeans. Defaults to `false`. type | The type of the metric, can be `GAUGE`, `COUNTER` or `UNTYPED`. `name` must be set to use this. Defaults to `UNTYPED`. Metric names and label names are sanitized. All characters other than `[a-zA-Z0-9:_]` are replaced with underscores, From 1b1d2f5c8c86ce33be1483af00e48d862f4c0d14 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Fri, 21 Aug 2020 10:47:41 +0100 Subject: [PATCH 13/19] Update README.md Signed-off-by: Flavien Raynaud --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3a6d1a2f..4dd147ba 100644 --- a/README.md +++ b/README.md @@ -75,7 +75,7 @@ value | Value for the metric. Static values and capture groups from valueFactor | Optional number that `value` (or the scraped mBean value if `value` is not specified) is multiplied by, mainly used to convert mBean values from milliseconds to seconds. labels | A map of label name to label value pairs. Capture groups from `pattern` can be used in each. `name` must be set to use this. Empty names and values are ignored. If not specified and the default format is not being used, no labels are set. help | Help text for the metric. Capture groups from `pattern` can be used. `name` must be set to use this. Defaults to the mBean attribute description and the full name of the attribute. -cache | Whether to cache bean name expressions to rule computation (match and mismatch). Not recommended for rules matching on bean value only the first value will be cached and re-used. This can increase performance when collecting a lot of mbeans. Defaults to `false`. +cache | Whether to cache bean name expressions to rule computation (match and mismatch). Not recommended for rules matching on bean value, as only the value from the first scrape will be cached and re-used. This can increase performance when collecting a lot of mbeans. Defaults to `false`. type | The type of the metric, can be `GAUGE`, `COUNTER` or `UNTYPED`. `name` must be set to use this. Defaults to `UNTYPED`. Metric names and label names are sanitized. All characters other than `[a-zA-Z0-9:_]` are replaced with underscores, From d4fbe5c18fce250925129c41c2802639795daa23 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Fri, 21 Aug 2020 10:59:16 +0100 Subject: [PATCH 14/19] Make MatchedRulesCache part of the Config and reload it with a single lock Also populate the MatchedRulesCache with the set of rules from the config. Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 55 ++++++++++--------- .../io/prometheus/jmx/MatchedRulesCache.java | 29 +++------- 2 files changed, 36 insertions(+), 48 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index eb27a531..222ab0fc 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -63,6 +63,8 @@ private static class Config { List blacklistObjectNames = new ArrayList(); List rules = new ArrayList(); long lastUpdate = 0L; + + MatchedRulesCache rulesCache; } private Config config; @@ -70,7 +72,6 @@ private static class Config { private long createTimeNanoSecs = System.nanoTime(); private final JmxMBeanPropertyCache jmxMBeanPropertyCache = new JmxMBeanPropertyCache(); - private MatchedRulesCache cachedRules = new MatchedRulesCache(); public JmxCollector(File in) throws IOException, MalformedObjectNameException { configFile = in; @@ -108,7 +109,22 @@ private void reloadConfig() { } } - private Config loadConfig(Map yamlConfig) throws MalformedObjectNameException { + private void maybeReloadConfig() { + if (configFile != null) { + long mtime = configFile.lastModified(); + if (mtime > config.lastUpdate) { + synchronized (this) { + // check again in case two threads try to reload the config at the same time + if (mtime > config.lastUpdate) { + LOGGER.fine("Configuration file changed, reloading..."); + reloadConfig(); + } + } + } + } + } + + private Config loadConfig(Map yamlConfig) throws MalformedObjectNameException { Config cfg = new Config(); if (yamlConfig == null) { // Yaml config empty, set config to empty map. @@ -225,6 +241,8 @@ private Config loadConfig(Map yamlConfig) throws MalformedObject cfg.rules.add(new Rule()); } + cfg.rulesCache = new MatchedRulesCache(cfg.rules); + return cfg; } @@ -292,13 +310,13 @@ class Receiver implements JmxScraper.MBeanReceiver { Map metricFamilySamplesMap = new HashMap(); - MatchedRulesCache cachedRules; + MatchedRulesCache rulesCache; MatchedRulesCache.StalenessTracker stalenessTracker; private static final char SEP = '_'; - Receiver(MatchedRulesCache cachedRules, MatchedRulesCache.StalenessTracker stalenessTracker) { - this.cachedRules = cachedRules; + Receiver(MatchedRulesCache rulesCache, MatchedRulesCache.StalenessTracker stalenessTracker) { + this.rulesCache = rulesCache; this.stalenessTracker = stalenessTracker; } @@ -322,7 +340,7 @@ void addSample(MetricFamilySamples.Sample sample, Type type, String help) { // if the rule is configured to be cached private void addToCache(final Rule rule, final String cacheKey, final MatchedRule matchedRule) { if (rule.cache) { - cachedRules.put(rule, cacheKey, matchedRule); + rulesCache.put(rule, cacheKey, matchedRule); stalenessTracker.add(rule, cacheKey); } } @@ -396,7 +414,7 @@ public void recordBean( String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName) + ": " + beanValue; if (rule.cache) { - MatchedRule cachedRule = cachedRules.get(rule, cacheKey); + MatchedRule cachedRule = rulesCache.get(rule, cacheKey); if (cachedRule != null) { stalenessTracker.add(rule, cacheKey); if (cachedRule.isMatched()) { @@ -506,26 +524,11 @@ public void recordBean( } public List collect() { - MatchedRulesCache cachedRules = this.cachedRules; - - if (configFile != null) { - long mtime = configFile.lastModified(); - if (mtime > config.lastUpdate) { - LOGGER.fine("Configuration file changed, reloading..."); - reloadConfig(); - synchronized (this) { - // rules may have changed with the configuration, clear the cache - // another thread may have changed the cache already, clear only if it hasn't been changed - if (cachedRules == this.cachedRules) { - this.cachedRules = new MatchedRulesCache(); - } - cachedRules = this.cachedRules; - } - } - } + maybeReloadConfig(); + MatchedRulesCache rulesCache = config.rulesCache; MatchedRulesCache.StalenessTracker stalenessTracker = new MatchedRulesCache.StalenessTracker(); - Receiver receiver = new Receiver(cachedRules, stalenessTracker); + Receiver receiver = new Receiver(rulesCache, stalenessTracker); JmxScraper scraper = new JmxScraper(config.jmxUrl, config.username, config.password, config.ssl, config.whitelistObjectNames, config.blacklistObjectNames, receiver, jmxMBeanPropertyCache); long start = System.nanoTime(); @@ -542,7 +545,7 @@ public List collect() { e.printStackTrace(new PrintWriter(sw)); LOGGER.severe("JMX scrape failed: " + sw.toString()); } - cachedRules.evictStaleEntries(stalenessTracker); + rulesCache.evictStaleEntries(stalenessTracker); List mfsList = new ArrayList(); mfsList.addAll(receiver.metricFamilySamplesMap.values()); diff --git a/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java b/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java index 2608b108..978a60e3 100644 --- a/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java +++ b/collector/src/main/java/io/prometheus/jmx/MatchedRulesCache.java @@ -1,5 +1,6 @@ package io.prometheus.jmx; +import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -14,32 +15,20 @@ public class MatchedRulesCache { private final Map> cachedRules; - public MatchedRulesCache() { - this.cachedRules = new ConcurrentHashMap>(); - } - - public void clear() { - cachedRules.clear(); + public MatchedRulesCache(Collection rules) { + this.cachedRules = new HashMap>(rules.size()); + for (JmxCollector.Rule rule : rules) { + this.cachedRules.put(rule, new ConcurrentHashMap()); + } } public void put(final JmxCollector.Rule rule, final String cacheKey, final MatchedRule matchedRule) { Map cachedRulesForRule = cachedRules.get(rule); - if (cachedRulesForRule == null) { - synchronized (cachedRules) { - cachedRulesForRule = cachedRules.get(rule); - if (cachedRulesForRule == null) { - cachedRulesForRule = new ConcurrentHashMap(); - cachedRules.put(rule, cachedRulesForRule); - } - } - } - cachedRulesForRule.put(cacheKey, matchedRule); } public MatchedRule get(final JmxCollector.Rule rule, final String cacheKey) { - Map cachedRulesForRule = cachedRules.get(rule); - return (cachedRulesForRule == null) ? null : cachedRulesForRule.get(cacheKey); + return cachedRules.get(rule).get(cacheKey); } // Remove stale rules (in the cache but not collected in the last run of the collector) @@ -53,10 +42,6 @@ public void evictStaleEntries(final StalenessTracker stalenessTracker) { cachedRulesForRule.remove(cacheKey); } } - - if (cachedRulesForRule.size() == 0) { - cachedRules.remove(rule); - } } } From 500a8a4c380423d14342d87c6e7a5d881541d49d Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Fri, 21 Aug 2020 11:04:28 +0100 Subject: [PATCH 15/19] Use the same Config reference in collect() To avoid potential race conditions where a scrape could start with a Config object and finish with another one. Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 222ab0fc..7ccc6599 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -310,13 +310,13 @@ class Receiver implements JmxScraper.MBeanReceiver { Map metricFamilySamplesMap = new HashMap(); - MatchedRulesCache rulesCache; + Config config; MatchedRulesCache.StalenessTracker stalenessTracker; private static final char SEP = '_'; - Receiver(MatchedRulesCache rulesCache, MatchedRulesCache.StalenessTracker stalenessTracker) { - this.rulesCache = rulesCache; + Receiver(Config config, MatchedRulesCache.StalenessTracker stalenessTracker) { + this.config = config; this.stalenessTracker = stalenessTracker; } @@ -340,7 +340,7 @@ void addSample(MetricFamilySamples.Sample sample, Type type, String help) { // if the rule is configured to be cached private void addToCache(final Rule rule, final String cacheKey, final MatchedRule matchedRule) { if (rule.cache) { - rulesCache.put(rule, cacheKey, matchedRule); + config.rulesCache.put(rule, cacheKey, matchedRule); stalenessTracker.add(rule, cacheKey); } } @@ -414,7 +414,7 @@ public void recordBean( String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName) + ": " + beanValue; if (rule.cache) { - MatchedRule cachedRule = rulesCache.get(rule, cacheKey); + MatchedRule cachedRule = config.rulesCache.get(rule, cacheKey); if (cachedRule != null) { stalenessTracker.add(rule, cacheKey); if (cachedRule.isMatched()) { @@ -525,10 +525,13 @@ public void recordBean( public List collect() { maybeReloadConfig(); - MatchedRulesCache rulesCache = config.rulesCache; + + // Take a reference to the current config and collect with this one + // (to avoid race conditions in case another thread reloads the config in the meantime) + Config config = this.config; MatchedRulesCache.StalenessTracker stalenessTracker = new MatchedRulesCache.StalenessTracker(); - Receiver receiver = new Receiver(rulesCache, stalenessTracker); + Receiver receiver = new Receiver(config, stalenessTracker); JmxScraper scraper = new JmxScraper(config.jmxUrl, config.username, config.password, config.ssl, config.whitelistObjectNames, config.blacklistObjectNames, receiver, jmxMBeanPropertyCache); long start = System.nanoTime(); @@ -545,7 +548,7 @@ public List collect() { e.printStackTrace(new PrintWriter(sw)); LOGGER.severe("JMX scrape failed: " + sw.toString()); } - rulesCache.evictStaleEntries(stalenessTracker); + config.rulesCache.evictStaleEntries(stalenessTracker); List mfsList = new ArrayList(); mfsList.addAll(receiver.metricFamilySamplesMap.values()); From 19179bd632802bf661f8965c973124444939c4f9 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Fri, 21 Aug 2020 11:12:39 +0100 Subject: [PATCH 16/19] Add dummy in the match name if the rule is cached Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 7ccc6599..1df492b0 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -410,13 +410,16 @@ public void recordBean( MatchedRule matchedRule = MatchedRule.unmatched(); for (Rule rule : config.rules) { - String cacheKey = beanName + attrName; - String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName) + ": " + beanValue; + // Rules with bean values cannot be properly cached (only the value from the first scrape will be cached). + // If caching for the rule is enabled, replace the value with a dummy to avoid caching different values at different times. + Object matchBeanValue = rule.cache ? "" : beanValue; + + String matchName = beanName + (rule.attrNameSnakeCase ? attrNameSnakeCase : attrName) + ": " + matchBeanValue; if (rule.cache) { - MatchedRule cachedRule = config.rulesCache.get(rule, cacheKey); + MatchedRule cachedRule = config.rulesCache.get(rule, matchName); if (cachedRule != null) { - stalenessTracker.add(rule, cacheKey); + stalenessTracker.add(rule, matchName); if (cachedRule.isMatched()) { matchedRule = cachedRule; break; @@ -432,7 +435,7 @@ public void recordBean( if (rule.pattern != null) { matcher = rule.pattern.matcher(matchName); if (!matcher.matches()) { - addToCache(rule, cacheKey, MatchedRule.unmatched()); + addToCache(rule, matchName, MatchedRule.unmatched()); continue; } } @@ -451,7 +454,7 @@ public void recordBean( // If there's no name provided, use default export format. if (rule.name == null) { matchedRule = defaultExport(matchName, domain, beanProperties, attrKeys, rule.attrNameSnakeCase ? attrNameSnakeCase : attrName, help, value, rule.valueFactor, rule.type); - addToCache(rule, cacheKey, matchedRule); + addToCache(rule, matchName, matchedRule); break; } @@ -494,7 +497,7 @@ public void recordBean( } matchedRule = new MatchedRule(name, matchName, rule.type, help, labelNames, labelValues, value, rule.valueFactor); - addToCache(rule, cacheKey, matchedRule); + addToCache(rule, matchName, matchedRule); break; } From c3a51a2412b5593142b169fd043e0dee6e8ddee1 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Fri, 21 Aug 2020 17:26:46 +0100 Subject: [PATCH 17/19] Avoid double-check on config update It is easier to understand and will not yield a big performance hit. Also make `maybeReloadConfig` return the new Config. Signed-off-by: Flavien Raynaud --- .../java/io/prometheus/jmx/JmxCollector.java | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 1df492b0..15133068 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -109,19 +109,18 @@ private void reloadConfig() { } } - private void maybeReloadConfig() { + private Config maybeReloadConfig() { if (configFile != null) { - long mtime = configFile.lastModified(); - if (mtime > config.lastUpdate) { - synchronized (this) { - // check again in case two threads try to reload the config at the same time - if (mtime > config.lastUpdate) { - LOGGER.fine("Configuration file changed, reloading..."); - reloadConfig(); - } + synchronized (this) { + long mtime = configFile.lastModified(); + if (mtime > config.lastUpdate) { + LOGGER.fine("Configuration file changed, reloading..."); + reloadConfig(); } } } + + return config; } private Config loadConfig(Map yamlConfig) throws MalformedObjectNameException { @@ -527,11 +526,9 @@ public void recordBean( } public List collect() { - maybeReloadConfig(); - // Take a reference to the current config and collect with this one // (to avoid race conditions in case another thread reloads the config in the meantime) - Config config = this.config; + Config config = maybeReloadConfig(); MatchedRulesCache.StalenessTracker stalenessTracker = new MatchedRulesCache.StalenessTracker(); Receiver receiver = new Receiver(config, stalenessTracker); From 3c9bb6eed4541919462a06bea213db0243d99cd1 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Mon, 24 Aug 2020 14:11:27 +0100 Subject: [PATCH 18/19] Synchronize getLastConfig Signed-off-by: Flavien Raynaud --- collector/src/main/java/io/prometheus/jmx/JmxCollector.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 15133068..0007849b 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -109,15 +109,13 @@ private void reloadConfig() { } } - private Config maybeReloadConfig() { + private synchronized Config getLastConfig() { if (configFile != null) { - synchronized (this) { long mtime = configFile.lastModified(); if (mtime > config.lastUpdate) { LOGGER.fine("Configuration file changed, reloading..."); reloadConfig(); } - } } return config; @@ -528,7 +526,7 @@ public void recordBean( public List collect() { // Take a reference to the current config and collect with this one // (to avoid race conditions in case another thread reloads the config in the meantime) - Config config = maybeReloadConfig(); + Config config = getLastConfig(); MatchedRulesCache.StalenessTracker stalenessTracker = new MatchedRulesCache.StalenessTracker(); Receiver receiver = new Receiver(config, stalenessTracker); From 36be5d761c2b17f0d6a7263d6367d0d95270a345 Mon Sep 17 00:00:00 2001 From: Flavien Raynaud Date: Tue, 25 Aug 2020 13:09:13 +0100 Subject: [PATCH 19/19] getLastConfig -> getLatestConfig Signed-off-by: Flavien Raynaud --- collector/src/main/java/io/prometheus/jmx/JmxCollector.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java index 0007849b..7c5c4d4d 100644 --- a/collector/src/main/java/io/prometheus/jmx/JmxCollector.java +++ b/collector/src/main/java/io/prometheus/jmx/JmxCollector.java @@ -109,7 +109,7 @@ private void reloadConfig() { } } - private synchronized Config getLastConfig() { + private synchronized Config getLatestConfig() { if (configFile != null) { long mtime = configFile.lastModified(); if (mtime > config.lastUpdate) { @@ -526,7 +526,7 @@ public void recordBean( public List collect() { // Take a reference to the current config and collect with this one // (to avoid race conditions in case another thread reloads the config in the meantime) - Config config = getLastConfig(); + Config config = getLatestConfig(); MatchedRulesCache.StalenessTracker stalenessTracker = new MatchedRulesCache.StalenessTracker(); Receiver receiver = new Receiver(config, stalenessTracker);