Skip to content

Commit e9410e8

Browse files
jaydelucajaydeluca
committed
fix histogram detection when merging snapshots, ensure created with timestamps logic includes merging
Signed-off-by: Jay DeLuca <jaydeluca4@gmail.com>
1 parent 707e4dc commit e9410e8

File tree

8 files changed

+269
-11
lines changed

8 files changed

+269
-11
lines changed

‎prometheus-metrics-exposition-textformats/src/main/java/io/prometheus/metrics/expositionformats/PrometheusTextFormatWriter.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ public void write(OutputStream out, MetricSnapshots metricSnapshots, EscapingSch
137137
}
138138
}
139139
if (writeCreatedTimestamps) {
140-
for (MetricSnapshot s : metricSnapshots) {
140+
for (MetricSnapshot s : merged) {
141141
MetricSnapshot snapshot = escapeMetricSnapshot(s, scheme);
142142
if (!snapshot.getDataPoints().isEmpty()) {
143143
if (snapshot instanceof CounterSnapshot) {

‎prometheus-metrics-exposition-textformats/src/main/java/io/prometheus/metrics/expositionformats/TextFormatUtil.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,14 @@ private static MetricSnapshot mergeSnapshots(List<MetricSnapshot> snapshots) {
227227
+ " and "
228228
+ snapshot.getClass().getName());
229229
}
230+
if (first instanceof HistogramSnapshot) {
231+
HistogramSnapshot hFirst = (HistogramSnapshot) first;
232+
HistogramSnapshot hSnapshot = (HistogramSnapshot) snapshot;
233+
if (hFirst.isGaugeHistogram() != hSnapshot.isGaugeHistogram()) {
234+
throw new IllegalArgumentException(
235+
"Cannot merge histograms: gauge histogram and classic histogram");
236+
}
237+
}
230238
totalDataPoints += snapshot.getDataPoints().size();
231239
}
232240

@@ -244,7 +252,9 @@ private static MetricSnapshot mergeSnapshots(List<MetricSnapshot> snapshots) {
244252
first.getMetadata(),
245253
(Collection<GaugeSnapshot.GaugeDataPointSnapshot>) (Object) allDataPoints);
246254
} else if (first instanceof HistogramSnapshot) {
255+
HistogramSnapshot histFirst = (HistogramSnapshot) first;
247256
return new HistogramSnapshot(
257+
histFirst.isGaugeHistogram(),
248258
first.getMetadata(),
249259
(Collection<HistogramSnapshot.HistogramDataPointSnapshot>) (Object) allDataPoints);
250260
} else if (first instanceof SummarySnapshot) {

‎prometheus-metrics-exposition-textformats/src/test/java/io/prometheus/metrics/expositionformats/DuplicateNamesExpositionTest.java‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,4 +180,79 @@ void testOpenMetricsFormat_withDuplicateNames() throws IOException {
180180
""";
181181
assertThat(output).isEqualTo(expected);
182182
}
183+
184+
@Test
185+
void testDuplicateNames_withCreatedTimestamps_emitsSingleHelpTypeAndNoDuplicateCreatedSeries()
186+
throws IOException {
187+
long createdTs = 1672850385800L;
188+
PrometheusRegistry registry = new PrometheusRegistry();
189+
190+
registry.register(
191+
new Collector() {
192+
@Override
193+
public MetricSnapshot collect() {
194+
return CounterSnapshot.builder()
195+
.name("api_responses")
196+
.help("API responses")
197+
.dataPoint(
198+
CounterSnapshot.CounterDataPointSnapshot.builder()
199+
.labels(Labels.of("uri", "/hello", "outcome", "SUCCESS"))
200+
.value(100)
201+
.createdTimestampMillis(createdTs)
202+
.build())
203+
.build();
204+
}
205+
206+
@Override
207+
public String getPrometheusName() {
208+
return "api_responses_total";
209+
}
210+
});
211+
212+
registry.register(
213+
new Collector() {
214+
@Override
215+
public MetricSnapshot collect() {
216+
return CounterSnapshot.builder()
217+
.name("api_responses")
218+
.help("API responses")
219+
.dataPoint(
220+
CounterSnapshot.CounterDataPointSnapshot.builder()
221+
.labels(
222+
Labels.of("uri", "/hello", "outcome", "FAILURE", "error", "TIMEOUT"))
223+
.value(10)
224+
.createdTimestampMillis(createdTs + 1000)
225+
.build())
226+
.build();
227+
}
228+
229+
@Override
230+
public String getPrometheusName() {
231+
return "api_responses_total";
232+
}
233+
});
234+
235+
MetricSnapshots snapshots = registry.scrape();
236+
ByteArrayOutputStream out = new ByteArrayOutputStream();
237+
PrometheusTextFormatWriter writer =
238+
PrometheusTextFormatWriter.builder().setIncludeCreatedTimestamps(true).build();
239+
writer.write(out, snapshots);
240+
String output = out.toString(UTF_8);
241+
242+
// Merged snapshots: one metric family with two data points. Created-timestamp section uses
243+
// merged snapshots too, so single HELP/TYPE for _created and one _created line per label set.
244+
String expected =
245+
"""
246+
# HELP api_responses_total API responses
247+
# TYPE api_responses_total counter
248+
api_responses_total{error="TIMEOUT",outcome="FAILURE",uri="/hello"} 10.0
249+
api_responses_total{outcome="SUCCESS",uri="/hello"} 100.0
250+
# HELP api_responses_created API responses
251+
# TYPE api_responses_created gauge
252+
api_responses_created{error="TIMEOUT",outcome="FAILURE",uri="/hello"} 1672850386800
253+
api_responses_created{outcome="SUCCESS",uri="/hello"} 1672850385800
254+
""";
255+
256+
assertThat(output).isEqualTo(expected);
257+
}
183258
}

‎prometheus-metrics-exposition-textformats/src/test/java/io/prometheus/metrics/expositionformats/TextFormatUtilTest.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
import static org.assertj.core.api.Assertions.assertThat;
44
import static org.junit.jupiter.api.Assertions.assertEquals;
55

6+
import io.prometheus.metrics.model.snapshots.ClassicHistogramBuckets;
67
import io.prometheus.metrics.model.snapshots.CounterSnapshot;
8+
import io.prometheus.metrics.model.snapshots.HistogramSnapshot;
79
import io.prometheus.metrics.model.snapshots.Labels;
810
import io.prometheus.metrics.model.snapshots.MetricSnapshots;
911
import java.io.IOException;
@@ -126,4 +128,39 @@ void testMergeDuplicates_emptySnapshots_returnsEmpty() {
126128

127129
assertThat(result).isEmpty();
128130
}
131+
132+
@Test
133+
void testMergeDuplicates_histogramSameGaugeFlag_preservesGaugeHistogram() {
134+
HistogramSnapshot gauge1 =
135+
HistogramSnapshot.builder()
136+
.name("my_histogram")
137+
.gaugeHistogram(true)
138+
.dataPoint(
139+
HistogramSnapshot.HistogramDataPointSnapshot.builder()
140+
.labels(Labels.of("a", "1"))
141+
.classicHistogramBuckets(
142+
ClassicHistogramBuckets.of(
143+
new double[] {Double.POSITIVE_INFINITY}, new long[] {0}))
144+
.build())
145+
.build();
146+
HistogramSnapshot gauge2 =
147+
HistogramSnapshot.builder()
148+
.name("my_histogram")
149+
.gaugeHistogram(true)
150+
.dataPoint(
151+
HistogramSnapshot.HistogramDataPointSnapshot.builder()
152+
.labels(Labels.of("a", "2"))
153+
.classicHistogramBuckets(
154+
ClassicHistogramBuckets.of(
155+
new double[] {Double.POSITIVE_INFINITY}, new long[] {0}))
156+
.build())
157+
.build();
158+
MetricSnapshots snapshots = new MetricSnapshots(gauge1, gauge2);
159+
MetricSnapshots result = TextFormatUtil.mergeDuplicates(snapshots);
160+
161+
assertThat(result).hasSize(1);
162+
HistogramSnapshot merged = (HistogramSnapshot) result.get(0);
163+
assertThat(merged.isGaugeHistogram()).isTrue();
164+
assertThat(merged.getDataPoints()).hasSize(2);
165+
}
129166
}

‎prometheus-metrics-model/src/main/java/io/prometheus/metrics/model/registry/PrometheusRegistry.java‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,7 @@ private static class CollectorRegistration {
3434

3535
CollectorRegistration(String prometheusName, @Nullable Set<String> labelNames) {
3636
this.prometheusName = prometheusName;
37-
this.labelNames =
38-
(labelNames == null || labelNames.isEmpty()) ? Collections.emptySet() : labelNames;
37+
this.labelNames = immutableLabelNames(labelNames);
3938
}
4039
}
4140

@@ -49,8 +48,7 @@ private static class MultiCollectorRegistration {
4948

5049
MultiCollectorRegistration(String prometheusName, @Nullable Set<String> labelNames) {
5150
this.prometheusName = prometheusName;
52-
this.labelNames =
53-
(labelNames == null || labelNames.isEmpty()) ? Collections.emptySet() : labelNames;
51+
this.labelNames = immutableLabelNames(labelNames);
5452
}
5553
}
5654

@@ -62,8 +60,8 @@ private static class MultiCollectorRegistration {
6260
private static class RegistrationInfo {
6361
private final MetricType type;
6462
private final Set<Set<String>> labelSchemas;
65-
@Nullable private final String help;
66-
@Nullable private final Unit unit;
63+
@Nullable private String help;
64+
@Nullable private Unit unit;
6765

6866
private RegistrationInfo(
6967
MetricType type,
@@ -89,8 +87,9 @@ static RegistrationInfo of(
8987
}
9088

9189
/**
92-
* Validates that the given help and unit are consistent with this registration. Throws if both
93-
* sides have non-null help/unit and they differ.
90+
* Validates that the given help and unit are consistent with this registration. Throws if
91+
* non-null values conflict. When stored help/unit is null and the new value is non-null,
92+
* captures the first non-null so subsequent registrations are validated consistently.
9493
*/
9594
void validateMetadata(@Nullable String newHelp, @Nullable Unit newUnit) {
9695
if (help != null && newHelp != null && !Objects.equals(help, newHelp)) {
@@ -101,6 +100,12 @@ void validateMetadata(@Nullable String newHelp, @Nullable Unit newUnit) {
101100
throw new IllegalArgumentException(
102101
"Conflicting unit. Existing: " + unit + ", new: " + newUnit);
103102
}
103+
if (help == null && newHelp != null) {
104+
this.help = newHelp;
105+
}
106+
if (unit == null && newUnit != null) {
107+
this.unit = newUnit;
108+
}
104109
}
105110

106111
/**

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,10 @@ public MetricSnapshots(Collection<MetricSnapshot> snapshots) {
4141
String name2 = list.get(i + 1).getMetadata().getPrometheusName();
4242

4343
if (name1.equals(name2)) {
44-
Class<?> type1 = list.get(i).getClass();
45-
Class<?> type2 = list.get(i + 1).getClass();
44+
MetricSnapshot s1 = list.get(i);
45+
MetricSnapshot s2 = list.get(i + 1);
46+
Class<?> type1 = s1.getClass();
47+
Class<?> type2 = s2.getClass();
4648

4749
if (!type1.equals(type2)) {
4850
throw new IllegalArgumentException(
@@ -52,6 +54,16 @@ public MetricSnapshots(Collection<MetricSnapshot> snapshots) {
5254
+ " and "
5355
+ type2.getSimpleName());
5456
}
57+
58+
// HistogramSnapshot: gauge histogram vs classic histogram are semantically different
59+
if (s1 instanceof HistogramSnapshot) {
60+
HistogramSnapshot h1 = (HistogramSnapshot) s1;
61+
HistogramSnapshot h2 = (HistogramSnapshot) s2;
62+
if (h1.isGaugeHistogram() != h2.isGaugeHistogram()) {
63+
throw new IllegalArgumentException(
64+
name1 + ": conflicting histogram types: gauge histogram and classic histogram");
65+
}
66+
}
5567
}
5668
}
5769

‎prometheus-metrics-model/src/test/java/io/prometheus/metrics/model/registry/PrometheusRegistryTest.java‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -776,6 +776,97 @@ public MetricMetadata getMetadata() {
776776
assertThatCode(() -> registry.register(withoutHelp)).doesNotThrowAnyException();
777777
}
778778

779+
@Test
780+
public void register_firstOmitsHelp_secondProvidesHelp_thirdWithDifferentHelp_throws() {
781+
PrometheusRegistry registry = new PrometheusRegistry();
782+
783+
Collector withoutHelp =
784+
new Collector() {
785+
@Override
786+
public MetricSnapshot collect() {
787+
return CounterSnapshot.builder().name("requests").build();
788+
}
789+
790+
@Override
791+
public String getPrometheusName() {
792+
return "requests";
793+
}
794+
795+
@Override
796+
public MetricType getMetricType() {
797+
return MetricType.COUNTER;
798+
}
799+
800+
@Override
801+
public Set<String> getLabelNames() {
802+
return new HashSet<>(asList("path"));
803+
}
804+
};
805+
806+
Collector withHelpTotal =
807+
new Collector() {
808+
@Override
809+
public MetricSnapshot collect() {
810+
return CounterSnapshot.builder().name("requests").help("Total requests").build();
811+
}
812+
813+
@Override
814+
public String getPrometheusName() {
815+
return "requests";
816+
}
817+
818+
@Override
819+
public MetricType getMetricType() {
820+
return MetricType.COUNTER;
821+
}
822+
823+
@Override
824+
public Set<String> getLabelNames() {
825+
return new HashSet<>(asList("status"));
826+
}
827+
828+
@Override
829+
public MetricMetadata getMetadata() {
830+
return new MetricMetadata("requests", "Total requests", null);
831+
}
832+
};
833+
834+
Collector withHelpOther =
835+
new Collector() {
836+
@Override
837+
public MetricSnapshot collect() {
838+
return CounterSnapshot.builder().name("requests").help("Other help").build();
839+
}
840+
841+
@Override
842+
public String getPrometheusName() {
843+
return "requests";
844+
}
845+
846+
@Override
847+
public MetricType getMetricType() {
848+
return MetricType.COUNTER;
849+
}
850+
851+
@Override
852+
public Set<String> getLabelNames() {
853+
return new HashSet<>(asList("method"));
854+
}
855+
856+
@Override
857+
public MetricMetadata getMetadata() {
858+
return new MetricMetadata("requests", "Other help", null);
859+
}
860+
};
861+
862+
registry.register(withoutHelp);
863+
registry.register(withHelpTotal);
864+
// First had no help, second provided "Total requests" (captured). Third conflicts.
865+
assertThatThrownBy(() -> registry.register(withHelpOther))
866+
.isInstanceOf(IllegalArgumentException.class)
867+
.hasMessageContaining("Conflicting help strings");
868+
}
869+
779870
@Test
780871
public void unregister_lastCollector_removesPrometheusName() {
781872
PrometheusRegistry registry = new PrometheusRegistry();

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,34 @@ void testDuplicateName() {
6161
.isThrownBy(() -> new MetricSnapshots(c, g));
6262
}
6363

64+
@Test
65+
void testDuplicateName_histogramGaugeVsClassic_throws() {
66+
HistogramSnapshot classic =
67+
HistogramSnapshot.builder()
68+
.name("my_histogram")
69+
.dataPoint(
70+
HistogramSnapshot.HistogramDataPointSnapshot.builder()
71+
.classicHistogramBuckets(
72+
ClassicHistogramBuckets.of(
73+
new double[] {Double.POSITIVE_INFINITY}, new long[] {0}))
74+
.build())
75+
.build();
76+
HistogramSnapshot gauge =
77+
HistogramSnapshot.builder()
78+
.name("my_histogram")
79+
.gaugeHistogram(true)
80+
.dataPoint(
81+
HistogramSnapshot.HistogramDataPointSnapshot.builder()
82+
.classicHistogramBuckets(
83+
ClassicHistogramBuckets.of(
84+
new double[] {Double.POSITIVE_INFINITY}, new long[] {0}))
85+
.build())
86+
.build();
87+
assertThatExceptionOfType(IllegalArgumentException.class)
88+
.isThrownBy(() -> new MetricSnapshots(classic, gauge))
89+
.withMessageContaining("conflicting histogram types");
90+
}
91+
6492
@Test
6593
void testBuilder() {
6694
CounterSnapshot counter =

0 commit comments

Comments
 (0)