Handle non-numeric metric values #21

OwenCorrigan76 · 2024-06-10T10:00:25Z

Prometheus only supports numeric values for metrics. A few metrics emitted by Kafka brokers and clients have non-numeric values. This PR will move the non-numeric value to a label and then and set its value to 1.0, to satisfy Prometheus only supporting numeric values.

Reported in the following issue:
#8

Signed-off-by: Owen <[email protected]>

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

mimaison · 2024-06-10T12:09:15Z

Thanks for the PR.
I seems to only handle non-numeric metrics in KafkaMetricsCollector. You also need to do it in YammerMetricsCollector.convert()

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

Signed-off-by: Owen <[email protected]>

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

Signed-off-by: Owen <[email protected]>

src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

Signed-off-by: Owen <[email protected]>

mimaison

Thanks for the updates. I made another pass and left a few more comments.

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

mimaison · 2024-06-13T16:07:18Z

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

-        labels = new HashMap<>();
-        labels.put("key", "value");
-    }
+    private final Map<String, String> labels = Map.of("key", "value");


Since you're making this change here, do you want to do the same in YammerMetricsCollectorTest? as it has the exact same code.

We decided to keep as is because the ordering is clear

the reply doesn't really make sense, but I'm not too worried either way.

Sorry, wrong place for my last comment.

I'd still prefer to use the same logic in both KafkaMetricsCollectorTest and YammerMetricsCollectorTest. So either we revert this, or we do the same in YammerMetricsCollectorTest.

https://github.com/strimzi/metrics-reporter/pull/21/files#r1644743277 has not been addressed

Sorry, I missed this comment somehow. To keep uniformity, I will revert KafkaMetricsReporter to the former logic.
In trying to implement private final LinkedHashMap<String, String> tags = new LinkedHashMap<>(Map.of("key", "value", "key2", "value2")); , while the LinkedHashMap provided ordering guarantee, I think the Map.of method creates an immutable map, and using it directly to initialize a mutable LinkedHashMap might cause issues in some cases, causing the test to fail sometimes.

src/test/java/io/strimzi/kafka/metrics/YammerMetricsCollectorTest.java

Signed-off-by: Owen <[email protected]>

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java

Signed-off-by: Owen <[email protected]>

src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

k-wall

couple of nits, but I think this is looking close now.

mimaison

Thanks for the updates. I made a another pass and left a few comments.

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

mimaison · 2024-06-18T16:18:14Z

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

-        labels = new HashMap<>();
-        labels.put("key", "value");
-    }
+    private final Map<String, String> labels = Map.of("key", "value");


I'd still prefer to use the same logic in both KafkaMetricsCollectorTest and YammerMetricsCollectorTest. So either we revert this, or we do the same in YammerMetricsCollectorTest.

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

src/test/java/io/strimzi/kafka/metrics/YammerMetricsCollectorTest.java

mimaison · 2024-06-18T16:28:58Z

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

+        assertEquals("kafka_server_group_name", metricFamilySamples.name);
+        assertEquals(1, metricFamilySamples.samples.size());


These 2 assertions are checked again in assertMetricFamilySample(). Why are we checking them twice?

Duplicates removed

I think it would be more idiomatic to either check the size in assertMetricFamilySample(), or pass the unique sample (metricFamilySamples.samples.get(0)) to assertMetricFamilySample().

It's weird to check the sample size is one and still pass the list of samples to assertMetricFamilySample().

Same in YammerMetricsCollectorTest.

Signed-off-by: Owen <[email protected]>

k-wall

lgtm, thank you @OwenCorrigan76

scholzj

It would be great if you could provide a bit more details in the Pr description about what and how it does.

scholzj · 2024-06-20T14:05:52Z

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

-            LOG.info("Kafka metric {} is allowed", name);
-            LOG.info("labels " + metricName.tags());
-            MetricFamilySamples sample = convert(name, metricName.description(), kafkaMetric, metricName.tags());
+            LOG.info("Kafka metric {} is allowed", prometheusMetricName);


I know this was already here. But do we really need this on INFO level? Maybe DEBUG would be enough?

Yes we plan to address this in #6

src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java

mimaison

Thanks for the updates, I think we're almost there. I left a few small suggestions.

mimaison · 2024-06-20T14:21:50Z

src/main/java/io/strimzi/kafka/metrics/MetricFamilySamplesBuilder.java


 /**
 * Helper class to convert Kafka metrics into the Prometheus format.
 */
 public class MetricFamilySamplesBuilder {

+    private static final Logger LOG = LoggerFactory.getLogger(KafkaMetricsCollector.class.getName());


KafkaMetricsCollector -> MetricFamilySamplesBuilder

Silly mistake. Fixed

mimaison · 2024-06-20T14:23:46Z

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

-        labels = new HashMap<>();
-        labels.put("key", "value");
-    }
+    private final Map<String, String> labels = Map.of("key", "value");


https://github.com/strimzi/metrics-reporter/pull/21/files#r1644743277 has not been addressed

mimaison · 2024-06-20T14:29:28Z

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

+        assertEquals("kafka_server_group_name", metricFamilySamples.name);
+        assertEquals(1, metricFamilySamples.samples.size());


I think it would be more idiomatic to either check the size in assertMetricFamilySample(), or pass the unique sample (metricFamilySamples.samples.get(0)) to assertMetricFamilySample().

It's weird to check the sample size is one and still pass the list of samples to assertMetricFamilySample().

Same in YammerMetricsCollectorTest.

mimaison · 2024-06-20T14:30:10Z

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java

@@ -90,8 +113,8 @@ private KafkaMetric buildMetric(String name, String group, double value) {
                time);
    }

-    private KafkaMetric buildNonNumericMetric(String name, String group) {
-        Gauge<String> measurable = (config, now) -> "hello";
+    private KafkaMetric buildNonNumericMetric(String name, String group, String nonNumericValue) {


Nit: this can be just value. It's type String, so we know it's not numeric.

Signed-off-by: Owen <[email protected]>

OwenCorrigan76 · 2024-06-20T16:40:11Z

@scholzj Is the PR description better? Thanks

mimaison

Thanks for implementing this feature and addressing all the comments!

There are still a couple of things not quite right in the tests (for example we access the first item before checking the collection size is 1 in assertMetricFamilySample()) but we can address them in follow up PRs.

LGTM

handle non-numeric metric values

b5f20c3

Signed-off-by: Owen <[email protected]>

OwenCorrigan76 added this to the 0.1.0 milestone Jun 10, 2024

OwenCorrigan76 requested review from mimaison and k-wall June 10, 2024 10:05

k-wall reviewed Jun 10, 2024

View reviewed changes

mimaison reviewed Jun 10, 2024

View reviewed changes

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved

mimaison reviewed Jun 10, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java Outdated Show resolved Hide resolved

mimaison reviewed Jun 10, 2024

View reviewed changes

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved

addressing comments -TODO- YammerMetricsCollector.convert

43dc708

Signed-off-by: Owen <[email protected]>

OwenCorrigan76 force-pushed the non-numeric_values branch from f15637d to 43dc708 Compare June 10, 2024 15:30

k-wall reviewed Jun 11, 2024

View reviewed changes

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved

k-wall reviewed Jun 11, 2024

View reviewed changes

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved

addressing comments

9600bf7

Signed-off-by: Owen <[email protected]>

OwenCorrigan76 requested review from mimaison and k-wall June 12, 2024 09:51

k-wall reviewed Jun 12, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java Outdated Show resolved Hide resolved

k-wall reviewed Jun 12, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java Outdated Show resolved Hide resolved

k-wall reviewed Jun 12, 2024

View reviewed changes

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved

addressing assertMetricFamilySample in tests

fd67d75

Signed-off-by: Owen <[email protected]>

mimaison reviewed Jun 13, 2024

View reviewed changes

OwenCorrigan76 force-pushed the non-numeric_values branch from d3cb71a to 00e23c9 Compare June 17, 2024 11:10

addressing more comments

232c363

Signed-off-by: Owen <[email protected]>

OwenCorrigan76 force-pushed the non-numeric_values branch from 00e23c9 to 232c363 Compare June 17, 2024 11:14

OwenCorrigan76 requested review from mimaison and k-wall June 17, 2024 11:19

k-wall reviewed Jun 17, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java Outdated Show resolved Hide resolved

k-wall reviewed Jun 17, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java Outdated Show resolved Hide resolved

k-wall reviewed Jun 17, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java Outdated Show resolved Hide resolved

k-wall reviewed Jun 17, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java Outdated Show resolved Hide resolved

k-wall reviewed Jun 17, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java Outdated Show resolved Hide resolved

addressing Keith's comments

20f10a9

Signed-off-by: Owen <[email protected]>

k-wall reviewed Jun 18, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java Outdated Show resolved Hide resolved

k-wall reviewed Jun 18, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java Show resolved Hide resolved

k-wall reviewed Jun 18, 2024

View reviewed changes

mimaison reviewed Jun 18, 2024

View reviewed changes

OwenCorrigan76 added 2 commits June 20, 2024 12:46

addressing more comments

158a95a

Signed-off-by: Owen <[email protected]>

add missing javadoc

fd8ff29

Signed-off-by: Owen <[email protected]>

OwenCorrigan76 requested review from mimaison and k-wall June 20, 2024 12:14

k-wall approved these changes Jun 20, 2024

View reviewed changes

scholzj reviewed Jun 20, 2024

View reviewed changes

mimaison reviewed Jun 20, 2024

View reviewed changes

addressing more reviews

3c3bd8c

Signed-off-by: Owen <[email protected]>

OwenCorrigan76 requested review from mimaison and scholzj June 20, 2024 16:44

scholzj approved these changes Jun 20, 2024

View reviewed changes

k-wall approved these changes Jun 21, 2024

View reviewed changes

mimaison approved these changes Jun 21, 2024

View reviewed changes

mimaison merged commit 2e542e9 into strimzi:main Jun 21, 2024
7 checks passed

mimaison mentioned this pull request Jun 21, 2024

Handle non-numeric metric values #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle non-numeric metric values #21

Handle non-numeric metric values #21

OwenCorrigan76 commented Jun 10, 2024 •

edited

Loading

mimaison commented Jun 10, 2024

mimaison left a comment

mimaison Jun 13, 2024

OwenCorrigan76 Jun 17, 2024

k-wall Jun 17, 2024

OwenCorrigan76 Jun 18, 2024

mimaison Jun 18, 2024

mimaison Jun 20, 2024

OwenCorrigan76 Jun 20, 2024

k-wall left a comment

mimaison left a comment

mimaison Jun 18, 2024

mimaison Jun 18, 2024

OwenCorrigan76 Jun 20, 2024

mimaison Jun 20, 2024

k-wall left a comment

scholzj left a comment

scholzj Jun 20, 2024

mimaison Jun 20, 2024

mimaison left a comment

mimaison Jun 20, 2024

OwenCorrigan76 Jun 20, 2024

mimaison Jun 20, 2024

mimaison Jun 20, 2024

mimaison Jun 20, 2024

OwenCorrigan76 Jun 20, 2024

OwenCorrigan76 commented Jun 20, 2024

mimaison left a comment

		assertEquals("kafka_server_group_name", metricFamilySamples.name);
		assertEquals(1, metricFamilySamples.samples.size());

Handle non-numeric metric values #21

Handle non-numeric metric values #21

Conversation

OwenCorrigan76 commented Jun 10, 2024 • edited Loading

mimaison commented Jun 10, 2024

mimaison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-wall left a comment

Choose a reason for hiding this comment

mimaison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-wall left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimaison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenCorrigan76 commented Jun 20, 2024

mimaison left a comment

Choose a reason for hiding this comment

OwenCorrigan76 commented Jun 10, 2024 •

edited

Loading