prometheus / jmx_exporter

A process for exposing JMX Beans via HTTP for Prometheus consumption
Apache License 2.0
3.02k stars 1.2k forks source link

Positive match to a rule should not consume the metric #901

Open vbence opened 9 months ago

vbence commented 9 months ago

The exporter's current behavior is that the first matching rule consumes the metric which will not be visible to subsequent matches. In the example below only the first rule will display any metrics from MyLegacyMetricsBean.

# The old way when I had a single class
- pattern: 'com.example.myproduct.metrics<type=MyLegacyMetricsBean><>([^:]+)'
  name: myproduct_$1
  ...
# The new way with multiple classes
- pattern: 'com.example.myproduct.metrics<type=(.*)><>([^:]+)'
  name: myproduct_$1_$2
  ...

I am doing a refactoring of my metrics and I would like to support the old and the new metrics for the foreseeable future. These overlapping rules would provide backward compatibility while also exposing the legacy metrics on the new naming convention.

I recommend a property settable on the rule level to override the current behavior (e.g. consume which defaults to true).

dhoard commented 8 months ago

@vbence Looking at the code...

(https://github.com/prometheus/jmx_exporter/blob/9bc7c4343975943696f409843ef966fa2d1b7b48/collector/src/main/java/io/prometheus/jmx/JmxCollector.java#L589-L602)

... if cache is false (default) it looks like it should work as you expect.

Are you setting cache: true on the specific rules?

zefir6 commented 1 month ago
@dhoard I am seeing the same behavior (and would find option to not consume metric useful), and I don't have cache set anywhere in my configs. Also docs state that this is currently default behavior: 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.
dhoard commented 1 month ago

@zefir6 I have concerns that this will negatively impact performance for a large set of rules since this will run through all rules for all MBeans.

I feel changing the exporter behavior, potentially in a way that would negatively impact performance, to prevent dashboards changes encourages technical debt.

I will discuss this with @fstab.

dhoard commented 1 month ago

@vbence @zefir6 Prometheus recording rules should allow you to use the new metric, and generate the old metric.

Have you tried this approach?

vbence commented 2 weeks ago

Thanks @dhoard , that is definitely a workaround, but we not always have access to bespoke Prometheus rules, this feature would allow to solve this issue close to where it arises.

zefir6 commented 2 weeks ago

The same here @dhoard, as @vbence wrote, myself I have no much of influence over config of organisation-wide prometheus, so it may be hard for me. It would be useful to be able to do that on exporter. If its impossible, its impossible, but it would be nice to have this option.

dhoard commented 2 weeks ago

@vbence @zefir6 it's less about "can it be implemented" but more around "should it be implemented."

This feature will dramatically affect performance, introduce technical debt, and cause support burden by misuse.

IMHO, the real solution is to change rules and fix dashboards.

I am going defer adding this at this time.