Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jul 16, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c25469d
duplicate key warning
OwenCorrigan76 Jul 8, 2024
7ede3d8
duplicate key warning and tests
OwenCorrigan76 Jul 10, 2024
8f3c7f7
refactor to address comments
OwenCorrigan76 Jul 12, 2024
3660769
test non-equal labels
OwenCorrigan76 Jul 12, 2024
b24f143
fix checkstyle
OwenCorrigan76 Jul 12, 2024
5704539
remove unwanted empty line
OwenCorrigan76 Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/main/java/io/strimzi/kafka/metrics/DataPointSnapshotBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,32 @@
import io.prometheus.metrics.model.snapshots.PrometheusNaming;
import io.prometheus.metrics.model.snapshots.Quantiles;
import io.prometheus.metrics.model.snapshots.SummarySnapshot;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Builder methods to convert Kafka metrics into Prometheus metrics
*/
public class DataPointSnapshotBuilder {

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

/**
* Create a datapoint for a {@link InfoSnapshot} metric
* @param labels The labels associated with the datapoint
* @param value The value to insert as a label
* @param metricName The name of the new label
* @return The {@link InfoSnapshot.InfoDataPointSnapshot} datapoint
*/

scholzj marked this conversation as resolved.
Show resolved Hide resolved
public static InfoSnapshot.InfoDataPointSnapshot infoDataPoint(Labels labels, Object value, String metricName) {
Labels newLabels = labels.add(PrometheusNaming.sanitizeLabelName(metricName), String.valueOf(value));
String newLabelName = PrometheusNaming.sanitizeLabelName(metricName);
Labels newLabels = labels;
if (labels.contains(newLabelName)) {
LOG.warn("Ignoring metric value duplicate key: {} from metric: {}", newLabelName, metricName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored. Is the new message ok?

} else {
newLabels = labels.add(newLabelName, String.valueOf(value));
}
return InfoSnapshot.InfoDataPointSnapshot.builder()
.labels(newLabels)
.build();
Expand Down
12 changes: 10 additions & 2 deletions src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

/**
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@OwenCorrigan76 OwenCorrigan76 Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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());
   }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way around this? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@OwenCorrigan76 OwenCorrigan76 Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed protected

Labels.Builder builder = Labels.builder();
Set<String> labelNames = new HashSet<>();
for (Map.Entry<String, String> label : tags.entrySet()) {
builder.label(PrometheusNaming.sanitizeLabelName(label.getKey()), label.getValue());
String newLabelName = PrometheusNaming.sanitizeLabelName(label.getKey());
if (labelNames.add(newLabelName)) {
builder.label(newLabelName, label.getValue());
} else {
LOG.warn("Ignoring duplicate label key: {} with value: {}", newLabelName, label.getValue());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Same in YammerMetricsCollector.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metric name passed and added to the LOG

}
}
return builder.build();
}
Expand Down
10 changes: 9 additions & 1 deletion src/main/java/io/strimzi/kafka/metrics/YammerMetricsCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;

/**
* Prometheus Collector to store and export metrics retrieved by {@link YammerPrometheusMetricsReporter}.
Expand Down Expand Up @@ -145,11 +147,17 @@ private static String metricName(MetricName metricName) {

static Labels labelsFromScope(String scope) {
Labels.Builder builder = Labels.builder();
Set<String> labelNames = new HashSet<>();
if (scope != null) {
String[] parts = scope.split("\\.");
if (parts.length % 2 == 0) {
for (int i = 0; i < parts.length; i += 2) {
builder.label(PrometheusNaming.sanitizeLabelName(parts[i]), parts[i + 1]);
String newLabelName = PrometheusNaming.sanitizeLabelName(parts[i]);
if (labelNames.add(newLabelName)) {
builder.label(newLabelName, parts[i + 1]);
} else {
LOG.warn("Duplicate label name {} in scope {}", newLabelName, scope);
}
}
}
}
Expand Down
22 changes: 22 additions & 0 deletions src/test/java/io/strimzi/kafka/metrics/DataPointSnapshotBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright Strimzi authors.
* License: Apache License 2.0 (see the file LICENSE or http://apache.org/licenses/LICENSE-2.0.html).
*/
package io.strimzi.kafka.metrics;

import io.prometheus.metrics.model.snapshots.InfoSnapshot;
import io.prometheus.metrics.model.snapshots.Labels;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class DataPointSnapshotBuilderTest {

@Test
public void testNewLabelDuplicateWarning() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better name would be something like testCollidingNewLabelIsIgnored

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to testCollidingNewLabelIsIgnored

Labels labels = Labels.builder().label("k1", "v1").label("k2", "v2").build();
InfoSnapshot.InfoDataPointSnapshot snapshot = DataPointSnapshotBuilder.infoDataPoint(labels, "value", "k1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertEquals("v1", snapshot.getLabels().get("k1"));
}

}
13 changes: 13 additions & 0 deletions src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.prometheus.metrics.model.snapshots.Labels;
import io.prometheus.metrics.model.snapshots.MetricSnapshot;
import io.prometheus.metrics.model.snapshots.MetricSnapshots;
import io.prometheus.metrics.model.snapshots.PrometheusNaming;
import org.apache.kafka.common.MetricName;
import org.apache.kafka.common.metrics.Gauge;
import org.apache.kafka.common.metrics.KafkaMetric;
Expand Down Expand Up @@ -108,6 +109,18 @@ public void testCollectNonNumericMetric() {
assertEquals(expectedLabels, snapshot.getDataPoints().get(0).getLabels());
}

@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());
}

private void assertGaugeSnapshot(MetricSnapshot snapshot, double expectedValue, Labels expectedLabels) {
assertInstanceOf(GaugeSnapshot.class, snapshot);
GaugeSnapshot gaugeSnapshot = (GaugeSnapshot) snapshot;
Expand Down
6 changes: 6 additions & 0 deletions src/test/java/io/strimzi/kafka/metrics/YammerMetricsCollectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import io.prometheus.metrics.model.snapshots.Labels;
import io.prometheus.metrics.model.snapshots.MetricSnapshot;
import io.prometheus.metrics.model.snapshots.MetricSnapshots;
import io.prometheus.metrics.model.snapshots.PrometheusNaming;
import org.apache.kafka.server.metrics.KafkaYammerMetrics;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -120,6 +121,11 @@ public void testLabelsFromScope() {
assertEquals(Labels.EMPTY, YammerMetricsCollector.labelsFromScope("k1"));
assertEquals(Labels.EMPTY, YammerMetricsCollector.labelsFromScope("k1."));
assertEquals(Labels.EMPTY, YammerMetricsCollector.labelsFromScope("k1.v1.k"));

Labels labels = YammerMetricsCollector.labelsFromScope("k-1.v1.k_1.v2");
assertEquals("k_1", PrometheusNaming.sanitizeLabelName("k-1"));
assertEquals("v1", labels.get("k_1"));
assertEquals(1, labels.size());
}

public Counter newCounter(String group, String type, String name) {
Expand Down