-
Notifications
You must be signed in to change notification settings - Fork 917
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
jmx metrics attribute lowercase modifier #13385
Changes from 3 commits
588f6d3
ac96916
66d676c
ced88cc
2b297e4
c1e8801
80cfcad
3ad0f04
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 |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [for reviewer] making this package-private makes unit-testing easier in |
||
|
||
// an optional modifier may wrap the target | ||
// - lowercase(param(STRING)) | ||
boolean lowercase = false; | ||
if (target.startsWith("lowercase(")) { | ||
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'm hesitating if it should be ''lowercase' or 'lowerCase'. 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 really don't have any strong opinion on this, there might be a few inconsistencies but keeping it in line with 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. |
||
lowercase = true; | ||
target = target.substring(10, target.lastIndexOf(')')); | ||
SylvainJuge marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// | ||
// The recognized forms of target are: | ||
// - param(STRING) | ||
// - beanattr(STRING) | ||
|
@@ -145,24 +156,32 @@ private static MetricAttribute buildMetricAttribute(String key, String target) { | |
// or the direct value to use | ||
int k = target.indexOf(')'); | ||
|
||
MetricAttributeExtractor extractor = null; | ||
|
||
// Check for one of the cases as above | ||
if (target.startsWith("param(")) { | ||
if (k > 0) { | ||
String jmxAttribute = target.substring(6, k).trim(); | ||
return new MetricAttribute( | ||
key, MetricAttributeExtractor.fromObjectNameParameter(jmxAttribute)); | ||
extractor = MetricAttributeExtractor.fromObjectNameParameter(jmxAttribute); | ||
} | ||
} else if (target.startsWith("beanattr(")) { | ||
if (k > 0) { | ||
String jmxAttribute = target.substring(9, k).trim(); | ||
return new MetricAttribute(key, MetricAttributeExtractor.fromBeanAttribute(jmxAttribute)); | ||
extractor = MetricAttributeExtractor.fromBeanAttribute(jmxAttribute); | ||
} | ||
} else if (target.startsWith("const(")) { | ||
if (k > 0) { | ||
String constantValue = target.substring(6, k).trim(); | ||
return new MetricAttribute(key, MetricAttributeExtractor.fromConstant(constantValue)); | ||
extractor = MetricAttributeExtractor.fromConstant(constantValue); | ||
} | ||
} | ||
if (extractor != null) { | ||
if (lowercase) { | ||
extractor = MetricAttributeExtractor.toLowerCase(extractor); | ||
} | ||
|
||
return new MetricAttribute(key, extractor); | ||
} | ||
|
||
String msg = "Invalid metric attribute specification for '" + key + "': " + target; | ||
throw new IllegalArgumentException(msg); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -379,7 +379,6 @@ void testConf8() throws Exception { | |
@Test | ||
void testStateMetricConf() throws Exception { | ||
JmxConfig config = parseConf(CONF9); | ||
assertThat(config).isNotNull(); | ||
|
||
List<JmxRule> rules = config.getRules(); | ||
assertThat(rules).hasSize(1); | ||
|
@@ -452,6 +451,39 @@ void testStateMetricConf() throws Exception { | |
}); | ||
} | ||
|
||
private static final String CONF10 = | ||
"--- # keep stupid spotlessJava at bay\n" | ||
+ "rules:\n" | ||
+ " - bean: my-test:type=10_Hello\n" | ||
+ " mapping:\n" | ||
+ " jmxAttribute:\n" | ||
+ " type: counter\n" | ||
+ " metric: my_metric\n" | ||
+ " metricAttribute:\n" | ||
+ " to_lower_const: lowercase(const(Hello))\n" | ||
+ " to_lower_attribute: lowercase(beanattr(beanAttribute))\n" | ||
+ " to_lower_param: lowercase(param(type))\n"; | ||
|
||
@Test | ||
void attributeValueLowercase() { | ||
|
||
JmxConfig config = parseConf(CONF10); | ||
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. [for reviewer] here we only test the parsing as we can rely on unit tests in 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. Yes, however it would be nice to add also some negative test to verify parsing errors related to use of lowercase(...) |
||
|
||
List<JmxRule> rules = config.getRules(); | ||
assertThat(rules).hasSize(1); | ||
JmxRule jmxRule = rules.get(0); | ||
|
||
assertThat(jmxRule.getBean()).isEqualTo("my-test:type=10_Hello"); | ||
Metric metric = jmxRule.getMapping().get("jmxAttribute"); | ||
assertThat(metric.getMetricType()).isEqualTo(MetricInfo.Type.COUNTER); | ||
assertThat(metric.getMetric()).isEqualTo("my_metric"); | ||
assertThat(metric.getMetricAttribute()) | ||
.hasSize(3) | ||
.containsEntry("to_lower_const", "lowercase(const(Hello))") | ||
.containsEntry("to_lower_attribute", "lowercase(beanattr(beanAttribute))") | ||
.containsEntry("to_lower_param", "lowercase(param(type))"); | ||
} | ||
|
||
@Test | ||
void testEmptyConf() { | ||
JmxConfig config = parseConf(EMPTY_CONF); | ||
|
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. It's great you added this test ! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.jmx.yaml; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
import io.opentelemetry.instrumentation.jmx.engine.MetricAttribute; | ||
import javax.management.MBeanAttributeInfo; | ||
import javax.management.MBeanInfo; | ||
import javax.management.MBeanServerConnection; | ||
import javax.management.ObjectName; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.CsvSource; | ||
|
||
public class MetricStructureTest { | ||
|
||
@ParameterizedTest | ||
@CsvSource({"const(Hello),Hello", "lowercase(const(Hello)),hello"}) | ||
void metricAttribute_constant(String target, String expectedValue) { | ||
MetricAttribute ma = MetricStructure.buildMetricAttribute("name", target); | ||
assertThat(ma.getAttributeName()).isEqualTo("name"); | ||
assertThat(ma.isStateAttribute()).isFalse(); | ||
assertThat(ma.acquireAttributeValue(null, null)).isEqualTo(expectedValue); | ||
} | ||
|
||
@ParameterizedTest | ||
@CsvSource({ | ||
"beanattr(beanAttribute),Hello,Hello", | ||
"lowercase(beanattr(beanAttribute)),Hello,hello", | ||
}) | ||
void metricAttribute_beanAttribute(String target, String value, String expectedValue) | ||
throws Exception { | ||
MetricAttribute ma = MetricStructure.buildMetricAttribute("name", target); | ||
assertThat(ma.getAttributeName()).isEqualTo("name"); | ||
assertThat(ma.isStateAttribute()).isFalse(); | ||
|
||
ObjectName objectName = new ObjectName("test:name=_beanAttribute"); | ||
MBeanServerConnection mockConnection = mock(MBeanServerConnection.class); | ||
|
||
MBeanInfo mockBeanInfo = mock(MBeanInfo.class); | ||
when(mockBeanInfo.getAttributes()) | ||
.thenReturn( | ||
new MBeanAttributeInfo[] { | ||
new MBeanAttributeInfo("beanAttribute", "java.lang.String", "", true, false, false) | ||
}); | ||
when(mockConnection.getMBeanInfo(objectName)).thenReturn(mockBeanInfo); | ||
when(mockConnection.getAttribute(objectName, "beanAttribute")).thenReturn(value); | ||
|
||
assertThat(ma.acquireAttributeValue(mockConnection, objectName)).isEqualTo(expectedValue); | ||
} | ||
|
||
@ParameterizedTest | ||
@CsvSource({ | ||
"param(name),Hello,Hello", | ||
"lowercase(param(name)),Hello,hello", | ||
}) | ||
void metricAttribute_beanParam(String target, String value, String expectedValue) | ||
throws Exception { | ||
MetricAttribute ma = MetricStructure.buildMetricAttribute("name", target); | ||
assertThat(ma.getAttributeName()).isEqualTo("name"); | ||
assertThat(ma.isStateAttribute()).isFalse(); | ||
|
||
ObjectName objectName = new ObjectName("test:name=" + value); | ||
MBeanServerConnection mockConnection = mock(MBeanServerConnection.class); | ||
|
||
assertThat(ma.acquireAttributeValue(mockConnection, objectName)).isEqualTo(expectedValue); | ||
} | ||
} |
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.
[for reviewer] extractor return value can be
null