Added transformation closure to MBeanHelper #960

akats7 · 2023-07-16T22:55:33Z

This enhancement enables non-numeric mbean attributes to be transformed into numeric values that can be consumed by the instrument callbacks. The MbeanHelper component has been updated to also accept a map of closures for the attributes being targeted.

Example:

def exampleMBean = otel.mbean(
  "example.mbean", ["CustomAttrFromString": { mbean -> (mbean.getProperty("Attribute") == "running") ? 1 : 0 }]
)
otel.instrument(exampleMBean, "my-metric", "CustomAttrFromString", otel.&longUpDownCounterCallback)

Existing Issue(s):

Resolves #901

Testing:

Added unit tests to MbeanHelperTest to include transformation scenarios

Documentation:

Updated README to include usage instructions

rmfitzpatrick · 2023-07-17T21:22:39Z

Do you think my suggestion of #901 (comment) would be more expressive for general use cases? The closures accept an mbean instead of an attribute value.

rmfitzpatrick · 2023-07-17T21:24:49Z

jmx-metrics/src/main/groovy/io/opentelemetry/contrib/jmxmetrics/MBeanHelper.groovy

@@ -106,7 +125,9 @@ class MBeanHelper {
        return [ofInterest, attributes].combinations().collect { pair ->
            def (bean, attribute) = pair
            try {
-                new Tuple3(bean, attribute, bean.getProperty(attribute))
+                def extractedAttribute = bean.getProperty(attribute)


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.

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?

akats7 · 2023-07-18T15:59:40Z

Do you think my suggestion of #901 (comment) would be more expressive for general use cases? The closures accept an mbean instead of an attribute value.

Hey @rmfitzpatrick, that's valid, I can make that adjustment.

akats7 · 2023-07-24T16:14:27Z

Hey @rmfitzpatrick, I addressed your comments and just added another test case for custom attributes.

cc. @breedx-splk @dehaansa @Mrod1598

jmx-metrics/src/test/java/io/opentelemetry/contrib/jmxmetrics/MBeanHelperTest.java

breedx-splk · 2023-07-24T23:27:22Z

jmx-metrics/src/main/groovy/io/opentelemetry/contrib/jmxmetrics/MBeanHelper.groovy

        return ofInterest.collect {
            try {
-                it.getProperty(attribute)
+                attributeTransformation.containsKey(attribute) ? attributeTransformation[attribute](it) : it.getProperty(attribute)


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)

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

breedx-splk

I like it. I think this is a worthy change that we can continue improving on if needed. Thanks!

dehaansa

Tiny naming nit, otherwise looks great.

dehaansa · 2023-07-29T04:45:44Z

jmx-metrics/src/main/groovy/io/opentelemetry/contrib/jmxmetrics/MBeanHelper.groovy

        }
    }

+    Object getBeanAttributeTransform(GroovyMBean bean, String attribute){


Should this be getBeanAttribute*or*Transform?

Yep fair point, I changed it to WithTransform since its kind of a proxy function

linux-foundation-easycla · 2023-07-31T15:40:31Z

The committers listed above are authorized under a signed CLA.

✅ login: akats7 / name: Alex Kats (c66c9bf, e1d3908, 643af9b, 964cea6, d77f5cb, 88d30ae, f886400, 4c96c83, bd615a1)

rmfitzpatrick · 2023-07-31T17:54:18Z

jmx-metrics/src/test/java/io/opentelemetry/contrib/jmxmetrics/MBeanHelperTest.java

+                        "{mbean -> mbean.getProperty(\"SomeAttribute\") == 'someValue' ? 'newValue' : 'someValue'}")
+                  },
+                  {
+                    "AnotherAttribute",


Do you mind updating this to try to get a nonexistent property for error handling vetting? I think it repeats existing added coverage.

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

I think the same error behavior makes sense and it could be helpful to add coverage to better enforce that moving forward.

Yeah thats a good point, I can create an additional PR for error handling the closure based args

Added transformation closure to MBeanHelper

c66c9bf

akats7 requested a review from a team July 16, 2023 22:55

github-actions bot assigned breedx-splk, dehaansa, Mrod1598 and rmfitzpatrick Jul 16, 2023

github-actions bot requested review from breedx-splk, dehaansa, Mrod1598 and rmfitzpatrick July 16, 2023 22:55

Updated format in README

e1d3908

rmfitzpatrick reviewed Jul 17, 2023

View reviewed changes

dehaansa added the component: jmx-metrics label Jul 19, 2023

akats7 added 4 commits July 20, 2023 18:31

Updated to accept mbean in closure

643af9b

Updated README

964cea6

spotless apply

d77f5cb

Added test case for custom attributes

88d30ae

breedx-splk reviewed Jul 24, 2023

View reviewed changes

jmx-metrics/src/test/java/io/opentelemetry/contrib/jmxmetrics/MBeanHelperTest.java Outdated Show resolved Hide resolved

breedx-splk reviewed Jul 24, 2023

View reviewed changes

breedx-splk approved these changes Jul 24, 2023

View reviewed changes

trask added this to the v1.29.0 milestone Jul 25, 2023

akats7 force-pushed the transform-attributes branch from 1b2cfe5 to 351c964 Compare July 28, 2023 20:46

Resolve collect in callback changes

f886400

akats7 force-pushed the transform-attributes branch from 168446c to f886400 Compare July 28, 2023 21:36

dehaansa approved these changes Jul 29, 2023

View reviewed changes

akats7 force-pushed the transform-attributes branch from da6e91c to f886400 Compare July 31, 2023 16:03

akats7 and others added 2 commits July 31, 2023 12:05

Updated function name

4c96c83

Merge branch 'main' into transform-attributes

bd615a1

rmfitzpatrick reviewed Jul 31, 2023

View reviewed changes

rmfitzpatrick approved these changes Jul 31, 2023

View reviewed changes

trask approved these changes Aug 4, 2023

View reviewed changes

trask merged commit aa0ca10 into open-telemetry:main Aug 4, 2023
13 checks passed

akats7 mentioned this pull request Nov 17, 2023

Add Error handling for closure parameters #1102

Merged

akats7 mentioned this pull request Jan 12, 2024

REQUEST: New membership for akats7 open-telemetry/community#1872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added transformation closure to MBeanHelper #960

Added transformation closure to MBeanHelper #960

akats7 commented Jul 16, 2023 •

edited

Loading

rmfitzpatrick commented Jul 17, 2023

rmfitzpatrick Jul 17, 2023 •

edited

Loading

akats7 Jul 18, 2023

akats7 commented Jul 18, 2023

akats7 commented Jul 24, 2023 •

edited

Loading

breedx-splk Jul 24, 2023

akats7 Jul 28, 2023

breedx-splk left a comment

dehaansa left a comment

dehaansa Jul 29, 2023

akats7 Jul 31, 2023

linux-foundation-easycla bot commented Jul 31, 2023 •

edited

Loading

rmfitzpatrick Jul 31, 2023

akats7 Jul 31, 2023

rmfitzpatrick Jul 31, 2023

akats7 Aug 4, 2023

Added transformation closure to MBeanHelper #960

Added transformation closure to MBeanHelper #960

Conversation

akats7 commented Jul 16, 2023 • edited Loading

rmfitzpatrick commented Jul 17, 2023

rmfitzpatrick Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akats7 commented Jul 18, 2023

akats7 commented Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breedx-splk left a comment

Choose a reason for hiding this comment

dehaansa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akats7 commented Jul 16, 2023 •

edited

Loading

rmfitzpatrick Jul 17, 2023 •

edited

Loading

akats7 commented Jul 24, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jul 31, 2023 •

edited

Loading