From e02029bb8eac74a53617453099bedea528e864a0 Mon Sep 17 00:00:00 2001 From: Alexandre <4717855+alexcsf@users.noreply.github.com> Date: Tue, 13 Sep 2022 15:07:37 +0100 Subject: [PATCH] Append missing _total suffix when registering Counter metrics. (#177) * Append missing _total suffix when registering Counter metrics. This ensures consistent metric names across the Prometheus registry, the MetricsContainer and across the JSON and Prometheus exposition formats available at /metrics. Also adds basic logging and fixes docs. See: https://github.com/jitsi/jitsi-videobridge/pull/1942 * fix: MediaType annotation for application/json. Fixes JSON output showing up in text/html when querying via browser. * log: Change log level to DEBUG. * test: Add tests to cover counter "_total" suffix. * Add check for base metric name when registering a counter. --- .../org/jitsi/rest/prometheus/Prometheus.java | 2 +- .../kotlin/org/jitsi/metrics/MetricsContainer.kt | 16 +++++++++++----- .../org/jitsi/metrics/MetricsContainerTest.kt | 7 +++++++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/jicoco-metrics/src/main/java/org/jitsi/rest/prometheus/Prometheus.java b/jicoco-metrics/src/main/java/org/jitsi/rest/prometheus/Prometheus.java index 1d6a045..8341730 100644 --- a/jicoco-metrics/src/main/java/org/jitsi/rest/prometheus/Prometheus.java +++ b/jicoco-metrics/src/main/java/org/jitsi/rest/prometheus/Prometheus.java @@ -58,7 +58,7 @@ public String getPrometheusOpenMetrics() } @GET - @Produces({MediaType.APPLICATION_JSON, "*/*;q=0"}) + @Produces(MediaType.APPLICATION_JSON) public String getJsonString() { return metricsContainer.getJsonString(); diff --git a/jicoco-metrics/src/main/kotlin/org/jitsi/metrics/MetricsContainer.kt b/jicoco-metrics/src/main/kotlin/org/jitsi/metrics/MetricsContainer.kt index b12d4eb..e0b0c62 100644 --- a/jicoco-metrics/src/main/kotlin/org/jitsi/metrics/MetricsContainer.kt +++ b/jicoco-metrics/src/main/kotlin/org/jitsi/metrics/MetricsContainer.kt @@ -17,6 +17,7 @@ package org.jitsi.metrics import io.prometheus.client.CollectorRegistry import io.prometheus.client.exporter.common.TextFormat +import org.jitsi.utils.logging2.createLogger import org.json.simple.JSONObject import java.io.IOException import java.io.StringWriter @@ -28,6 +29,7 @@ open class MetricsContainer @JvmOverloads constructor( /** the registry used to register metrics */ val registry: CollectorRegistry = CollectorRegistry.defaultRegistry ) { + private val logger = createLogger() /** * Namespace prefix added to all metrics. @@ -98,6 +100,7 @@ open class MetricsContainer @JvmOverloads constructor( /** * Creates and registers a [CounterMetric] with the given [name], [help] string and optional [initialValue]. + * If omitted, a "_total" suffix is added to the metric name to ensure consistent (Counter) metric naming. * * Throws an exception if a metric with the same name but a different type exists. */ @@ -110,17 +113,20 @@ open class MetricsContainer @JvmOverloads constructor( /** the optional initial value of the metric */ initialValue: Long = 0 ): CounterMetric { - if (metrics.containsKey(name)) { + val newName = if (name.endsWith("_total")) name else "${name}_total".also { + logger.debug("Counter '$name' was renamed to '$it' to ensure consistent metric naming.") + } + if (metrics.containsKey(newName) or metrics.containsKey(name)) { if (checkForNameConflicts) { - throw RuntimeException("Could not register metric '$name'. A metric with that name already exists.") + throw RuntimeException("Could not register metric '$newName'. A metric with that name already exists.") } - return metrics[name] as CounterMetric + return metrics[newName] as CounterMetric } - return CounterMetric(name, help, namespace, initialValue).apply { metrics[name] = register(registry) } + return CounterMetric(newName, help, namespace, initialValue).apply { metrics[newName] = register(registry) } } /** - * Creates and registers a [CounterMetric] with the given [name], [help] string and optional [initialValue]. + * Creates and registers a [LongGaugeMetric] with the given [name], [help] string and optional [initialValue]. * * Throws an exception if a metric with the same name but a different type exists. */ diff --git a/jicoco-metrics/src/test/kotlin/org/jitsi/metrics/MetricsContainerTest.kt b/jicoco-metrics/src/test/kotlin/org/jitsi/metrics/MetricsContainerTest.kt index fc80bb9..cfed1a1 100644 --- a/jicoco-metrics/src/test/kotlin/org/jitsi/metrics/MetricsContainerTest.kt +++ b/jicoco-metrics/src/test/kotlin/org/jitsi/metrics/MetricsContainerTest.kt @@ -38,13 +38,20 @@ class MetricsContainerTest : ShouldSpec() { context("while checking for name conflicts") { should("throw a RuntimeException") { shouldThrow { mc.registerBooleanMetric("boolean", "A boolean metric") } + // "counter" is renamed to "counter_total" so both should throw an exception + shouldThrow { mc.registerCounter("counter", "A counter metric") } + shouldThrow { mc.registerCounter("counter_total", "A counter metric") } + // we test this because the Prometheus JVM library stores Counters without the "_total" suffix + shouldThrow { mc.registerCounter("boolean_total", "A counter metric") } } } context("without checking for name conflicts") { mc.checkForNameConflicts = false should("return an existing metric") { booleanMetric shouldBe mc.registerBooleanMetric("boolean", "A boolean metric") + // "counter" is renamed to "counter_total" so both should return the same metric counter shouldBe mc.registerCounter("counter", "A counter metric") + counter shouldBe mc.registerCounter("counter_total", "A counter metric") info shouldBe mc.registerInfo("info", "An info metric", "value") longGauge shouldBe mc.registerLongGauge("gauge", "A gauge metric") }