post-review: test syntax errors

Author: SylvainJuge

instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java

@@ -138,13 +138,18 @@ private List<MetricAttribute> addMetricAttributes(Map<String, Object> metricAttr
 
   // package protected for testing
   static MetricAttribute buildMetricAttribute(String key, String target) {
+    String errorMsg =
+        String.format("Invalid metric attribute expression for '%s' : '%s'", key, target);
+
+    String targetExpr = target;
 
     // an optional modifier may wrap the target
     // - lowercase(param(STRING))
     boolean lowercase = false;
-    if (target.startsWith("lowercase(")) {
+    String lowerCaseExpr = tryParseFunction("lowercase", targetExpr, target);
+    if (lowerCaseExpr != null) {
       lowercase = true;
-      target = target.substring(10, target.lastIndexOf(')'));
+      targetExpr = lowerCaseExpr;
     }
 
     //
@@ -153,38 +158,62 @@ static MetricAttribute buildMetricAttribute(String key, String target) {
     //  - beanattr(STRING)
     //  - const(STRING)
     // where STRING is the name of the corresponding parameter key, attribute name,
-    // or the direct value to use
-    int k = target.indexOf(')');
+    // or the constant value to use
 
     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();
-        extractor = MetricAttributeExtractor.fromObjectNameParameter(jmxAttribute);
-      }
-    } else if (target.startsWith("beanattr(")) {
-      if (k > 0) {
-        String jmxAttribute = target.substring(9, k).trim();
-        extractor = MetricAttributeExtractor.fromBeanAttribute(jmxAttribute);
+    String paramName = tryParseFunction("param", targetExpr, target);
+    if (paramName != null) {
+      extractor = MetricAttributeExtractor.fromObjectNameParameter(paramName);
+    }
+
+    if (extractor == null) {
+      String attributeName = tryParseFunction("beanattr", targetExpr, target);
+      if (attributeName != null) {
+        extractor = MetricAttributeExtractor.fromBeanAttribute(attributeName);
       }
-    } else if (target.startsWith("const(")) {
-      if (k > 0) {
-        String constantValue = target.substring(6, k).trim();
+    }
+
+    if (extractor == null) {
+      String constantValue = tryParseFunction("const", targetExpr, target);
+      if (constantValue != null) {
         extractor = MetricAttributeExtractor.fromConstant(constantValue);
       }
     }
-    if (extractor != null) {
-      if (lowercase) {
-        extractor = MetricAttributeExtractor.toLowerCase(extractor);
-      }
 
-      return new MetricAttribute(key, extractor);
+    if (extractor == null) {
+      // expression did not match any supported syntax
+      throw new IllegalArgumentException(errorMsg);
+    }
+
+    if (lowercase) {
+      extractor = MetricAttributeExtractor.toLowerCase(extractor);
     }
+    return new MetricAttribute(key, extractor);
+  }
 
-    String msg = "Invalid metric attribute specification for '" + key + "': " + target;
-    throw new IllegalArgumentException(msg);
+  /**
+   * Parses a function expression for metric attributes
+   *
+   * @param function function name to attempt parsing from expression
+   * @param expression expression to parse
+   * @param errorMsg error message to use when syntax error is present
+   * @return {@literal null} if expression does not start with function
+   * @throws IllegalArgumentException if expression syntax is invalid
+   */
+  private static String tryParseFunction(String function, String expression, String errorMsg) {
+    if (!expression.startsWith(function)) {
+      return null;
+    }
+    String expr = expression.substring(function.length());
+    if (expr.charAt(0) != '(' || expr.charAt(expr.length() - 1) != ')') {
+      throw new IllegalArgumentException(errorMsg);
+    }
+    expr = expr.substring(1, expr.length() - 1);
+    if (expr.isEmpty()) {
+      throw new IllegalArgumentException(errorMsg);
+    }
+    return expr;
   }
 
   private MetricAttribute buildStateMetricAttribute(String key, Map<?, ?> stateMap) {

instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructureTest.java

@@ -6,6 +6,7 @@
 package io.opentelemetry.instrumentation.jmx.yaml;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -16,6 +17,7 @@
 import javax.management.ObjectName;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.ValueSource;
 
 public class MetricStructureTest {
 
@@ -70,4 +72,24 @@ void metricAttribute_beanParam(String target, String value, String expectedValue
 
     assertThat(ma.acquireAttributeValue(mockConnection, objectName)).isEqualTo(expectedValue);
   }
+
+  @ParameterizedTest
+  @ValueSource(
+      strings = {
+        "missing(name)", // non-existing target
+        "param()", // missing parameter
+        "param(name)a", // something after parenthesis
+        "lowercase()", // misng target in modifier
+        "lowercase(param(name)", // missing parenthesis for modifier
+        "lowercase(missing(name))", // non-existing target within modifier
+        "lowercase(param())", // missing parameter in modifier
+        "lowercase(param))", // missing parenthesis within modifier
+      })
+  void invalidTargetSyntax(String target) {
+    assertThatThrownBy(() -> MetricStructure.buildMetricAttribute("metric_attribute", target))
+        .isInstanceOf(IllegalArgumentException.class)
+        .describedAs(
+            "exception should be thrown with original expression to help end-user understand the syntax error")
+        .hasMessageContaining(target);
+  }
 }