Skip to content

Commit 48e12a1

Browse files
bwplotkabwplotka
authored
Merge pull request #1645 from prometheus/cut-1204-pr1424
undefined
2 parents 05fcde9 + 504ad9b commit 48e12a1

File tree

4 files changed

+66
-110
lines changed

4 files changed

+66
-110
lines changed

‎CHANGELOG.md

+7-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
## Unreleased
22

3+
## 1.20.5 / 2024-10-15
4+
5+
* [BUGFIX] testutil: Reverted #1424; functions using compareMetricFamilies are (again) only failing if filtered metricNames are in the expected input.
6+
7+
## 1.20.4 / 2024-09-07
8+
39
* [BUGFIX] histograms: Fix possible data race when appending exemplars vs metrics gather. #1623
410

511
## 1.20.3 / 2024-09-05
@@ -28,7 +34,7 @@
2834
* [FEATURE] promlint: Add duplicated metric lint rule. #1472
2935
* [BUGFIX] promlint: Relax metric type in name linter rule. #1455
3036
* [BUGFIX] promhttp: Make sure server instrumentation wrapping supports new and future extra responseWriter methods. #1480
31-
* [BUGFIX] testutil: Functions using compareMetricFamilies are now failing if filtered metricNames are not in the input. #1424
37+
* [BUGFIX] **breaking** testutil: Functions using compareMetricFamilies are now failing if filtered metricNames are not in the input. #1424 (reverted in 1.20.5)
3238

3339
## 1.19.0 / 2024-02-27
3440

‎VERSION

+1-1
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
1.20.2
1+
1.20.5

‎prometheus/testutil/testutil.go

+12-18
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,9 @@ func GatherAndCount(g prometheus.Gatherer, metricNames ...string) (int, error) {
158158
// ScrapeAndCompare calls a remote exporter's endpoint which is expected to return some metrics in
159159
// plain text format. Then it compares it with the results that the `expected` would return.
160160
// If the `metricNames` is not empty it would filter the comparison only to the given metric names.
161+
//
162+
// NOTE: Be mindful of accidental discrepancies between expected and metricNames; metricNames filter
163+
// both expected and scraped metrics. See https://github.com/prometheus/client_golang/issues/1351.
161164
func ScrapeAndCompare(url string, expected io.Reader, metricNames ...string) error {
162165
resp, err := http.Get(url)
163166
if err != nil {
@@ -185,6 +188,9 @@ func ScrapeAndCompare(url string, expected io.Reader, metricNames ...string) err
185188

186189
// CollectAndCompare collects the metrics identified by `metricNames` and compares them in the Prometheus text
187190
// exposition format to the data read from expected.
191+
//
192+
// NOTE: Be mindful of accidental discrepancies between expected and metricNames; metricNames filter
193+
// both expected and collected metrics. See https://github.com/prometheus/client_golang/issues/1351.
188194
func CollectAndCompare(c prometheus.Collector, expected io.Reader, metricNames ...string) error {
189195
reg := prometheus.NewPedanticRegistry()
190196
if err := reg.Register(c); err != nil {
@@ -197,6 +203,9 @@ func CollectAndCompare(c prometheus.Collector, expected io.Reader, metricNames .
197203
// it to an expected output read from the provided Reader in the Prometheus text
198204
// exposition format. If any metricNames are provided, only metrics with those
199205
// names are compared.
206+
//
207+
// NOTE: Be mindful of accidental discrepancies between expected and metricNames; metricNames filter
208+
// both expected and gathered metrics. See https://github.com/prometheus/client_golang/issues/1351.
200209
func GatherAndCompare(g prometheus.Gatherer, expected io.Reader, metricNames ...string) error {
201210
return TransactionalGatherAndCompare(prometheus.ToTransactionalGatherer(g), expected, metricNames...)
202211
}
@@ -205,6 +214,9 @@ func GatherAndCompare(g prometheus.Gatherer, expected io.Reader, metricNames ...
205214
// it to an expected output read from the provided Reader in the Prometheus text
206215
// exposition format. If any metricNames are provided, only metrics with those
207216
// names are compared.
217+
//
218+
// NOTE: Be mindful of accidental discrepancies between expected and metricNames; metricNames filter
219+
// both expected and gathered metrics. See https://github.com/prometheus/client_golang/issues/1351.
208220
func TransactionalGatherAndCompare(g prometheus.TransactionalGatherer, expected io.Reader, metricNames ...string) error {
209221
got, done, err := g.Gather()
210222
defer done()
@@ -277,15 +289,6 @@ func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...str
277289
if metricNames != nil {
278290
got = filterMetrics(got, metricNames)
279291
expected = filterMetrics(expected, metricNames)
280-
if len(metricNames) > len(got) {
281-
var missingMetricNames []string
282-
for _, name := range metricNames {
283-
if ok := hasMetricByName(got, name); !ok {
284-
missingMetricNames = append(missingMetricNames, name)
285-
}
286-
}
287-
return fmt.Errorf("expected metric name(s) not found: %v", missingMetricNames)
288-
}
289292
}
290293

291294
return compare(got, expected)
@@ -327,12 +330,3 @@ func filterMetrics(metrics []*dto.MetricFamily, names []string) []*dto.MetricFam
327330
}
328331
return filtered
329332
}
330-
331-
func hasMetricByName(metrics []*dto.MetricFamily, name string) bool {
332-
for _, mf := range metrics {
333-
if mf.GetName() == name {
334-
return true
335-
}
336-
}
337-
return false
338-
}

‎prometheus/testutil/testutil_test.go

+46-90
Original file line numberDiff line numberDiff line change
@@ -328,46 +328,27 @@ func TestMetricNotFound(t *testing.T) {
328328
}
329329

330330
func TestScrapeAndCompare(t *testing.T) {
331-
scenarios := map[string]struct {
332-
want string
333-
metricNames []string
334-
// expectedErr if empty, means no fail is expected for the comparison.
335-
expectedErr string
336-
}{
337-
"empty metric Names": {
338-
want: `
331+
const expected = `
339332
# HELP some_total A value that represents a counter.
340333
# TYPE some_total counter
341334
342335
some_total{ label1 = "value1" } 1
343-
`,
344-
metricNames: []string{},
345-
},
346-
"one metric": {
347-
want: `
348-
# HELP some_total A value that represents a counter.
349-
# TYPE some_total counter
336+
`
350337

351-
some_total{ label1 = "value1" } 1
352-
`,
353-
metricNames: []string{"some_total"},
354-
},
355-
"multiple expected": {
356-
want: `
357-
# HELP some_total A value that represents a counter.
358-
# TYPE some_total counter
338+
expectedReader := strings.NewReader(expected)
359339

360-
some_total{ label1 = "value1" } 1
340+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
341+
fmt.Fprintln(w, expected)
342+
}))
343+
defer ts.Close()
361344

362-
# HELP some_total2 A value that represents a counter.
363-
# TYPE some_total2 counter
345+
if err := ScrapeAndCompare(ts.URL, expectedReader, "some_total"); err != nil {
346+
t.Errorf("unexpected scraping result:\n%s", err)
347+
}
348+
}
364349

365-
some_total2{ label2 = "value2" } 1
366-
`,
367-
metricNames: []string{"some_total2"},
368-
},
369-
"expected metric name is not scraped": {
370-
want: `
350+
func TestScrapeAndCompareWithMultipleExpected(t *testing.T) {
351+
const expected = `
371352
# HELP some_total A value that represents a counter.
372353
# TYPE some_total counter
373354
@@ -377,78 +358,53 @@ func TestScrapeAndCompare(t *testing.T) {
377358
# TYPE some_total2 counter
378359
379360
some_total2{ label2 = "value2" } 1
380-
`,
381-
metricNames: []string{"some_total3"},
382-
expectedErr: "expected metric name(s) not found: [some_total3]",
383-
},
384-
"one of multiple expected metric names is not scraped": {
385-
want: `
386-
# HELP some_total A value that represents a counter.
387-
# TYPE some_total counter
361+
`
388362

389-
some_total{ label1 = "value1" } 1
363+
expectedReader := strings.NewReader(expected)
390364

391-
# HELP some_total2 A value that represents a counter.
392-
# TYPE some_total2 counter
365+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
366+
fmt.Fprintln(w, expected)
367+
}))
368+
defer ts.Close()
393369

394-
some_total2{ label2 = "value2" } 1
395-
`,
396-
metricNames: []string{"some_total1", "some_total3"},
397-
expectedErr: "expected metric name(s) not found: [some_total1 some_total3]",
398-
},
399-
}
400-
for name, scenario := range scenarios {
401-
t.Run(name, func(t *testing.T) {
402-
expectedReader := strings.NewReader(scenario.want)
403-
404-
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
405-
fmt.Fprintln(w, scenario.want)
406-
}))
407-
defer ts.Close()
408-
if err := ScrapeAndCompare(ts.URL, expectedReader, scenario.metricNames...); err != nil {
409-
if scenario.expectedErr == "" || err.Error() != scenario.expectedErr {
410-
t.Errorf("unexpected error happened: %s", err)
411-
}
412-
} else if scenario.expectedErr != "" {
413-
t.Errorf("expected an error but got nil")
414-
}
415-
})
370+
if err := ScrapeAndCompare(ts.URL, expectedReader, "some_total2"); err != nil {
371+
t.Errorf("unexpected scraping result:\n%s", err)
416372
}
373+
}
417374

418-
t.Run("fetching fail", func(t *testing.T) {
419-
err := ScrapeAndCompare("some_url", strings.NewReader("some expectation"), "some_total")
420-
if err == nil {
421-
t.Errorf("expected an error but got nil")
422-
}
423-
if !strings.HasPrefix(err.Error(), "scraping metrics failed") {
424-
t.Errorf("unexpected error happened: %s", err)
425-
}
426-
})
375+
func TestScrapeAndCompareFetchingFail(t *testing.T) {
376+
err := ScrapeAndCompare("some_url", strings.NewReader("some expectation"), "some_total")
377+
if err == nil {
378+
t.Errorf("expected an error but got nil")
379+
}
380+
if !strings.HasPrefix(err.Error(), "scraping metrics failed") {
381+
t.Errorf("unexpected error happened: %s", err)
382+
}
383+
}
427384

428-
t.Run("bad status code", func(t *testing.T) {
429-
const expected = `
385+
func TestScrapeAndCompareBadStatusCode(t *testing.T) {
386+
const expected = `
430387
# HELP some_total A value that represents a counter.
431388
# TYPE some_total counter
432389
433390
some_total{ label1 = "value1" } 1
434391
`
435392

436-
expectedReader := strings.NewReader(expected)
393+
expectedReader := strings.NewReader(expected)
437394

438-
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
439-
w.WriteHeader(http.StatusBadGateway)
440-
fmt.Fprintln(w, expected)
441-
}))
442-
defer ts.Close()
395+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
396+
w.WriteHeader(http.StatusBadGateway)
397+
fmt.Fprintln(w, expected)
398+
}))
399+
defer ts.Close()
443400

444-
err := ScrapeAndCompare(ts.URL, expectedReader, "some_total")
445-
if err == nil {
446-
t.Errorf("expected an error but got nil")
447-
}
448-
if !strings.HasPrefix(err.Error(), "the scraping target returned a status code other than 200") {
449-
t.Errorf("unexpected error happened: %s", err)
450-
}
451-
})
401+
err := ScrapeAndCompare(ts.URL, expectedReader, "some_total")
402+
if err == nil {
403+
t.Errorf("expected an error but got nil")
404+
}
405+
if !strings.HasPrefix(err.Error(), "the scraping target returned a status code other than 200") {
406+
t.Errorf("unexpected error happened: %s", err)
407+
}
452408
}
453409

454410
func TestCollectAndCount(t *testing.T) {

0 commit comments

Comments
 (0)