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

Add class name and class regex filters #277

Merged
merged 9 commits into from
Mar 3, 2020

Conversation

AlexandreYang
Copy link
Member

@AlexandreYang AlexandreYang commented Feb 25, 2020

What this PR does

Add class name and class regex filters

Motivation

Allow creating rules based on class name filters.

For example Kafka / Confluent Platform uses yammer metrics that provide different MBean classes like Gauge, Histogram, Timer, Meter. Since each class have different attributes and require specific rules, a filter by class can be helpful in targeting all metrics from a specific MBean class.

The classes are like follow:

- com.yammer.metrics.reporting.JmxReporter$Gauge
- com.yammer.metrics.reporting.JmxReporter$Histogram
- com.yammer.metrics.reporting.JmxReporter$Timer
- com.yammer.metrics.reporting.JmxReporter$Meter

That will allow creating rules tailored to each class:

jmx_metrics:
  - include:
      domain_regex: kafka\..*
      class_regex: .*$Gauge
      attribute:
        Value:
          alias: $domain.$type.$name
          metric_type: gauge
  - include:
      domain_regex: kafka\..*
      class_regex: .*$Histogram
      attribute:
        Count:
          alias: $domain.$type.$name.count
          metric_type: count
        Mean:
          alias: $domain.$type.$name.avg
          metric_type: gauge
        99thPercentile:
          alias: $domain.$type.$name.99percentile
          metric_type: gauge

@AlexandreYang AlexandreYang marked this pull request as ready for review March 1, 2020 12:54
@AlexandreYang AlexandreYang changed the title [DRAFT] Add class name filter Add class name and class regex filters Mar 1, 2020
Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me, fits in nicely with previous pattern matching code.

I think we can get rid of the Mbean empty class file. Let me know if what I mean is not clear.

@@ -0,0 +1,5 @@
package org.datadog.jmxfetch;

public class SimpleTestJavaApp2 extends SimpleTestJavaApp {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need this in a new file, I think it'll be better if you use an inner class in TestApp.java.

Copy link
Member Author

@AlexandreYang AlexandreYang Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@truthbk thx for the feedback.

Yes, the initial reason to use a new file was to have a simpler name to match. But it's it's probably fine to use an inner class too. Changed in my last commit.

Using a separate class:

class: org.datadog.jmxfetch.SimpleTestJavaApp2

Using a inner class:

class: org.datadog.jmxfetch.TestApp$1SimpleTestJavaAnotherApp

try {
log.debug("Getting class name for bean: " + beanName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self: I was going to say these new log lines perhaps didn't add much value, but since we're calling methods on connection we can probably trigger an IOException with these, so it's probably justified.

@@ -256,6 +256,61 @@ public void testDomainRegex() throws Exception {
assertEquals(15, metrics.size());
}

@Test
public void testClassInclude() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍰

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🚀

@AlexandreYang AlexandreYang merged commit 3485cb3 into DataDog:master Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants