fix: include counter names in negative value errors

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

Author: zeitlinger

prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Counter.java

@@ -92,8 +92,9 @@ public CounterSnapshot collect() {
   @Override
   protected CounterSnapshot collect(List<Labels> labels, List<DataPoint> metricData) {
     List<CounterSnapshot.CounterDataPointSnapshot> data = new ArrayList<>(labels.size());
+    String metricName = metadata.getName();
     for (int i = 0; i < labels.size(); i++) {
-      data.add(metricData.get(i).collect(labels.get(i)));
+      data.add(metricData.get(i).collect(labels.get(i), metricName));
     }
     return new CounterSnapshot(metadata, data);
   }
@@ -199,7 +200,7 @@ private void validateAndAdd(double amount) {
       doubleValue.add(amount);
     }
 
-    private CounterSnapshot.CounterDataPointSnapshot collect(Labels labels) {
+    private CounterSnapshot.CounterDataPointSnapshot collect(Labels labels, String metricName) {
       // Read the exemplar first. Otherwise, there is a race condition where you might
       // see an Exemplar for a value that's not counted yet.
       // If there are multiple Exemplars (by default it's just one), use the newest.
@@ -212,8 +213,13 @@ private CounterSnapshot.CounterDataPointSnapshot collect(Labels labels) {
           }
         }
       }
-      return new CounterSnapshot.CounterDataPointSnapshot(
-          get(), labels, latestExemplar, createdTimeMillis);
+      return CounterSnapshot.CounterDataPointSnapshot.builder()
+          .value(get())
+          .labels(labels)
+          .exemplar(latestExemplar)
+          .createdTimestampMillis(createdTimeMillis)
+          .metricName(metricName)
+          .build();
     }
   }
 

prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/CounterWithCallback.java

@@ -47,8 +47,11 @@ public CounterSnapshot collect() {
     callback.accept(
         (value, labelValues) -> {
           dataPoints.add(
-              new CounterSnapshot.CounterDataPointSnapshot(
-                  value, makeLabels(labelValues), null, 0L));
+              CounterSnapshot.CounterDataPointSnapshot.builder()
+                  .value(value)
+                  .labels(makeLabels(labelValues))
+                  .metricName(metadata.getName())
+                  .build());
         });
     return new CounterSnapshot(metadata, dataPoints);
   }

prometheus-metrics-core/src/test/java/io/prometheus/metrics/core/metrics/CounterWithCallbackTest.java

@@ -43,4 +43,17 @@ void testCounterNoCallback() {
         .isThrownBy(
             () -> CounterWithCallback.builder().name("counter").labelNames("l1", "l2").build());
   }
+
+  @Test
+  void testNegativeValueIncludesMetricName() {
+    CounterWithCallback counter =
+        CounterWithCallback.builder()
+            .name("negative_counter")
+            .callback(callback -> callback.call(-1.0))
+            .build();
+
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(counter::collect)
+        .withMessage("negative_counter=-1.0: counters cannot have a negative value");
+  }
 }

prometheus-metrics-model/src/main/java/io/prometheus/metrics/model/snapshots/CounterSnapshot.java

@@ -49,6 +49,9 @@ public static class CounterDataPointSnapshot extends DataPointSnapshot {
     private final double value;
     @Nullable private final Exemplar exemplar;
 
+    /** Optional metric name used only in validation error messages. */
+    @Nullable private final String metricName;
+
     /**
      * To create a new {@link CounterDataPointSnapshot}, you can either call the constructor
      * directly or use the Builder with {@link CounterDataPointSnapshot#builder()}.
@@ -62,7 +65,7 @@ public static class CounterDataPointSnapshot extends DataPointSnapshot {
      */
     public CounterDataPointSnapshot(
         double value, Labels labels, @Nullable Exemplar exemplar, long createdTimestampMillis) {
-      this(value, labels, exemplar, createdTimestampMillis, 0);
+      this(value, labels, exemplar, createdTimestampMillis, 0, false, null);
     }
 
     /**
@@ -77,7 +80,7 @@ public CounterDataPointSnapshot(
         @Nullable Exemplar exemplar,
         long createdTimestampMillis,
         long scrapeTimestampMillis) {
-      this(value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis, false);
+      this(value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis, false, null);
     }
 
     @SuppressWarnings("this-escape")
@@ -88,9 +91,22 @@ public CounterDataPointSnapshot(
         long createdTimestampMillis,
         long scrapeTimestampMillis,
         boolean internal) {
+      this(value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis, internal, null);
+    }
+
+    @SuppressWarnings("this-escape")
+    private CounterDataPointSnapshot(
+        double value,
+        Labels labels,
+        @Nullable Exemplar exemplar,
+        long createdTimestampMillis,
+        long scrapeTimestampMillis,
+        boolean internal,
+        @Nullable String metricName) {
       super(labels, createdTimestampMillis, scrapeTimestampMillis, internal);
       this.value = value;
       this.exemplar = exemplar;
+      this.metricName = metricName;
       if (!internal) {
         validate();
       }
@@ -107,7 +123,16 @@ public Exemplar getExemplar() {
 
     protected void validate() {
       if (value < 0.0) {
-        throw new IllegalArgumentException(value + ": counters cannot have a negative value");
+        StringBuilder message = new StringBuilder();
+        if (metricName != null && !metricName.isEmpty()) {
+          message.append(metricName).append('=');
+        }
+        message.append(value).append(": counters cannot have a negative value");
+        Labels labels = getLabels();
+        if (labels != null && !labels.isEmpty()) {
+          message.append(" (labels=").append(labels).append(')');
+        }
+        throw new IllegalArgumentException(message.toString());
       }
     }
 
@@ -119,7 +144,8 @@ DataPointSnapshot escape(EscapingScheme escapingScheme) {
           SnapshotEscaper.escapeExemplar(exemplar, escapingScheme),
           getCreatedTimestampMillis(),
           getScrapeTimestampMillis(),
-          true);
+          true,
+          metricName);
     }
 
     public static Builder builder() {
@@ -131,6 +157,7 @@ public static class Builder extends DataPointSnapshot.Builder<Builder> {
       @Nullable private Exemplar exemplar = null;
       @Nullable private Double value = null;
       private long createdTimestampMillis = 0L;
+      @Nullable private String metricName = null;
 
       private Builder() {}
 
@@ -150,12 +177,27 @@ public Builder createdTimestampMillis(long createdTimestampMillis) {
         return this;
       }
 
+      /**
+       * Optional metric name included in the exception message when {@link #value(double)} is
+       * negative. Does not change the snapshot identity.
+       */
+      public Builder metricName(@Nullable String metricName) {
+        this.metricName = metricName;
+        return this;
+      }
+
       public CounterDataPointSnapshot build() {
         if (value == null) {
           throw new IllegalArgumentException("Missing required field: value is null.");
         }
         return new CounterDataPointSnapshot(
-            value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis);
+            value,
+            labels,
+            exemplar,
+            createdTimestampMillis,
+            scrapeTimestampMillis,
+            false,
+            metricName);
       }
 
       @Override

prometheus-metrics-model/src/test/java/io/prometheus/metrics/model/snapshots/CounterSnapshotTest.java

@@ -130,4 +130,22 @@ void testDataImmutable() {
     iterator.next();
     assertThatExceptionOfType(UnsupportedOperationException.class).isThrownBy(iterator::remove);
   }
+
+  @Test
+  void testNegativeValueIncludesMetricNameInMessage() {
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(
+            () ->
+                CounterDataPointSnapshot.builder().metricName("http_requests").value(-2.0).build())
+        .withMessageContaining("http_requests")
+        .withMessageContaining("-2.0")
+        .withMessageContaining("counters cannot have a negative value");
+  }
+
+  @Test
+  void testNegativeValueWithoutMetricNameKeepsLegacyMessage() {
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(() -> CounterDataPointSnapshot.builder().value(-1.0).build())
+        .withMessage("-1.0: counters cannot have a negative value");
+  }
 }