Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
vbence opened this issue Jan 2, 2024 · 7 comments
Closed

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

vbence opened this issue Jan 2, 2024 · 7 comments

Comments

@vbence
Copy link

vbence commented Jan 2, 2024

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
Copy link
Collaborator

dhoard commented Jan 23, 2024

@vbence Looking at the code...

(

if (rule.cache) {
MatchedRule cachedRule = config.rulesCache.get(rule, matchName);
if (cachedRule != null) {
stalenessTracker.add(rule, matchName);
if (cachedRule.isMatched()) {
matchedRule = cachedRule;
break;
}
// The bean was cached earlier, but did not match the current rule.
// Skip it to avoid matching against the same pattern again
continue;
}
}
)

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

Are you setting cache: true on the specific rules?

@zefir6
Copy link

zefir6 commented Aug 27, 2024

@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 dhoard self-assigned this Aug 29, 2024
@dhoard
Copy link
Collaborator

dhoard commented Aug 29, 2024

@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
Copy link
Collaborator

dhoard commented Sep 6, 2024

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

Have you tried this approach?

@vbence
Copy link
Author

vbence commented Sep 17, 2024

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
Copy link

zefir6 commented Sep 17, 2024

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
Copy link
Collaborator

dhoard commented Sep 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants