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

Support for exposing attributes as labels #996

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MaBiConti
Copy link

@MaBiConti MaBiConti commented Aug 28, 2024

The mbeans that we are working with have a lot of string-type attributes which are not converted into metrics by your project.
However, we need to expose the information offered by these string attributes to Prometheus, such that they can be used in queries, or be displayed in graphs.
The best way to do this (that we were able to find) is to expose these attributes as labels on the other metrics. We made this behavior configurable in the rules section of the yaml configuration file. Here is an example:

rules:
  - pattern: '{some mbean pattern}'
    name: some_metric
    attributesAsLabels:
       - firstStringAttribute
       - secondStringAttribute

Given an mbean having the attributes:

duration=123
firstStringAttribute="something"
secondStringAttribute="something else"

This produces a metric that looks like this:

some_metric_duration{firstStringAttribute="something",secondStringAttribute="something else"} 123

@MaBiConti MaBiConti force-pushed the attributes-as-labels branch from f2631f2 to d60a828 Compare August 28, 2024 15:53
@dhoard dhoard self-assigned this Aug 29, 2024
@dhoard
Copy link
Collaborator

dhoard commented Aug 29, 2024

@MaBiConti ...

  1. I want to discuss this with @fstab since adding pattern matching can negatively impact performance.
private Map<String, String> getAttributesAsLabelsWithValues(ObjectName mBeanName, Attribute attribute, Map<String, Object> attributeMap) {
// ... code omitted ...
}

2, If the functionality/PR is considered for inclusion, it requires both unit and integration tests. (I would hold off on implementation the tests pending the outcome regarding inclusion.)

@karina-calma
Copy link

Hi @dhoard, we noted your performance concern, and made a slight improvement by avoiding pattern matching if the feature is not used. Technically, if one does not explicitly configure attributesAsLabels, they should see no impact to performance.
However, when the feature is used, we need to match every single rule to every single metric. We noted that this matching is already performed in JmxCollector, but we could not find a way to easily reuse that without an extensive refactoring. If you have any suggestions in this direction, please advise.

@karina-calma
Copy link

Hi @dhoard,
Have you made a decision regarding this new functionality? Or do you need more time to consider it?
Considering that performance will not be impacted if the feature is not used, do you still think that that pattern matching is a problem?
We would appreciate an update when you have a chance.

@dhoard
Copy link
Collaborator

dhoard commented Oct 8, 2024

@karina-calma after discussing with the team, we have chosen not to implement this at this time due to the performance impact.

As a workaround, you can created and register your own collector to expose such metrics.

@karina-calma
Copy link

karina-calma commented Nov 18, 2024

Hi @dhoard ,

We would like to propose an alternative implementation for the functionality shown initially in order to expose to Prometheus the information offered by string-type attributes, which are not converted into metrics by your project. The new functionality would be enabled through a block of metricCustomizers in the yaml configuration file as follows:

startDelaySeconds: 0
jmxUrl: <jmxUrl>
metricCustomizers:
  - mbeanFilter:
       domain: '{domain}'
       properties:
         '{property1}': '{value1}'
    attributesAsLabels:
      - firstStringAttribute
      - secondStringAttribute
rules:
  - pattern: ".*"

Technically, if one does not explicitly configure metricCustomizers, they should see no impact to performance. When the feature is used, we will go through all the elements in the metricCustomizers block and perform string comparison (not pattern matching) on the domain and the (optional) properties with every MBean that is matched by the rules defined in the config file. For those metrics that match, we will add the elements from the attributesAsLabels block as attributes to the existing metrics.

@dhoard
Copy link
Collaborator

dhoard commented Nov 20, 2024

@karina-calma the method...

private Map<String, String> getAttributesAsLabelsWithValues(ObjectName mBeanName, Attribute attribute, Map<String, Object> attributeMap) {

... is doing regex pattern matching ...

                    Matcher matcher = rule.pattern.matcher(matchName);
                    if (matcher.matches()) {

... which will cause performance issues when configured.

Introducing core/built-in functionality (even if disabled by default) will be configured and used by end users, resulting in performance complaints.

@karina-calma karina-calma force-pushed the attributes-as-labels branch 2 times, most recently from 136d2dc to 3cef693 Compare November 20, 2024 10:41
@MaBiConti MaBiConti force-pushed the attributes-as-labels branch from 3cef693 to 9a6005c Compare November 20, 2024 10:59
Co-authored-by: Karina Calma <[email protected]>
Signed-off-by: Martin Bickel <[email protected]>
@MaBiConti MaBiConti force-pushed the attributes-as-labels branch from 9a6005c to 8b555ed Compare November 20, 2024 11:07
@karina-calma
Copy link

Hi @dhoard ,

We just updated the code, when you have time, please take another look.

@MaBiConti
Copy link
Author

Hi @dhoard ,
originally, we just want to get your feedback whether you would accept the approach. I consider a dedicated metricCustomizers configuration block to be not as elegant as a customization directly in the rule, but it has clear performance benefits.

And we did actually see the performance issues you were concerned about on our own system. That's why we went ahead and changed the implementation, which we just pushed.

This code is still missing the automatic tests, but we'd like to get your feedback on the approach first before writing the tests.

@dhoard dhoard changed the title Support for exposing atttributes as labels Support for exposing attributes as labels Dec 2, 2024
@dhoard
Copy link
Collaborator

dhoard commented Dec 8, 2024

@MaBiConti @karina-calma now that the 1.1.0 release is out I can review the PR more closely.

Can you provide a concrete example of why this is needed? What is the concrete use case? Example metrics?

@karina-calma
Copy link

Hi @dhoard,
We need to expose the information offered by string attributes to Prometheus, such that they can be used in queries, or be displayed in graphs in Grafana.

Here is an example:

startDelaySeconds: 0
jmxUrl: <jmxUrl>
metricCustomizers:
  - mbeanFilter:
       domain: '{domain}'
       properties:
         '{property1}': '{value1}'
    attributesAsLabels:
      - firstStringAttribute
      - secondStringAttribute

Given an mbean having the attributes:

duration=123
firstStringAttribute="something"
secondStringAttribute="something else"

This produces a metric that looks like this:

some_metric_duration{firstStringAttribute="something",secondStringAttribute="something else"} 123

@MaBiConti
Copy link
Author

We were instructed by our legal department not to mention any trademarked names.
So I can just say we need this to monitor a toolchain named after a musical style offered by a company nicknamed 'Big Blue' .

It is exposing mbeans providing statistics for different types of jobs. The job types are distinguished by a string attribute in their mbean, which is something we can't influence.

In order to report on these statistics by job type, we need to get the job name into Prometheus. And the only way to get such a string into Prometheus is by having it as a label - it then fits nicely into the data model to be a dimension of the metric.

I'm now making up some job names to illustrate the approach:

duration=123
jobType="consistency check"

and

duration=25000
jobType="backup"

To chart the duration of backup jobs in Grafana , we want to have "jobType" as a dimension of the duration metric, so we can filter on "backup" and see the duration of backups, or filter on "consistency checks" to see how the duration of that jobs developed over time.

@dhoard
Copy link
Collaborator

dhoard commented Dec 9, 2024

Can you provide an example of the current metrics output (with names tokenized/obfuscated)?

…utes-as-labels

# Conflicts:
#	collector/src/main/java/io/prometheus/jmx/JmxScraper.java
#	docs/README.md
@karina-calma
Copy link

Hi @dhoard,

Let's say we have an MBean called "JobMBean". This MBean has an attribute duration of type int, and an attribute jobType of type string.

When we use the official release of the jmx_exporter, this MBean produces one metric, from the duration attribute, which looks like this:
some_domain_job_duration{type="jobMetrics"} 123

This metric will only contain mbean properties as labels.

The jobType attribute of the MBean is completely ignored, and the jobType is never exported.

Through the metricCustomizers block that we are proposing, we instruct the jmx_exporter to use the jobType attribute of the MBean as a label of the duration metric, giving us an output that looks like this:

some_domain_job_duration{type="jobMetrics", jobType="backup"} 123

This new metric will contain, in addition to all existing mbean properties, a new list of attributes also added as labels.

@dhoard
Copy link
Collaborator

dhoard commented Dec 11, 2024

@karina-calma I understand the purpose of the change/what you are trying to do, but can you provide an example of the current metrics output (with names tokenized/obfuscated)?

@MaBiConti @karina-calma, my concern is the increase in memory usage (impact on garbage collection) and performance implications.

@MaBiConti
Copy link
Author

@dhoard : I'm sorry, but we don't really understand your request. Do you want to see the output of JMXExporter, showing the metrics of the application we are monitoring? Like how much heap it is currently using? And do you want to see them before filtering (using includeObjectNames ), or after?
Or do you want to see metrics of JMXConnector itself? As it happens, we do run it as a separate process.
And with current , do you mean the current official release (1.1.0), or the current state using this branch?

@dhoard
Copy link
Collaborator

dhoard commented Dec 11, 2024

@MaBiConti current text metric output (i.e.http://jmx_exporter:port/metrics)

@MaBiConti
Copy link
Author

@dhoard : here are the metrics: metrics.zip

Our attributes-as-labels function is visible in com_ibm_team_foundation_scenarios

For runtime and memory, there should be no impact when the feature is not used. And when it is used, its cost is linear to the number of customized metrics.

@dhoard
Copy link
Collaborator

dhoard commented Dec 16, 2024

@MaBiConti are these metrics without your PR? Can you provide the metrics output with and without the PR?

@MaBiConti
Copy link
Author

MaBiConti commented Dec 18, 2024

@dhoard here are the metrics with and without our change:
metrics.zip

This should be the configuration that is used:

metricCustomizers:
   - mbeanFilter:
        domain: com.ibm.team.foundation.activeservices
        properties:
           type: activeServicesMetrics
     attributesAsLabels:
        - serviceId
        - serviceMethod
        - userName
        - userId
        - startTime
        - name
        - threadName
   - mbeanFilter:
        domain: com.ibm.team.foundation.scenarios
        properties:
           type: expensiveScenarioDetailsMetrics
     attributesAsLabels:
        - scenarioName
        - scenarioId
        - userName
        - startTime
        - endTime
        - isActive

@dhoard
Copy link
Collaborator

dhoard commented Dec 20, 2024

@MaBiConti @karina-calma After some review/team discussion, I think this approach should be viable.

Can you rebase/update the PR and create JUnit 5/integration tests?

If you have any questions regarding the integration tests, please let me know.

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

Successfully merging this pull request may close these issues.

3 participants