-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add class name and class regex filters #277
Conversation
cd045d7
to
3e9207d
Compare
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍰
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🚀
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:
That will allow creating rules tailored to each class: