jmx metrics: require explicit unit in yaml (open-telemetry#13796)

Co-authored-by: Jay DeLuca <[email protected]>
Co-authored-by: Lauri Tulmin <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Alix <[email protected]>
Co-authored-by: Steve Rao <[email protected]>
Co-authored-by: Gregor Zeitlinger <[email protected]>

Author: 7 people

instrumentation/jmx-metrics/README.md

@@ -287,6 +287,8 @@ In the particular case where only two values are defined, we can simplify furthe
             off: '*'
 ```
 
+State metrics do not have a unit (nor source unit) and use an empty string `""` as unit.
+
 ### Metric attributes modifiers
 
 JMX attributes values may require modification or normalization in order to fit semantic conventions.
@@ -382,21 +384,21 @@ rules:                                # start of list of configuration rules
       <ATTRIBUTE2>: beanattr(<ATTR>)  # <ATTR> is used as the MBean attribute name to extract the value
       <ATTRIBUTE3>: const(<CONST>)    # <CONST> is used as a constant
     prefix: <METRIC_NAME_PREFIX>      # optional, useful for avoiding specifying metric names below
-    unit: <UNIT>                      # optional, redefines the default unit for the whole rule
+    unit: <UNIT>                      # optional, defines the default unit for the whole rule. must be defined at metric level if not set here
     type: <TYPE>                      # optional, redefines the default type for the whole rule
     dropNegativeValues: <BOOL>        # optional, redefines if negative values are dropped for the whole rule
     mapping:
       <BEANATTR1>:                    # an MBean attribute name defining the metric value
         metric: <METRIC_NAME1>        # metric name will be <METRIC_NAME_PREFIX><METRIC_NAME1>
         type: <TYPE>                  # optional, the default type is gauge
         desc: <DESCRIPTION1>          # optional
-        unit: <UNIT1>                 # optional
+        unit: <UNIT1>                 # optional if unit is already defined on rule level (redefines it in such a case), required otherwise
         dropNegativeValues: <BOOL>    # optional, defines if negative values are dropped for the metric
         metricAttribute:              # optional, will be used in addition to the shared metric attributes above
           <ATTRIBUTE3>: const(<STR>)  # direct value for the metric attribute
       <BEANATTR2>:                    # use a.b to get access into CompositeData
         metric: <METRIC_NAME2>        # optional, the default is the MBean attribute name
-        unit: <UNIT2>                 # optional
+        unit: <UNIT2>                 # optional if unit is already defined on rule level (redefines it in such a case), required otherwise
       <BEANATTR3>:                    # metric name will be <METRIC_NAME_PREFIX><BEANATTR3>
       <BEANATTR4>:                    # metric name will be <METRIC_NAME_PREFIX><BEANATTR4>
   - beans:                            # alternatively, if multiple object names are needed

instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanGroup.java

@@ -6,7 +6,6 @@
 package io.opentelemetry.instrumentation.jmx.engine;
 
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import javax.annotation.Nullable;
 import javax.management.MalformedObjectNameException;
@@ -34,10 +33,6 @@ private BeanGroup(@Nullable QueryExp queryExp, List<ObjectName> namePatterns) {
     this.namePatterns = namePatterns;
   }
 
-  public static BeanGroup forSingleBean(String bean) throws MalformedObjectNameException {
-    return new BeanGroup(null, Collections.singletonList(new ObjectName(bean)));
-  }
-
   public static BeanGroup forBeans(List<String> beans) throws MalformedObjectNameException {
     List<ObjectName> list = new ArrayList<>();
     for (String name : beans) {

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

@@ -41,24 +41,22 @@ public class JmxRule extends MetricStructure {
   //     ATTRIBUTE3:
   //       METRIC_FIELDS3
   // The parser never calls setters for these fields with null arguments
-  @Nullable private String bean;
+
   private List<String> beans;
   @Nullable private String prefix;
   private Map<String, Metric> mapping;
 
-  @Nullable
-  public String getBean() {
-    return bean;
-  }
-
-  public void setBean(String bean) {
-    this.bean = validateBean(bean);
-  }
-
   public List<String> getBeans() {
     return beans;
   }
 
+  public void addBean(String bean) {
+    if (beans == null) {
+      beans = new ArrayList<>();
+    }
+    beans.add(validateBean(bean));
+  }
+
   private static String validateBean(String name) {
     try {
       String trimmed = name.trim();
@@ -70,14 +68,6 @@ private static String validateBean(String name) {
     }
   }
 
-  public void setBeans(List<String> beans) {
-    List<String> list = new ArrayList<>();
-    for (String name : beans) {
-      list.add(validateBean(name));
-    }
-    this.beans = list;
-  }
-
   public void setPrefix(String prefix) {
     this.prefix = validatePrefix(prefix.trim());
   }
@@ -128,13 +118,10 @@ private static Map<String, Metric> validateAttributeMapping(Map<String, Metric>
    */
   public MetricDef buildMetricDef() throws Exception {
     BeanGroup group;
-    if (bean != null) {
-      group = BeanGroup.forSingleBean(bean);
-    } else if (beans != null && !beans.isEmpty()) {
-      group = BeanGroup.forBeans(beans);
-    } else {
+    if (beans == null || beans.isEmpty()) {
       throw new IllegalStateException("No ObjectName specified");
     }
+    group = BeanGroup.forBeans(beans);
 
     if (mapping == null || mapping.isEmpty()) {
       throw new IllegalStateException("No MBean attributes specified");
@@ -157,9 +144,7 @@ public MetricDef buildMetricDef() throws Exception {
                 getUnit(),
                 getMetricType());
       } else {
-        metricInfo =
-            m.buildMetricInfo(
-                prefix, niceAttributeName, getSourceUnit(), getUnit(), getMetricType());
+        metricInfo = m.buildMetricInfo(niceAttributeName, this);
       }
 
       List<MetricAttribute> ownAttributes = getAttributeList();
@@ -216,13 +201,13 @@ private static List<MetricExtractor> createStateMappingExtractors(
       BeanAttributeExtractor extractor =
           BeanAttributeExtractor.forStateMetric(attrExtractor, key, stateMapping);
 
-      // state metric are always up/down counters
+      // state metric are always up/down counters, empty '' unit and no source unit
       MetricInfo stateMetricInfo =
           new MetricInfo(
               metricInfo.getMetricName(),
               metricInfo.getDescription(),
-              metricInfo.getSourceUnit(),
-              metricInfo.getUnit(),
+              null,
+              "",
               MetricInfo.Type.UPDOWNCOUNTER);
 
       extractors.add(new MetricExtractor(extractor, stateMetricInfo, stateMetricAttributes));

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

@@ -21,6 +21,8 @@ public class Metric extends MetricStructure {
   @Nullable private String metric;
   @Nullable private String desc;
 
+  public Metric() {}
+
   @Nullable
   public String getMetric() {
     return metric;
@@ -46,13 +48,16 @@ public void setDesc(String desc) {
     this.desc = desc.trim();
   }
 
-  MetricInfo buildMetricInfo(
-      @Nullable String prefix,
-      String attributeName,
-      String defaultSourceUnit,
-      String defaultUnit,
-      MetricInfo.Type defaultType) {
+  /**
+   * @param attributeName attribute name
+   * @param parentJmxRule parent JMX rule where metric is defined
+   * @return metric info
+   * @throws IllegalStateException when effective metric definition is invalid
+   */
+  MetricInfo buildMetricInfo(String attributeName, JmxRule parentJmxRule) {
     String metricName;
+
+    String prefix = parentJmxRule.getPrefix();
     if (metric == null) {
       metricName = prefix == null ? attributeName : (prefix + attributeName);
     } else {
@@ -61,17 +66,30 @@ MetricInfo buildMetricInfo(
 
     MetricInfo.Type metricType = getMetricType();
     if (metricType == null) {
-      metricType = defaultType;
+      metricType = parentJmxRule.getMetricType();
+    }
+    if (metricType == null) {
+      metricType = MetricInfo.Type.GAUGE;
     }
 
     String sourceUnit = getSourceUnit();
     if (sourceUnit == null) {
-      sourceUnit = defaultSourceUnit;
+      sourceUnit = parentJmxRule.getSourceUnit();
     }
 
-    String unit = getUnit();
-    if (unit == null) {
-      unit = defaultUnit;
+    String unit;
+    if (!getStateMapping().isEmpty()) {
+      // state metrics do not have a unit, use empty string
+      unit = "";
+    } else {
+      unit = getUnit();
+      if (unit == null) {
+        unit = parentJmxRule.getUnit();
+      }
+      if (unit == null) {
+        throw new IllegalStateException(
+            String.format("Metric unit is required for metric '%s'", metricName));
+      }
     }
 
     return new MetricInfo(metricName, desc, sourceUnit, unit, metricType);

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

@@ -24,7 +24,7 @@
  *
  *     <p>Known subclasses are {@link JmxRule} and {@link Metric}.
  */
-abstract class MetricStructure {
+public abstract class MetricStructure {
 
   // Used by the YAML parser
   //
@@ -47,8 +47,9 @@ abstract class MetricStructure {
   private Map<String, Object> metricAttribute;
   private StateMapping stateMapping = StateMapping.empty();
   private static final String STATE_MAPPING_DEFAULT = "*";
-  private String sourceUnit;
-  private String unit;
+
+  @Nullable private String sourceUnit;
+  @Nullable private String unit;
   @Nullable private Boolean dropNegativeValues;
 
   private MetricInfo.Type metricType;
@@ -59,9 +60,14 @@ abstract class MetricStructure {
   void setType(String t) {
     // Do not complain about case variations
     t = t.trim().toUpperCase(Locale.ROOT);
-    this.metricType = MetricInfo.Type.valueOf(t);
+    try {
+      this.metricType = MetricInfo.Type.valueOf(t);
+    } catch (IllegalArgumentException e) {
+      throw new IllegalArgumentException("Invalid metric type: " + t, e);
+    }
   }
 
+  @Nullable
   public String getSourceUnit() {
     return sourceUnit;
   }
@@ -70,12 +76,13 @@ public void setSourceUnit(String unit) {
     this.sourceUnit = validateUnit(unit.trim());
   }
 
+  @Nullable
   public String getUnit() {
     return unit;
   }
 
   public void setUnit(String unit) {
-    this.unit = validateUnit(unit.trim());
+    this.unit = unit.trim();
   }
 
   public void setDropNegativeValues(Boolean dropNegativeValues) {
@@ -167,7 +174,7 @@ static MetricAttribute buildMetricAttribute(String key, String target) {
     // an optional modifier may wrap the target
     // - lowercase(param(STRING))
     boolean lowercase = false;
-    String lowerCaseExpr = tryParseFunction("lowercase", targetExpr, target);
+    String lowerCaseExpr = tryParseFunction("lowercase", targetExpr, errorMsg);
     if (lowerCaseExpr != null) {
       lowercase = true;
       targetExpr = lowerCaseExpr;
@@ -183,20 +190,20 @@ static MetricAttribute buildMetricAttribute(String key, String target) {
 
     MetricAttributeExtractor extractor = null;
 
-    String paramName = tryParseFunction("param", targetExpr, target);
+    String paramName = tryParseFunction("param", targetExpr, errorMsg);
     if (paramName != null) {
       extractor = MetricAttributeExtractor.fromObjectNameParameter(paramName);
     }
 
     if (extractor == null) {
-      String attributeName = tryParseFunction("beanattr", targetExpr, target);
+      String attributeName = tryParseFunction("beanattr", targetExpr, errorMsg);
       if (attributeName != null) {
         extractor = MetricAttributeExtractor.fromBeanAttribute(attributeName);
       }
     }
 
     if (extractor == null) {
-      String constantValue = tryParseFunction("const", targetExpr, target);
+      String constantValue = tryParseFunction("const", targetExpr, errorMsg);
       if (constantValue != null) {
         extractor = MetricAttributeExtractor.fromConstant(constantValue);
       }

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

@@ -75,11 +75,11 @@ private static JmxRule parseJmxRule(Map<String, Object> ruleYaml) {
 
     String bean = (String) ruleYaml.remove("bean");
     if (bean != null) {
-      jmxRule.setBean(bean);
+      jmxRule.addBean(bean);
     }
     List<String> beans = (List<String>) ruleYaml.remove("beans");
     if (beans != null) {
-      jmxRule.setBeans(beans);
+      beans.forEach(jmxRule::addBean);
     }
     String prefix = (String) ruleYaml.remove("prefix");
     if (prefix != null) {
@@ -153,7 +153,7 @@ private static void parseMetricStructure(
 
   private static void failOnExtraKeys(Map<String, Object> yaml) {
     if (!yaml.isEmpty()) {
-      throw new IllegalArgumentException("Unrecognized keys found: " + yaml.keySet());
+      throw new IllegalArgumentException("Unrecognized key(s) found: " + yaml.keySet());
     }
   }