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

Added transformation closure to MBeanHelper #960

Merged
merged 9 commits into from
Aug 4, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions jmx-metrics/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,26 @@ In cases where you'd like to share instrument names while creating datapoints fo
- `otel.instrument(MBeanHelper mBeanHelper, String name, String description, Map<String, Map<String, Closure>> attributeLabelFuncs, Closure instrument)` - `unit` is "1" and `labelFuncs` are empty map.
- `otel.instrument(MBeanHelper mBeanHelper, String name, Map<String, Map<String, Closure>> attributeLabelFuncs, Closure instrument)` - `description` is empty string, `unit` is "1" and `labelFuncs` are empty map

### MBeans with non-numeric attributes

In cases where you'd like to create metrics based on non-numeric MBean attributes, the mbean helper methods provide the ability to pass a map of closures, to transform the original extracted attribute into one that can be consumed by the instrument callbacks.

- `otel.mbean(String objectNameStr, Map<String,Closure<?>> attributeTransformation)`

- `otel.mbeans(String objectNameStr, Map<String,Closure<?>> attributeTransformation)`

- `otel.mbeans(List<String> objectNameStrs, Map<String,Closure<?>> attributeTransformation)`

These methods provide the ability to easily convert the attributes you will be extracting from the mbeans, at the time of creation for the MBeanHelper.

```groovy
// In this example a String based health attribute is converted to a numeric binary value
def someBean = otel.mbean(
"SomeMBean", ["CustomAttrFromString": { mbean -> mbean.getProperty("Attribute") == "running" ? 1 : 0 }]
)
otel.instrument(someBean, "my-metric", "CustomAttrFromString", otel.&longUpDownCounterCallback)
```

## OpenTelemetry Synchronous Instrument Helpers

- `otel.doubleCounter(String name, String description, String unit)`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,36 @@ class MBeanHelper {
private final JmxClient jmxClient
private final boolean isSingle
private final List<String> objectNames
private final Map<String, Closure> attributeTransformation

private List<GroovyMBean> mbeans

MBeanHelper(JmxClient jmxClient, String objectName, boolean isSingle) {
this.jmxClient = jmxClient
this.objectNames = Collections.unmodifiableList([objectName])
this.isSingle = isSingle
this.attributeTransformation = [:] as Map<String,Closure<?>>
}

MBeanHelper(JmxClient jmxClient, List<String> objectNames) {
this.jmxClient = jmxClient
this.objectNames = Collections.unmodifiableList(objectNames)
this.isSingle = false
this.attributeTransformation = [:] as Map<String,Closure<?>>
}

MBeanHelper(JmxClient jmxClient, String objectName, boolean isSingle, Map<String,Closure<?>> attributeTransformation ) {
this.jmxClient = jmxClient
this.objectNames = Collections.unmodifiableList([objectName])
this.isSingle = isSingle
this.attributeTransformation = attributeTransformation
}

MBeanHelper(JmxClient jmxClient, List<String> objectNames, Map<String,Closure<?>> attributeTransformation) {
this.jmxClient = jmxClient
this.objectNames = Collections.unmodifiableList(objectNames)
this.isSingle = false
this.attributeTransformation = attributeTransformation
}

@PackageScope static List<GroovyMBean> queryJmx(JmxClient jmxClient, String objNameStr) {
Expand Down Expand Up @@ -87,9 +104,10 @@ class MBeanHelper {
}

def ofInterest = isSingle ? [mbeans[0]]: mbeans

return ofInterest.collect {
try {
it.getProperty(attribute)
attributeTransformation.containsKey(attribute) ? attributeTransformation[attribute](it) : it.getProperty(attribute)
Copy link
Contributor

Choose a reason for hiding this comment

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

Small point -- I'm not sure about groovy, but in java calling containsKey() immediately followed by get() could (in early versions at least) incur two lookups. It's probably fine, and the size of the number of beans in our maps here is so small to render this moot....however, if you did factor out a method at least you could avoid the code duplication below. Something like getPropertyValue(bean, attribute)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @breedx-splk, yep I refactored this when resolving the conflict with the callback change

} catch (AttributeNotFoundException e) {
logger.warning("Expected attribute ${attribute} not found in mbean ${it.name()}")
null
Expand All @@ -106,7 +124,8 @@ class MBeanHelper {
return [ofInterest, attributes].combinations().collect { pair ->
def (bean, attribute) = pair
try {
new Tuple3(bean, attribute, bean.getProperty(attribute))
def extractedAttribute = attributeTransformation.containsKey(attribute) ? attributeTransformation[attribute](bean) : bean.getProperty(attribute)
new Tuple3(bean, attribute, extractedAttribute)
} catch (AttributeNotFoundException e) {
logger.info("Expected attribute ${attribute} not found in mbean ${bean.name()}")
new Tuple3(bean, attribute, null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,17 @@ class OtelHelper {
return mbeanHelper
}

MBeanHelper mbean(String objNameStr, Map<String,Closure<?>> attributeTransformation) {
def mbeanHelper = new MBeanHelper(jmxClient, objNameStr, true, attributeTransformation)
mbeanHelper.fetch()
return mbeanHelper
}

MBeanHelper mbeans(List<String> objNameStrs, Map<String,Closure<?>> attributeTransformation) {
def mbeanHelper = new MBeanHelper(jmxClient, objNameStrs, attributeTransformation)
mbeanHelper.fetch()
return mbeanHelper
}
/**
* Returns an updated @{link InstrumentHelper} associated with the provided {@link MBeanHelper} and its specified
* attribute value(s). The parameters map to the InstrumentHelper constructor.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<>();
Expand Down Expand Up @@ -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()));
Expand Down Expand Up @@ -116,6 +120,77 @@ 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(
"{mbean -> mbean.getProperty(\"SomeAttribute\") == '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(
"{mbean -> mbean.getProperty(\"SomeAttribute\") == 'someValue' ? 'newValue' : 'someValue'}")
},
{
"AnotherAttribute",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(
"{mbean -> mbean.getProperty(\"AnotherAttribute\") == '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()));
}

@Test
void customAttribute() throws Exception {
String thingName = "io.opentelemetry.contrib.jmxmetrics:type=custom";
Thing thing = new Thing("");
mbeanServer.registerMBean(thing, new ObjectName(thingName));
Map<String, Closure<?>> map =
Stream.of(
new Object[][] {
{"CustomAttribute", Eval.me("{mbean -> 'customValue'}")},
})
.collect(Collectors.toMap(data -> (String) data[0], data -> (Closure<?>) data[1]));
MBeanHelper mBeanHelper = new MBeanHelper(jmxClient, thingName, true, map);
mBeanHelper.fetch();

assertThat(mBeanHelper.getAttribute("CustomAttribute"))
.hasSameElementsAs(Stream.of(new String[] {"customValue"}).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));
Expand All @@ -127,19 +202,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;
}
}
}
Loading