From fd073f9b9fc039c7bfd7e62ece4670b9a4a6197b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20St=C3=A4ber?= Date: Fri, 15 Dec 2023 13:06:17 +0100 Subject: [PATCH] Code review fulfillment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fabian Stäber --- dependencyManagement/build.gradle.kts | 1 + exporters/prometheus/build.gradle.kts | 5 +- .../prometheus/Otel2PrometheusConverter.java | 10 +- .../prometheus/PrometheusHttpServer.java | 5 +- .../PrometheusHttpServerBuilder.java | 9 +- .../prometheus/PrometheusUnitsHelper.java | 22 +++- .../PrometheusMetricReaderTest.java | 44 ++++++++ .../prometheus/PrometheusUnitsHelperTest.java | 103 ++++++++++++++++++ 8 files changed, 180 insertions(+), 19 deletions(-) create mode 100644 exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelperTest.java diff --git a/dependencyManagement/build.gradle.kts b/dependencyManagement/build.gradle.kts index 9e1ba7ef698..b7f0cb5eec9 100644 --- a/dependencyManagement/build.gradle.kts +++ b/dependencyManagement/build.gradle.kts @@ -60,6 +60,7 @@ val DEPENDENCIES = listOf( "com.google.guava:guava-beta-checker:1.0", "com.sun.net.httpserver:http:20070405", "com.tngtech.archunit:archunit-junit5:1.2.1", + "io.prometheus:prometheus-metrics-exporter-httpserver:1.1.0", "com.uber.nullaway:nullaway:0.10.18", "edu.berkeley.cs.jqf:jqf-fuzz:1.7", // jqf-fuzz version 1.8+ requires Java 11+ "eu.rekawek.toxiproxy:toxiproxy-java:2.1.7", diff --git a/exporters/prometheus/build.gradle.kts b/exporters/prometheus/build.gradle.kts index f2d8497e6ec..31044c70b23 100644 --- a/exporters/prometheus/build.gradle.kts +++ b/exporters/prometheus/build.gradle.kts @@ -12,16 +12,15 @@ dependencies { api(project(":sdk:metrics")) implementation(project(":sdk-extensions:autoconfigure-spi")) - implementation("io.prometheus:prometheus-metrics-exporter-httpserver:1.1.0") + implementation("io.prometheus:prometheus-metrics-exporter-httpserver") - compileOnly("com.sun.net.httpserver:http") compileOnly("com.google.auto.value:auto-value-annotations") annotationProcessor("com.google.auto.value:auto-value") testImplementation(project(":sdk:testing")) testImplementation("io.opentelemetry.proto:opentelemetry-proto") - + testImplementation("com.sun.net.httpserver:http") testImplementation("com.google.guava:guava") testImplementation("com.linecorp.armeria:armeria") testImplementation("com.linecorp.armeria:armeria-junit5") diff --git a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java index 30247f6237f..5ed5b14d71f 100644 --- a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java +++ b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java @@ -65,7 +65,7 @@ import javax.annotation.Nullable; /** Convert OpenTelemetry {@link MetricData} to Prometheus {@link MetricSnapshots}. */ -public class Otel2PrometheusConverter { +final class Otel2PrometheusConverter { private static final Logger LOGGER = Logger.getLogger(Otel2PrometheusConverter.class.getName()); private static final ThrottlingLogger THROTTLING_LOGGER = new ThrottlingLogger(LOGGER); @@ -74,21 +74,17 @@ public class Otel2PrometheusConverter { private static final String OTEL_SCOPE_VERSION = "otel_scope_version"; private static final long NANOS_PER_MILLISECOND = TimeUnit.MILLISECONDS.toNanos(1); - public Otel2PrometheusConverter() { - this(true); - } - /** * Constructor with feature flag parameter. * * @param otelScopeEnabled enable generation of the OpenTelemetry instrumentation scope info * metric and labels. */ - public Otel2PrometheusConverter(boolean otelScopeEnabled) { + Otel2PrometheusConverter(boolean otelScopeEnabled) { this.otelScopeEnabled = otelScopeEnabled; } - public MetricSnapshots convert(@Nullable Collection metricDataCollection) { + MetricSnapshots convert(@Nullable Collection metricDataCollection) { if (metricDataCollection == null || metricDataCollection.isEmpty()) { return MetricSnapshots.of(); } diff --git a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java index 38213dffb27..d6330b50ecd 100644 --- a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java +++ b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java @@ -54,8 +54,9 @@ public static PrometheusHttpServerBuilder builder() { String host, int port, @Nullable ExecutorService executor, - PrometheusRegistry prometheusRegistry) { - this.prometheusMetricReader = new PrometheusMetricReader(/* otelScopeEnabled= */ true); + PrometheusRegistry prometheusRegistry, + boolean otelScopeEnabled) { + this.prometheusMetricReader = new PrometheusMetricReader(otelScopeEnabled); this.host = host; this.prometheusRegistry = prometheusRegistry; prometheusRegistry.register(prometheusMetricReader); diff --git a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerBuilder.java b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerBuilder.java index e9fc5a1a0e2..db99362ad31 100644 --- a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerBuilder.java +++ b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerBuilder.java @@ -21,6 +21,7 @@ public final class PrometheusHttpServerBuilder { private String host = DEFAULT_HOST; private int port = DEFAULT_PORT; private PrometheusRegistry prometheusRegistry = PrometheusRegistry.defaultRegistry; + private boolean otelScopeEnabled = true; @Nullable private ExecutorService executor; @@ -53,12 +54,18 @@ public PrometheusHttpServerBuilder setPrometheusRegistry(PrometheusRegistry prom return this; } + /** Set if the {@code otel_scope_*} attributes are generated. Default is {@code true}. */ + public PrometheusHttpServerBuilder setOtelScopeEnabled(boolean otelScopeEnabled) { + this.otelScopeEnabled = otelScopeEnabled; + return this; + } + /** * Returns a new {@link PrometheusHttpServer} with the configuration of this builder which can be * registered with a {@link io.opentelemetry.sdk.metrics.SdkMeterProvider}. */ public PrometheusHttpServer build() { - return new PrometheusHttpServer(host, port, executor, prometheusRegistry); + return new PrometheusHttpServer(host, port, executor, prometheusRegistry, otelScopeEnabled); } PrometheusHttpServerBuilder() {} diff --git a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelper.java b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelper.java index 94c8bf15698..9657b88ada5 100644 --- a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelper.java +++ b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelper.java @@ -21,6 +21,9 @@ class PrometheusUnitsHelper { // https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/c3b2997563106e11d39f66eec629fde25dce2bdd/pkg/translator/prometheus/normalize_name.go#L19-L19 static { // Time + initUnit("a", "years", "year"); + initUnit("mo", "months", "month"); + initUnit("wk", "weeks", "week"); initUnit("d", "days", "day"); initUnit("h", "hours", "hour"); initUnit("min", "minutes", "minute"); @@ -71,17 +74,24 @@ static Unit convertUnit(String otelUnit) { // SDK. return null; } + if (otelUnit.contains("{")) { + otelUnit = otelUnit.replaceAll("\\{[^}]*}", "").trim(); + if (otelUnit.isEmpty() || otelUnit.equals("/")) { + return null; + } + } if (predefinedUnits.containsKey(otelUnit)) { return predefinedUnits.get(otelUnit); } - if (otelUnit.contains("{")) { - return null; - } if (otelUnit.contains("/")) { String[] parts = otelUnit.split("/", 2); - String part1 = pluralNames.getOrDefault(parts[0], parts[0]); - String part2 = singularNames.getOrDefault(parts[1], parts[1]); - return new Unit(part1 + "_per_" + part2); + String part1 = pluralNames.getOrDefault(parts[0], parts[0]).trim(); + String part2 = singularNames.getOrDefault(parts[1], parts[1]).trim(); + if (part1.isEmpty()) { + return new Unit("per_" + part2); + } else { + return new Unit(part1 + "_per_" + part2); + } } return new Unit(otelUnit); } diff --git a/exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusMetricReaderTest.java b/exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusMetricReaderTest.java index efb8f9cdbee..426c3e66d7b 100644 --- a/exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusMetricReaderTest.java +++ b/exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusMetricReaderTest.java @@ -826,6 +826,50 @@ public void testExponentialLongHistogramScaleDown() throws IOException { Assertions.assertEquals(10, buckets.getCount(2)); } + @Test + public void testInstrumentationScope() throws IOException { + SdkMeterProvider meterProvider = + SdkMeterProvider.builder() + .setClock(testClock) + .registerMetricReader(this.reader) + .setResource( + Resource.getDefault().toBuilder().put("telemetry.sdk.version", "1.x.x").build()) + .build(); + Meter meter1 = meterProvider.meterBuilder("scopeA").setInstrumentationVersion("1.1").build(); + Meter meter2 = meterProvider.meterBuilder("scopeB").setInstrumentationVersion("1.2").build(); + meter1 + .counterBuilder("processing.time") + .setDescription("processing time in seconds") + .setUnit("s") + .ofDoubles() + .build() + .add(3.3, Attributes.builder().put("a", "b").build()); + meter2 + .counterBuilder("processing.time") + .setDescription("processing time in seconds") + .setUnit("s") + .ofDoubles() + .build() + .add(3.3, Attributes.builder().put("a", "b").build()); + String expected = + "" + + "# TYPE processing_time_seconds counter\n" + + "# UNIT processing_time_seconds seconds\n" + + "# HELP processing_time_seconds processing time in seconds\n" + + "processing_time_seconds_total{a=\"b\",otel_scope_name=\"scopeA\",otel_scope_version=\"1.1\"} 3.3\n" + + "processing_time_seconds_created{a=\"b\",otel_scope_name=\"scopeA\",otel_scope_version=\"1.1\"} " + + createdTimestamp + + "\n" + + "processing_time_seconds_total{a=\"b\",otel_scope_name=\"scopeB\",otel_scope_version=\"1.2\"} 3.3\n" + + "processing_time_seconds_created{a=\"b\",otel_scope_name=\"scopeB\",otel_scope_version=\"1.2\"} " + + createdTimestamp + + "\n" + + "# TYPE target info\n" + + "target_info{service_name=\"unknown_service:java\",telemetry_sdk_language=\"java\",telemetry_sdk_name=\"opentelemetry\",telemetry_sdk_version=\"1.x.x\"} 1\n" + + "# EOF\n"; + assertEquals(expected, toOpenMetrics(reader.collect())); + } + @Test public void testNameSuffix() throws IOException { LongCounter unitAndTotal = diff --git a/exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelperTest.java b/exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelperTest.java new file mode 100644 index 00000000000..aa44005978d --- /dev/null +++ b/exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelperTest.java @@ -0,0 +1,103 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.exporter.prometheus; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import io.prometheus.metrics.model.snapshots.Unit; +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class PrometheusUnitsHelperTest { + + @ParameterizedTest + @MethodSource("providePrometheusOTelUnitEquivalentPairs") + public void testPrometheusUnitEquivalency(String otlpUnit, String expectedPrometheusUnit) { + Unit actualPrometheusUnit = PrometheusUnitsHelper.convertUnit(otlpUnit); + if (expectedPrometheusUnit == null) { + assertNull(actualPrometheusUnit); + } else { + assertEquals(expectedPrometheusUnit, actualPrometheusUnit.toString()); + } + } + + private static Stream providePrometheusOTelUnitEquivalentPairs() { + return Stream.of( + // Simple expansion - storage Bytes + Arguments.of("By", "bytes"), + // Simple expansion - storage KBy + Arguments.of("KBy", "kilobytes"), + // Simple expansion - storage MBy + Arguments.of("MBy", "megabytes"), + // Simple expansion - storage GBy + Arguments.of("GBy", "gigabytes"), + // Simple expansion - storage TBy + Arguments.of("TBy", "terabytes"), + // Simple expansion - storage KiBy + Arguments.of("KiBy", "kibibytes"), + // Simple expansion - storage MiBy + Arguments.of("MiBy", "mebibytes"), + // Simple expansion - storage GiBy + Arguments.of("GiBy", "gibibytes"), + // Simple expansion - storage TiBy + Arguments.of("TiBy", "tibibytes"), + // Simple expansion - Time unit d + Arguments.of("d", "days"), + // Simple expansion - Time unit h + Arguments.of("h", "hours"), + // Simple expansion - Time unit s + Arguments.of("s", "seconds"), + // Simple expansion - Time unit ms + Arguments.of("ms", "milliseconds"), + // Simple expansion - Time unit us + Arguments.of("us", "microseconds"), + // Simple expansion - Time unit ns + Arguments.of("ns", "nanoseconds"), + // Simple expansion - Time unit min + Arguments.of("min", "minutes"), + // Simple expansion - special symbol - % + Arguments.of("%", "percent"), + // Simple expansion - frequency + Arguments.of("Hz", "hertz"), + // Simple expansion - temperature + Arguments.of("Cel", "celsius"), + // Unit not found - Case sensitive + Arguments.of("S", "S"), + // Special case - 1 + Arguments.of("1", null), + // Special Case - Drop metric units in {} + Arguments.of("{packets}", null), + // Special Case - Dropped metric units only in {} + Arguments.of("{packets}V", "volts"), + // Special Case - Dropped metric units with 'per' unit handling applicable + Arguments.of("{scanned}/{returned}", null), + // Special Case - Dropped metric units with 'per' unit handling applicable + Arguments.of("{objects}/s", "per_second"), + // Units expressing rate - 'per' units, both units expanded + Arguments.of("m/s", "meters_per_second"), + // Units expressing rate - per minute + Arguments.of("m/min", "meters_per_minute"), + // Units expressing rate - per day + Arguments.of("A/d", "amperes_per_day"), + // Units expressing rate - per week + Arguments.of("W/wk", "watts_per_week"), + // Units expressing rate - per month + Arguments.of("J/mo", "joules_per_month"), + // Units expressing rate - per year + Arguments.of("TBy/a", "terabytes_per_year"), + // Units expressing rate - 'per' units, both units unknown + Arguments.of("v/v", "v_per_v"), + // Units expressing rate - 'per' units, first unit unknown + Arguments.of("km/h", "km_per_hour"), + // Units expressing rate - 'per' units, 'per' unit unknown + Arguments.of("g/x", "grams_per_x"), + // Misc - unit containing known abbreviations improperly formatted + Arguments.of("watts_W", "watts_W")); + } +}