Allow caching regular expression matching in rules

[flavray]

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.

---

In our case, this lead to reducing collection time from ~70 sec to ~3 sec on
some of our applications (Kafka brokers). I opened a thread on the mailing list
a couple of weeks ago and haven't heard back, but definitely happy to discuss
and make changes if needed, if you think this is worth merging. 🙂

[brian-brazil]

From a quick peek, this has a memory leak as you never remove old cache entries.

Having the setting apply to the whole config also means that a single string mbean that you want to work with would prevent you from being able to use this, per-rule would make more sense.

[flavray]

Good catch, I've added some logic to only retain beans that have just been scraped in the cache.

Regarding the per-rule caching, I think it makes sense to try to have per-rule caching. However I'm not able to find a decent way to support all of: per-rule caching, caching "unmatched rules" and allowing bean value in patterns.

If we allow patterns with the bean value in them, caching a miss for a given value will lead to false negatives; on the other hand, not caching unmatched rules will limit the performance of the cache. I could try to "detect" rules that have a bean value in them and treat them differently, but that seems really prone to errors too.

[brian-brazil]

not caching unmatched rules will limit the performance of the cache.

This is what I'd suggest. If it matters to a user, the impact can be minimised by having those rules last.

[flavray]

not caching unmatched rules will limit the performance of the cache.

This is what I'd suggest. If it matters to a user, the impact can be minimised by having those rules last.

Are you suggesting having a catch-all rule at the end of the list of rules, that would effectively cache unmatched bean names (at the expense of actually exposing them)? If so, that sounds reasonable to me. If not, happy to hear more about the idea!

In the meantime, I had switched to having a per-rule flag to cache regular expression matching. The code looks quite different from before (with the introduction of MatchedRulesCache) and I am not entirely conviced with the approach, still around dealing with rule matching against bean value, which has a different behaviour now (albeit limited to the given rule rather than every rule). Happy to have more feedback on this.

---

Commit message

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).

[brian-brazil]

Are you suggesting having a catch-all rule at the end of the list of rules, that would effectively cache unmatched bean names (at the expense of actually exposing them)?

I'm not, that'd be potentially incorrect. I'm proposing that the setting be per-rule, and if this is a concern for the user that they put the non-cached rules last so that they're not applied to everything that has already been matched. There's also already a way to halt processing of a given attribute.

[flavray]

I'm not, that'd be potentially incorrect. I'm proposing that the setting be per-rule, and if this is a concern for the user that they put the non-cached rules last so that they're not applied to everything that has already been matched. There's also already a way to halt processing of a given attribute.

Gotcha. I've had another go at this, and removed the matchBeanValue flag.

[brian-brazil]

Keep in mind that scrapes can be concurrent.

[brian-brazil]

Thanks!

[flavray]

Awesome! Thank you for all the constructive feedback!

[amuraru]

@qinghui-xu thanks for this improvement. Would it make sense to add an example of how to use cache functionality in https://github.com/prometheus/jmx_exporter/blob/master/example_configs/kafka-2_0_0.yml ?

[brian-brazil]

PRs welcome to update the existing example configs to use this.