jmx metrics attribute lowercase modifier #13385
Conversation
| @@ -136,7 +136,18 @@ private List<MetricAttribute> addMetricAttributes(Map<String, Object> metricAttr | |||
| return list; | |||
| } | |||
|
|
|||
| private static MetricAttribute buildMetricAttribute(String key, String target) { | |||
| // package protected for testing | |||
| static MetricAttribute buildMetricAttribute(String key, String target) { | |||
There was a problem hiding this comment.
[for reviewer] making this package-private makes unit-testing easier in MetricStructureTest (see below).
| @@ -30,7 +31,8 @@ public String getAttributeName() { | |||
| return name; | |||
| } | |||
|
|
|||
| String acquireAttributeValue(MBeanServerConnection connection, ObjectName objectName) { | |||
| @Nullable | |||
| public String acquireAttributeValue(MBeanServerConnection connection, ObjectName objectName) { | |||
There was a problem hiding this comment.
[for reviewer] extractor return value can be null
| @Test | ||
| void attributeValueLowercase() { | ||
|
|
||
| JmxConfig config = parseConf(CONF10); |
There was a problem hiding this comment.
[for reviewer] here we only test the parsing as we can rely on unit tests in MetricStructureTest to validate the expected value modifications.
There was a problem hiding this comment.
Yes, however it would be nice to add also some negative test to verify parsing errors related to use of lowercase(...)
...rary/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttributeExtractor.java
Outdated
Show resolved
Hide resolved
...rary/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttributeExtractor.java
Outdated
Show resolved
Hide resolved
| // an optional modifier may wrap the target | ||
| // - lowercase(param(STRING)) | ||
| boolean lowercase = false; | ||
| if (target.startsWith("lowercase(")) { |
There was a problem hiding this comment.
I'm hesitating if it should be ''lowercase' or 'lowerCase'.
Currently it looks like we have no consistent convention in the yaml file. We already have camelcase and lowercase naming used, for example metricAttribute and beanattr.
I understand that you followed existing attribute extractor yaml naming pattern, but this is inconsistent with the rest of yaml names and maybe this is one more item to discuss on the SIG meeting
There was a problem hiding this comment.
I really don't have any strong opinion on this, there might be a few inconsistencies but keeping it in line with param, beanattr and const should be a safe choice for now.
If we aim to fix those inconsistencies, I would suggest to do it in a separate PR and it would be relevant to also check what is the convention in the declarative configuration because at some point the JMX yaml configuration will be nested in it.
...metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
It's great you added this test !
| @Test | ||
| void attributeValueLowercase() { | ||
|
|
||
| JmxConfig config = parseConf(CONF10); |
There was a problem hiding this comment.
Yes, however it would be nice to add also some negative test to verify parsing errors related to use of lowercase(...)
…nstrumentation into jmx-metric-lowercase-attribute
…nstrumentation into jmx-metric-lowercase-attribute
| // an optional modifier may wrap the target | ||
| // - lowercase(param(STRING)) | ||
| boolean lowercase = false; | ||
| String lowerCaseExpr = tryParseFunction("lowercase", targetExpr, target); |
There was a problem hiding this comment.
I guess for now it doesn't matter, but if you plan to add more transforms then it would be beneficial to restructure the parsing logic so that lowercase(lowercase(const(Hello))) would be valid.
Fixes #13361
Adds the ability to use
lowercase(...)function to the metric attributes.Other modifiers might be added in the future but for now only the lower-case variant is needed for #13361.