Add warning log when duplicate leables are created or detected #29

OwenCorrigan76 · 2024-07-08T14:17:19Z

The logic that handles non-numeric metrics is in the DataPointSnapshotBuilder class. Here is where a new label with a metric value is created.
Currently we unconditionally add our new label without checking if a label with the same name exists already.
This PR logs a warning if duplicate labes are detected rather than throwing an IllegalArgumentException

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

OwenCorrigan76 · 2024-07-08T14:31:16Z

@mimaison I've refactored the infoDataPoint to use the contains method. This takes care of the newly created labels right?
Is it in gaugeDataPoint that I should handle already existing lables?

mimaison

Thanks for the PR. I left a few suggestions. This also does not currently address the first issue identified in #22 (comment)

mimaison · 2024-07-08T14:31:26Z

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

@@ -12,20 +12,30 @@
 import io.prometheus.metrics.model.snapshots.Quantiles;
 import io.prometheus.metrics.model.snapshots.SummarySnapshot;

+import java.util.logging.Logger;


We should use org.slf4j.Logger instead of java.util.logging.Logger

That's why I couldn't use interpolation with {}. I was using the java.util logger. Changed to sl4j

mimaison · 2024-07-08T14:32:53Z

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

    public static InfoSnapshot.InfoDataPointSnapshot infoDataPoint(Labels labels, Object value, String metricName) {
-        Labels newLabels = labels.add(PrometheusNaming.sanitizeLabelName(metricName), String.valueOf(value));
+        String sanitizedMetricName = PrometheusNaming.sanitizeLabelName(metricName);


I think naming this newLabelName would better describe its use.

Changed to 'newLabelName`

mimaison · 2024-07-08T14:33:32Z

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

+        String sanitizedMetricName = PrometheusNaming.sanitizeLabelName(metricName);
+        Labels newLabels = labels;
+        if (labels.contains(sanitizedMetricName)) {
+            LOG.warning("Ignoring metric value duplicate key: " + sanitizedMetricName);


It's probably worth print a bit more information such as the metric name so operators can identify the culprit.

Also we should not use concatenation in log lines but instead use interpolation with the {} syntax which is possible with the slf4j Logger

I appended from metric; metricName. Is that sufficient.
{} interpolation now used

mimaison · 2024-07-08T14:35:32Z

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

@@ -108,6 +109,19 @@ public void testCollectNonNumericMetric() {
        assertEquals(expectedLabels, snapshot.getDataPoints().get(0).getLabels());
    }

+    @Test
+    public void testLabelNameSanitizing() {


We want to avoid tests that only exercise code from dependencies. This test only validates PrometheusNaming.sanitizeLabelName() and it does not check anything from our metric reporter.

mimaison · 2024-07-08T14:37:43Z

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

+    @Test
+    public void testLabelDuplicateWarning() {
+        Labels labels = Labels.builder().label("k1", "v1").build();
+        InfoSnapshot.InfoDataPointSnapshot snapshot = DataPointSnapshotBuilder.infoDataPoint(labels, "v1", "k1");


We should use a different value than v1 here. Otherwise this does not assert we did not replace the value.

Yes, I missed that. Changed to just value.

mimaison · 2024-07-08T14:46:52Z

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

+    }
+
+    @Test
+    public void testLabelDuplicateWarning() {


I think we should put this test in a new class DataPointSnapshotBuilderTest

New DataPointSnapshotBuilderTest class created and this test is moved to there

mimaison · 2024-07-08T14:51:21Z

@mimaison I've refactored the infoDataPoint to use the contains method. This takes care of the newly created labels right? Is it in gaugeDataPoint that I should handle already existing lables?

No, it's in the 2 places where we create Labels objects:

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

mimaison

Thanks for the updates! It looks good overall. I left a few minor suggestions.

mimaison · 2024-07-10T15:17:08Z

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

+        String newLabelName = PrometheusNaming.sanitizeLabelName(metricName);
+        Labels newLabels = labels;
+        if (labels.contains(newLabelName)) {
+            LOG.warn("Ignoring metric value duplicate key: {} from metric: {}",  newLabelName, metricName);


I wonder if it would be worth checking the value of the existing label and only print the warning if the value is different from value. WDYT?

Can we also improve the message? We're not ignoring the metric value, we're unable to add a new label.

Refactored. Is the new message ok?

mimaison · 2024-07-10T15:19:00Z

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

@@ -122,10 +124,16 @@ private String metricName(MetricName metricName) {
                .toLowerCase(Locale.ROOT);
    }

-    private static Labels labelsFromTags(Map<String, String> tags) {
+    protected static Labels labelsFromTags(Map<String, String> tags) {


We don't need protected, we don't expect subclasses

@mimaison I'm calling labelsFromTags in the test:

@Test public void testLabelsFromTags() { Map<String, String> tags = new LinkedHashMap<>(); tags.put("k-1", "v1"); tags.put("k_1", "v2"); Labels labels = KafkaMetricsCollector.labelsFromTags(tags); assertEquals("k_1", PrometheusNaming.sanitizeLabelName("k-1")); assertEquals("v1", labels.get("k_1")); assertEquals(1, labels.size()); }

Is there another way around this? Thanks

I'm calling labelsFromTags in the test
How is this an issue?

Have you tried removing protected and see what happens?
You may want to review https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html

Yes it goes to package-private. I misunderstood your comment. I thought you wanted to keep it as private and maybe had forgotten I was calling it from the test. My bad!

Removed protected

mimaison · 2024-07-10T15:20:17Z

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

+            if (labelNames.add(newLabelName)) {
+                builder.label(newLabelName, label.getValue());
+            } else {
+                LOG.warn("Ignoring duplicate label key: {} with value: {}", newLabelName, label.getValue());


If wonder if it's worth passing the metric name so the message helps identify the impacted metric.

Same in YammerMetricsCollector.

Metric name passed and added to the LOG

mimaison · 2024-07-10T15:25:13Z

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

+    @Test
+    public void testNewLabelDuplicateWarning() {
+        Labels labels = Labels.builder().label("k1", "v1").label("k2", "v2").build();
+        InfoSnapshot.InfoDataPointSnapshot snapshot = DataPointSnapshotBuilder.infoDataPoint(labels, "value", "k1");


I think it would be interesting to also test with labels that are not equal but collide when turned into Prometheus labels, for example k-1 and k_1

Just like in the other tests?
assertEquals("k_1", PrometheusNaming.sanitizeLabelName("k-1"));

No, I mean using label names in Labels.builder().label() and DataPointSnapshotBuilder.infoDataPoint() that are not equal but collide. Currently we use k1 in both.

Changed to also test with labels that are not equal but collide when turned into Prometheus labels

mimaison · 2024-07-10T15:26:20Z

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

+public class DataPointSnapshotBuilderTest {
+
+    @Test
+    public void testNewLabelDuplicateWarning() {


I think a better name would be something like testCollidingNewLabelIsIgnored

Changed to testCollidingNewLabelIsIgnored

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

mimaison · 2024-07-12T12:24:05Z

@OwenCorrigan76 There's a checkstyle issue that's failing the build: https://dev.azure.com/cncf/strimzi/_build/results?buildId=174948&view=logs&j=441a6687-78c7-548e-9324-bade41cb68f6&t=cce61017-8585-548c-ef2e-c6a3bc420f66. Can you fix it? Thanks

OwenCorrigan76 · 2024-07-12T13:32:08Z

Sorry, didn't do a mvn compile .Fixing now.

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

mimaison

Thanks for the PR! LGTM

scholzj

One nit. LGTM otherwise.

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

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

duplicate key warning

c25469d

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

OwenCorrigan76 added this to the 0.1.0 milestone Jul 8, 2024

mimaison reviewed Jul 8, 2024

View reviewed changes

duplicate key warning and tests

7ede3d8

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

OwenCorrigan76 requested a review from mimaison July 10, 2024 10:22

mimaison reviewed Jul 10, 2024

View reviewed changes

refactor to address comments

8f3c7f7

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

OwenCorrigan76 requested review from mimaison and scholzj July 12, 2024 09:19

test non-equal labels

3660769

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

fix checkstyle

b24f143

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

mimaison approved these changes Jul 12, 2024

View reviewed changes

scholzj approved these changes Jul 15, 2024

View reviewed changes

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

remove unwanted empty line

5704539

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

mimaison merged commit 210e4d0 into strimzi:main Jul 16, 2024
7 checks passed

Add warning log when duplicate leables are created or detected #29

Add warning log when duplicate leables are created or detected #29

Conversation

OwenCorrigan76 commented Jul 8, 2024

OwenCorrigan76 commented Jul 8, 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

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

mimaison commented Jul 8, 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

OwenCorrigan76 Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenCorrigan76 Jul 11, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimaison commented Jul 12, 2024

OwenCorrigan76 commented Jul 12, 2024

mimaison left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

OwenCorrigan76 Jul 11, 2024 •

edited

Loading

OwenCorrigan76 Jul 11, 2024 •

edited

Loading