-
Notifications
You must be signed in to change notification settings - Fork 124
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
Added transformation closure to MBeanHelper #960
Changes from 2 commits
c66c9bf
e1d3908
643af9b
964cea6
d77f5cb
88d30ae
f886400
4c96c83
bd615a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,13 +9,17 @@ | |
import static java.util.concurrent.TimeUnit.SECONDS; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
import groovy.lang.Closure; | ||
import groovy.util.Eval; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.HashSet; | ||
import java.util.Map; | ||
import java.util.Properties; | ||
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.IntStream; | ||
import java.util.stream.Stream; | ||
import javax.management.MBeanServer; | ||
import javax.management.ObjectInstance; | ||
import javax.management.ObjectName; | ||
|
@@ -32,6 +36,7 @@ | |
@Timeout(value = 10, unit = SECONDS) | ||
class MBeanHelperTest { | ||
|
||
// private static final Logger logger = Logger.getLogger(MBeanHelperTest.class.getName()); | ||
akats7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private static final MBeanServer mbeanServer = getPlatformMBeanServer(); | ||
|
||
private static final Set<ObjectInstance> registeredBeans = new HashSet<>(); | ||
|
@@ -81,7 +86,6 @@ void multiObj() throws Exception { | |
MBeanHelper mBeanHelper = | ||
new MBeanHelper(jmxClient, Arrays.asList(thingName + ",thing=0", thingName + ",thing=1")); | ||
mBeanHelper.fetch(); | ||
|
||
assertThat(mBeanHelper.getAttribute("SomeAttribute")) | ||
.hasSameElementsAs( | ||
IntStream.range(0, 2).mapToObj(Integer::toString).collect(Collectors.toList())); | ||
|
@@ -116,6 +120,57 @@ void multiple() throws Exception { | |
IntStream.range(0, 100).mapToObj(unused -> null).collect(Collectors.toList())); | ||
} | ||
|
||
@Test | ||
void transform() throws Exception { | ||
String thingName = "io.opentelemetry.contrib.jmxmetrics:type=transform"; | ||
Thing thing = new Thing("someValue"); | ||
mbeanServer.registerMBean(thing, new ObjectName(thingName)); | ||
Map<String, Closure<?>> map = | ||
Stream.of( | ||
new Object[][] { | ||
{ | ||
"SomeAttribute", | ||
Eval.me("{attribute -> attribute == 'someValue' ? 'otherValue' : 'someValue'}") | ||
}, | ||
}) | ||
.collect(Collectors.toMap(data -> (String) data[0], data -> (Closure<?>) data[1])); | ||
MBeanHelper mBeanHelper = new MBeanHelper(jmxClient, thingName + ",*", true, map); | ||
mBeanHelper.fetch(); | ||
|
||
assertThat(mBeanHelper.getAttribute("SomeAttribute")) | ||
.hasSameElementsAs(Stream.of(new String[] {"otherValue"}).collect(Collectors.toList())); | ||
} | ||
|
||
@Test | ||
void transformMultipleAttributes() throws Exception { | ||
String thingName = "io.opentelemetry.contrib.jmxmetrics:type=transformMultiple"; | ||
Thing thing1 = new Thing("someValue", "anotherValue"); | ||
ObjectName mbeanName = new ObjectName(thingName); | ||
mbeanServer.registerMBean(thing1, mbeanName); | ||
Map<String, Closure<?>> map = | ||
Stream.of( | ||
new Object[][] { | ||
{ | ||
"SomeAttribute", | ||
Eval.me("{attribute -> attribute == 'someValue' ? 'newValue' : 'someValue'}") | ||
}, | ||
{ | ||
"AnotherAttribute", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind updating this to try to get a nonexistent property for error handling vetting? I think it repeats existing added coverage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In its current state I think that'll throw an AttributeNotFound exception, do we want to handle that error in the same manner as if a nonexistent property was passed to an instrument helper? I just followed the pattern of labelFuncs and the other closure params where it doesn't look like theres any additional error handling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the same error behavior makes sense and it could be helpful to add coverage to better enforce that moving forward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah thats a good point, I can create an additional PR for error handling the closure based args |
||
Eval.me( | ||
"{attribute -> attribute == 'anotherValue' ? 'anotherNewValue' : 'anotherValue'}") | ||
}, | ||
}) | ||
.collect(Collectors.toMap(data -> (String) data[0], data -> (Closure<?>) data[1])); | ||
MBeanHelper mBeanHelper = new MBeanHelper(jmxClient, thingName + ",*", true, map); | ||
mBeanHelper.fetch(); | ||
|
||
assertThat(mBeanHelper.getAttribute("SomeAttribute")) | ||
.hasSameElementsAs(Stream.of(new String[] {"newValue"}).collect(Collectors.toList())); | ||
assertThat(mBeanHelper.getAttribute("AnotherAttribute")) | ||
.hasSameElementsAs( | ||
Stream.of(new String[] {"anotherNewValue"}).collect(Collectors.toList())); | ||
} | ||
|
||
private static void registerThings(String thingName) throws Exception { | ||
for (int i = 0; i < 100; i++) { | ||
Thing thing = new Thing(Integer.toString(i)); | ||
|
@@ -127,19 +182,34 @@ private static void registerThings(String thingName) throws Exception { | |
public interface ThingMBean { | ||
|
||
String getSomeAttribute(); | ||
|
||
String getAnotherAttribute(); | ||
} | ||
|
||
static class Thing implements ThingMBean { | ||
|
||
private final String attrValue; | ||
private final String attrValue1; | ||
|
||
private final String attrValue2; | ||
|
||
Thing(String attrValue) { | ||
this.attrValue = attrValue; | ||
this.attrValue1 = attrValue; | ||
this.attrValue2 = ""; | ||
} | ||
|
||
Thing(String attrValue1, String attrValue2) { | ||
this.attrValue1 = attrValue1; | ||
this.attrValue2 = attrValue2; | ||
} | ||
|
||
@Override | ||
public String getSomeAttribute() { | ||
return attrValue; | ||
return attrValue1; | ||
} | ||
|
||
@Override | ||
public String getAnotherAttribute() { | ||
return attrValue2; | ||
} | ||
} | ||
} |
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.
I would expect this to throw the AttributeNotFoundException* for custom ones. As is this would require all passed maps to be value-ovewriting processors instead of allowing a general computed value capability.
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.
Right, I guest when I first conceptualized this I only thought about it through the lens of transforming existing attributes. Do you think it would be good to allow the instrument helpers to target attributes that don't actually exist on the mbeans?