From 8d93dbfe6d5e5cd752692b47062e9a4bb6546526 Mon Sep 17 00:00:00 2001 From: robsunday Date: Tue, 4 Mar 2025 10:41:13 +0100 Subject: [PATCH 01/17] Added unit conversion support for YAML --- .../instrumentation/jmx/engine/MetricDef.java | 2 + .../jmx/engine/MetricInfo.java | 14 +++++- .../jmx/engine/MetricRegistrar.java | 44 ++++++++++++++----- .../instrumentation/jmx/yaml/JmxRule.java | 2 + .../instrumentation/jmx/yaml/Metric.java | 4 +- .../jmx/yaml/MetricStructure.java | 9 ++++ .../instrumentation/jmx/yaml/RuleParser.java | 4 ++ .../jmx/engine/RuleParserTest.java | 3 ++ 8 files changed, 69 insertions(+), 13 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricDef.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricDef.java index 3feb99e4741a..c2a42ae49194 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricDef.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricDef.java @@ -42,12 +42,14 @@ // new MetricInfo( // "my.own.jvm.memory.pool.used", // "Pool memory currently used", +// null, // "By", // MetricInfo.Type.UPDOWNCOUNTER); // MetricInfo poolLimitInfo = // new MetricInfo( // "my.own.jvm.memory.pool.limit", // "Maximum obtainable memory pool size", +// null, // "By", // MetricInfo.Type.UPDOWNCOUNTER); // diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java index 9cb35ae53b15..f4e71f62d1af 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java @@ -27,6 +27,7 @@ public enum Type { // How to report the metric using OpenTelemetry API private final String metricName; // used as Instrument name @Nullable private final String description; + @Nullable private final String sourceUnit; @Nullable private final String unit; private final Type type; @@ -35,13 +36,19 @@ public enum Type { * * @param metricName a String that will be used as a metric name, it should be unique * @param description a human readable description of the metric + * @param sourceUnit a human readable unit of measurement that is received from metric source * @param unit a human readable unit of measurement * @param type the instrument typ to be used for the metric */ public MetricInfo( - String metricName, @Nullable String description, String unit, @Nullable Type type) { + String metricName, + @Nullable String description, + @Nullable String sourceUnit, + @Nullable String unit, + @Nullable Type type) { this.metricName = metricName; this.description = description; + this.sourceUnit = sourceUnit; this.unit = unit; this.type = type == null ? Type.GAUGE : type; } @@ -55,6 +62,11 @@ public String getDescription() { return description; } + @Nullable + public String getSourceUnit() { + return sourceUnit; + } + @Nullable public String getUnit() { return unit; diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java index 482b1e99f3b7..e1d97849030f 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java @@ -16,10 +16,13 @@ import io.opentelemetry.api.metrics.Meter; import io.opentelemetry.api.metrics.ObservableDoubleMeasurement; import io.opentelemetry.api.metrics.ObservableLongMeasurement; +import io.opentelemetry.instrumentation.jmx.engine.unit.UnitConverter; +import io.opentelemetry.instrumentation.jmx.engine.unit.UnitConverterFactory; import java.util.Collection; import java.util.Optional; import java.util.function.Consumer; import java.util.logging.Logger; +import javax.annotation.Nullable; import javax.management.MBeanServerConnection; import javax.management.ObjectName; @@ -61,6 +64,7 @@ void enrollExtractor( return; } + boolean recordDoubleValue = attributeInfo.usesDoubleValues(); MetricInfo metricInfo = extractor.getInfo(); String metricName = metricInfo.getMetricName(); MetricInfo.Type instrumentType = metricInfo.getType(); @@ -69,6 +73,12 @@ void enrollExtractor( ? metricInfo.getDescription() : attributeInfo.getDescription(); String unit = metricInfo.getUnit(); + String sourceUnit = metricInfo.getSourceUnit(); + + UnitConverter unitConverter = UnitConverterFactory.getConverter(sourceUnit, unit); + if (unitConverter != null) { + recordDoubleValue = unitConverter.isConvertingToDouble(); + } switch (instrumentType) { // CHECKSTYLE:OFF @@ -79,10 +89,10 @@ void enrollExtractor( Optional.ofNullable(description).ifPresent(builder::setDescription); Optional.ofNullable(unit).ifPresent(builder::setUnit); - if (attributeInfo.usesDoubleValues()) { - builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor)); + if (recordDoubleValue) { + builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor, unitConverter)); } else { - builder.buildWithCallback(longTypeCallback(extractor)); + builder.buildWithCallback(longTypeCallback(extractor, unitConverter)); } logger.log(INFO, "Created Counter for {0}", metricName); } @@ -96,10 +106,10 @@ void enrollExtractor( Optional.ofNullable(description).ifPresent(builder::setDescription); Optional.ofNullable(unit).ifPresent(builder::setUnit); - if (attributeInfo.usesDoubleValues()) { - builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor)); + if (recordDoubleValue) { + builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor, unitConverter)); } else { - builder.buildWithCallback(longTypeCallback(extractor)); + builder.buildWithCallback(longTypeCallback(extractor, unitConverter)); } logger.log(INFO, "Created UpDownCounter for {0}", metricName); } @@ -113,10 +123,10 @@ void enrollExtractor( Optional.ofNullable(description).ifPresent(builder::setDescription); Optional.ofNullable(unit).ifPresent(builder::setUnit); - if (attributeInfo.usesDoubleValues()) { - builder.buildWithCallback(doubleTypeCallback(extractor)); + if (recordDoubleValue) { + builder.buildWithCallback(doubleTypeCallback(extractor, unitConverter)); } else { - builder.ofLongs().buildWithCallback(longTypeCallback(extractor)); + builder.ofLongs().buildWithCallback(longTypeCallback(extractor, unitConverter)); } logger.log(INFO, "Created Gauge for {0}", metricName); } @@ -133,8 +143,10 @@ void enrollExtractor( /* * A method generating metric collection callback for asynchronous Measurement * of Double type. + * If unit converter is provided then conversion is applied before metric is recorded. */ - static Consumer doubleTypeCallback(MetricExtractor extractor) { + static Consumer doubleTypeCallback( + MetricExtractor extractor, @Nullable UnitConverter unitConverter) { return measurement -> { DetectionStatus status = extractor.getStatus(); if (status != null) { @@ -145,6 +157,10 @@ static Consumer doubleTypeCallback(MetricExtractor if (metricValue != null) { // get the metric attributes Attributes attr = createMetricAttributes(connection, objectName, extractor); + + if (unitConverter != null) { + metricValue = unitConverter.convert(metricValue); + } measurement.record(metricValue.doubleValue(), attr); } } @@ -155,8 +171,10 @@ static Consumer doubleTypeCallback(MetricExtractor /* * A method generating metric collection callback for asynchronous Measurement * of Long type. + * If unit converter is provided then conversion is applied before metric is recorded. */ - static Consumer longTypeCallback(MetricExtractor extractor) { + static Consumer longTypeCallback( + MetricExtractor extractor, @Nullable UnitConverter unitConverter) { return measurement -> { DetectionStatus status = extractor.getStatus(); if (status != null) { @@ -167,6 +185,10 @@ static Consumer longTypeCallback(MetricExtractor extr if (metricValue != null) { // get the metric attributes Attributes attr = createMetricAttributes(connection, objectName, extractor); + + if (unitConverter != null) { + metricValue = unitConverter.convert(metricValue); + } measurement.record(metricValue.longValue(), attr); } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java index ab09fe3f18f9..02c496d0c396 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java @@ -152,6 +152,7 @@ public MetricDef buildMetricDef() throws Exception { new MetricInfo( prefix == null ? niceAttributeName : (prefix + niceAttributeName), null, + getSourceUnit(), getUnit(), getMetricType()); } else { @@ -226,6 +227,7 @@ protected Number extractNumericalAttribute( new MetricInfo( metricInfo.getMetricName(), metricInfo.getDescription(), + metricInfo.getSourceUnit(), metricInfo.getUnit(), MetricInfo.Type.UPDOWNCOUNTER); diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java index 77c313fb28d4..8e8bac157e73 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java @@ -61,11 +61,13 @@ MetricInfo buildMetricInfo( metricType = defaultType; } + String sourceUnit = getSourceUnit(); + String unit = getUnit(); if (unit == null) { unit = defaultUnit; } - return new MetricInfo(metricName, desc, unit, metricType); + return new MetricInfo(metricName, desc, sourceUnit, unit, metricType); } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java index 9967ac080c73..a6249f74d227 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java @@ -45,6 +45,7 @@ abstract class MetricStructure { private Map metricAttribute; private StateMapping stateMapping = StateMapping.empty(); private static final String STATE_MAPPING_DEFAULT = "*"; + private String sourcetUnit; private String unit; private MetricInfo.Type metricType; @@ -58,6 +59,14 @@ void setType(String t) { this.metricType = MetricInfo.Type.valueOf(t); } + public String getSourceUnit() { + return sourcetUnit; + } + + public void setSourceUnit(String unit) { + this.sourcetUnit = validateUnit(unit.trim()); + } + public String getUnit() { return unit; } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/RuleParser.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/RuleParser.java index c4da81c7337d..d7042b80d0a6 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/RuleParser.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/RuleParser.java @@ -142,6 +142,10 @@ private static void parseMetricStructure( if (unit != null) { out.setUnit(unit); } + String sourceUnit = (String) metricStructureYaml.remove("sourceUnit"); + if (sourceUnit != null) { + out.setSourceUnit(sourceUnit); + } } private static void failOnExtraKeys(Map yaml) { diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java index 15b10df7cba9..b359eab2c614 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java @@ -63,6 +63,7 @@ static void setup() { + " ATTRIBUTE2:\n" + " metric: METRIC_NAME2\n" + " desc: DESCRIPTION2\n" + + " sourceUnit: SOURCE_UNIT2\n" + " unit: UNIT2\n" + " ATTRIBUTE3:\n" + " ATTRIBUTE4:\n" @@ -95,6 +96,7 @@ void testConf2() { assertThat(m1).isNotNull(); assertThat(m1.getMetric()).isEqualTo("METRIC_NAME1"); assertThat(m1.getMetricType()).isEqualTo(MetricInfo.Type.GAUGE); + assertThat(m1.getSourceUnit()).isNull(); assertThat(m1.getUnit()).isEqualTo("UNIT1"); assertThat(m1.getMetricAttribute()).containsExactly(entry("LABEL_KEY3", "const(CONSTANT)")); @@ -102,6 +104,7 @@ void testConf2() { assertThat(m2).isNotNull(); assertThat(m2.getMetric()).isEqualTo("METRIC_NAME2"); assertThat(m2.getDesc()).isEqualTo("DESCRIPTION2"); + assertThat(m2.getSourceUnit()).isEqualTo("SOURCE_UNIT2"); assertThat(m2.getUnit()).isEqualTo("UNIT2"); JmxRule def2 = defs.get(1); From b7aaa5219408459e8b0780dbe91cf8280930709d Mon Sep 17 00:00:00 2001 From: robsunday Date: Tue, 4 Mar 2025 10:47:07 +0100 Subject: [PATCH 02/17] Fixed few warnings --- .../java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java | 2 ++ .../java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java | 2 ++ 2 files changed, 4 insertions(+) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java index 02c496d0c396..ef575273fb1a 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java @@ -47,6 +47,7 @@ public class JmxRule extends MetricStructure { @Nullable private String prefix; private Map mapping; + @Nullable public String getBean() { return bean; } @@ -90,6 +91,7 @@ private String validatePrefix(String prefix) { return prefix; } + @Nullable public String getPrefix() { return prefix; } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java index 8e8bac157e73..b21cf155e7db 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java @@ -21,6 +21,7 @@ public class Metric extends MetricStructure { @Nullable private String metric; @Nullable private String desc; + @Nullable public String getMetric() { return metric; } @@ -35,6 +36,7 @@ private String validateMetricName(String name) { return name; } + @Nullable public String getDesc() { return desc; } From ad56046e2b688f4646fe82cf5654464b4ce575ea Mon Sep 17 00:00:00 2001 From: robsunday Date: Tue, 4 Mar 2025 12:16:29 +0100 Subject: [PATCH 03/17] Missing files added --- .../jmx/engine/unit/UnitConverter.java | 34 ++++++++ .../jmx/engine/unit/UnitConverterFactory.java | 52 ++++++++++++ .../engine/unit/UnitConverterFactoryTest.java | 79 +++++++++++++++++++ 3 files changed, 165 insertions(+) create mode 100644 instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverter.java create mode 100644 instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java create mode 100644 instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverter.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverter.java new file mode 100644 index 000000000000..3ca7a0407991 --- /dev/null +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverter.java @@ -0,0 +1,34 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.jmx.engine.unit; + +import java.util.function.Function; + +/** This class is responsible for converting a value using provided algorithm. */ +public class UnitConverter { + private final Function convertingFunction; + private final boolean convertToDouble; + + /** + * Create an instance of converter + * + * @param convertingFunction an algorithm applied when converting value + * @param convertToDouble indicates of algorithm will return floating point result. This must be + * in-sync with algorithm implementation. + */ + UnitConverter(Function convertingFunction, boolean convertToDouble) { + this.convertingFunction = convertingFunction; + this.convertToDouble = convertToDouble; + } + + public Number convert(Number value) { + return convertingFunction.apply(value); + } + + public boolean isConvertingToDouble() { + return convertToDouble; + } +} diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java new file mode 100644 index 000000000000..31776b2f207c --- /dev/null +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java @@ -0,0 +1,52 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.jmx.engine.unit; + +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.TimeUnit; +import java.util.function.Function; +import javax.annotation.Nullable; + +public class UnitConverterFactory { + + private static final Map> conversionMappings = new HashMap<>(); + + static { + registerConverter("ms", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMillis(1), true); + registerConverter("ns", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toNanos(1), true); + } + + private UnitConverterFactory() {} + + public static UnitConverter getConverter(@Nullable String fromUnit, @Nullable String toUnit) { + if (fromUnit == null || toUnit == null) { + return null; + } + + Map converters = conversionMappings.get(fromUnit); + if (converters == null) { + return null; + } + + return converters.get(toUnit); + } + + public static void registerConverter( + String sourceUnit, + String targetUnit, + Function convertingFunction, + boolean convertToDouble) { + Map converters = + conversionMappings.computeIfAbsent(sourceUnit, k -> new HashMap<>()); + + if (converters.containsKey(targetUnit)) { + throw new IllegalArgumentException( + "Converter from " + sourceUnit + " to " + targetUnit + " already registered"); + } + converters.put(targetUnit, new UnitConverter(convertingFunction, convertToDouble)); + } +} diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java new file mode 100644 index 000000000000..17bf1f69c9e2 --- /dev/null +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java @@ -0,0 +1,79 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.jmx.engine.unit; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.MethodSource; + +class UnitConverterFactoryTest { + + @ParameterizedTest + @CsvSource({ + "1000000000,ns, 1.0", + "25,ns, 0.000000025", + "96614101945,ns, 96.614101945", + "0,ns, 0", + "1000,ms, 1.0", + "25,ms, 0.025", + "9661410,ms, 9661.41", + "0,ms, 0", + }) + void shouldSupportPredefined_to_s_Converters( + Long originalValue, String originalUnit, Double expectedConvertedValue) { + // Given + String targetUnit = "s"; + + // When + UnitConverter converter = UnitConverterFactory.getConverter(originalUnit, targetUnit); + Number actualValue = converter.convert(originalValue); + + // Then + assertEquals(expectedConvertedValue, actualValue); + assertTrue(converter.isConvertingToDouble()); + } + + @Test + void shouldSupportCustomConverter() { + // Given + String sourceUnit = "MB"; + String targetUnit = "By"; + + // When + UnitConverterFactory.registerConverter( + sourceUnit, targetUnit, (megaBytes) -> megaBytes.doubleValue() * 1024 * 1024, false); + UnitConverter converter = UnitConverterFactory.getConverter(sourceUnit, targetUnit); + Number actualValue = converter.convert(1.5); + + // Then + assertEquals(1572864, actualValue.longValue()); + assertFalse(converter.isConvertingToDouble()); + } + + @ParameterizedTest + @MethodSource("provideUnitsForMissingConverter") + void shouldHandleMissingConverter(String sourceUnit, String targetUnit) { + UnitConverter converter = UnitConverterFactory.getConverter(sourceUnit, targetUnit); + assertNull(converter); + } + + private static Stream provideUnitsForMissingConverter() { + return Stream.of( + Arguments.of(null, null), + Arguments.of("ms", null), + Arguments.of("ms", ""), + Arguments.of("ms", "--"), + Arguments.of("--", "--")); + } +} From da63cef1a104081e921bda1f9ada4177138e5cf0 Mon Sep 17 00:00:00 2001 From: robsunday Date: Thu, 6 Mar 2025 17:22:01 +0100 Subject: [PATCH 04/17] Fixes for @Nullable Converters stored in a single flat map Exception thrown when converter not found --- .../jmx/engine/MetricInfo.java | 5 +- .../jmx/engine/MetricRegistrar.java | 6 +- .../jmx/engine/unit/UnitConverterFactory.java | 43 +++++++++----- .../engine/unit/UnitConverterFactoryTest.java | 56 +++++++++++++++---- 4 files changed, 79 insertions(+), 31 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java index f4e71f62d1af..d357ea0bb882 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java @@ -28,7 +28,7 @@ public enum Type { private final String metricName; // used as Instrument name @Nullable private final String description; @Nullable private final String sourceUnit; - @Nullable private final String unit; + private final String unit; private final Type type; /** @@ -44,7 +44,7 @@ public MetricInfo( String metricName, @Nullable String description, @Nullable String sourceUnit, - @Nullable String unit, + String unit, @Nullable Type type) { this.metricName = metricName; this.description = description; @@ -67,7 +67,6 @@ public String getSourceUnit() { return sourceUnit; } - @Nullable public String getUnit() { return unit; } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java index e1d97849030f..2229dfd92b1e 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java @@ -87,7 +87,7 @@ void enrollExtractor( // CHECKSTYLE:ON LongCounterBuilder builder = meter.counterBuilder(metricName); Optional.ofNullable(description).ifPresent(builder::setDescription); - Optional.ofNullable(unit).ifPresent(builder::setUnit); + builder.setUnit(unit); if (recordDoubleValue) { builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor, unitConverter)); @@ -104,7 +104,7 @@ void enrollExtractor( // CHECKSTYLE:ON LongUpDownCounterBuilder builder = meter.upDownCounterBuilder(metricName); Optional.ofNullable(description).ifPresent(builder::setDescription); - Optional.ofNullable(unit).ifPresent(builder::setUnit); + builder.setUnit(unit); if (recordDoubleValue) { builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor, unitConverter)); @@ -121,7 +121,7 @@ void enrollExtractor( // CHECKSTYLE:ON DoubleGaugeBuilder builder = meter.gaugeBuilder(metricName); Optional.ofNullable(description).ifPresent(builder::setDescription); - Optional.ofNullable(unit).ifPresent(builder::setUnit); + builder.setUnit(unit); if (recordDoubleValue) { builder.buildWithCallback(doubleTypeCallback(extractor, unitConverter)); diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java index 31776b2f207c..b0f835eab14e 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java @@ -12,8 +12,7 @@ import javax.annotation.Nullable; public class UnitConverterFactory { - - private static final Map> conversionMappings = new HashMap<>(); + private static final Map conversionMappings = new HashMap<>(); static { registerConverter("ms", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMillis(1), true); @@ -22,17 +21,25 @@ public class UnitConverterFactory { private UnitConverterFactory() {} - public static UnitConverter getConverter(@Nullable String fromUnit, @Nullable String toUnit) { - if (fromUnit == null || toUnit == null) { - return null; + @Nullable + public static UnitConverter getConverter(@Nullable String sourceUnit, String targetUnit) { + if (targetUnit.isEmpty()) { + throw new IllegalArgumentException("Non empty targetUnit must be provided"); } - Map converters = conversionMappings.get(fromUnit); - if (converters == null) { + if (sourceUnit == null || sourceUnit.isEmpty()) { + // No conversion is needed return null; } - return converters.get(toUnit); + String converterKey = getConverterKey(sourceUnit, targetUnit); + UnitConverter converter = conversionMappings.get(converterKey); + if (converter == null) { + throw new IllegalStateException( + "No [" + sourceUnit + "] to [" + targetUnit + "] unit converter"); + } + + return converter; } public static void registerConverter( @@ -40,13 +47,23 @@ public static void registerConverter( String targetUnit, Function convertingFunction, boolean convertToDouble) { - Map converters = - conversionMappings.computeIfAbsent(sourceUnit, k -> new HashMap<>()); + if (sourceUnit.isEmpty()) { + throw new IllegalArgumentException("Non empty sourceUnit must be provided"); + } + if (targetUnit.isEmpty()) { + throw new IllegalArgumentException("Non empty targetUnit must be provided"); + } + + String converterKey = getConverterKey(sourceUnit, targetUnit); - if (converters.containsKey(targetUnit)) { + if (conversionMappings.containsKey(converterKey)) { throw new IllegalArgumentException( - "Converter from " + sourceUnit + " to " + targetUnit + " already registered"); + "Converter from [" + sourceUnit + "] to [" + targetUnit + "] already registered"); } - converters.put(targetUnit, new UnitConverter(convertingFunction, convertToDouble)); + conversionMappings.put(converterKey, new UnitConverter(convertingFunction, convertToDouble)); + } + + private static String getConverterKey(String sourceUnit, String targetUnit) { + return sourceUnit + "->" + targetUnit; } } diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java index 17bf1f69c9e2..deaf9dedc6d2 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java @@ -5,17 +5,16 @@ package io.opentelemetry.instrumentation.jmx.engine.unit; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.CsvSource; -import org.junit.jupiter.params.provider.MethodSource; class UnitConverterFactoryTest { @@ -62,18 +61,51 @@ void shouldSupportCustomConverter() { } @ParameterizedTest - @MethodSource("provideUnitsForMissingConverter") - void shouldHandleMissingConverter(String sourceUnit, String targetUnit) { + @CsvSource({ + "--, --", + "ms, non-existing", + "non-existing, s", + }) + void shouldHandleNonExistingConverter(String sourceUnit, String targetUnit) { + IllegalStateException exception = + assertThrows( + IllegalStateException.class, + () -> UnitConverterFactory.getConverter(sourceUnit, targetUnit)); + assertEquals( + "No [" + sourceUnit + "] to [" + targetUnit + "] unit converter", exception.getMessage()); + } + + @ParameterizedTest + @CsvSource({ + ", s", // null -> "s" + "'', s", // "" -> "s" + }) + void shouldSkipConversionWhenSourceUnitNotSpecified(String sourceUnit, String targetUnit) { UnitConverter converter = UnitConverterFactory.getConverter(sourceUnit, targetUnit); assertNull(converter); } - private static Stream provideUnitsForMissingConverter() { - return Stream.of( - Arguments.of(null, null), - Arguments.of("ms", null), - Arguments.of("ms", ""), - Arguments.of("ms", "--"), - Arguments.of("--", "--")); + @ParameterizedTest + @CsvSource({ + "'', By", "By, ''", + }) + void shouldThrowExceptionWhenRegisteringConverterWithAnyUnitEmpty( + String sourceUnit, String targetUnit) { + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, + () -> + UnitConverterFactory.registerConverter( + sourceUnit, targetUnit, (value) -> 0, false)); + assertThat(exception.getMessage()).matches("Non empty .+Unit must be provided"); + } + + @Test + void shouldNotAllowRegisteringAgainAlreadyExistingConverter() { + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, + () -> UnitConverterFactory.registerConverter("ms", "s", (v) -> 0, false)); + assertEquals("Converter from [ms] to [s] already registered", exception.getMessage()); } } From d6b87e269ff5b127c0f64fc9b5f3f2167ec56b6c Mon Sep 17 00:00:00 2001 From: robsunday Date: Fri, 7 Mar 2025 17:21:01 +0100 Subject: [PATCH 05/17] Fixed typo Added support for default sourceUnit defined on rule level --- .../opentelemetry/instrumentation/jmx/yaml/JmxRule.java | 2 +- .../io/opentelemetry/instrumentation/jmx/yaml/Metric.java | 4 ++++ .../instrumentation/jmx/yaml/MetricStructure.java | 7 ++++--- .../instrumentation/jmx/engine/RuleParserTest.java | 8 ++++++++ 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java index ef575273fb1a..7476d683f60a 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java @@ -158,7 +158,7 @@ public MetricDef buildMetricDef() throws Exception { getUnit(), getMetricType()); } else { - metricInfo = m.buildMetricInfo(prefix, niceAttributeName, getUnit(), getMetricType()); + metricInfo = m.buildMetricInfo(prefix, niceAttributeName, getSourceUnit(), getUnit(), getMetricType()); } List ownAttributes = getAttributeList(); diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java index b21cf155e7db..4ed1c66341be 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java @@ -49,6 +49,7 @@ public void setDesc(String desc) { MetricInfo buildMetricInfo( @Nullable String prefix, String attributeName, + String defaultSourceUnit, String defaultUnit, MetricInfo.Type defaultType) { String metricName; @@ -64,6 +65,9 @@ MetricInfo buildMetricInfo( } String sourceUnit = getSourceUnit(); + if (sourceUnit == null) { + sourceUnit = defaultSourceUnit; + } String unit = getUnit(); if (unit == null) { diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java index afd84bb12e05..980a78545f87 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java @@ -19,6 +19,7 @@ *
  • the metric type *
  • the metric attributes *
  • the unit + *
  • the sourceUnit * *

    Known subclasses are {@link JmxRule} and {@link Metric}. */ @@ -45,7 +46,7 @@ abstract class MetricStructure { private Map metricAttribute; private StateMapping stateMapping = StateMapping.empty(); private static final String STATE_MAPPING_DEFAULT = "*"; - private String sourcetUnit; + private String sourceUnit; private String unit; private MetricInfo.Type metricType; @@ -60,11 +61,11 @@ void setType(String t) { } public String getSourceUnit() { - return sourcetUnit; + return sourceUnit; } public void setSourceUnit(String unit) { - this.sourcetUnit = validateUnit(unit.trim()); + this.sourceUnit = validateUnit(unit.trim()); } public String getUnit() { diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java index 543a02204095..23883672f912 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java @@ -163,12 +163,14 @@ void testConf3() { + " LABEL_KEY2: beanattr(ATTRIBUTE)\n" + " prefix: PREFIX.\n" + " type: upDownCounter\n" + + " sourceUnit: DEFAULT_SOURCE_UNIT\n" + " unit: DEFAULT_UNIT\n" + " mapping:\n" + " A.b:\n" + " metric: METRIC_NAME1\n" + " type: counter\n" + " desc: DESCRIPTION1\n" + + " sourceUnit: SOURCE_UNIT1\n" + " unit: UNIT1\n" + " metricAttribute:\n" + " LABEL_KEY3: const(CONSTANT)\n" @@ -186,6 +188,7 @@ void testConf4() throws Exception { assertThat(defs).hasSize(1); JmxRule jmxDef = defs.get(0); + assertThat(jmxDef.getSourceUnit()).isEqualTo("DEFAULT_SOURCE_UNIT"); assertThat(jmxDef.getUnit()).isEqualTo("DEFAULT_UNIT"); assertThat(jmxDef.getMetricType()).isEqualTo(MetricInfo.Type.UPDOWNCOUNTER); @@ -205,6 +208,7 @@ void testConf4() throws Exception { MetricInfo metricInfo = m.getInfo(); assertThat(metricInfo.getMetricName()).isEqualTo("PREFIX.METRIC_NAME1"); assertThat(metricInfo.getDescription()).isEqualTo("DESCRIPTION1"); + assertThat(metricInfo.getSourceUnit()).isEqualTo("SOURCE_UNIT1"); assertThat(metricInfo.getUnit()).isEqualTo("UNIT1"); assertThat(metricInfo.getType()).isEqualTo(MetricInfo.Type.COUNTER); }) @@ -219,6 +223,7 @@ void testConf4() throws Exception { MetricInfo metricInfo = m.getInfo(); assertThat(metricInfo.getMetricName()).isEqualTo("PREFIX.METRIC_NAME2"); assertThat(metricInfo.getDescription()).isEqualTo("DESCRIPTION2"); + assertThat(metricInfo.getSourceUnit()).isEqualTo(jmxDef.getSourceUnit()); assertThat(metricInfo.getUnit()).isEqualTo("UNIT2"); }) .anySatisfy( @@ -236,6 +241,9 @@ void testConf4() throws Exception { assertThat(metricInfo.getUnit()) .describedAs("default unit should match jmx rule definition") .isEqualTo(jmxDef.getUnit()); + assertThat(metricInfo.getSourceUnit()) + .describedAs("default sourceUnit should match jmx rule definition") + .isEqualTo(jmxDef.getSourceUnit()); }); } From 061e3c7ad8a58db884a40deaca2ffb61a80fb320 Mon Sep 17 00:00:00 2001 From: robsunday Date: Tue, 11 Mar 2025 15:47:42 +0100 Subject: [PATCH 06/17] Documentation updated. Converter registration is now package private. --- .../jmx-metrics/javaagent/README.md | 33 +++++++++++++++++++ .../jmx/engine/unit/UnitConverterFactory.java | 3 +- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/instrumentation/jmx-metrics/javaagent/README.md b/instrumentation/jmx-metrics/javaagent/README.md index 8272e893e327..19c6bc037d19 100644 --- a/instrumentation/jmx-metrics/javaagent/README.md +++ b/instrumentation/jmx-metrics/javaagent/README.md @@ -307,6 +307,39 @@ rules: For now, only the `lowercase` transformation is supported, other additions might be added in the future if needed. +### Unit conversions + +Sometimes JMX attributes values are reported in units not aligned with semantic conventions. +For example duration values are usually reported as milliseconds while semantic conventions recommend using seconds. + +This issue can be solved by providing optional `sourceUnit` metric property together with `unit` metric property. +`sourceUnit` defines native unit of value retrieved from JMX attribute, while `unit` defines a semantic convention compatible metric unit that will be reported to the backend. +If conversion between `sourceUnit` and `unit` is available then it is automatically applied before reporting the metric. +If such a conversion is not available then an error is reported during JMX metrics processing. + +Currently available unit conversions: + +| `sourceUnit` | `unit` | +|-------------|-------| +| ms | s | +| ns | s | + +Example of defining unit conversion in yaml file: +```yaml +rules: + - beans: + - Catalina:type=GlobalRequestProcessor,name=* + prefix: http.server.tomcat. + mapping: + maxTime: + metric: maxTime + type: gauge + sourceUnit: ms + unit: s + desc: The longest request processing time +``` +`sourceUnit` can also be defined on rule level (see [Making shortcuts](#making-shortcuts)) + ### General Syntax Here is the general description of the accepted configuration file syntax. The whole contents of the file is case-sensitive, with exception for `type` as described in the table below. diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java index b0f835eab14e..2575908b7527 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java @@ -42,7 +42,8 @@ public static UnitConverter getConverter(@Nullable String sourceUnit, String tar return converter; } - public static void registerConverter( + // visible for testing + static void registerConverter( String sourceUnit, String targetUnit, Function convertingFunction, From 1391ad73be8f0142aba10800e069f0612ad63da2 Mon Sep 17 00:00:00 2001 From: robsunday Date: Tue, 11 Mar 2025 15:49:40 +0100 Subject: [PATCH 07/17] spotless --- instrumentation/jmx-metrics/javaagent/README.md | 2 +- .../io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/instrumentation/jmx-metrics/javaagent/README.md b/instrumentation/jmx-metrics/javaagent/README.md index 19c6bc037d19..61c86126cf27 100644 --- a/instrumentation/jmx-metrics/javaagent/README.md +++ b/instrumentation/jmx-metrics/javaagent/README.md @@ -338,7 +338,7 @@ rules: unit: s desc: The longest request processing time ``` -`sourceUnit` can also be defined on rule level (see [Making shortcuts](#making-shortcuts)) +`sourceUnit` can also be defined on rule level (see [Making shortcuts](#making-shortcuts)) ### General Syntax diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java index 7476d683f60a..2bb0c4b631e3 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java @@ -158,7 +158,9 @@ public MetricDef buildMetricDef() throws Exception { getUnit(), getMetricType()); } else { - metricInfo = m.buildMetricInfo(prefix, niceAttributeName, getSourceUnit(), getUnit(), getMetricType()); + metricInfo = + m.buildMetricInfo( + prefix, niceAttributeName, getSourceUnit(), getUnit(), getMetricType()); } List ownAttributes = getAttributeList(); From 05b1a3dd102de457052fe49450f3c6f3dcdaf2c9 Mon Sep 17 00:00:00 2001 From: Robert Niedziela <175605712+robsunday@users.noreply.github.com> Date: Thu, 13 Mar 2025 09:44:48 +0100 Subject: [PATCH 08/17] Update instrumentation/jmx-metrics/javaagent/README.md Co-authored-by: SylvainJuge <763082+SylvainJuge@users.noreply.github.com> --- instrumentation/jmx-metrics/javaagent/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/jmx-metrics/javaagent/README.md b/instrumentation/jmx-metrics/javaagent/README.md index 61c86126cf27..0b86eb046eca 100644 --- a/instrumentation/jmx-metrics/javaagent/README.md +++ b/instrumentation/jmx-metrics/javaagent/README.md @@ -313,7 +313,7 @@ Sometimes JMX attributes values are reported in units not aligned with semantic For example duration values are usually reported as milliseconds while semantic conventions recommend using seconds. This issue can be solved by providing optional `sourceUnit` metric property together with `unit` metric property. -`sourceUnit` defines native unit of value retrieved from JMX attribute, while `unit` defines a semantic convention compatible metric unit that will be reported to the backend. +`sourceUnit` defines native unit of value retrieved from JMX attribute, while `unit` defines the unit of the metric reported to the backend. If conversion between `sourceUnit` and `unit` is available then it is automatically applied before reporting the metric. If such a conversion is not available then an error is reported during JMX metrics processing. From 4ffbbfe950ef84572b45c950d8ad384e46fad2b9 Mon Sep 17 00:00:00 2001 From: robsunday Date: Thu, 13 Mar 2025 10:45:28 +0100 Subject: [PATCH 09/17] Microseconds support added --- .../instrumentation/jmx/engine/unit/UnitConverterFactory.java | 1 + .../jmx/engine/unit/UnitConverterFactoryTest.java | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java index 2575908b7527..8b93b3a5af15 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java @@ -16,6 +16,7 @@ public class UnitConverterFactory { static { registerConverter("ms", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMillis(1), true); + registerConverter("us", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMicros(1), true); registerConverter("ns", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toNanos(1), true); } diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java index deaf9dedc6d2..4c289703c805 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java @@ -24,6 +24,10 @@ class UnitConverterFactoryTest { "25,ns, 0.000000025", "96614101945,ns, 96.614101945", "0,ns, 0", + "1000000,us, 1.0", + "25,us, 0.000025", + "96614101945,us, 96614.101945", + "0,ns, 0", "1000,ms, 1.0", "25,ms, 0.025", "9661410,ms, 9661.41", From 8f481f3c34e677324ed07281ae95ce2e1a7367b0 Mon Sep 17 00:00:00 2001 From: robsunday Date: Thu, 13 Mar 2025 10:59:12 +0100 Subject: [PATCH 10/17] Exception class changed --- .../instrumentation/jmx/engine/unit/UnitConverterFactory.java | 2 +- .../jmx/engine/unit/UnitConverterFactoryTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java index 8b93b3a5af15..f97884a0806a 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java @@ -36,7 +36,7 @@ public static UnitConverter getConverter(@Nullable String sourceUnit, String tar String converterKey = getConverterKey(sourceUnit, targetUnit); UnitConverter converter = conversionMappings.get(converterKey); if (converter == null) { - throw new IllegalStateException( + throw new IllegalArgumentException( "No [" + sourceUnit + "] to [" + targetUnit + "] unit converter"); } diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java index 4c289703c805..91ea036ac5e1 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java @@ -71,9 +71,9 @@ void shouldSupportCustomConverter() { "non-existing, s", }) void shouldHandleNonExistingConverter(String sourceUnit, String targetUnit) { - IllegalStateException exception = + IllegalArgumentException exception = assertThrows( - IllegalStateException.class, + IllegalArgumentException.class, () -> UnitConverterFactory.getConverter(sourceUnit, targetUnit)); assertEquals( "No [" + sourceUnit + "] to [" + targetUnit + "] unit converter", exception.getMessage()); From b9f2052d425c8b2eb8aeb9b2bcac892771702e81 Mon Sep 17 00:00:00 2001 From: robsunday Date: Thu, 13 Mar 2025 11:16:10 +0100 Subject: [PATCH 11/17] Unit test improvements --- .../engine/unit/UnitConverterFactoryTest.java | 31 ++++++------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java index 91ea036ac5e1..ee91076a5be0 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java @@ -5,11 +5,10 @@ package io.opentelemetry.instrumentation.jmx.engine.unit; -import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; @@ -48,7 +47,7 @@ void shouldSupportPredefined_to_s_Converters( } @Test - void shouldSupportCustomConverter() { + void shouldRegisterConverter() { // Given String sourceUnit = "MB"; String targetUnit = "By"; @@ -71,12 +70,8 @@ void shouldSupportCustomConverter() { "non-existing, s", }) void shouldHandleNonExistingConverter(String sourceUnit, String targetUnit) { - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> UnitConverterFactory.getConverter(sourceUnit, targetUnit)); - assertEquals( - "No [" + sourceUnit + "] to [" + targetUnit + "] unit converter", exception.getMessage()); + assertThatThrownBy(() -> UnitConverterFactory.getConverter(sourceUnit, targetUnit)) + .hasMessage("No [" + sourceUnit + "] to [" + targetUnit + "] unit converter"); } @ParameterizedTest @@ -93,23 +88,15 @@ void shouldSkipConversionWhenSourceUnitNotSpecified(String sourceUnit, String ta @CsvSource({ "'', By", "By, ''", }) - void shouldThrowExceptionWhenRegisteringConverterWithAnyUnitEmpty( + void shouldNotAllowRegisteringConverterWithAnyUnitEmpty( String sourceUnit, String targetUnit) { - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> - UnitConverterFactory.registerConverter( - sourceUnit, targetUnit, (value) -> 0, false)); - assertThat(exception.getMessage()).matches("Non empty .+Unit must be provided"); + assertThatThrownBy(() -> UnitConverterFactory.registerConverter(sourceUnit, targetUnit, (value) -> 0, false)) + .hasMessageMatching("Non empty .+Unit must be provided"); } @Test void shouldNotAllowRegisteringAgainAlreadyExistingConverter() { - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> UnitConverterFactory.registerConverter("ms", "s", (v) -> 0, false)); - assertEquals("Converter from [ms] to [s] already registered", exception.getMessage()); + assertThatThrownBy(() -> UnitConverterFactory.registerConverter("ms", "s", (v) -> 0, false)) + .hasMessage("Converter from [ms] to [s] already registered"); } } From 95cde460f06bf8081f9082b93077cb457fbc0e11 Mon Sep 17 00:00:00 2001 From: Robert Niedziela <175605712+robsunday@users.noreply.github.com> Date: Thu, 13 Mar 2025 11:17:26 +0100 Subject: [PATCH 12/17] Update instrumentation/jmx-metrics/javaagent/README.md Co-authored-by: Jay DeLuca --- instrumentation/jmx-metrics/javaagent/README.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/instrumentation/jmx-metrics/javaagent/README.md b/instrumentation/jmx-metrics/javaagent/README.md index 0b86eb046eca..197db9c971af 100644 --- a/instrumentation/jmx-metrics/javaagent/README.md +++ b/instrumentation/jmx-metrics/javaagent/README.md @@ -309,13 +309,13 @@ For now, only the `lowercase` transformation is supported, other additions might ### Unit conversions -Sometimes JMX attributes values are reported in units not aligned with semantic conventions. -For example duration values are usually reported as milliseconds while semantic conventions recommend using seconds. +Sometimes JMX attribute values are reported in units that are not aligned with semantic conventions. +For example, duration values are usually reported as milliseconds while semantic conventions recommend using seconds. -This issue can be solved by providing optional `sourceUnit` metric property together with `unit` metric property. -`sourceUnit` defines native unit of value retrieved from JMX attribute, while `unit` defines the unit of the metric reported to the backend. -If conversion between `sourceUnit` and `unit` is available then it is automatically applied before reporting the metric. -If such a conversion is not available then an error is reported during JMX metrics processing. +This issue can be solved by providing an optional `sourceUnit` metric property together with the `unit` metric property. +`sourceUnit` defines the native unit of value retrieved from JMX attribute, while `unit` defines the unit of the metric reported to the backend. +If a conversion between `sourceUnit` and `unit` is available, then it is automatically applied before reporting the metric. +If such a conversion is not available, then an error is reported during JMX metrics processing. Currently available unit conversions: From 7559b7ab80713f0ff5c5976c6bfc5bf986ca4b33 Mon Sep 17 00:00:00 2001 From: robsunday Date: Thu, 13 Mar 2025 11:21:27 +0100 Subject: [PATCH 13/17] Readme update --- instrumentation/jmx-metrics/javaagent/README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/instrumentation/jmx-metrics/javaagent/README.md b/instrumentation/jmx-metrics/javaagent/README.md index 197db9c971af..b91ce05a7b79 100644 --- a/instrumentation/jmx-metrics/javaagent/README.md +++ b/instrumentation/jmx-metrics/javaagent/README.md @@ -312,7 +312,7 @@ For now, only the `lowercase` transformation is supported, other additions might Sometimes JMX attribute values are reported in units that are not aligned with semantic conventions. For example, duration values are usually reported as milliseconds while semantic conventions recommend using seconds. -This issue can be solved by providing an optional `sourceUnit` metric property together with the `unit` metric property. +This issue can be solved by providing an optional `sourceUnit` metric property together with the `unit` metric property. `sourceUnit` defines the native unit of value retrieved from JMX attribute, while `unit` defines the unit of the metric reported to the backend. If a conversion between `sourceUnit` and `unit` is available, then it is automatically applied before reporting the metric. If such a conversion is not available, then an error is reported during JMX metrics processing. @@ -320,9 +320,10 @@ If such a conversion is not available, then an error is reported during JMX metr Currently available unit conversions: | `sourceUnit` | `unit` | -|-------------|-------| -| ms | s | -| ns | s | +|--------------|-------| +| ms | s | +| us | s | +| ns | s | Example of defining unit conversion in yaml file: ```yaml From 1964f8261fffe9cf20e80f56971667f44c19c47f Mon Sep 17 00:00:00 2001 From: robsunday Date: Thu, 13 Mar 2025 11:32:19 +0100 Subject: [PATCH 14/17] Spotless --- .../jmx/engine/unit/UnitConverterFactoryTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java index ee91076a5be0..16e347ff5f8a 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java @@ -88,9 +88,10 @@ void shouldSkipConversionWhenSourceUnitNotSpecified(String sourceUnit, String ta @CsvSource({ "'', By", "By, ''", }) - void shouldNotAllowRegisteringConverterWithAnyUnitEmpty( - String sourceUnit, String targetUnit) { - assertThatThrownBy(() -> UnitConverterFactory.registerConverter(sourceUnit, targetUnit, (value) -> 0, false)) + void shouldNotAllowRegisteringConverterWithAnyUnitEmpty(String sourceUnit, String targetUnit) { + assertThatThrownBy( + () -> + UnitConverterFactory.registerConverter(sourceUnit, targetUnit, (value) -> 0, false)) .hasMessageMatching("Non empty .+Unit must be provided"); } From a2c68458dc7315c60c0ca5d365a58c1099c9a50e Mon Sep 17 00:00:00 2001 From: robsunday Date: Mon, 17 Mar 2025 11:57:10 +0100 Subject: [PATCH 15/17] Class structure update Comments added --- .../jmx/engine/MetricRegistrar.java | 4 +-- .../{unit => internal}/UnitConverter.java | 11 +++++--- .../UnitConverterFactory.java | 25 ++++++++++++++++++- .../UnitConverterFactoryTest.java | 2 +- 4 files changed, 34 insertions(+), 8 deletions(-) rename instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/{unit => internal}/UnitConverter.java (63%) rename instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/{unit => internal}/UnitConverterFactory.java (66%) rename instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/{unit => internal}/UnitConverterFactoryTest.java (98%) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java index 2229dfd92b1e..f569b3e4dbeb 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java @@ -16,8 +16,8 @@ import io.opentelemetry.api.metrics.Meter; import io.opentelemetry.api.metrics.ObservableDoubleMeasurement; import io.opentelemetry.api.metrics.ObservableLongMeasurement; -import io.opentelemetry.instrumentation.jmx.engine.unit.UnitConverter; -import io.opentelemetry.instrumentation.jmx.engine.unit.UnitConverterFactory; +import io.opentelemetry.instrumentation.jmx.engine.internal.UnitConverter; +import io.opentelemetry.instrumentation.jmx.engine.internal.UnitConverterFactory; import java.util.Collection; import java.util.Optional; import java.util.function.Consumer; diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverter.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverter.java similarity index 63% rename from instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverter.java rename to instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverter.java index 3ca7a0407991..ea817a8bd95a 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverter.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverter.java @@ -3,11 +3,14 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.instrumentation.jmx.engine.unit; +package io.opentelemetry.instrumentation.jmx.engine.internal; import java.util.function.Function; -/** This class is responsible for converting a value using provided algorithm. */ +/** + * This class is responsible for converting a value using provided algorithm. This class is internal + * and is hence not for public use. Its APIs are unstable and can change at any time. + */ public class UnitConverter { private final Function convertingFunction; private final boolean convertToDouble; @@ -16,8 +19,8 @@ public class UnitConverter { * Create an instance of converter * * @param convertingFunction an algorithm applied when converting value - * @param convertToDouble indicates of algorithm will return floating point result. This must be - * in-sync with algorithm implementation. + * @param convertToDouble {@code true} indicates that conversion result is of type Double, {@code + * false} indicates that conversion result is of type Long */ UnitConverter(Function convertingFunction, boolean convertToDouble) { this.convertingFunction = convertingFunction; diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactory.java similarity index 66% rename from instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java rename to instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactory.java index f97884a0806a..ccc1ad75c866 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactory.java @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.instrumentation.jmx.engine.unit; +package io.opentelemetry.instrumentation.jmx.engine.internal; import java.util.HashMap; import java.util.Map; @@ -11,6 +11,10 @@ import java.util.function.Function; import javax.annotation.Nullable; +/** + * Manage available unit converters. This class is internal and is hence not for public use. Its + * APIs are unstable and can change at any time. + */ public class UnitConverterFactory { private static final Map conversionMappings = new HashMap<>(); @@ -22,6 +26,14 @@ public class UnitConverterFactory { private UnitConverterFactory() {} + /** + * Get the instance of converter that is able to convert a value from given source to target unit. + * + * @param sourceUnit a source unit supported by requested converter + * @param targetUnit a target unit supported by requested converter + * @return an instance of converter, or {@literal null} if no converter exists for {@code + * }sourceUnit} to {@code targetUnit} conversion + */ @Nullable public static UnitConverter getConverter(@Nullable String sourceUnit, String targetUnit) { if (targetUnit.isEmpty()) { @@ -43,6 +55,17 @@ public static UnitConverter getConverter(@Nullable String sourceUnit, String tar return converter; } + /** + * Register new instance of a converter that can then be retrieved with {@link + * #getConverter(String, String)}. + * + * @param sourceUnit a source unit supported by the converter + * @param targetUnit a target unit supported by the converter + * @param convertingFunction a function that implements algorithm of conversion between {@code + * sourceUnit} and {@code targetUnit} + * @param convertToDouble {@code true} indicates that conversion result is of type Double, {@code + * false} indicates that conversion result is of type Long + */ // visible for testing static void registerConverter( String sourceUnit, diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactoryTest.java similarity index 98% rename from instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java rename to instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactoryTest.java index 16e347ff5f8a..0640eeab0fdd 100644 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactoryTest.java @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.instrumentation.jmx.engine.unit; +package io.opentelemetry.instrumentation.jmx.engine.internal; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertEquals; From 40fe2b2addee22f1a209fd359ebb2a26f6e8b717 Mon Sep 17 00:00:00 2001 From: robsunday Date: Tue, 18 Mar 2025 16:30:44 +0100 Subject: [PATCH 16/17] Got rid of isConvertingToDouble flag which is not needed now UnitConverter merged with UnitConverterFactory and made package-private --- .../jmx/engine/MetricRegistrar.java | 20 +--- ...nverterFactory.java => UnitConverter.java} | 63 ++++++----- .../jmx/engine/internal/UnitConverter.java | 37 ------- .../jmx/engine/UnitConverterTest.java | 96 ++++++++++++++++ .../internal/UnitConverterFactoryTest.java | 103 ------------------ 5 files changed, 139 insertions(+), 180 deletions(-) rename instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/{internal/UnitConverterFactory.java => UnitConverter.java} (50%) delete mode 100644 instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverter.java create mode 100644 instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverterTest.java delete mode 100644 instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactoryTest.java diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java index f569b3e4dbeb..ce500a3c6a20 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java @@ -16,8 +16,6 @@ import io.opentelemetry.api.metrics.Meter; import io.opentelemetry.api.metrics.ObservableDoubleMeasurement; import io.opentelemetry.api.metrics.ObservableLongMeasurement; -import io.opentelemetry.instrumentation.jmx.engine.internal.UnitConverter; -import io.opentelemetry.instrumentation.jmx.engine.internal.UnitConverterFactory; import java.util.Collection; import java.util.Optional; import java.util.function.Consumer; @@ -75,9 +73,9 @@ void enrollExtractor( String unit = metricInfo.getUnit(); String sourceUnit = metricInfo.getSourceUnit(); - UnitConverter unitConverter = UnitConverterFactory.getConverter(sourceUnit, unit); + UnitConverter unitConverter = UnitConverter.getInstance(sourceUnit, unit); if (unitConverter != null) { - recordDoubleValue = unitConverter.isConvertingToDouble(); + recordDoubleValue = true; } switch (instrumentType) { @@ -92,7 +90,7 @@ void enrollExtractor( if (recordDoubleValue) { builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor, unitConverter)); } else { - builder.buildWithCallback(longTypeCallback(extractor, unitConverter)); + builder.buildWithCallback(longTypeCallback(extractor)); } logger.log(INFO, "Created Counter for {0}", metricName); } @@ -109,7 +107,7 @@ void enrollExtractor( if (recordDoubleValue) { builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor, unitConverter)); } else { - builder.buildWithCallback(longTypeCallback(extractor, unitConverter)); + builder.buildWithCallback(longTypeCallback(extractor)); } logger.log(INFO, "Created UpDownCounter for {0}", metricName); } @@ -126,7 +124,7 @@ void enrollExtractor( if (recordDoubleValue) { builder.buildWithCallback(doubleTypeCallback(extractor, unitConverter)); } else { - builder.ofLongs().buildWithCallback(longTypeCallback(extractor, unitConverter)); + builder.ofLongs().buildWithCallback(longTypeCallback(extractor)); } logger.log(INFO, "Created Gauge for {0}", metricName); } @@ -171,10 +169,8 @@ static Consumer doubleTypeCallback( /* * A method generating metric collection callback for asynchronous Measurement * of Long type. - * If unit converter is provided then conversion is applied before metric is recorded. */ - static Consumer longTypeCallback( - MetricExtractor extractor, @Nullable UnitConverter unitConverter) { + static Consumer longTypeCallback(MetricExtractor extractor) { return measurement -> { DetectionStatus status = extractor.getStatus(); if (status != null) { @@ -185,10 +181,6 @@ static Consumer longTypeCallback( if (metricValue != null) { // get the metric attributes Attributes attr = createMetricAttributes(connection, objectName, extractor); - - if (unitConverter != null) { - metricValue = unitConverter.convert(metricValue); - } measurement.record(metricValue.longValue(), attr); } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactory.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverter.java similarity index 50% rename from instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactory.java rename to instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverter.java index ccc1ad75c866..420a250ae745 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactory.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverter.java @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.instrumentation.jmx.engine.internal; +package io.opentelemetry.instrumentation.jmx.engine; import java.util.HashMap; import java.util.Map; @@ -12,30 +12,33 @@ import javax.annotation.Nullable; /** - * Manage available unit converters. This class is internal and is hence not for public use. Its - * APIs are unstable and can change at any time. + * This class is responsible for converting a value between metric units using defined conversion + * algorithms. */ -public class UnitConverterFactory { +class UnitConverter { private static final Map conversionMappings = new HashMap<>(); static { - registerConverter("ms", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMillis(1), true); - registerConverter("us", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMicros(1), true); - registerConverter("ns", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toNanos(1), true); + registerConversion("ms", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMillis(1)); + registerConversion("us", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMicros(1)); + registerConversion("ns", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toNanos(1)); } - private UnitConverterFactory() {} + private final Function convertingFunction; /** - * Get the instance of converter that is able to convert a value from given source to target unit. + * Get the instance of converter that is able to convert a value from a given source to a target + * unit. * * @param sourceUnit a source unit supported by requested converter * @param targetUnit a target unit supported by requested converter - * @return an instance of converter, or {@literal null} if no converter exists for {@code - * }sourceUnit} to {@code targetUnit} conversion + * @return an instance of converter, or {@literal null} if {@code sourceUnit} is {@literal null} + * or empty, which means that there is no conversion needed. + * @throws IllegalArgumentException if {@code targetUnit} is empty, or matching converter was not + * found for provided units. */ @Nullable - public static UnitConverter getConverter(@Nullable String sourceUnit, String targetUnit) { + public static UnitConverter getInstance(@Nullable String sourceUnit, String targetUnit) { if (targetUnit.isEmpty()) { throw new IllegalArgumentException("Non empty targetUnit must be provided"); } @@ -45,11 +48,11 @@ public static UnitConverter getConverter(@Nullable String sourceUnit, String tar return null; } - String converterKey = getConverterKey(sourceUnit, targetUnit); + String converterKey = buildConverterKey(sourceUnit, targetUnit); UnitConverter converter = conversionMappings.get(converterKey); if (converter == null) { throw new IllegalArgumentException( - "No [" + sourceUnit + "] to [" + targetUnit + "] unit converter"); + "Unsupported conversion from [" + sourceUnit + "] to [" + targetUnit + "]"); } return converter; @@ -57,21 +60,16 @@ public static UnitConverter getConverter(@Nullable String sourceUnit, String tar /** * Register new instance of a converter that can then be retrieved with {@link - * #getConverter(String, String)}. + * #getInstance(String, String)}. * * @param sourceUnit a source unit supported by the converter * @param targetUnit a target unit supported by the converter * @param convertingFunction a function that implements algorithm of conversion between {@code * sourceUnit} and {@code targetUnit} - * @param convertToDouble {@code true} indicates that conversion result is of type Double, {@code - * false} indicates that conversion result is of type Long */ // visible for testing - static void registerConverter( - String sourceUnit, - String targetUnit, - Function convertingFunction, - boolean convertToDouble) { + static void registerConversion( + String sourceUnit, String targetUnit, Function convertingFunction) { if (sourceUnit.isEmpty()) { throw new IllegalArgumentException("Non empty sourceUnit must be provided"); } @@ -79,16 +77,29 @@ static void registerConverter( throw new IllegalArgumentException("Non empty targetUnit must be provided"); } - String converterKey = getConverterKey(sourceUnit, targetUnit); + String converterKey = buildConverterKey(sourceUnit, targetUnit); if (conversionMappings.containsKey(converterKey)) { throw new IllegalArgumentException( - "Converter from [" + sourceUnit + "] to [" + targetUnit + "] already registered"); + "Conversion from [" + sourceUnit + "] to [" + targetUnit + "] already defined"); } - conversionMappings.put(converterKey, new UnitConverter(convertingFunction, convertToDouble)); + conversionMappings.put(converterKey, new UnitConverter(convertingFunction)); } - private static String getConverterKey(String sourceUnit, String targetUnit) { + private static String buildConverterKey(String sourceUnit, String targetUnit) { return sourceUnit + "->" + targetUnit; } + + /** + * Create an instance of converter + * + * @param convertingFunction an algorithm applied when converting value + */ + UnitConverter(Function convertingFunction) { + this.convertingFunction = convertingFunction; + } + + public Number convert(Number value) { + return convertingFunction.apply(value); + } } diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverter.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverter.java deleted file mode 100644 index ea817a8bd95a..000000000000 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverter.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.jmx.engine.internal; - -import java.util.function.Function; - -/** - * This class is responsible for converting a value using provided algorithm. This class is internal - * and is hence not for public use. Its APIs are unstable and can change at any time. - */ -public class UnitConverter { - private final Function convertingFunction; - private final boolean convertToDouble; - - /** - * Create an instance of converter - * - * @param convertingFunction an algorithm applied when converting value - * @param convertToDouble {@code true} indicates that conversion result is of type Double, {@code - * false} indicates that conversion result is of type Long - */ - UnitConverter(Function convertingFunction, boolean convertToDouble) { - this.convertingFunction = convertingFunction; - this.convertToDouble = convertToDouble; - } - - public Number convert(Number value) { - return convertingFunction.apply(value); - } - - public boolean isConvertingToDouble() { - return convertToDouble; - } -} diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverterTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverterTest.java new file mode 100644 index 000000000000..465e2487698e --- /dev/null +++ b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverterTest.java @@ -0,0 +1,96 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.jmx.engine; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +class UnitConverterTest { + + @ParameterizedTest + @CsvSource({ + "1000000000,ns, 1.0", + "25,ns, 0.000000025", + "96614101945,ns, 96.614101945", + "0,ns, 0", + "1000000,us, 1.0", + "25,us, 0.000025", + "96614101945,us, 96614.101945", + "0,ns, 0", + "1000,ms, 1.0", + "25,ms, 0.025", + "9661410,ms, 9661.41", + "0,ms, 0", + }) + void shouldSupportPredefined_to_s_Conversions( + Long originalValue, String originalUnit, Double expectedConvertedValue) { + // Given + String targetUnit = "s"; + + // When + UnitConverter converter = UnitConverter.getInstance(originalUnit, targetUnit); + Number actualValue = converter.convert(originalValue); + + // Then + assertEquals(expectedConvertedValue, actualValue); + } + + @ParameterizedTest + @CsvSource({ + "--, --", + "ms, non-existing", + "non-existing, s", + }) + void shouldHandleUnsupportedConversion(String sourceUnit, String targetUnit) { + assertThatThrownBy(() -> UnitConverter.getInstance(sourceUnit, targetUnit)) + .hasMessage("Unsupported conversion from [" + sourceUnit + "] to [" + targetUnit + "]"); + } + + @ParameterizedTest + @CsvSource({ + ", s", // null -> "s" + "'', s", // "" -> "s" + }) + void shouldSkipConversionWhenSourceUnitNotSpecified(String sourceUnit, String targetUnit) { + UnitConverter converter = UnitConverter.getInstance(sourceUnit, targetUnit); + assertNull(converter); + } + + @Test + void shouldRegisterNewConversion() { + // Given + String sourceUnit = "h"; + String targetUnit = "s"; + + // When + UnitConverter.registerConversion("h", "s", hours -> hours.doubleValue() * 3600.0); + UnitConverter converter = UnitConverter.getInstance(sourceUnit, targetUnit); + Number actualValue = converter.convert(1.5); + + // Then + assertEquals(5400.0, actualValue); + } + + @ParameterizedTest + @CsvSource({ + "'', By", "By, ''", + }) + void shouldNotAllowRegisteringConversionWithAnyUnitEmpty(String sourceUnit, String targetUnit) { + assertThatThrownBy(() -> UnitConverter.registerConversion(sourceUnit, targetUnit, (value) -> 0)) + .hasMessageMatching("Non empty .+Unit must be provided"); + } + + @Test + void shouldNotAllowRegisteringAgainAlreadyExistingConversion() { + assertThatThrownBy(() -> UnitConverter.registerConversion("ms", "s", (v) -> 0)) + .hasMessage("Conversion from [ms] to [s] already defined"); + } +} diff --git a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactoryTest.java b/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactoryTest.java deleted file mode 100644 index 0640eeab0fdd..000000000000 --- a/instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/internal/UnitConverterFactoryTest.java +++ /dev/null @@ -1,103 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.jmx.engine.internal; - -import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.CsvSource; - -class UnitConverterFactoryTest { - - @ParameterizedTest - @CsvSource({ - "1000000000,ns, 1.0", - "25,ns, 0.000000025", - "96614101945,ns, 96.614101945", - "0,ns, 0", - "1000000,us, 1.0", - "25,us, 0.000025", - "96614101945,us, 96614.101945", - "0,ns, 0", - "1000,ms, 1.0", - "25,ms, 0.025", - "9661410,ms, 9661.41", - "0,ms, 0", - }) - void shouldSupportPredefined_to_s_Converters( - Long originalValue, String originalUnit, Double expectedConvertedValue) { - // Given - String targetUnit = "s"; - - // When - UnitConverter converter = UnitConverterFactory.getConverter(originalUnit, targetUnit); - Number actualValue = converter.convert(originalValue); - - // Then - assertEquals(expectedConvertedValue, actualValue); - assertTrue(converter.isConvertingToDouble()); - } - - @Test - void shouldRegisterConverter() { - // Given - String sourceUnit = "MB"; - String targetUnit = "By"; - - // When - UnitConverterFactory.registerConverter( - sourceUnit, targetUnit, (megaBytes) -> megaBytes.doubleValue() * 1024 * 1024, false); - UnitConverter converter = UnitConverterFactory.getConverter(sourceUnit, targetUnit); - Number actualValue = converter.convert(1.5); - - // Then - assertEquals(1572864, actualValue.longValue()); - assertFalse(converter.isConvertingToDouble()); - } - - @ParameterizedTest - @CsvSource({ - "--, --", - "ms, non-existing", - "non-existing, s", - }) - void shouldHandleNonExistingConverter(String sourceUnit, String targetUnit) { - assertThatThrownBy(() -> UnitConverterFactory.getConverter(sourceUnit, targetUnit)) - .hasMessage("No [" + sourceUnit + "] to [" + targetUnit + "] unit converter"); - } - - @ParameterizedTest - @CsvSource({ - ", s", // null -> "s" - "'', s", // "" -> "s" - }) - void shouldSkipConversionWhenSourceUnitNotSpecified(String sourceUnit, String targetUnit) { - UnitConverter converter = UnitConverterFactory.getConverter(sourceUnit, targetUnit); - assertNull(converter); - } - - @ParameterizedTest - @CsvSource({ - "'', By", "By, ''", - }) - void shouldNotAllowRegisteringConverterWithAnyUnitEmpty(String sourceUnit, String targetUnit) { - assertThatThrownBy( - () -> - UnitConverterFactory.registerConverter(sourceUnit, targetUnit, (value) -> 0, false)) - .hasMessageMatching("Non empty .+Unit must be provided"); - } - - @Test - void shouldNotAllowRegisteringAgainAlreadyExistingConverter() { - assertThatThrownBy(() -> UnitConverterFactory.registerConverter("ms", "s", (v) -> 0, false)) - .hasMessage("Converter from [ms] to [s] already registered"); - } -} From 5dcc6fecae4cf7625bff2faec56b2a908221af78 Mon Sep 17 00:00:00 2001 From: robsunday Date: Tue, 18 Mar 2025 19:23:36 +0100 Subject: [PATCH 17/17] JavaDoc update --- .../instrumentation/jmx/engine/UnitConverter.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverter.java b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverter.java index 420a250ae745..b7627645ed5a 100644 --- a/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverter.java +++ b/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/UnitConverter.java @@ -27,7 +27,7 @@ class UnitConverter { private final Function convertingFunction; /** - * Get the instance of converter that is able to convert a value from a given source to a target + * Get an instance of converter that is able to convert a value from a given source to a target * unit. * * @param sourceUnit a source unit supported by requested converter @@ -59,13 +59,15 @@ public static UnitConverter getInstance(@Nullable String sourceUnit, String targ } /** - * Register new instance of a converter that can then be retrieved with {@link - * #getInstance(String, String)}. + * Register new converter instance that can then be retrieved with {@link #getInstance(String, + * String)}. * * @param sourceUnit a source unit supported by the converter * @param targetUnit a target unit supported by the converter * @param convertingFunction a function that implements algorithm of conversion between {@code * sourceUnit} and {@code targetUnit} + * @throws IllegalArgumentException if source or target unit is empty, or when there is converter + * already registered for given {@code sourceUnit} and {@code targetUnit} */ // visible for testing static void registerConversion(