Skip to content

Commit fbcf1d6

Browse files
zeitlingerzeitlinger
committed
fix: include counter names in negative value errors
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
1 parent 02622cb commit fbcf1d6

5 files changed

Lines changed: 93 additions & 11 deletions

File tree

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,9 @@ public CounterSnapshot collect() {
9292
@Override
9393
protected CounterSnapshot collect(List<Labels> labels, List<DataPoint> metricData) {
9494
List<CounterSnapshot.CounterDataPointSnapshot> data = new ArrayList<>(labels.size());
95+
String metricName = metadata.getName();
9596
for (int i = 0; i < labels.size(); i++) {
96-
data.add(metricData.get(i).collect(labels.get(i)));
97+
data.add(metricData.get(i).collect(labels.get(i), metricName));
9798
}
9899
return new CounterSnapshot(metadata, data);
99100
}
@@ -199,7 +200,7 @@ private void validateAndAdd(double amount) {
199200
doubleValue.add(amount);
200201
}
201202

202-
private CounterSnapshot.CounterDataPointSnapshot collect(Labels labels) {
203+
private CounterSnapshot.CounterDataPointSnapshot collect(Labels labels, String metricName) {
203204
// Read the exemplar first. Otherwise, there is a race condition where you might
204205
// see an Exemplar for a value that's not counted yet.
205206
// If there are multiple Exemplars (by default it's just one), use the newest.
@@ -212,8 +213,13 @@ private CounterSnapshot.CounterDataPointSnapshot collect(Labels labels) {
212213
}
213214
}
214215
}
215-
return new CounterSnapshot.CounterDataPointSnapshot(
216-
get(), labels, latestExemplar, createdTimeMillis);
216+
return CounterSnapshot.CounterDataPointSnapshot.builder()
217+
.value(get())
218+
.labels(labels)
219+
.exemplar(latestExemplar)
220+
.createdTimestampMillis(createdTimeMillis)
221+
.metricName(metricName)
222+
.build();
217223
}
218224
}
219225

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,11 @@ public CounterSnapshot collect() {
4747
callback.accept(
4848
(value, labelValues) -> {
4949
dataPoints.add(
50-
new CounterSnapshot.CounterDataPointSnapshot(
51-
value, makeLabels(labelValues), null, 0L));
50+
CounterSnapshot.CounterDataPointSnapshot.builder()
51+
.value(value)
52+
.labels(makeLabels(labelValues))
53+
.metricName(metadata.getName())
54+
.build());
5255
});
5356
return new CounterSnapshot(metadata, dataPoints);
5457
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,4 +43,17 @@ void testCounterNoCallback() {
4343
.isThrownBy(
4444
() -> CounterWithCallback.builder().name("counter").labelNames("l1", "l2").build());
4545
}
46+
47+
@Test
48+
void testNegativeValueIncludesMetricName() {
49+
CounterWithCallback counter =
50+
CounterWithCallback.builder()
51+
.name("negative_counter")
52+
.callback(callback -> callback.call(-1.0))
53+
.build();
54+
55+
assertThatExceptionOfType(IllegalArgumentException.class)
56+
.isThrownBy(counter::collect)
57+
.withMessage("negative_counter=-1.0: counters cannot have a negative value");
58+
}
4659
}

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

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ public static class CounterDataPointSnapshot extends DataPointSnapshot {
4949
private final double value;
5050
@Nullable private final Exemplar exemplar;
5151

52+
/** Optional metric name used only in validation error messages. */
53+
@Nullable private final String metricName;
54+
5255
/**
5356
* To create a new {@link CounterDataPointSnapshot}, you can either call the constructor
5457
* directly or use the Builder with {@link CounterDataPointSnapshot#builder()}.
@@ -62,7 +65,7 @@ public static class CounterDataPointSnapshot extends DataPointSnapshot {
6265
*/
6366
public CounterDataPointSnapshot(
6467
double value, Labels labels, @Nullable Exemplar exemplar, long createdTimestampMillis) {
65-
this(value, labels, exemplar, createdTimestampMillis, 0);
68+
this(value, labels, exemplar, createdTimestampMillis, 0, false, null);
6669
}
6770

6871
/**
@@ -77,7 +80,7 @@ public CounterDataPointSnapshot(
7780
@Nullable Exemplar exemplar,
7881
long createdTimestampMillis,
7982
long scrapeTimestampMillis) {
80-
this(value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis, false);
83+
this(value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis, false, null);
8184
}
8285

8386
@SuppressWarnings("this-escape")
@@ -88,9 +91,22 @@ public CounterDataPointSnapshot(
8891
long createdTimestampMillis,
8992
long scrapeTimestampMillis,
9093
boolean internal) {
94+
this(value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis, internal, null);
95+
}
96+
97+
@SuppressWarnings("this-escape")
98+
private CounterDataPointSnapshot(
99+
double value,
100+
Labels labels,
101+
@Nullable Exemplar exemplar,
102+
long createdTimestampMillis,
103+
long scrapeTimestampMillis,
104+
boolean internal,
105+
@Nullable String metricName) {
91106
super(labels, createdTimestampMillis, scrapeTimestampMillis, internal);
92107
this.value = value;
93108
this.exemplar = exemplar;
109+
this.metricName = metricName;
94110
if (!internal) {
95111
validate();
96112
}
@@ -107,7 +123,16 @@ public Exemplar getExemplar() {
107123

108124
protected void validate() {
109125
if (value < 0.0) {
110-
throw new IllegalArgumentException(value + ": counters cannot have a negative value");
126+
StringBuilder message = new StringBuilder();
127+
if (metricName != null && !metricName.isEmpty()) {
128+
message.append(metricName).append('=');
129+
}
130+
message.append(value).append(": counters cannot have a negative value");
131+
Labels labels = getLabels();
132+
if (labels != null && !labels.isEmpty()) {
133+
message.append(" (labels=").append(labels).append(')');
134+
}
135+
throw new IllegalArgumentException(message.toString());
111136
}
112137
}
113138

@@ -119,7 +144,8 @@ DataPointSnapshot escape(EscapingScheme escapingScheme) {
119144
SnapshotEscaper.escapeExemplar(exemplar, escapingScheme),
120145
getCreatedTimestampMillis(),
121146
getScrapeTimestampMillis(),
122-
true);
147+
true,
148+
metricName);
123149
}
124150

125151
public static Builder builder() {
@@ -131,6 +157,7 @@ public static class Builder extends DataPointSnapshot.Builder<Builder> {
131157
@Nullable private Exemplar exemplar = null;
132158
@Nullable private Double value = null;
133159
private long createdTimestampMillis = 0L;
160+
@Nullable private String metricName = null;
134161

135162
private Builder() {}
136163

@@ -150,12 +177,27 @@ public Builder createdTimestampMillis(long createdTimestampMillis) {
150177
return this;
151178
}
152179

180+
/**
181+
* Optional metric name included in the exception message when {@link #value(double)} is
182+
* negative. Does not change the snapshot identity.
183+
*/
184+
public Builder metricName(@Nullable String metricName) {
185+
this.metricName = metricName;
186+
return this;
187+
}
188+
153189
public CounterDataPointSnapshot build() {
154190
if (value == null) {
155191
throw new IllegalArgumentException("Missing required field: value is null.");
156192
}
157193
return new CounterDataPointSnapshot(
158-
value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis);
194+
value,
195+
labels,
196+
exemplar,
197+
createdTimestampMillis,
198+
scrapeTimestampMillis,
199+
false,
200+
metricName);
159201
}
160202

161203
@Override

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,4 +130,22 @@ void testDataImmutable() {
130130
iterator.next();
131131
assertThatExceptionOfType(UnsupportedOperationException.class).isThrownBy(iterator::remove);
132132
}
133+
134+
@Test
135+
void testNegativeValueIncludesMetricNameInMessage() {
136+
assertThatExceptionOfType(IllegalArgumentException.class)
137+
.isThrownBy(
138+
() ->
139+
CounterDataPointSnapshot.builder().metricName("http_requests").value(-2.0).build())
140+
.withMessageContaining("http_requests")
141+
.withMessageContaining("-2.0")
142+
.withMessageContaining("counters cannot have a negative value");
143+
}
144+
145+
@Test
146+
void testNegativeValueWithoutMetricNameKeepsLegacyMessage() {
147+
assertThatExceptionOfType(IllegalArgumentException.class)
148+
.isThrownBy(() -> CounterDataPointSnapshot.builder().value(-1.0).build())
149+
.withMessage("-1.0: counters cannot have a negative value");
150+
}
133151
}

0 commit comments

Comments
 (0)