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 1 commit
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
12 changes: 11 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 @@ -12,20 +12,30 @@
import io.prometheus.metrics.model.snapshots.Quantiles;
import io.prometheus.metrics.model.snapshots.SummarySnapshot;

import java.util.logging.Logger;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

private static final Logger LOG = Logger.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 sanitizedMetricName = PrometheusNaming.sanitizeLabelName(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 think naming this newLabelName would better describe its use.

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 'newLabelName`

Labels newLabels = labels;
if (labels.contains(sanitizedMetricName)) {
LOG.warning("Ignoring metric value duplicate key: " + sanitizedMetricName);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

} else {
newLabels = labels.add(sanitizedMetricName, String.valueOf(value));
}
return InfoSnapshot.InfoDataPointSnapshot.builder()
.labels(newLabels)
.build();
Expand Down
14 changes: 14 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,19 @@ public void testCollectNonNumericMetric() {
assertEquals(expectedLabels, snapshot.getDataPoints().get(0).getLabels());
}

@Test
public void testLabelNameSanitizing() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

String labelName = PrometheusNaming.sanitizeLabelName("k-1");
assertEquals("k_1", labelName);
}

@Test
public void testLabelDuplicateWarning() {
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 we should put this test in a new class DataPointSnapshotBuilderTest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New DataPointSnapshotBuilderTest class created and this test is moved to there

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I missed that. Changed to just value.

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

private void assertGaugeSnapshot(MetricSnapshot snapshot, double expectedValue, Labels expectedLabels) {
assertInstanceOf(GaugeSnapshot.class, snapshot);
GaugeSnapshot gaugeSnapshot = (GaugeSnapshot) snapshot;
Expand Down