From 6100e756a8ffb8aec255f0964e5471aafbe87de3 Mon Sep 17 00:00:00 2001 From: Filip Petkovski Date: Fri, 26 Jul 2024 09:49:57 +0200 Subject: [PATCH 01/20] Ignore stale histograms for counter reset detection The histogram stats decoder keeps track of the last seen histogram sample in order to properly detect counter resets. We are seeing an issue where a histogram with UnknownResetHint gets treated as a counter reset when it follows a stale histogram sample. I believe that this is incorrect since stale samples should be completely ignored in PromQL. As a result, they should not be stored in the histogram stats iterator and the counter reset detection needs to be done against the last non-stale sample. Signed-off-by: Filip Petkovski --- promql/histogram_stats_iterator.go | 2 - promql/histogram_stats_iterator_test.go | 123 +++++++++++++++--------- tsdb/tsdbutil/histogram.go | 10 +- 3 files changed, 84 insertions(+), 51 deletions(-) diff --git a/promql/histogram_stats_iterator.go b/promql/histogram_stats_iterator.go index dfafea5f8..0a5f67ae7 100644 --- a/promql/histogram_stats_iterator.go +++ b/promql/histogram_stats_iterator.go @@ -48,7 +48,6 @@ func (f *histogramStatsIterator) AtHistogram(h *histogram.Histogram) (int64, *hi var t int64 t, f.currentH = f.Iterator.AtHistogram(f.currentH) if value.IsStaleNaN(f.currentH.Sum) { - f.setLastH(f.currentH) h = &histogram.Histogram{Sum: f.currentH.Sum} return t, h } @@ -77,7 +76,6 @@ func (f *histogramStatsIterator) AtFloatHistogram(fh *histogram.FloatHistogram) var t int64 t, f.currentFH = f.Iterator.AtFloatHistogram(f.currentFH) if value.IsStaleNaN(f.currentFH.Sum) { - f.setLastFH(f.currentFH) return t, &histogram.FloatHistogram{Sum: f.currentFH.Sum} } diff --git a/promql/histogram_stats_iterator_test.go b/promql/histogram_stats_iterator_test.go index b71a9d602..d5c081348 100644 --- a/promql/histogram_stats_iterator_test.go +++ b/promql/histogram_stats_iterator_test.go @@ -14,62 +14,99 @@ package promql import ( + "fmt" + "math" "testing" "github.com/stretchr/testify/require" "github.com/prometheus/prometheus/model/histogram" "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/model/value" "github.com/prometheus/prometheus/tsdb/chunkenc" "github.com/prometheus/prometheus/tsdb/tsdbutil" ) func TestHistogramStatsDecoding(t *testing.T) { - histograms := []*histogram.Histogram{ - tsdbutil.GenerateTestHistogram(0), - tsdbutil.GenerateTestHistogram(1), - tsdbutil.GenerateTestHistogram(2), - tsdbutil.GenerateTestHistogram(2), - } - histograms[0].CounterResetHint = histogram.NotCounterReset - histograms[1].CounterResetHint = histogram.UnknownCounterReset - histograms[2].CounterResetHint = histogram.CounterReset - histograms[3].CounterResetHint = histogram.UnknownCounterReset - - expectedHints := []histogram.CounterResetHint{ - histogram.NotCounterReset, - histogram.NotCounterReset, - histogram.CounterReset, - histogram.NotCounterReset, + cases := []struct { + name string + histograms []*histogram.Histogram + expectedHints []histogram.CounterResetHint + }{ + { + name: "unknown counter reset triggers detection", + histograms: []*histogram.Histogram{ + tsdbutil.GenerateTestHistogramWithHint(0, histogram.NotCounterReset), + tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset), + tsdbutil.GenerateTestHistogramWithHint(2, histogram.CounterReset), + tsdbutil.GenerateTestHistogramWithHint(2, histogram.UnknownCounterReset), + }, + expectedHints: []histogram.CounterResetHint{ + histogram.NotCounterReset, + histogram.NotCounterReset, + histogram.CounterReset, + histogram.NotCounterReset, + }, + }, + { + name: "stale sample before unknown reset hint", + histograms: []*histogram.Histogram{ + tsdbutil.GenerateTestHistogramWithHint(0, histogram.NotCounterReset), + tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset), + {Sum: math.Float64frombits(value.StaleNaN)}, + tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset), + }, + expectedHints: []histogram.CounterResetHint{ + histogram.NotCounterReset, + histogram.NotCounterReset, + histogram.UnknownCounterReset, + histogram.NotCounterReset, + }, + }, } - t.Run("histogram_stats", func(t *testing.T) { - decodedStats := make([]*histogram.Histogram, 0) - statsIterator := NewHistogramStatsIterator(newHistogramSeries(histograms).Iterator(nil)) - for statsIterator.Next() != chunkenc.ValNone { - _, h := statsIterator.AtHistogram(nil) - decodedStats = append(decodedStats, h) - } - for i := 0; i < len(histograms); i++ { - require.Equal(t, expectedHints[i], decodedStats[i].CounterResetHint) - require.Equal(t, histograms[i].Count, decodedStats[i].Count) - require.Equal(t, histograms[i].Sum, decodedStats[i].Sum) - } - }) - t.Run("float_histogram_stats", func(t *testing.T) { - decodedStats := make([]*histogram.FloatHistogram, 0) - statsIterator := NewHistogramStatsIterator(newHistogramSeries(histograms).Iterator(nil)) - for statsIterator.Next() != chunkenc.ValNone { - _, h := statsIterator.AtFloatHistogram(nil) - decodedStats = append(decodedStats, h) - } - for i := 0; i < len(histograms); i++ { - fh := histograms[i].ToFloat(nil) - require.Equal(t, expectedHints[i], decodedStats[i].CounterResetHint) - require.Equal(t, fh.Count, decodedStats[i].Count) - require.Equal(t, fh.Sum, decodedStats[i].Sum) - } - }) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Run("histogram_stats", func(t *testing.T) { + decodedStats := make([]*histogram.Histogram, 0) + statsIterator := NewHistogramStatsIterator(newHistogramSeries(tc.histograms).Iterator(nil)) + for statsIterator.Next() != chunkenc.ValNone { + _, h := statsIterator.AtHistogram(nil) + decodedStats = append(decodedStats, h) + } + for i := 0; i < len(tc.histograms); i++ { + require.Equal(t, tc.expectedHints[i], decodedStats[i].CounterResetHint, fmt.Sprintf("mismatch in counter reset hint for histogram %d", i)) + h := tc.histograms[i] + if value.IsStaleNaN(h.Sum) { + require.True(t, value.IsStaleNaN(decodedStats[i].Sum)) + require.Equal(t, uint64(0), decodedStats[i].Count) + } else { + require.Equal(t, tc.histograms[i].Count, decodedStats[i].Count) + require.Equal(t, tc.histograms[i].Sum, decodedStats[i].Sum) + } + } + }) + t.Run("float_histogram_stats", func(t *testing.T) { + decodedStats := make([]*histogram.FloatHistogram, 0) + statsIterator := NewHistogramStatsIterator(newHistogramSeries(tc.histograms).Iterator(nil)) + for statsIterator.Next() != chunkenc.ValNone { + _, h := statsIterator.AtFloatHistogram(nil) + decodedStats = append(decodedStats, h) + } + for i := 0; i < len(tc.histograms); i++ { + require.Equal(t, tc.expectedHints[i], decodedStats[i].CounterResetHint) + fh := tc.histograms[i].ToFloat(nil) + if value.IsStaleNaN(fh.Sum) { + require.True(t, value.IsStaleNaN(decodedStats[i].Sum)) + require.Equal(t, float64(0), decodedStats[i].Count) + } else { + require.Equal(t, fh.Count, decodedStats[i].Count) + require.Equal(t, fh.Sum, decodedStats[i].Sum) + } + } + }) + }) + } } type histogramSeries struct { diff --git a/tsdb/tsdbutil/histogram.go b/tsdb/tsdbutil/histogram.go index 3c7349cf7..ce934a638 100644 --- a/tsdb/tsdbutil/histogram.go +++ b/tsdb/tsdbutil/histogram.go @@ -30,12 +30,10 @@ func GenerateTestHistograms(n int) (r []*histogram.Histogram) { return r } -func GenerateTestHistogramsWithUnknownResetHint(n int) []*histogram.Histogram { - hs := GenerateTestHistograms(n) - for i := range hs { - hs[i].CounterResetHint = histogram.UnknownCounterReset - } - return hs +func GenerateTestHistogramWithHint(n int, hint histogram.CounterResetHint) *histogram.Histogram { + h := GenerateTestHistogram(n) + h.CounterResetHint = hint + return h } // GenerateTestHistogram but it is up to the user to set any known counter reset hint. From 02d9d874a2f02d09201f77c7d122b042a3da4ab9 Mon Sep 17 00:00:00 2001 From: Filip Petkovski Date: Mon, 29 Jul 2024 14:53:32 +0200 Subject: [PATCH 02/20] Add more test cases Signed-off-by: Filip Petkovski --- promql/histogram_stats_iterator_test.go | 33 +++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/promql/histogram_stats_iterator_test.go b/promql/histogram_stats_iterator_test.go index d5c081348..7a2953d3e 100644 --- a/promql/histogram_stats_iterator_test.go +++ b/promql/histogram_stats_iterator_test.go @@ -63,6 +63,39 @@ func TestHistogramStatsDecoding(t *testing.T) { histogram.NotCounterReset, }, }, + { + name: "unknown counter reset at the beginning", + histograms: []*histogram.Histogram{ + tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset), + }, + expectedHints: []histogram.CounterResetHint{ + histogram.NotCounterReset, + }, + }, + { + name: "detect real counter reset", + histograms: []*histogram.Histogram{ + tsdbutil.GenerateTestHistogramWithHint(2, histogram.UnknownCounterReset), + tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset), + }, + expectedHints: []histogram.CounterResetHint{ + histogram.NotCounterReset, + histogram.CounterReset, + }, + }, + { + name: "detect real counter reset after stale NaN", + histograms: []*histogram.Histogram{ + tsdbutil.GenerateTestHistogramWithHint(2, histogram.UnknownCounterReset), + {Sum: math.Float64frombits(value.StaleNaN)}, + tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset), + }, + expectedHints: []histogram.CounterResetHint{ + histogram.NotCounterReset, + histogram.UnknownCounterReset, + histogram.CounterReset, + }, + }, } for _, tc := range cases { From 8f9751b00ddef4b06aa91b7213442726076c193f Mon Sep 17 00:00:00 2001 From: Filip Petkovski Date: Wed, 31 Jul 2024 11:17:09 +0200 Subject: [PATCH 03/20] Use CopyTo when resetting histogram in stats iterator The histogram stats iterator does not fully clear the histogram object and is not resilient to new fields being added to the histogram type. To resolve the issue, the commit uses the CopyTo methods which should be future proof to new fields being added. Signed-off-by: Filip Petkovski --- promql/histogram_stats_iterator.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/promql/histogram_stats_iterator.go b/promql/histogram_stats_iterator.go index dfafea5f8..c1b7746ec 100644 --- a/promql/histogram_stats_iterator.go +++ b/promql/histogram_stats_iterator.go @@ -63,9 +63,13 @@ func (f *histogramStatsIterator) AtHistogram(h *histogram.Histogram) (int64, *hi return t, h } - h.CounterResetHint = f.getResetHint(f.currentH) - h.Count = f.currentH.Count - h.Sum = f.currentH.Sum + returnValue := histogram.Histogram{ + CounterResetHint: f.getResetHint(f.currentH), + Count: f.currentH.Count, + Sum: f.currentH.Sum, + } + returnValue.CopyTo(h) + f.setLastH(f.currentH) return t, h } @@ -91,9 +95,13 @@ func (f *histogramStatsIterator) AtFloatHistogram(fh *histogram.FloatHistogram) return t, fh } - fh.CounterResetHint = f.getFloatResetHint(f.currentFH.CounterResetHint) - fh.Count = f.currentFH.Count - fh.Sum = f.currentFH.Sum + returnValue := histogram.FloatHistogram{ + CounterResetHint: f.getFloatResetHint(f.currentFH.CounterResetHint), + Count: f.currentFH.Count, + Sum: f.currentFH.Sum, + } + returnValue.CopyTo(fh) + f.setLastFH(f.currentFH) return t, fh } From 909785b97fd4b48317739377283c699d81ffe5e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Tue, 6 Aug 2024 07:46:26 +0200 Subject: [PATCH 04/20] Fix histogram pool poisoning bu chunkenc.Iterator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit chunkenc.Iterator.AtFloatHistogram may do a shallow copy if it receives nil as input pointer. This can in turn share the span slice with multiple histograms in the matrixSelectorHPool, leading to unexpected errors. Signed-off-by: György Krajcsovits --- promql/engine.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/promql/engine.go b/promql/engine.go index 25e67db63..621c2116e 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -2356,6 +2356,11 @@ loop: } else { histograms = append(histograms, HPoint{H: &histogram.FloatHistogram{}}) } + if histograms[n].H == nil { + // Initialize to non zero to AtFloatHistogram does a copy for sure. + // Not an issue in the loop above since that uses an intermediate buffer. + histograms[n].H = &histogram.FloatHistogram{} + } histograms[n].T, histograms[n].H = it.AtFloatHistogram(histograms[n].H) if value.IsStaleNaN(histograms[n].H.Sum) { histograms = histograms[:n] From 1fb0ff7e4516d5993e84c2bee0f4e303b7f0c8c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Tue, 6 Aug 2024 20:48:16 +0200 Subject: [PATCH 05/20] Add unit test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- promql/engine_test.go | 59 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/promql/engine_test.go b/promql/engine_test.go index 523c0613d..9ed3dd939 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -3798,3 +3798,62 @@ func makeInt64Pointer(val int64) *int64 { *valp = val return valp } + +func TestHistogramCopyFromIteratorRegression(t *testing.T) { + // Loading the following histograms creates two chunks because there's a + // counter reset. Not only the counter is lower in the last histogram + // but also there's missing buckets. + // This in turns means that chunk iterators will have different spans. + load := `load 1m +histogram {{sum:4 count:4 buckets:[2 2]}} {{sum:6 count:6 buckets:[3 3]}} {{sum:1 count:1 buckets:[1]}} +` + storage := promqltest.LoadedStorage(t, load) + t.Cleanup(func() { storage.Close() }) + engine := promqltest.NewTestEngine(false, 0, promqltest.DefaultMaxSamplesPerQuery) + + verify := func(t *testing.T, qry promql.Query, expected []histogram.FloatHistogram) { + res := qry.Exec(context.Background()) + require.NoError(t, res.Err) + + m, ok := res.Value.(promql.Matrix) + require.True(t, ok) + + require.Len(t, m, 1) + series := m[0] + + require.Empty(t, series.Floats) + require.Len(t, series.Histograms, len(expected)) + for i, e := range expected { + series.Histograms[i].H.CounterResetHint = histogram.UnknownCounterReset // Don't care. + require.Equal(t, &e, series.Histograms[i].H) + } + } + + qry, err := engine.NewRangeQuery(context.Background(), storage, nil, "increase(histogram[60s])", time.Unix(0, 0), time.Unix(0, 0).Add(1*time.Minute), time.Minute) + require.NoError(t, err) + verify(t, qry, []histogram.FloatHistogram{ + { + Count: 2, + Sum: 2, // Increase from 4 to 6 is 2. + PositiveSpans: []histogram.Span{{Offset: 0, Length: 2}}, // Two buckets changed between the first and second histogram. + PositiveBuckets: []float64{1, 1}, // Increase from 2 to 3 is 1 in both buckets. + }, + }) + + qry, err = engine.NewInstantQuery(context.Background(), storage, nil, "histogram[60s]", time.Unix(0, 0).Add(2*time.Minute)) + require.NoError(t, err) + verify(t, qry, []histogram.FloatHistogram{ + { + Count: 6, + Sum: 6, + PositiveSpans: []histogram.Span{{Offset: 0, Length: 2}}, + PositiveBuckets: []float64{3, 3}, + }, + { + Count: 1, + Sum: 1, + PositiveSpans: []histogram.Span{{Offset: 0, Length: 1}}, + PositiveBuckets: []float64{1}, + }, + }) +} From 1d7fe4be5c581265a5c102f3a4fbdc26af3fd357 Mon Sep 17 00:00:00 2001 From: George Krajcsovits Date: Wed, 7 Aug 2024 20:15:46 +0200 Subject: [PATCH 06/20] Update promql/engine.go Signed-off-by: George Krajcsovits --- promql/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/promql/engine.go b/promql/engine.go index 621c2116e..f6b79f3a4 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -2357,7 +2357,7 @@ loop: histograms = append(histograms, HPoint{H: &histogram.FloatHistogram{}}) } if histograms[n].H == nil { - // Initialize to non zero to AtFloatHistogram does a copy for sure. + // Make sure to pass non zero H to AtFloatHistogram so that it does a deep-copy. // Not an issue in the loop above since that uses an intermediate buffer. histograms[n].H = &histogram.FloatHistogram{} } From 8978f3ef71fb8d92d9b430cd3a6591b6ca231c7f Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 9 Aug 2024 12:07:28 +0100 Subject: [PATCH 07/20] Cut release 2.54.0 Signed-off-by: Bryan Boreham --- CHANGELOG.md | 8 ++------ VERSION | 2 +- web/ui/module/codemirror-promql/package.json | 4 ++-- web/ui/module/lezer-promql/package.json | 2 +- web/ui/package-lock.json | 14 +++++++------- web/ui/package.json | 2 +- web/ui/react-app/package.json | 4 ++-- 7 files changed, 16 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e338d9c5c..885749504 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,6 @@ # Changelog -## 2.54.0-rc.1 / 2024-08-05 - -* [BUGFIX] TSDB: Exclude OOO chunks mapped after compaction starts (introduced by #14396). #14584 - -## 2.54.0-rc.0 / 2024-07-19 +## 2.54.0 / 2024-08-09 Release 2.54 brings a release candidate of a major new version of [Remote Write: 2.0](https://prometheus.io/docs/specs/remote_write_spec_2_0/). This is experimental at this time and may still change. @@ -22,7 +18,7 @@ Remote-write v2 is enabled by default, but can be disabled via feature-flag `web * [ENHANCEMENT] TSDB: Optimise seek within index. #14393 * [ENHANCEMENT] TSDB: Optimise deletion of stale series. #14307 * [ENHANCEMENT] TSDB: Reduce locking to optimise adding and removing series. #13286,#14286 -* [ENHANCEMENT] TSDB: Small optimisation: streamline special handling for out-of-order data. #14396 +* [ENHANCEMENT] TSDB: Small optimisation: streamline special handling for out-of-order data. #14396,#14584 * [ENHANCEMENT] Regexps: Optimize patterns with multiple prefixes. #13843,#14368 * [ENHANCEMENT] Regexps: Optimize patterns containing multiple literal strings. #14173 * [ENHANCEMENT] AWS SD: expose Primary IPv6 addresses as __meta_ec2_primary_ipv6_addresses. #14156 diff --git a/VERSION b/VERSION index 149ab4732..99aed793a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.54.0-rc.1 +2.54.0 diff --git a/web/ui/module/codemirror-promql/package.json b/web/ui/module/codemirror-promql/package.json index 26823cdfd..6307f12f3 100644 --- a/web/ui/module/codemirror-promql/package.json +++ b/web/ui/module/codemirror-promql/package.json @@ -1,6 +1,6 @@ { "name": "@prometheus-io/codemirror-promql", - "version": "0.54.0-rc.1", + "version": "0.54.0", "description": "a CodeMirror mode for the PromQL language", "types": "dist/esm/index.d.ts", "module": "dist/esm/index.js", @@ -29,7 +29,7 @@ }, "homepage": "https://github.com/prometheus/prometheus/blob/main/web/ui/module/codemirror-promql/README.md", "dependencies": { - "@prometheus-io/lezer-promql": "0.54.0-rc.1", + "@prometheus-io/lezer-promql": "0.54.0", "lru-cache": "^7.18.3" }, "devDependencies": { diff --git a/web/ui/module/lezer-promql/package.json b/web/ui/module/lezer-promql/package.json index 774c95967..14d263241 100644 --- a/web/ui/module/lezer-promql/package.json +++ b/web/ui/module/lezer-promql/package.json @@ -1,6 +1,6 @@ { "name": "@prometheus-io/lezer-promql", - "version": "0.54.0-rc.1", + "version": "0.54.0", "description": "lezer-based PromQL grammar", "main": "dist/index.cjs", "type": "module", diff --git a/web/ui/package-lock.json b/web/ui/package-lock.json index 3243fe6aa..0f5db9f6d 100644 --- a/web/ui/package-lock.json +++ b/web/ui/package-lock.json @@ -1,12 +1,12 @@ { "name": "prometheus-io", - "version": "0.54.0-rc.1", + "version": "0.54.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "prometheus-io", - "version": "0.54.0-rc.1", + "version": "0.54.0", "workspaces": [ "react-app", "module/*" @@ -30,10 +30,10 @@ }, "module/codemirror-promql": { "name": "@prometheus-io/codemirror-promql", - "version": "0.54.0-rc.1", + "version": "0.54.0", "license": "Apache-2.0", "dependencies": { - "@prometheus-io/lezer-promql": "0.54.0-rc.1", + "@prometheus-io/lezer-promql": "0.54.0", "lru-cache": "^7.18.3" }, "devDependencies": { @@ -69,7 +69,7 @@ }, "module/lezer-promql": { "name": "@prometheus-io/lezer-promql", - "version": "0.54.0-rc.1", + "version": "0.54.0", "license": "Apache-2.0", "devDependencies": { "@lezer/generator": "^1.7.0", @@ -19332,7 +19332,7 @@ }, "react-app": { "name": "@prometheus-io/app", - "version": "0.54.0-rc.1", + "version": "0.54.0", "dependencies": { "@codemirror/autocomplete": "^6.17.0", "@codemirror/commands": "^6.6.0", @@ -19350,7 +19350,7 @@ "@lezer/lr": "^1.4.1", "@nexucis/fuzzy": "^0.4.1", "@nexucis/kvsearch": "^0.8.1", - "@prometheus-io/codemirror-promql": "0.54.0-rc.1", + "@prometheus-io/codemirror-promql": "0.54.0", "bootstrap": "^4.6.2", "css.escape": "^1.5.1", "downshift": "^9.0.6", diff --git a/web/ui/package.json b/web/ui/package.json index fe1e77ef5..01dd71f34 100644 --- a/web/ui/package.json +++ b/web/ui/package.json @@ -28,5 +28,5 @@ "ts-jest": "^29.2.2", "typescript": "^4.9.5" }, - "version": "0.54.0-rc.1" + "version": "0.54.0" } diff --git a/web/ui/react-app/package.json b/web/ui/react-app/package.json index b65354730..01620531a 100644 --- a/web/ui/react-app/package.json +++ b/web/ui/react-app/package.json @@ -1,6 +1,6 @@ { "name": "@prometheus-io/app", - "version": "0.54.0-rc.1", + "version": "0.54.0", "private": true, "dependencies": { "@codemirror/autocomplete": "^6.17.0", @@ -19,7 +19,7 @@ "@lezer/lr": "^1.4.1", "@nexucis/fuzzy": "^0.4.1", "@nexucis/kvsearch": "^0.8.1", - "@prometheus-io/codemirror-promql": "0.54.0-rc.1", + "@prometheus-io/codemirror-promql": "0.54.0", "bootstrap": "^4.6.2", "css.escape": "^1.5.1", "downshift": "^9.0.6", From 144470c7b011be44ab73e4fb3d7fe228f401a016 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 19 Aug 2024 10:58:35 +0100 Subject: [PATCH 08/20] [BUGFIX] Scraping: allow multiple samples on same series (#14685) So long as they specify timestamps. We don't check that the timestamps are different. Extend test, and use client_golang/prometheus/testutil to simplify metric check. Signed-off-by: Bryan Boreham --- scrape/scrape.go | 2 +- scrape/scrape_test.go | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/scrape/scrape.go b/scrape/scrape.go index 68411a62e..ccb068b68 100644 --- a/scrape/scrape.go +++ b/scrape/scrape.go @@ -1631,7 +1631,7 @@ loop: updateMetadata(lset, true) } - if seriesAlreadyScraped { + if seriesAlreadyScraped && parsedTimestamp == nil { err = storage.ErrDuplicateSampleForTimestamp } else { if ctMs := p.CreatedTimestamp(); sl.enableCTZeroIngestion && ctMs != nil { diff --git a/scrape/scrape_test.go b/scrape/scrape_test.go index a3fe6ac1a..c5a0b8b83 100644 --- a/scrape/scrape_test.go +++ b/scrape/scrape_test.go @@ -35,6 +35,7 @@ import ( "github.com/gogo/protobuf/proto" "github.com/google/go-cmp/cmp" "github.com/prometheus/client_golang/prometheus" + prom_testutil "github.com/prometheus/client_golang/prometheus/testutil" dto "github.com/prometheus/client_model/go" config_util "github.com/prometheus/common/config" "github.com/prometheus/common/model" @@ -3627,6 +3628,7 @@ func TestScrapeLoopSeriesAddedDuplicates(t *testing.T) { require.Equal(t, 3, total) require.Equal(t, 3, added) require.Equal(t, 1, seriesAdded) + require.Equal(t, 2.0, prom_testutil.ToFloat64(sl.metrics.targetScrapeSampleDuplicate)) slApp = sl.appender(ctx) total, added, seriesAdded, err = sl.append(slApp, []byte("test_metric 1\ntest_metric 1\ntest_metric 1\n"), "", time.Time{}) @@ -3635,12 +3637,18 @@ func TestScrapeLoopSeriesAddedDuplicates(t *testing.T) { require.Equal(t, 3, total) require.Equal(t, 3, added) require.Equal(t, 0, seriesAdded) + require.Equal(t, 4.0, prom_testutil.ToFloat64(sl.metrics.targetScrapeSampleDuplicate)) - metric := dto.Metric{} - err = sl.metrics.targetScrapeSampleDuplicate.Write(&metric) + // When different timestamps are supplied, multiple samples are accepted. + slApp = sl.appender(ctx) + total, added, seriesAdded, err = sl.append(slApp, []byte("test_metric 1 1001\ntest_metric 1 1002\ntest_metric 1 1003\n"), "", time.Time{}) require.NoError(t, err) - value := metric.GetCounter().GetValue() - require.Equal(t, 4.0, value) + require.NoError(t, slApp.Commit()) + require.Equal(t, 3, total) + require.Equal(t, 3, added) + require.Equal(t, 0, seriesAdded) + // Metric is not higher than last time. + require.Equal(t, 4.0, prom_testutil.ToFloat64(sl.metrics.targetScrapeSampleDuplicate)) } // This tests running a full scrape loop and checking that the scrape option From 89dee48cc81df4091ae1f9c1e402f32b47183440 Mon Sep 17 00:00:00 2001 From: "ouyang1204@gmail.com" Date: Sun, 11 Aug 2024 15:31:11 +0800 Subject: [PATCH 09/20] fix the issue of failing to match the first network when the container is reconnected to a new network Signed-off-by: ouyang1204@gmail.com --- discovery/moby/docker.go | 33 +++-- discovery/moby/docker_test.go | 119 ++++++++++++++++-- .../testdata/dockerprom/containers/json.json | 69 ++++++++++ docs/configuration/configuration.md | 4 +- 4 files changed, 198 insertions(+), 27 deletions(-) diff --git a/discovery/moby/docker.go b/discovery/moby/docker.go index 11445092e..68f6fe3cc 100644 --- a/discovery/moby/docker.go +++ b/discovery/moby/docker.go @@ -19,6 +19,7 @@ import ( "net" "net/http" "net/url" + "sort" "strconv" "time" @@ -251,28 +252,26 @@ func (d *DockerDiscovery) refresh(ctx context.Context) ([]*targetgroup.Group, er } if d.matchFirstNetwork && len(networks) > 1 { - // Match user defined network - if containerNetworkMode.IsUserDefined() { - networkMode := string(containerNetworkMode) - networks = map[string]*network.EndpointSettings{networkMode: networks[networkMode]} - } else { - // Get first network if container network mode has "none" value. - // This case appears under certain condition: - // 1. Container created with network set to "--net=none". - // 2. Disconnect network "none". - // 3. Reconnect network with user defined networks. - var first string - for k, n := range networks { - if n != nil { - first = k - break - } + // Sort networks by name and take first non-nil network. + keys := make([]string, 0, len(networks)) + for k, n := range networks { + if n != nil { + keys = append(keys, k) } - networks = map[string]*network.EndpointSettings{first: networks[first]} + } + if len(keys) > 0 { + sort.Strings(keys) + firstNetworkMode := keys[0] + firstNetwork := networks[firstNetworkMode] + networks = map[string]*network.EndpointSettings{firstNetworkMode: firstNetwork} } } for _, n := range networks { + if n == nil { + continue + } + var added bool for _, p := range c.Ports { diff --git a/discovery/moby/docker_test.go b/discovery/moby/docker_test.go index c108ddf58..398393a15 100644 --- a/discovery/moby/docker_test.go +++ b/discovery/moby/docker_test.go @@ -60,9 +60,9 @@ host: %s tg := tgs[0] require.NotNil(t, tg) require.NotNil(t, tg.Targets) - require.Len(t, tg.Targets, 6) + require.Len(t, tg.Targets, 8) - for i, lbls := range []model.LabelSet{ + expected := []model.LabelSet{ { "__address__": "172.19.0.2:9100", "__meta_docker_container_id": "c301b928faceb1a18fe379f6bc178727ef920bb30b0f9b8592b32b36255a0eca", @@ -163,7 +163,43 @@ host: %s "__meta_docker_network_scope": "local", "__meta_docker_port_private": "9104", }, - } { + { + "__address__": "172.20.0.3:3306", + "__meta_docker_container_id": "f84b2a0cfaa58d9e70b0657e2b3c6f44f0e973de4163a871299b4acf127b224f", + "__meta_docker_container_label_com_docker_compose_project": "dockersd", + "__meta_docker_container_label_com_docker_compose_service": "mysql", + "__meta_docker_container_label_com_docker_compose_version": "2.2.2", + "__meta_docker_container_name": "/dockersd_multi_networks", + "__meta_docker_container_network_mode": "dockersd_private_none", + "__meta_docker_network_id": "e804771e55254a360fdb70dfdd78d3610fdde231b14ef2f837a00ac1eeb9e601", + "__meta_docker_network_ingress": "false", + "__meta_docker_network_internal": "false", + "__meta_docker_network_ip": "172.20.0.3", + "__meta_docker_network_name": "dockersd_private", + "__meta_docker_network_scope": "local", + "__meta_docker_port_private": "3306", + }, + { + "__address__": "172.20.0.3:33060", + "__meta_docker_container_id": "f84b2a0cfaa58d9e70b0657e2b3c6f44f0e973de4163a871299b4acf127b224f", + "__meta_docker_container_label_com_docker_compose_project": "dockersd", + "__meta_docker_container_label_com_docker_compose_service": "mysql", + "__meta_docker_container_label_com_docker_compose_version": "2.2.2", + "__meta_docker_container_name": "/dockersd_multi_networks", + "__meta_docker_container_network_mode": "dockersd_private_none", + "__meta_docker_network_id": "e804771e55254a360fdb70dfdd78d3610fdde231b14ef2f837a00ac1eeb9e601", + "__meta_docker_network_ingress": "false", + "__meta_docker_network_internal": "false", + "__meta_docker_network_ip": "172.20.0.3", + "__meta_docker_network_name": "dockersd_private", + "__meta_docker_network_scope": "local", + "__meta_docker_port_private": "33060", + }, + } + sortFunc(expected) + sortFunc(tg.Targets) + + for i, lbls := range expected { t.Run(fmt.Sprintf("item %d", i), func(t *testing.T) { require.Equal(t, lbls, tg.Targets[i]) }) @@ -202,13 +238,8 @@ host: %s tg := tgs[0] require.NotNil(t, tg) require.NotNil(t, tg.Targets) - require.Len(t, tg.Targets, 9) + require.Len(t, tg.Targets, 13) - sortFunc := func(labelSets []model.LabelSet) { - sort.Slice(labelSets, func(i, j int) bool { - return labelSets[i]["__address__"] < labelSets[j]["__address__"] - }) - } expected := []model.LabelSet{ { "__address__": "172.19.0.2:9100", @@ -359,6 +390,70 @@ host: %s "__meta_docker_network_scope": "local", "__meta_docker_port_private": "9104", }, + { + "__address__": "172.20.0.3:3306", + "__meta_docker_container_id": "f84b2a0cfaa58d9e70b0657e2b3c6f44f0e973de4163a871299b4acf127b224f", + "__meta_docker_container_label_com_docker_compose_project": "dockersd", + "__meta_docker_container_label_com_docker_compose_service": "mysql", + "__meta_docker_container_label_com_docker_compose_version": "2.2.2", + "__meta_docker_container_name": "/dockersd_multi_networks", + "__meta_docker_container_network_mode": "dockersd_private_none", + "__meta_docker_network_id": "e804771e55254a360fdb70dfdd78d3610fdde231b14ef2f837a00ac1eeb9e601", + "__meta_docker_network_ingress": "false", + "__meta_docker_network_internal": "false", + "__meta_docker_network_ip": "172.20.0.3", + "__meta_docker_network_name": "dockersd_private", + "__meta_docker_network_scope": "local", + "__meta_docker_port_private": "3306", + }, + { + "__address__": "172.20.0.3:33060", + "__meta_docker_container_id": "f84b2a0cfaa58d9e70b0657e2b3c6f44f0e973de4163a871299b4acf127b224f", + "__meta_docker_container_label_com_docker_compose_project": "dockersd", + "__meta_docker_container_label_com_docker_compose_service": "mysql", + "__meta_docker_container_label_com_docker_compose_version": "2.2.2", + "__meta_docker_container_name": "/dockersd_multi_networks", + "__meta_docker_container_network_mode": "dockersd_private_none", + "__meta_docker_network_id": "e804771e55254a360fdb70dfdd78d3610fdde231b14ef2f837a00ac1eeb9e601", + "__meta_docker_network_ingress": "false", + "__meta_docker_network_internal": "false", + "__meta_docker_network_ip": "172.20.0.3", + "__meta_docker_network_name": "dockersd_private", + "__meta_docker_network_scope": "local", + "__meta_docker_port_private": "33060", + }, + { + "__address__": "172.21.0.3:3306", + "__meta_docker_container_id": "f84b2a0cfaa58d9e70b0657e2b3c6f44f0e973de4163a871299b4acf127b224f", + "__meta_docker_container_label_com_docker_compose_project": "dockersd", + "__meta_docker_container_label_com_docker_compose_service": "mysql", + "__meta_docker_container_label_com_docker_compose_version": "2.2.2", + "__meta_docker_container_name": "/dockersd_multi_networks", + "__meta_docker_container_network_mode": "dockersd_private_none", + "__meta_docker_network_id": "bfcf66a6b64f7d518f009e34290dc3f3c66a08164257ad1afc3bd31d75f656e8", + "__meta_docker_network_ingress": "false", + "__meta_docker_network_internal": "false", + "__meta_docker_network_ip": "172.21.0.3", + "__meta_docker_network_name": "dockersd_private1", + "__meta_docker_network_scope": "local", + "__meta_docker_port_private": "3306", + }, + { + "__address__": "172.21.0.3:33060", + "__meta_docker_container_id": "f84b2a0cfaa58d9e70b0657e2b3c6f44f0e973de4163a871299b4acf127b224f", + "__meta_docker_container_label_com_docker_compose_project": "dockersd", + "__meta_docker_container_label_com_docker_compose_service": "mysql", + "__meta_docker_container_label_com_docker_compose_version": "2.2.2", + "__meta_docker_container_name": "/dockersd_multi_networks", + "__meta_docker_container_network_mode": "dockersd_private_none", + "__meta_docker_network_id": "bfcf66a6b64f7d518f009e34290dc3f3c66a08164257ad1afc3bd31d75f656e8", + "__meta_docker_network_ingress": "false", + "__meta_docker_network_internal": "false", + "__meta_docker_network_ip": "172.21.0.3", + "__meta_docker_network_name": "dockersd_private1", + "__meta_docker_network_scope": "local", + "__meta_docker_port_private": "33060", + }, } sortFunc(expected) @@ -370,3 +465,9 @@ host: %s }) } } + +func sortFunc(labelSets []model.LabelSet) { + sort.Slice(labelSets, func(i, j int) bool { + return labelSets[i]["__address__"] < labelSets[j]["__address__"] + }) +} diff --git a/discovery/moby/testdata/dockerprom/containers/json.json b/discovery/moby/testdata/dockerprom/containers/json.json index ebfc56b6d..33406bf9a 100644 --- a/discovery/moby/testdata/dockerprom/containers/json.json +++ b/discovery/moby/testdata/dockerprom/containers/json.json @@ -228,5 +228,74 @@ "Networks": {} }, "Mounts": [] + }, + { + "Id": "f84b2a0cfaa58d9e70b0657e2b3c6f44f0e973de4163a871299b4acf127b224f", + "Names": [ + "/dockersd_multi_networks" + ], + "Image": "mysql:5.7.29", + "ImageID": "sha256:16ae2f4625ba63a250462bedeece422e741de9f0caf3b1d89fd5b257aca80cd1", + "Command": "mysqld", + "Created": 1616273136, + "Ports": [ + { + "PrivatePort": 3306, + "Type": "tcp" + }, + { + "PrivatePort": 33060, + "Type": "tcp" + } + ], + "Labels": { + "com.docker.compose.project": "dockersd", + "com.docker.compose.service": "mysql", + "com.docker.compose.version": "2.2.2" + }, + "State": "running", + "Status": "Up 40 seconds", + "HostConfig": { + "NetworkMode": "dockersd_private_none" + }, + "NetworkSettings": { + "Networks": { + "dockersd_private": { + "IPAMConfig": null, + "Links": null, + "Aliases": null, + "NetworkID": "e804771e55254a360fdb70dfdd78d3610fdde231b14ef2f837a00ac1eeb9e601", + "EndpointID": "972d6807997369605ace863af58de6cb90c787a5bf2ffc4105662d393ae539b7", + "Gateway": "172.20.0.1", + "IPAddress": "172.20.0.3", + "IPPrefixLen": 16, + "IPv6Gateway": "", + "GlobalIPv6Address": "", + "GlobalIPv6PrefixLen": 0, + "MacAddress": "02:42:ac:14:00:02", + "DriverOpts": null + }, + "dockersd_private1": { + "IPAMConfig": {}, + "Links": null, + "Aliases": [ + "mysql", + "mysql", + "f9ade4b83199" + ], + "NetworkID": "bfcf66a6b64f7d518f009e34290dc3f3c66a08164257ad1afc3bd31d75f656e8", + "EndpointID": "91a98405344ee1cb7d977cafabe634837876651544b32da20a5e0155868e6f5f", + "Gateway": "172.21.0.1", + "IPAddress": "172.21.0.3", + "IPPrefixLen": 24, + "IPv6Gateway": "", + "GlobalIPv6Address": "", + "GlobalIPv6PrefixLen": 0, + "MacAddress": "02:42:ac:15:00:02", + "DriverOpts": null + } + } + }, + "Mounts": [] } ] diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md index ff24082e4..6ce3d9c48 100644 --- a/docs/configuration/configuration.md +++ b/docs/configuration/configuration.md @@ -943,7 +943,9 @@ tls_config: # The host to use if the container is in host networking mode. [ host_networking_host: | default = "localhost" ] -# Match the first network if the container has multiple networks defined, thus avoiding collecting duplicate targets. +# Sort all non-nil networks in ascending order based on network name and +# get the first network if the container has multiple networks defined, +# thus avoiding collecting duplicate targets. [ match_first_network: | default = true ] # Optional filters to limit the discovery process to a subset of available From 8f828d45c11c2c3555f8aab4798295290ffc3e67 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Sun, 4 Aug 2024 08:35:09 +0800 Subject: [PATCH 10/20] convert TestNativeHistogram_Sum_Count_Add_AvgOperator into testing framework Signed-off-by: Ziqi Zhao --- model/histogram/float_histogram.go | 14 +- promql/engine_test.go | 211 ------------------ promql/promqltest/test.go | 4 +- .../testdata/native_histograms.test | 36 +++ util/almost/almost.go | 10 +- 5 files changed, 57 insertions(+), 218 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index 2a37ea66d..b3baaffb0 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -17,6 +17,12 @@ import ( "fmt" "math" "strings" + + "github.com/prometheus/prometheus/util/almost" +) + +const ( + defaultEpsilon = 0.000001 ) // FloatHistogram is similar to Histogram but uses float64 for all @@ -456,8 +462,8 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { } if h.Schema != h2.Schema || - math.Float64bits(h.Count) != math.Float64bits(h2.Count) || - math.Float64bits(h.Sum) != math.Float64bits(h2.Sum) { + !almost.Equal(h.Count, h2.Count, defaultEpsilon) || + !almost.Equal(h.Sum, h2.Sum, defaultEpsilon) { return false } @@ -468,7 +474,7 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { } if h.ZeroThreshold != h2.ZeroThreshold || - math.Float64bits(h.ZeroCount) != math.Float64bits(h2.ZeroCount) { + !almost.Equal(h.ZeroCount, h2.ZeroCount, defaultEpsilon) { return false } @@ -1310,7 +1316,7 @@ func FloatBucketsMatch(b1, b2 []float64) bool { return false } for i, b := range b1 { - if math.Float64bits(b) != math.Float64bits(b2[i]) { + if !almost.Equal(b, b2[i], defaultEpsilon) { return false } } diff --git a/promql/engine_test.go b/promql/engine_test.go index 8d5f87244..37cba975b 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -3097,217 +3097,6 @@ func TestInstantQueryWithRangeVectorSelector(t *testing.T) { } } -func TestNativeHistogram_Sum_Count_Add_AvgOperator(t *testing.T) { - // TODO(codesome): Integrate histograms into the PromQL testing framework - // and write more tests there. - cases := []struct { - histograms []histogram.Histogram - expected histogram.FloatHistogram - expectedAvg histogram.FloatHistogram - }{ - { - histograms: []histogram.Histogram{ - { - CounterResetHint: histogram.GaugeType, - Schema: 0, - Count: 25, - Sum: 1234.5, - ZeroThreshold: 0.001, - ZeroCount: 4, - PositiveSpans: []histogram.Span{ - {Offset: 0, Length: 2}, - {Offset: 1, Length: 2}, - }, - PositiveBuckets: []int64{1, 1, -1, 0}, - NegativeSpans: []histogram.Span{ - {Offset: 0, Length: 2}, - {Offset: 2, Length: 2}, - }, - NegativeBuckets: []int64{2, 2, -3, 8}, - }, - { - CounterResetHint: histogram.GaugeType, - Schema: 0, - Count: 41, - Sum: 2345.6, - ZeroThreshold: 0.001, - ZeroCount: 5, - PositiveSpans: []histogram.Span{ - {Offset: 0, Length: 4}, - {Offset: 0, Length: 0}, - {Offset: 0, Length: 3}, - }, - PositiveBuckets: []int64{1, 2, -2, 1, -1, 0, 0}, - NegativeSpans: []histogram.Span{ - {Offset: 1, Length: 4}, - {Offset: 2, Length: 0}, - {Offset: 2, Length: 3}, - }, - NegativeBuckets: []int64{1, 3, -2, 5, -2, 0, -3}, - }, - { - CounterResetHint: histogram.GaugeType, - Schema: 0, - Count: 41, - Sum: 1111.1, - ZeroThreshold: 0.001, - ZeroCount: 5, - PositiveSpans: []histogram.Span{ - {Offset: 0, Length: 4}, - {Offset: 0, Length: 0}, - {Offset: 0, Length: 3}, - }, - PositiveBuckets: []int64{1, 2, -2, 1, -1, 0, 0}, - NegativeSpans: []histogram.Span{ - {Offset: 1, Length: 4}, - {Offset: 2, Length: 0}, - {Offset: 2, Length: 3}, - }, - NegativeBuckets: []int64{1, 3, -2, 5, -2, 0, -3}, - }, - { - CounterResetHint: histogram.GaugeType, - Schema: 1, // Everything is 0 just to make the count 4 so avg has nicer numbers. - }, - }, - expected: histogram.FloatHistogram{ - CounterResetHint: histogram.GaugeType, - Schema: 0, - ZeroThreshold: 0.001, - ZeroCount: 14, - Count: 107, - Sum: 4691.2, - PositiveSpans: []histogram.Span{ - {Offset: 0, Length: 7}, - }, - PositiveBuckets: []float64{3, 8, 2, 5, 3, 2, 2}, - NegativeSpans: []histogram.Span{ - {Offset: 0, Length: 6}, - {Offset: 3, Length: 3}, - }, - NegativeBuckets: []float64{2, 6, 8, 4, 15, 9, 10, 10, 4}, - }, - expectedAvg: histogram.FloatHistogram{ - CounterResetHint: histogram.GaugeType, - Schema: 0, - ZeroThreshold: 0.001, - ZeroCount: 3.5, - Count: 26.75, - Sum: 1172.8, - PositiveSpans: []histogram.Span{ - {Offset: 0, Length: 7}, - }, - PositiveBuckets: []float64{0.75, 2, 0.5, 1.25, 0.75, 0.5, 0.5}, - NegativeSpans: []histogram.Span{ - {Offset: 0, Length: 6}, - {Offset: 3, Length: 3}, - }, - NegativeBuckets: []float64{0.5, 1.5, 2, 1, 3.75, 2.25, 2.5, 2.5, 1}, - }, - }, - } - - idx0 := int64(0) - for _, c := range cases { - for _, floatHisto := range []bool{true, false} { - t.Run(fmt.Sprintf("floatHistogram=%t %d", floatHisto, idx0), func(t *testing.T) { - storage := teststorage.New(t) - t.Cleanup(func() { storage.Close() }) - - seriesName := "sparse_histogram_series" - seriesNameOverTime := "sparse_histogram_series_over_time" - - engine := newTestEngine() - - ts := idx0 * int64(10*time.Minute/time.Millisecond) - app := storage.Appender(context.Background()) - _, err := app.Append(0, labels.FromStrings("__name__", "float_series", "idx", "0"), ts, 42) - require.NoError(t, err) - for idx1, h := range c.histograms { - lbls := labels.FromStrings("__name__", seriesName, "idx", strconv.Itoa(idx1)) - // Since we mutate h later, we need to create a copy here. - var err error - if floatHisto { - _, err = app.AppendHistogram(0, lbls, ts, nil, h.Copy().ToFloat(nil)) - } else { - _, err = app.AppendHistogram(0, lbls, ts, h.Copy(), nil) - } - require.NoError(t, err) - - lbls = labels.FromStrings("__name__", seriesNameOverTime) - newTs := ts + int64(idx1)*int64(time.Minute/time.Millisecond) - // Since we mutate h later, we need to create a copy here. - if floatHisto { - _, err = app.AppendHistogram(0, lbls, newTs, nil, h.Copy().ToFloat(nil)) - } else { - _, err = app.AppendHistogram(0, lbls, newTs, h.Copy(), nil) - } - require.NoError(t, err) - } - require.NoError(t, app.Commit()) - - queryAndCheck := func(queryString string, ts int64, exp promql.Vector) { - qry, err := engine.NewInstantQuery(context.Background(), storage, nil, queryString, timestamp.Time(ts)) - require.NoError(t, err) - - res := qry.Exec(context.Background()) - require.NoError(t, res.Err) - require.Empty(t, res.Warnings) - - vector, err := res.Vector() - require.NoError(t, err) - - testutil.RequireEqual(t, exp, vector) - } - queryAndCheckAnnotations := func(queryString string, ts int64, expWarnings annotations.Annotations) { - qry, err := engine.NewInstantQuery(context.Background(), storage, nil, queryString, timestamp.Time(ts)) - require.NoError(t, err) - - res := qry.Exec(context.Background()) - require.NoError(t, res.Err) - require.Equal(t, expWarnings, res.Warnings) - } - - // sum(). - queryString := fmt.Sprintf("sum(%s)", seriesName) - queryAndCheck(queryString, ts, []promql.Sample{{T: ts, H: &c.expected, Metric: labels.EmptyLabels()}}) - - queryString = `sum({idx="0"})` - var annos annotations.Annotations - annos.Add(annotations.NewMixedFloatsHistogramsAggWarning(posrange.PositionRange{Start: 4, End: 13})) - queryAndCheckAnnotations(queryString, ts, annos) - - // + operator. - queryString = fmt.Sprintf(`%s{idx="0"}`, seriesName) - for idx := 1; idx < len(c.histograms); idx++ { - queryString += fmt.Sprintf(` + ignoring(idx) %s{idx="%d"}`, seriesName, idx) - } - queryAndCheck(queryString, ts, []promql.Sample{{T: ts, H: &c.expected, Metric: labels.EmptyLabels()}}) - - // count(). - queryString = fmt.Sprintf("count(%s)", seriesName) - queryAndCheck(queryString, ts, []promql.Sample{{T: ts, F: 4, Metric: labels.EmptyLabels()}}) - - // avg(). - queryString = fmt.Sprintf("avg(%s)", seriesName) - queryAndCheck(queryString, ts, []promql.Sample{{T: ts, H: &c.expectedAvg, Metric: labels.EmptyLabels()}}) - - offset := int64(len(c.histograms) - 1) - newTs := ts + offset*int64(time.Minute/time.Millisecond) - - // sum_over_time(). - queryString = fmt.Sprintf("sum_over_time(%s[%dm:1m])", seriesNameOverTime, offset) - queryAndCheck(queryString, newTs, []promql.Sample{{T: newTs, H: &c.expected, Metric: labels.EmptyLabels()}}) - - // avg_over_time(). - queryString = fmt.Sprintf("avg_over_time(%s[%dm:1m])", seriesNameOverTime, offset) - queryAndCheck(queryString, newTs, []promql.Sample{{T: newTs, H: &c.expectedAvg, Metric: labels.EmptyLabels()}}) - }) - idx0++ - } - } -} - func TestNativeHistogram_SubOperator(t *testing.T) { // TODO(codesome): Integrate histograms into the PromQL testing framework // and write more tests there. diff --git a/promql/promqltest/test.go b/promql/promqltest/test.go index 8b1ec381a..de67cae83 100644 --- a/promql/promqltest/test.go +++ b/promql/promqltest/test.go @@ -769,7 +769,7 @@ func (ev *evalCmd) compareResult(result parser.Value) error { return fmt.Errorf("expected histogram value at index %v for %s to have timestamp %v, but it had timestamp %v (result has %s)", i, ev.metrics[hash], expected.T, actual.T, formatSeriesResult(s)) } - if !actual.H.Equals(expected.H.Compact(0)) { + if !actual.H.Compact(0).Equals(expected.H.Compact(0)) { return fmt.Errorf("expected histogram value at index %v (t=%v) for %s to be %v, but got %v (result has %s)", i, actual.T, ev.metrics[hash], expected.H, actual.H, formatSeriesResult(s)) } } @@ -804,7 +804,7 @@ func (ev *evalCmd) compareResult(result parser.Value) error { if expH != nil && v.H == nil { return fmt.Errorf("expected histogram %s for %s but got float value %v", HistogramTestExpression(expH), v.Metric, v.F) } - if expH != nil && !expH.Compact(0).Equals(v.H) { + if expH != nil && !expH.Compact(0).Equals(v.H.Compact(0)) { return fmt.Errorf("expected %v for %s but got %s", HistogramTestExpression(expH), v.Metric, HistogramTestExpression(v.H)) } if !almost.Equal(exp0.Value, v.F, defaultEpsilon) { diff --git a/promql/promqltest/testdata/native_histograms.test b/promql/promqltest/testdata/native_histograms.test index ea838f1e3..110fd69e7 100644 --- a/promql/promqltest/testdata/native_histograms.test +++ b/promql/promqltest/testdata/native_histograms.test @@ -886,3 +886,39 @@ eval_warn instant at 0 sum by (group) (metric) {group="just-floats"} 5 {group="just-exponential-histograms"} {{sum:5 count:7 buckets:[2 3 2]}} {group="just-custom-histograms"} {{schema:-53 sum:4 count:5 custom_values:[2] buckets:[8]}} + +clear + +# Test native histograms with sum, count, avg. +load 10m + histogram_sum{idx="0"} {{schema:0 count:25 sum:1234.5 z_bucket:4 z_bucket_w:0.001 buckets:[1 2 0 1 1] n_buckets:[2 4 0 0 1 9]}}x1 + histogram_sum{idx="1"} {{schema:0 count:41 sum:2345.6 z_bucket:5 z_bucket_w:0.001 buckets:[1 3 1 2 1 1 1] n_buckets:[0 1 4 2 7 0 0 0 0 5 5 2]}}x1 + histogram_sum{idx="2"} {{schema:0 count:41 sum:1111.1 z_bucket:5 z_bucket_w:0.001 buckets:[1 3 1 2 1 1 1] n_buckets:[0 1 4 2 7 0 0 0 0 5 5 2]}}x1 + histogram_sum{idx="3"} {{schema:1 count:0}}x1 + histogram_sum_float{idx="0"} 42.0x1 + +eval instant at 10m sum(histogram_sum) + {} {{schema:0 count:107 sum:4691.2 z_bucket:14 z_bucket_w:0.001 buckets:[3 8 2 5 3 2 2] n_buckets:[2 6 8 4 15 9 0 0 0 10 10 4]}} + +eval_warn instant at 10m sum({idx="0"}) + +eval instant at 10m sum(histogram_sum{idx="0"} + ignoring(idx) histogram_sum{idx="1"} + ignoring(idx) histogram_sum{idx="2"} + ignoring(idx) histogram_sum{idx="3"}) + {} {{schema:0 count:107 sum:4691.2 z_bucket:14 z_bucket_w:0.001 buckets:[3 8 2 5 3 2 2] n_buckets:[2 6 8 4 15 9 0 0 0 10 10 4]}} + +eval instant at 10m count(histogram_sum) + {} 4 + +eval instant at 10m avg(histogram_sum) + {} {{schema:0 count:26.75 sum:1172.8 z_bucket:3.5 z_bucket_w:0.001 buckets:[0.75 2 0.5 1.25 0.75 0.5 0.5] n_buckets:[0.5 1.5 2 1 3.75 2.25 0 0 0 2.5 2.5 1]}} + +clear + +# Test native histograms with sum_over_time, avg_over_time. +load 1m + histogram_sum_over_time {{schema:0 count:25 sum:1234.5 z_bucket:4 z_bucket_w:0.001 buckets:[1 2 0 1 1] n_buckets:[2 4 0 0 1 9]}} {{schema:0 count:41 sum:2345.6 z_bucket:5 z_bucket_w:0.001 buckets:[1 3 1 2 1 1 1] n_buckets:[0 1 4 2 7 0 0 0 0 5 5 2]}} {{schema:0 count:41 sum:1111.1 z_bucket:5 z_bucket_w:0.001 buckets:[1 3 1 2 1 1 1] n_buckets:[0 1 4 2 7 0 0 0 0 5 5 2]}} {{schema:1 count:0}} + +eval instant at 3m sum_over_time(histogram_sum_over_time[3m:1m]) + {} {{schema:0 count:107 sum:4691.2 z_bucket:14 z_bucket_w:0.001 buckets:[3 8 2 5 3 2 2] n_buckets:[2 6 8 4 15 9 0 0 0 10 10 4]}} + +eval instant at 3m avg_over_time(histogram_sum_over_time[3m:1m]) + {} {{schema:0 count:26.75 sum:1172.8 z_bucket:3.5 z_bucket_w:0.001 buckets:[0.75 2 0.5 1.25 0.75 0.5 0.5] n_buckets:[0.5 1.5 2 1 3.75 2.25 0 0 0 2.5 2.5 1]}} diff --git a/util/almost/almost.go b/util/almost/almost.go index 34f1290a5..1803c4b43 100644 --- a/util/almost/almost.go +++ b/util/almost/almost.go @@ -13,13 +13,21 @@ package almost -import "math" +import ( + "math" + + "github.com/prometheus/prometheus/model/value" +) var minNormal = math.Float64frombits(0x0010000000000000) // The smallest positive normal value of type float64. // Equal returns true if a and b differ by less than their sum // multiplied by epsilon. func Equal(a, b, epsilon float64) bool { + if value.IsStaleNaN(a) || value.IsStaleNaN(b) { + return value.IsStaleNaN(a) && value.IsStaleNaN(b) + } + // NaN has no equality but for testing we still want to know whether both values // are NaN. if math.IsNaN(a) && math.IsNaN(b) { From c7c4a5c3476c39c4776eb5146ad9c93b85e4fb58 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Fri, 23 Aug 2024 09:28:31 +0800 Subject: [PATCH 11/20] add helper function to compare native histograms in testing Signed-off-by: Ziqi Zhao --- model/histogram/float_histogram.go | 14 +--- promql/promqltest/test.go | 119 ++++++++++++++++++++++++++++- util/almost/almost.go | 2 + 3 files changed, 123 insertions(+), 12 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index b3baaffb0..2a37ea66d 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -17,12 +17,6 @@ import ( "fmt" "math" "strings" - - "github.com/prometheus/prometheus/util/almost" -) - -const ( - defaultEpsilon = 0.000001 ) // FloatHistogram is similar to Histogram but uses float64 for all @@ -462,8 +456,8 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { } if h.Schema != h2.Schema || - !almost.Equal(h.Count, h2.Count, defaultEpsilon) || - !almost.Equal(h.Sum, h2.Sum, defaultEpsilon) { + math.Float64bits(h.Count) != math.Float64bits(h2.Count) || + math.Float64bits(h.Sum) != math.Float64bits(h2.Sum) { return false } @@ -474,7 +468,7 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { } if h.ZeroThreshold != h2.ZeroThreshold || - !almost.Equal(h.ZeroCount, h2.ZeroCount, defaultEpsilon) { + math.Float64bits(h.ZeroCount) != math.Float64bits(h2.ZeroCount) { return false } @@ -1316,7 +1310,7 @@ func FloatBucketsMatch(b1, b2 []float64) bool { return false } for i, b := range b1 { - if !almost.Equal(b, b2[i], defaultEpsilon) { + if math.Float64bits(b) != math.Float64bits(b2[i]) { return false } } diff --git a/promql/promqltest/test.go b/promql/promqltest/test.go index de67cae83..1773b6341 100644 --- a/promql/promqltest/test.go +++ b/promql/promqltest/test.go @@ -769,7 +769,7 @@ func (ev *evalCmd) compareResult(result parser.Value) error { return fmt.Errorf("expected histogram value at index %v for %s to have timestamp %v, but it had timestamp %v (result has %s)", i, ev.metrics[hash], expected.T, actual.T, formatSeriesResult(s)) } - if !actual.H.Compact(0).Equals(expected.H.Compact(0)) { + if !compareNativeHistogram(expected.H.Compact(0), actual.H.Compact(0)) { return fmt.Errorf("expected histogram value at index %v (t=%v) for %s to be %v, but got %v (result has %s)", i, actual.T, ev.metrics[hash], expected.H, actual.H, formatSeriesResult(s)) } } @@ -804,7 +804,7 @@ func (ev *evalCmd) compareResult(result parser.Value) error { if expH != nil && v.H == nil { return fmt.Errorf("expected histogram %s for %s but got float value %v", HistogramTestExpression(expH), v.Metric, v.F) } - if expH != nil && !expH.Compact(0).Equals(v.H.Compact(0)) { + if expH != nil && !compareNativeHistogram(expH.Compact(0), v.H.Compact(0)) { return fmt.Errorf("expected %v for %s but got %s", HistogramTestExpression(expH), v.Metric, HistogramTestExpression(v.H)) } if !almost.Equal(exp0.Value, v.F, defaultEpsilon) { @@ -837,6 +837,121 @@ func (ev *evalCmd) compareResult(result parser.Value) error { return nil } +// compareNativeHistogram is helper function to compare two native histograms +// which can tolerate some differ in the field of float type, such as Count, Sum +func compareNativeHistogram(exp, cur *histogram.FloatHistogram) bool { + if exp == nil || cur == nil { + return false + } + + if exp.Schema != cur.Schema || + !almost.Equal(exp.Count, cur.Count, defaultEpsilon) || + !almost.Equal(exp.Sum, cur.Sum, defaultEpsilon) { + return false + } + + if exp.UsesCustomBuckets() { + if !histogram.FloatBucketsMatch(exp.CustomValues, cur.CustomValues) { + return false + } + } + + if exp.ZeroThreshold != cur.ZeroThreshold || + !almost.Equal(exp.ZeroCount, cur.ZeroCount, defaultEpsilon) { + return false + } + + if !spansMatch(exp.NegativeSpans, cur.NegativeSpans) { + return false + } + if !floatBucketsMatch(exp.NegativeBuckets, cur.NegativeBuckets) { + return false + } + + if !spansMatch(exp.PositiveSpans, cur.PositiveSpans) { + return false + } + if !floatBucketsMatch(exp.PositiveBuckets, cur.PositiveBuckets) { + return false + } + + return true +} + +func floatBucketsMatch(b1, b2 []float64) bool { + if len(b1) != len(b2) { + return false + } + for i, b := range b1 { + if !almost.Equal(b, b2[i], defaultEpsilon) { + return false + } + } + return true +} + +func spansMatch(s1, s2 []histogram.Span) bool { + if len(s1) == 0 && len(s2) == 0 { + return true + } + + s1idx, s2idx := 0, 0 + for { + if s1idx >= len(s1) { + return allEmptySpans(s2[s2idx:]) + } + if s2idx >= len(s2) { + return allEmptySpans(s1[s1idx:]) + } + + currS1, currS2 := s1[s1idx], s2[s2idx] + s1idx++ + s2idx++ + if currS1.Length == 0 { + // This span is zero length, so we add consecutive such spans + // until we find a non-zero span. + for ; s1idx < len(s1) && s1[s1idx].Length == 0; s1idx++ { + currS1.Offset += s1[s1idx].Offset + } + if s1idx < len(s1) { + currS1.Offset += s1[s1idx].Offset + currS1.Length = s1[s1idx].Length + s1idx++ + } + } + if currS2.Length == 0 { + // This span is zero length, so we add consecutive such spans + // until we find a non-zero span. + for ; s2idx < len(s2) && s2[s2idx].Length == 0; s2idx++ { + currS2.Offset += s2[s2idx].Offset + } + if s2idx < len(s2) { + currS2.Offset += s2[s2idx].Offset + currS2.Length = s2[s2idx].Length + s2idx++ + } + } + + if currS1.Length == 0 && currS2.Length == 0 { + // The last spans of both set are zero length. Previous spans match. + return true + } + + if currS1.Offset != currS2.Offset || currS1.Length != currS2.Length { + return false + } + } +} + +func allEmptySpans(s []histogram.Span) bool { + for _, ss := range s { + if ss.Length > 0 { + return false + } + } + return true +} + func (ev *evalCmd) checkExpectedFailure(actual error) error { if ev.expectedFailMessage != "" { if ev.expectedFailMessage != actual.Error() { diff --git a/util/almost/almost.go b/util/almost/almost.go index 1803c4b43..e0b5acb82 100644 --- a/util/almost/almost.go +++ b/util/almost/almost.go @@ -24,6 +24,8 @@ var minNormal = math.Float64frombits(0x0010000000000000) // The smallest positiv // Equal returns true if a and b differ by less than their sum // multiplied by epsilon. func Equal(a, b, epsilon float64) bool { + // StaleNaN is a special value that is used as staleness maker, so + // the two values are equal when both are exactly equals to stale NaN if value.IsStaleNaN(a) || value.IsStaleNaN(b) { return value.IsStaleNaN(a) && value.IsStaleNaN(b) } From 87eab0aaad8af2348415b2d3582aacbffb613b23 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Fri, 23 Aug 2024 09:32:04 +0800 Subject: [PATCH 12/20] fix golang lint Signed-off-by: Ziqi Zhao --- promql/promqltest/test.go | 2 +- util/almost/almost.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/promql/promqltest/test.go b/promql/promqltest/test.go index 1773b6341..4d12dead9 100644 --- a/promql/promqltest/test.go +++ b/promql/promqltest/test.go @@ -838,7 +838,7 @@ func (ev *evalCmd) compareResult(result parser.Value) error { } // compareNativeHistogram is helper function to compare two native histograms -// which can tolerate some differ in the field of float type, such as Count, Sum +// which can tolerate some differ in the field of float type, such as Count, Sum. func compareNativeHistogram(exp, cur *histogram.FloatHistogram) bool { if exp == nil || cur == nil { return false diff --git a/util/almost/almost.go b/util/almost/almost.go index e0b5acb82..a40462658 100644 --- a/util/almost/almost.go +++ b/util/almost/almost.go @@ -25,7 +25,7 @@ var minNormal = math.Float64frombits(0x0010000000000000) // The smallest positiv // multiplied by epsilon. func Equal(a, b, epsilon float64) bool { // StaleNaN is a special value that is used as staleness maker, so - // the two values are equal when both are exactly equals to stale NaN + // the two values are equal when both are exactly equals to stale NaN. if value.IsStaleNaN(a) || value.IsStaleNaN(b) { return value.IsStaleNaN(a) && value.IsStaleNaN(b) } From 436a439ed2aa8a1dfd36555b4cbaf22019619314 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Fri, 23 Aug 2024 14:13:41 -0400 Subject: [PATCH 13/20] fix(utf8): fix config logic for name validation We should only overwrite the ScrapeConfig if it is empty. Added tests part of https://github.com/prometheus/prometheus/issues/13095 Signed-off-by: Owen Williams --- config/config.go | 4 +- config/config_test.go | 50 +++++++++++++++++++ .../scrape_config_default_validation_mode.yml | 2 + .../scrape_config_global_validation_mode.yml | 4 ++ ...pe_config_local_global_validation_mode.yml | 5 ++ .../scrape_config_local_validation_mode.yml | 3 ++ 6 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 config/testdata/scrape_config_default_validation_mode.yml create mode 100644 config/testdata/scrape_config_global_validation_mode.yml create mode 100644 config/testdata/scrape_config_local_global_validation_mode.yml create mode 100644 config/testdata/scrape_config_local_validation_mode.yml diff --git a/config/config.go b/config/config.go index 4326b0a99..d1e463d86 100644 --- a/config/config.go +++ b/config/config.go @@ -781,7 +781,9 @@ func (c *ScrapeConfig) Validate(globalConfig GlobalConfig) error { default: return fmt.Errorf("unknown name validation method specified, must be either 'legacy' or 'utf8', got %s", globalConfig.MetricNameValidationScheme) } - c.MetricNameValidationScheme = globalConfig.MetricNameValidationScheme + if c.MetricNameValidationScheme == "" { + c.MetricNameValidationScheme = globalConfig.MetricNameValidationScheme + } return nil } diff --git a/config/config_test.go b/config/config_test.go index 9b074bef1..774aba1e2 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -16,6 +16,7 @@ package config import ( "crypto/tls" "encoding/json" + "fmt" "net/url" "os" "path/filepath" @@ -2300,3 +2301,52 @@ func TestScrapeConfigDisableCompression(t *testing.T) { require.False(t, got.ScrapeConfigs[0].EnableCompression) } + +func TestScrapeConfigNameValidationSettings(t *testing.T) { + model.NameValidationScheme = model.UTF8Validation + defer func() { + model.NameValidationScheme = model.LegacyValidation + }() + + tests := []struct { + name string + inputFile string + expectScheme string + }{ + { + name: "blank config implies default", + inputFile: "scrape_config_default_validation_mode", + expectScheme: "", + }, + { + name: "global setting implies local settings", + inputFile: "scrape_config_global_validation_mode", + expectScheme: "utf8", + }, + { + name: "local setting", + inputFile: "scrape_config_local_validation_mode", + expectScheme: "utf8", + }, + { + name: "local setting overrides global setting", + inputFile: "scrape_config_local_global_validation_mode", + expectScheme: "legacy", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + want, err := LoadFile(fmt.Sprintf("testdata/%s.yml", tc.inputFile), false, false, log.NewNopLogger()) + require.NoError(t, err) + + out, err := yaml.Marshal(want) + + require.NoError(t, err) + got := &Config{} + require.NoError(t, yaml.UnmarshalStrict(out, got)) + + require.Equal(t, tc.expectScheme, got.ScrapeConfigs[0].MetricNameValidationScheme) + }) + } +} diff --git a/config/testdata/scrape_config_default_validation_mode.yml b/config/testdata/scrape_config_default_validation_mode.yml new file mode 100644 index 000000000..96680d643 --- /dev/null +++ b/config/testdata/scrape_config_default_validation_mode.yml @@ -0,0 +1,2 @@ +scrape_configs: + - job_name: prometheus diff --git a/config/testdata/scrape_config_global_validation_mode.yml b/config/testdata/scrape_config_global_validation_mode.yml new file mode 100644 index 000000000..154855439 --- /dev/null +++ b/config/testdata/scrape_config_global_validation_mode.yml @@ -0,0 +1,4 @@ +global: + metric_name_validation_scheme: utf8 +scrape_configs: + - job_name: prometheus diff --git a/config/testdata/scrape_config_local_global_validation_mode.yml b/config/testdata/scrape_config_local_global_validation_mode.yml new file mode 100644 index 000000000..d13605e21 --- /dev/null +++ b/config/testdata/scrape_config_local_global_validation_mode.yml @@ -0,0 +1,5 @@ +global: + metric_name_validation_scheme: utf8 +scrape_configs: + - job_name: prometheus + metric_name_validation_scheme: legacy diff --git a/config/testdata/scrape_config_local_validation_mode.yml b/config/testdata/scrape_config_local_validation_mode.yml new file mode 100644 index 000000000..fad423580 --- /dev/null +++ b/config/testdata/scrape_config_local_validation_mode.yml @@ -0,0 +1,3 @@ +scrape_configs: + - job_name: prometheus + metric_name_validation_scheme: utf8 From ef649d59688f71fbd624e849c8e20c6f4b8c98c4 Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Mon, 26 Aug 2024 08:48:37 +0200 Subject: [PATCH 14/20] Revert " Store `mmMaxTime` in same field as `seriesShard`" Signed-off-by: Marco Pracucci --- tsdb/head.go | 46 +++++++++++----------------------------------- tsdb/head_read.go | 2 +- tsdb/head_test.go | 38 -------------------------------------- tsdb/head_wal.go | 13 +++++-------- 4 files changed, 17 insertions(+), 82 deletions(-) diff --git a/tsdb/head.go b/tsdb/head.go index 9d81b24ae..b7bfaa0fd 100644 --- a/tsdb/head.go +++ b/tsdb/head.go @@ -178,7 +178,6 @@ type HeadOptions struct { WALReplayConcurrency int // EnableSharding enables ShardedPostings() support in the Head. - // EnableSharding is temporarily disabled during Init(). EnableSharding bool } @@ -610,7 +609,7 @@ const cardinalityCacheExpirationTime = time.Duration(30) * time.Second // Init loads data from the write ahead log and prepares the head for writes. // It should be called before using an appender so that it // limits the ingested samples to the head min valid time. -func (h *Head) Init(minValidTime int64) (err error) { +func (h *Head) Init(minValidTime int64) error { h.minValidTime.Store(minValidTime) defer func() { h.postings.EnsureOrder(h.opts.WALReplayConcurrency) @@ -624,24 +623,6 @@ func (h *Head) Init(minValidTime int64) (err error) { } }() - // If sharding is enabled, disable it while initializing, and calculate the shards later. - // We're going to use that field for other purposes during WAL replay, - // so we don't want to waste time on calculating the shard that we're going to lose anyway. - if h.opts.EnableSharding { - h.opts.EnableSharding = false - defer func() { - h.opts.EnableSharding = true - if err == nil { - // No locking is needed here as nobody should be writing while we're in Init. - for _, stripe := range h.series.series { - for _, s := range stripe { - s.shardHashOrMemoryMappedMaxTime = labels.StableHash(s.lset) - } - } - } - }() - } - level.Info(h.logger).Log("msg", "Replaying on-disk memory mappable chunks if any") start := time.Now() @@ -702,6 +683,7 @@ func (h *Head) Init(minValidTime int64) (err error) { mmappedChunks map[chunks.HeadSeriesRef][]*mmappedChunk oooMmappedChunks map[chunks.HeadSeriesRef][]*mmappedChunk lastMmapRef chunks.ChunkDiskMapperRef + err error mmapChunkReplayDuration time.Duration ) @@ -2086,11 +2068,9 @@ type memSeries struct { ref chunks.HeadSeriesRef meta *metadata.Metadata - // Series labels hash to use for sharding purposes. - // The value is always 0 when sharding has not been explicitly enabled in TSDB. - // While the WAL replay the value stored here is the max time of any mmapped chunk, - // and the shard hash is re-calculated after WAL replay is complete. - shardHashOrMemoryMappedMaxTime uint64 + // Series labels hash to use for sharding purposes. The value is always 0 when sharding has not + // been explicitly enabled in TSDB. + shardHash uint64 // Everything after here should only be accessed with the lock held. sync.Mutex @@ -2115,6 +2095,8 @@ type memSeries struct { ooo *memSeriesOOOFields + mmMaxTime int64 // Max time of any mmapped chunk, only used during WAL replay. + nextAt int64 // Timestamp at which to cut the next chunk. histogramChunkHasComputedEndTime bool // True if nextAt has been predicted for the current histograms chunk; false otherwise. pendingCommit bool // Whether there are samples waiting to be committed to this series. @@ -2145,10 +2127,10 @@ type memSeriesOOOFields struct { func newMemSeries(lset labels.Labels, id chunks.HeadSeriesRef, shardHash uint64, isolationDisabled bool) *memSeries { s := &memSeries{ - lset: lset, - ref: id, - nextAt: math.MinInt64, - shardHashOrMemoryMappedMaxTime: shardHash, + lset: lset, + ref: id, + nextAt: math.MinInt64, + shardHash: shardHash, } if !isolationDisabled { s.txs = newTxRing(0) @@ -2236,12 +2218,6 @@ func (s *memSeries) truncateChunksBefore(mint int64, minOOOMmapRef chunks.ChunkD return removedInOrder + removedOOO } -// shardHash returns the shard hash of the series, only available after WAL replay. -func (s *memSeries) shardHash() uint64 { return s.shardHashOrMemoryMappedMaxTime } - -// mmMaxTime returns the max time of any mmapped chunk in the series, only available during WAL replay. -func (s *memSeries) mmMaxTime() int64 { return int64(s.shardHashOrMemoryMappedMaxTime) } - // cleanupAppendIDsBelow cleans up older appendIDs. Has to be called after // acquiring lock. func (s *memSeries) cleanupAppendIDsBelow(bound uint64) { diff --git a/tsdb/head_read.go b/tsdb/head_read.go index 5e3a76273..26cda3089 100644 --- a/tsdb/head_read.go +++ b/tsdb/head_read.go @@ -170,7 +170,7 @@ func (h *headIndexReader) ShardedPostings(p index.Postings, shardIndex, shardCou } // Check if the series belong to the shard. - if s.shardHash()%shardCount != shardIndex { + if s.shardHash%shardCount != shardIndex { continue } diff --git a/tsdb/head_test.go b/tsdb/head_test.go index 0ce60b849..a7a8847e8 100644 --- a/tsdb/head_test.go +++ b/tsdb/head_test.go @@ -23,7 +23,6 @@ import ( "path" "path/filepath" "reflect" - "runtime/pprof" "sort" "strconv" "strings" @@ -90,43 +89,6 @@ func newTestHeadWithOptions(t testing.TB, compressWAL wlog.CompressionType, opts return h, wal } -// BenchmarkLoadRealWLs will be skipped unless the BENCHMARK_LOAD_REAL_WLS_DIR environment variable is set. -// BENCHMARK_LOAD_REAL_WLS_DIR should be the folder where `wal` and `chunks_head` are located. -// Optionally, BENCHMARK_LOAD_REAL_WLS_PROFILE can be set to a file path to write a CPU profile. -func BenchmarkLoadRealWLs(b *testing.B) { - dir := os.Getenv("BENCHMARK_LOAD_REAL_WLS_DIR") - if dir == "" { - b.Skipped() - } - - profileFile := os.Getenv("BENCHMARK_LOAD_REAL_WLS_PROFILE") - if profileFile != "" { - b.Logf("Will profile in %s", profileFile) - f, err := os.Create(profileFile) - require.NoError(b, err) - b.Cleanup(func() { f.Close() }) - require.NoError(b, pprof.StartCPUProfile(f)) - b.Cleanup(pprof.StopCPUProfile) - } - - wal, err := wlog.New(nil, nil, filepath.Join(dir, "wal"), wlog.CompressionNone) - require.NoError(b, err) - b.Cleanup(func() { wal.Close() }) - - wbl, err := wlog.New(nil, nil, filepath.Join(dir, "wbl"), wlog.CompressionNone) - require.NoError(b, err) - b.Cleanup(func() { wbl.Close() }) - - // Load the WAL. - for i := 0; i < b.N; i++ { - opts := DefaultHeadOptions() - opts.ChunkDirRoot = dir - h, err := NewHead(nil, nil, wal, wbl, opts, nil) - require.NoError(b, err) - h.Init(0) - } -} - func BenchmarkCreateSeries(b *testing.B) { series := genSeries(b.N, 10, 0, 0) h, _ := newTestHead(b, 10000, wlog.CompressionNone, false) diff --git a/tsdb/head_wal.go b/tsdb/head_wal.go index 7397bbf41..ef96b5330 100644 --- a/tsdb/head_wal.go +++ b/tsdb/head_wal.go @@ -435,8 +435,6 @@ Outer: return nil } -func minInt64() int64 { return math.MinInt64 } - // resetSeriesWithMMappedChunks is only used during the WAL replay. func (h *Head) resetSeriesWithMMappedChunks(mSeries *memSeries, mmc, oooMmc []*mmappedChunk, walSeriesRef chunks.HeadSeriesRef) (overlapped bool) { if mSeries.ref != walSeriesRef { @@ -483,11 +481,10 @@ func (h *Head) resetSeriesWithMMappedChunks(mSeries *memSeries, mmc, oooMmc []*m } // Cache the last mmapped chunk time, so we can skip calling append() for samples it will reject. if len(mmc) == 0 { - mSeries.shardHashOrMemoryMappedMaxTime = uint64(minInt64()) + mSeries.mmMaxTime = math.MinInt64 } else { - mmMaxTime := mmc[len(mmc)-1].maxTime - mSeries.shardHashOrMemoryMappedMaxTime = uint64(mmMaxTime) - h.updateMinMaxTime(mmc[0].minTime, mmMaxTime) + mSeries.mmMaxTime = mmc[len(mmc)-1].maxTime + h.updateMinMaxTime(mmc[0].minTime, mSeries.mmMaxTime) } if len(oooMmc) != 0 { // Mint and maxt can be in any chunk, they are not sorted. @@ -588,7 +585,7 @@ func (wp *walSubsetProcessor) processWALSamples(h *Head, mmappedChunks, oooMmapp unknownRefs++ continue } - if s.T <= ms.mmMaxTime() { + if s.T <= ms.mmMaxTime { continue } if _, chunkCreated := ms.append(s.T, s.V, 0, appendChunkOpts); chunkCreated { @@ -617,7 +614,7 @@ func (wp *walSubsetProcessor) processWALSamples(h *Head, mmappedChunks, oooMmapp unknownHistogramRefs++ continue } - if s.t <= ms.mmMaxTime() { + if s.t <= ms.mmMaxTime { continue } var chunkCreated bool From da28f88910660f852aa37017dcc240afc83359c0 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 27 Aug 2024 10:14:16 +0100 Subject: [PATCH 15/20] Cut release 2.54.1 Signed-off-by: Bryan Boreham --- CHANGELOG.md | 8 ++++++++ VERSION | 2 +- web/ui/module/codemirror-promql/package.json | 4 ++-- web/ui/module/lezer-promql/package.json | 2 +- web/ui/package-lock.json | 14 +++++++------- web/ui/package.json | 2 +- web/ui/react-app/package.json | 4 ++-- 7 files changed, 22 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 885749504..c07e5b578 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 2.54.1 / 2024-08-27 + +* [BUGFIX] Scraping: allow multiple samples on same series, with explicit timestamps. #14685 +* [BUGFIX] Docker SD: fix crash in `match_first_network` mode when container is reconnected to a new network. #14654 +* [BUGFIX] PromQL: fix experimental native histograms getting corrupted due to vector selector bug in range queries. #14538 +* [BUGFIX] PromQL: fix experimental native histogram counter reset detection on stale samples. #14514 +* [BUGFIX] PromQL: fix native histograms getting corrupted due to vector selector bug in range queries. #14605 + ## 2.54.0 / 2024-08-09 Release 2.54 brings a release candidate of a major new version of [Remote Write: 2.0](https://prometheus.io/docs/specs/remote_write_spec_2_0/). diff --git a/VERSION b/VERSION index 99aed793a..3a40665f5 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.54.0 +2.54.1 diff --git a/web/ui/module/codemirror-promql/package.json b/web/ui/module/codemirror-promql/package.json index 6307f12f3..1c459b090 100644 --- a/web/ui/module/codemirror-promql/package.json +++ b/web/ui/module/codemirror-promql/package.json @@ -1,6 +1,6 @@ { "name": "@prometheus-io/codemirror-promql", - "version": "0.54.0", + "version": "0.54.1", "description": "a CodeMirror mode for the PromQL language", "types": "dist/esm/index.d.ts", "module": "dist/esm/index.js", @@ -29,7 +29,7 @@ }, "homepage": "https://github.com/prometheus/prometheus/blob/main/web/ui/module/codemirror-promql/README.md", "dependencies": { - "@prometheus-io/lezer-promql": "0.54.0", + "@prometheus-io/lezer-promql": "0.54.1", "lru-cache": "^7.18.3" }, "devDependencies": { diff --git a/web/ui/module/lezer-promql/package.json b/web/ui/module/lezer-promql/package.json index 14d263241..8f9c0b1c8 100644 --- a/web/ui/module/lezer-promql/package.json +++ b/web/ui/module/lezer-promql/package.json @@ -1,6 +1,6 @@ { "name": "@prometheus-io/lezer-promql", - "version": "0.54.0", + "version": "0.54.1", "description": "lezer-based PromQL grammar", "main": "dist/index.cjs", "type": "module", diff --git a/web/ui/package-lock.json b/web/ui/package-lock.json index 0f5db9f6d..e305ee964 100644 --- a/web/ui/package-lock.json +++ b/web/ui/package-lock.json @@ -1,12 +1,12 @@ { "name": "prometheus-io", - "version": "0.54.0", + "version": "0.54.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "prometheus-io", - "version": "0.54.0", + "version": "0.54.1", "workspaces": [ "react-app", "module/*" @@ -30,10 +30,10 @@ }, "module/codemirror-promql": { "name": "@prometheus-io/codemirror-promql", - "version": "0.54.0", + "version": "0.54.1", "license": "Apache-2.0", "dependencies": { - "@prometheus-io/lezer-promql": "0.54.0", + "@prometheus-io/lezer-promql": "0.54.1", "lru-cache": "^7.18.3" }, "devDependencies": { @@ -69,7 +69,7 @@ }, "module/lezer-promql": { "name": "@prometheus-io/lezer-promql", - "version": "0.54.0", + "version": "0.54.1", "license": "Apache-2.0", "devDependencies": { "@lezer/generator": "^1.7.0", @@ -19332,7 +19332,7 @@ }, "react-app": { "name": "@prometheus-io/app", - "version": "0.54.0", + "version": "0.54.1", "dependencies": { "@codemirror/autocomplete": "^6.17.0", "@codemirror/commands": "^6.6.0", @@ -19350,7 +19350,7 @@ "@lezer/lr": "^1.4.1", "@nexucis/fuzzy": "^0.4.1", "@nexucis/kvsearch": "^0.8.1", - "@prometheus-io/codemirror-promql": "0.54.0", + "@prometheus-io/codemirror-promql": "0.54.1", "bootstrap": "^4.6.2", "css.escape": "^1.5.1", "downshift": "^9.0.6", diff --git a/web/ui/package.json b/web/ui/package.json index 01dd71f34..f97d7098b 100644 --- a/web/ui/package.json +++ b/web/ui/package.json @@ -28,5 +28,5 @@ "ts-jest": "^29.2.2", "typescript": "^4.9.5" }, - "version": "0.54.0" + "version": "0.54.1" } diff --git a/web/ui/react-app/package.json b/web/ui/react-app/package.json index 01620531a..ace3ef264 100644 --- a/web/ui/react-app/package.json +++ b/web/ui/react-app/package.json @@ -1,6 +1,6 @@ { "name": "@prometheus-io/app", - "version": "0.54.0", + "version": "0.54.1", "private": true, "dependencies": { "@codemirror/autocomplete": "^6.17.0", @@ -19,7 +19,7 @@ "@lezer/lr": "^1.4.1", "@nexucis/fuzzy": "^0.4.1", "@nexucis/kvsearch": "^0.8.1", - "@prometheus-io/codemirror-promql": "0.54.0", + "@prometheus-io/codemirror-promql": "0.54.1", "bootstrap": "^4.6.2", "css.escape": "^1.5.1", "downshift": "^9.0.6", From 7757794bb3f5c1335b7634f0e82bb588149486b2 Mon Sep 17 00:00:00 2001 From: Suraj Patil <31805557+patilsuraj767@users.noreply.github.com> Date: Wed, 28 Aug 2024 07:42:24 +0530 Subject: [PATCH 16/20] [ENHANCEMENT] Promtool: Adding labels to time series while creating tsdb blocks (#14403) * feat: #14402 - Adding labels to time series while creating tsdb blocks Signed-off-by: Suraj Patil --- cmd/promtool/backfill.go | 16 +++++++++--- cmd/promtool/backfill_test.go | 46 ++++++++++++++++++++++++++++++++++- cmd/promtool/main.go | 3 ++- cmd/promtool/tsdb.go | 4 +-- cmd/promtool/tsdb_test.go | 2 +- docs/command-line/promtool.md | 9 +++++++ 6 files changed, 71 insertions(+), 9 deletions(-) diff --git a/cmd/promtool/backfill.go b/cmd/promtool/backfill.go index 400cae421..16491f041 100644 --- a/cmd/promtool/backfill.go +++ b/cmd/promtool/backfill.go @@ -85,7 +85,7 @@ func getCompatibleBlockDuration(maxBlockDuration int64) int64 { return blockDuration } -func createBlocks(input []byte, mint, maxt, maxBlockDuration int64, maxSamplesInAppender int, outputDir string, humanReadable, quiet bool) (returnErr error) { +func createBlocks(input []byte, mint, maxt, maxBlockDuration int64, maxSamplesInAppender int, outputDir string, humanReadable, quiet bool, customLabels map[string]string) (returnErr error) { blockDuration := getCompatibleBlockDuration(maxBlockDuration) mint = blockDuration * (mint / blockDuration) @@ -102,6 +102,8 @@ func createBlocks(input []byte, mint, maxt, maxBlockDuration int64, maxSamplesIn nextSampleTs int64 = math.MaxInt64 ) + lb := labels.NewBuilder(labels.EmptyLabels()) + for t := mint; t <= maxt; t += blockDuration { tsUpper := t + blockDuration if nextSampleTs != math.MaxInt64 && nextSampleTs >= tsUpper { @@ -162,7 +164,13 @@ func createBlocks(input []byte, mint, maxt, maxBlockDuration int64, maxSamplesIn l := labels.Labels{} p.Metric(&l) - if _, err := app.Append(0, l, *ts, v); err != nil { + lb.Reset(l) + for name, value := range customLabels { + lb.Set(name, value) + } + lbls := lb.Labels() + + if _, err := app.Append(0, lbls, *ts, v); err != nil { return fmt.Errorf("add sample: %w", err) } @@ -221,13 +229,13 @@ func createBlocks(input []byte, mint, maxt, maxBlockDuration int64, maxSamplesIn return nil } -func backfill(maxSamplesInAppender int, input []byte, outputDir string, humanReadable, quiet bool, maxBlockDuration time.Duration) (err error) { +func backfill(maxSamplesInAppender int, input []byte, outputDir string, humanReadable, quiet bool, maxBlockDuration time.Duration, customLabels map[string]string) (err error) { p := textparse.NewOpenMetricsParser(input, nil) // Don't need a SymbolTable to get max and min timestamps. maxt, mint, err := getMinAndMaxTimestamps(p) if err != nil { return fmt.Errorf("getting min and max timestamp: %w", err) } - if err = createBlocks(input, mint, maxt, int64(maxBlockDuration/time.Millisecond), maxSamplesInAppender, outputDir, humanReadable, quiet); err != nil { + if err = createBlocks(input, mint, maxt, int64(maxBlockDuration/time.Millisecond), maxSamplesInAppender, outputDir, humanReadable, quiet, customLabels); err != nil { return fmt.Errorf("block creation: %w", err) } return nil diff --git a/cmd/promtool/backfill_test.go b/cmd/promtool/backfill_test.go index 32abfa46a..b818194e8 100644 --- a/cmd/promtool/backfill_test.go +++ b/cmd/promtool/backfill_test.go @@ -92,6 +92,7 @@ func TestBackfill(t *testing.T) { Description string MaxSamplesInAppender int MaxBlockDuration time.Duration + Labels map[string]string Expected struct { MinTime int64 MaxTime int64 @@ -636,6 +637,49 @@ http_requests_total{code="400"} 1024 7199 }, }, }, + { + ToParse: `# HELP http_requests_total The total number of HTTP requests. +# TYPE http_requests_total counter +http_requests_total{code="200"} 1 1624463088.000 +http_requests_total{code="200"} 2 1629503088.000 +http_requests_total{code="200"} 3 1629863088.000 +# EOF +`, + IsOk: true, + Description: "Sample with external labels.", + MaxSamplesInAppender: 5000, + MaxBlockDuration: 2048 * time.Hour, + Labels: map[string]string{"cluster_id": "123", "org_id": "999"}, + Expected: struct { + MinTime int64 + MaxTime int64 + NumBlocks int + BlockDuration int64 + Samples []backfillSample + }{ + MinTime: 1624463088000, + MaxTime: 1629863088000, + NumBlocks: 2, + BlockDuration: int64(1458 * time.Hour / time.Millisecond), + Samples: []backfillSample{ + { + Timestamp: 1624463088000, + Value: 1, + Labels: labels.FromStrings("__name__", "http_requests_total", "code", "200", "cluster_id", "123", "org_id", "999"), + }, + { + Timestamp: 1629503088000, + Value: 2, + Labels: labels.FromStrings("__name__", "http_requests_total", "code", "200", "cluster_id", "123", "org_id", "999"), + }, + { + Timestamp: 1629863088000, + Value: 3, + Labels: labels.FromStrings("__name__", "http_requests_total", "code", "200", "cluster_id", "123", "org_id", "999"), + }, + }, + }, + }, { ToParse: `# HELP rpc_duration_seconds A summary of the RPC duration in seconds. # TYPE rpc_duration_seconds summary @@ -689,7 +733,7 @@ after_eof 1 2 outputDir := t.TempDir() - err := backfill(test.MaxSamplesInAppender, []byte(test.ToParse), outputDir, false, false, test.MaxBlockDuration) + err := backfill(test.MaxSamplesInAppender, []byte(test.ToParse), outputDir, false, false, test.MaxBlockDuration, test.Labels) if !test.IsOk { require.Error(t, err, test.Description) diff --git a/cmd/promtool/main.go b/cmd/promtool/main.go index 4d033c3c0..e713a177f 100644 --- a/cmd/promtool/main.go +++ b/cmd/promtool/main.go @@ -253,6 +253,7 @@ func main() { importQuiet := importCmd.Flag("quiet", "Do not print created blocks.").Short('q').Bool() maxBlockDuration := importCmd.Flag("max-block-duration", "Maximum duration created blocks may span. Anything less than 2h is ignored.").Hidden().PlaceHolder("").Duration() openMetricsImportCmd := importCmd.Command("openmetrics", "Import samples from OpenMetrics input and produce TSDB blocks. Please refer to the storage docs for more details.") + openMetricsLabels := openMetricsImportCmd.Flag("label", "Label to attach to metrics. Can be specified multiple times. Example --label=label_name=label_value").StringMap() importFilePath := openMetricsImportCmd.Arg("input file", "OpenMetrics file to read samples from.").Required().String() importDBPath := openMetricsImportCmd.Arg("output directory", "Output directory for generated blocks.").Default(defaultDBPath).String() importRulesCmd := importCmd.Command("rules", "Create blocks of data for new recording rules.") @@ -408,7 +409,7 @@ func main() { os.Exit(checkErr(dumpSamples(ctx, *dumpOpenMetricsPath, *dumpOpenMetricsSandboxDirRoot, *dumpOpenMetricsMinTime, *dumpOpenMetricsMaxTime, *dumpOpenMetricsMatch, formatSeriesSetOpenMetrics))) // TODO(aSquare14): Work on adding support for custom block size. case openMetricsImportCmd.FullCommand(): - os.Exit(backfillOpenMetrics(*importFilePath, *importDBPath, *importHumanReadable, *importQuiet, *maxBlockDuration)) + os.Exit(backfillOpenMetrics(*importFilePath, *importDBPath, *importHumanReadable, *importQuiet, *maxBlockDuration, *openMetricsLabels)) case importRulesCmd.FullCommand(): os.Exit(checkErr(importRules(serverURL, httpRoundTripper, *importRulesStart, *importRulesEnd, *importRulesOutputDir, *importRulesEvalInterval, *maxBlockDuration, *importRulesFiles...))) diff --git a/cmd/promtool/tsdb.go b/cmd/promtool/tsdb.go index b85a4fae8..971ea8ab0 100644 --- a/cmd/promtool/tsdb.go +++ b/cmd/promtool/tsdb.go @@ -823,7 +823,7 @@ func checkErr(err error) int { return 0 } -func backfillOpenMetrics(path, outputDir string, humanReadable, quiet bool, maxBlockDuration time.Duration) int { +func backfillOpenMetrics(path, outputDir string, humanReadable, quiet bool, maxBlockDuration time.Duration, customLabels map[string]string) int { inputFile, err := fileutil.OpenMmapFile(path) if err != nil { return checkErr(err) @@ -834,7 +834,7 @@ func backfillOpenMetrics(path, outputDir string, humanReadable, quiet bool, maxB return checkErr(fmt.Errorf("create output dir: %w", err)) } - return checkErr(backfill(5000, inputFile.Bytes(), outputDir, humanReadable, quiet, maxBlockDuration)) + return checkErr(backfill(5000, inputFile.Bytes(), outputDir, humanReadable, quiet, maxBlockDuration, customLabels)) } func displayHistogram(dataType string, datas []int, total int) { diff --git a/cmd/promtool/tsdb_test.go b/cmd/promtool/tsdb_test.go index d7cc56088..ffc5467b4 100644 --- a/cmd/promtool/tsdb_test.go +++ b/cmd/promtool/tsdb_test.go @@ -186,7 +186,7 @@ func TestTSDBDumpOpenMetricsRoundTrip(t *testing.T) { dbDir := t.TempDir() // Import samples from OM format - err = backfill(5000, initialMetrics, dbDir, false, false, 2*time.Hour) + err = backfill(5000, initialMetrics, dbDir, false, false, 2*time.Hour, map[string]string{}) require.NoError(t, err) db, err := tsdb.Open(dbDir, nil, nil, tsdb.DefaultOptions(), nil) require.NoError(t, err) diff --git a/docs/command-line/promtool.md b/docs/command-line/promtool.md index 0c397387f..6d74200e6 100644 --- a/docs/command-line/promtool.md +++ b/docs/command-line/promtool.md @@ -641,6 +641,15 @@ Import samples from OpenMetrics input and produce TSDB blocks. Please refer to t +###### Flags + +| Flag | Description | +| --- | --- | +| --label | Label to attach to metrics. Can be specified multiple times. Example --label=label_name=label_value | + + + + ###### Arguments | Argument | Description | Default | Required | From 406bf775aa1f8e319d557165ddf5ab39de0c8a19 Mon Sep 17 00:00:00 2001 From: riskrole Date: Wed, 28 Aug 2024 11:26:57 +0800 Subject: [PATCH 17/20] chore: fix some comments Signed-off-by: riskrole --- model/textparse/protobufparse.go | 4 ++-- rules/alerting_test.go | 2 +- tsdb/compact_test.go | 2 +- tsdb/head_read.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/model/textparse/protobufparse.go b/model/textparse/protobufparse.go index ea3a2e1a3..f56def012 100644 --- a/model/textparse/protobufparse.go +++ b/model/textparse/protobufparse.go @@ -47,7 +47,7 @@ import ( // the re-arrangement work is actually causing problems (which has to be seen), // that expectation needs to be changed. type ProtobufParser struct { - in []byte // The intput to parse. + in []byte // The input to parse. inPos int // Position within the input. metricPos int // Position within Metric slice. // fieldPos is the position within a Summary or (legacy) Histogram. -2 @@ -71,7 +71,7 @@ type ProtobufParser struct { mf *dto.MetricFamily - // Wether to also parse a classic histogram that is also present as a + // Whether to also parse a classic histogram that is also present as a // native histogram. parseClassicHistograms bool diff --git a/rules/alerting_test.go b/rules/alerting_test.go index 5ebd049f6..406332946 100644 --- a/rules/alerting_test.go +++ b/rules/alerting_test.go @@ -648,7 +648,7 @@ func TestAlertingRuleLimit(t *testing.T) { case err != nil: require.EqualError(t, err, test.err) case test.err != "": - t.Errorf("Expected errror %s, got none", test.err) + t.Errorf("Expected error %s, got none", test.err) } } } diff --git a/tsdb/compact_test.go b/tsdb/compact_test.go index 0ea155d10..e7998abf7 100644 --- a/tsdb/compact_test.go +++ b/tsdb/compact_test.go @@ -2018,7 +2018,7 @@ func TestDelayedCompaction(t *testing.T) { // This implies that the compaction delay doesn't block or wait on the initial trigger. // 3 is an arbitrary value because it's difficult to determine the precise value. require.GreaterOrEqual(t, prom_testutil.ToFloat64(db.metrics.compactionsTriggered)-prom_testutil.ToFloat64(db.metrics.compactionsSkipped), 3.0) - // The delay doesn't change the head blocks alignement. + // The delay doesn't change the head blocks alignment. require.Eventually(t, func() bool { return db.head.MinTime() == db.compactor.(*LeveledCompactor).ranges[0]+1 }, 500*time.Millisecond, 10*time.Millisecond) diff --git a/tsdb/head_read.go b/tsdb/head_read.go index 5e3a76273..24d33ed5c 100644 --- a/tsdb/head_read.go +++ b/tsdb/head_read.go @@ -430,7 +430,7 @@ func (s *memSeries) chunk(id chunks.HeadChunkID, chunkDiskMapper *chunks.ChunkDi // incremented by 1 when new chunk is created, hence (id - firstChunkID) gives the slice index. // The max index for the s.mmappedChunks slice can be len(s.mmappedChunks)-1, hence if the ix // is >= len(s.mmappedChunks), it represents one of the chunks on s.headChunks linked list. - // The order of elemens is different for slice and linked list. + // The order of elements is different for slice and linked list. // For s.mmappedChunks slice newer chunks are appended to it. // For s.headChunks list newer chunks are prepended to it. // From 3a82cd5a7ed4c5e9b530fafcb779c2e9c81e2aab Mon Sep 17 00:00:00 2001 From: Justin Lei Date: Tue, 27 Aug 2024 23:23:54 -0700 Subject: [PATCH 18/20] Add streaming remote read to ReadClient (#11379) * Add streaming remote read to ReadClient Signed-off-by: Justin Lei * Apply suggestions from code review Co-authored-by: Bartlomiej Plotka Signed-off-by: Justin Lei * Remote read instrumentation tweaks Signed-off-by: Justin Lei * Minor cleanups Signed-off-by: Justin Lei * In-line handleChunkedResponse Signed-off-by: Justin Lei * Fix lints Signed-off-by: Justin Lei * Explicitly call cancel() when needed Signed-off-by: Justin Lei * Update chunkedSeries, chunkedSeriesIterator for new interfaces Signed-off-by: Justin Lei * Adapt remote.chunkedSeries to use prompb.ChunkedSeries Signed-off-by: Justin Lei * Fix lint Signed-off-by: Justin Lei --------- Signed-off-by: Justin Lei Signed-off-by: Justin Lei Co-authored-by: Bartlomiej Plotka --- config/config.go | 18 +- config/config_test.go | 10 +- storage/remote/chunked.go | 4 - storage/remote/client.go | 110 ++++++++---- storage/remote/client_test.go | 229 +++++++++++++++++++++++ storage/remote/codec.go | 223 ++++++++++++++++++++++- storage/remote/codec_test.go | 269 ++++++++++++++++++++++++++++ storage/remote/read.go | 4 +- storage/remote/read_handler_test.go | 4 +- storage/remote/read_test.go | 5 +- storage/remote/storage.go | 1 + web/api/v1/api_test.go | 3 + 12 files changed, 819 insertions(+), 61 deletions(-) diff --git a/config/config.go b/config/config.go index d1e463d86..c9e8efbf3 100644 --- a/config/config.go +++ b/config/config.go @@ -221,6 +221,7 @@ var ( // DefaultRemoteReadConfig is the default remote read configuration. DefaultRemoteReadConfig = RemoteReadConfig{ RemoteTimeout: model.Duration(1 * time.Minute), + ChunkedReadLimit: DefaultChunkedReadLimit, HTTPClientConfig: config.DefaultHTTPClientConfig, FilterExternalLabels: true, } @@ -1279,13 +1280,20 @@ type MetadataConfig struct { MaxSamplesPerSend int `yaml:"max_samples_per_send,omitempty"` } +const ( + // DefaultChunkedReadLimit is the default value for the maximum size of the protobuf frame client allows. + // 50MB is the default. This is equivalent to ~100k full XOR chunks and average labelset. + DefaultChunkedReadLimit = 5e+7 +) + // RemoteReadConfig is the configuration for reading from remote storage. type RemoteReadConfig struct { - URL *config.URL `yaml:"url"` - RemoteTimeout model.Duration `yaml:"remote_timeout,omitempty"` - Headers map[string]string `yaml:"headers,omitempty"` - ReadRecent bool `yaml:"read_recent,omitempty"` - Name string `yaml:"name,omitempty"` + URL *config.URL `yaml:"url"` + RemoteTimeout model.Duration `yaml:"remote_timeout,omitempty"` + ChunkedReadLimit uint64 `yaml:"chunked_read_limit,omitempty"` + Headers map[string]string `yaml:"headers,omitempty"` + ReadRecent bool `yaml:"read_recent,omitempty"` + Name string `yaml:"name,omitempty"` // We cannot do proper Go type embedding below as the parser will then parse // values arbitrarily into the overflow maps of further-down types. diff --git a/config/config_test.go b/config/config_test.go index 774aba1e2..221906182 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -165,10 +165,11 @@ var expectedConf = &Config{ RemoteReadConfigs: []*RemoteReadConfig{ { - URL: mustParseURL("http://remote1/read"), - RemoteTimeout: model.Duration(1 * time.Minute), - ReadRecent: true, - Name: "default", + URL: mustParseURL("http://remote1/read"), + RemoteTimeout: model.Duration(1 * time.Minute), + ChunkedReadLimit: DefaultChunkedReadLimit, + ReadRecent: true, + Name: "default", HTTPClientConfig: config.HTTPClientConfig{ FollowRedirects: true, EnableHTTP2: false, @@ -178,6 +179,7 @@ var expectedConf = &Config{ { URL: mustParseURL("http://remote3/read"), RemoteTimeout: model.Duration(1 * time.Minute), + ChunkedReadLimit: DefaultChunkedReadLimit, ReadRecent: false, Name: "read_special", RequiredMatchers: model.LabelSet{"job": "special"}, diff --git a/storage/remote/chunked.go b/storage/remote/chunked.go index 96ce483e0..aa5addd6a 100644 --- a/storage/remote/chunked.go +++ b/storage/remote/chunked.go @@ -26,10 +26,6 @@ import ( "github.com/gogo/protobuf/proto" ) -// DefaultChunkedReadLimit is the default value for the maximum size of the protobuf frame client allows. -// 50MB is the default. This is equivalent to ~100k full XOR chunks and average labelset. -const DefaultChunkedReadLimit = 5e+7 - // The table gets initialized with sync.Once but may still cause a race // with any other use of the crc32 package anywhere. Thus we initialize it // before. diff --git a/storage/remote/client.go b/storage/remote/client.go index 2a66739ed..62218cfba 100644 --- a/storage/remote/client.go +++ b/storage/remote/client.go @@ -16,6 +16,7 @@ package remote import ( "bytes" "context" + "errors" "fmt" "io" "net/http" @@ -36,13 +37,14 @@ import ( "github.com/prometheus/prometheus/config" "github.com/prometheus/prometheus/prompb" + "github.com/prometheus/prometheus/storage" "github.com/prometheus/prometheus/storage/remote/azuread" "github.com/prometheus/prometheus/storage/remote/googleiam" ) -const maxErrMsgLen = 1024 - const ( + maxErrMsgLen = 1024 + RemoteWriteVersionHeader = "X-Prometheus-Remote-Write-Version" RemoteWriteVersion1HeaderValue = "0.1.0" RemoteWriteVersion20HeaderValue = "2.0.0" @@ -68,9 +70,12 @@ var ( config.RemoteWriteProtoMsgV1: appProtoContentType, // Also application/x-protobuf;proto=prometheus.WriteRequest but simplified for compatibility with 1.x spec. config.RemoteWriteProtoMsgV2: appProtoContentType + ";proto=io.prometheus.write.v2.Request", } -) -var ( + AcceptedResponseTypes = []prompb.ReadRequest_ResponseType{ + prompb.ReadRequest_STREAMED_XOR_CHUNKS, + prompb.ReadRequest_SAMPLES, + } + remoteReadQueriesTotal = prometheus.NewCounterVec( prometheus.CounterOpts{ Namespace: namespace, @@ -78,7 +83,7 @@ var ( Name: "read_queries_total", Help: "The total number of remote read queries.", }, - []string{remoteName, endpoint, "code"}, + []string{remoteName, endpoint, "response_type", "code"}, ) remoteReadQueries = prometheus.NewGaugeVec( prometheus.GaugeOpts{ @@ -94,13 +99,13 @@ var ( Namespace: namespace, Subsystem: subsystem, Name: "read_request_duration_seconds", - Help: "Histogram of the latency for remote read requests.", + Help: "Histogram of the latency for remote read requests. Note that for streamed responses this is only the duration of the initial call and does not include the processing of the stream.", Buckets: append(prometheus.DefBuckets, 25, 60), NativeHistogramBucketFactor: 1.1, NativeHistogramMaxBucketNumber: 100, NativeHistogramMinResetDuration: 1 * time.Hour, }, - []string{remoteName, endpoint}, + []string{remoteName, endpoint, "response_type"}, ) ) @@ -116,10 +121,11 @@ type Client struct { timeout time.Duration retryOnRateLimit bool + chunkedReadLimit uint64 readQueries prometheus.Gauge readQueriesTotal *prometheus.CounterVec - readQueriesDuration prometheus.Observer + readQueriesDuration prometheus.ObserverVec writeProtoMsg config.RemoteWriteProtoMsg writeCompression Compression // Not exposed by ClientConfig for now. @@ -136,12 +142,13 @@ type ClientConfig struct { Headers map[string]string RetryOnRateLimit bool WriteProtoMsg config.RemoteWriteProtoMsg + ChunkedReadLimit uint64 } -// ReadClient uses the SAMPLES method of remote read to read series samples from remote server. -// TODO(bwplotka): Add streamed chunked remote read method as well (https://github.com/prometheus/prometheus/issues/5926). +// ReadClient will request the STREAMED_XOR_CHUNKS method of remote read but can +// also fall back to the SAMPLES method if necessary. type ReadClient interface { - Read(ctx context.Context, query *prompb.Query) (*prompb.QueryResult, error) + Read(ctx context.Context, query *prompb.Query, sortSeries bool) (storage.SeriesSet, error) } // NewReadClient creates a new client for remote read. @@ -162,9 +169,10 @@ func NewReadClient(name string, conf *ClientConfig) (ReadClient, error) { urlString: conf.URL.String(), Client: httpClient, timeout: time.Duration(conf.Timeout), + chunkedReadLimit: conf.ChunkedReadLimit, readQueries: remoteReadQueries.WithLabelValues(name, conf.URL.String()), readQueriesTotal: remoteReadQueriesTotal.MustCurryWith(prometheus.Labels{remoteName: name, endpoint: conf.URL.String()}), - readQueriesDuration: remoteReadQueryDuration.WithLabelValues(name, conf.URL.String()), + readQueriesDuration: remoteReadQueryDuration.MustCurryWith(prometheus.Labels{remoteName: name, endpoint: conf.URL.String()}), }, nil } @@ -278,8 +286,8 @@ func (c *Client) Store(ctx context.Context, req []byte, attempt int) (WriteRespo return WriteResponseStats{}, RecoverableError{err, defaultBackoff} } defer func() { - io.Copy(io.Discard, httpResp.Body) - httpResp.Body.Close() + _, _ = io.Copy(io.Discard, httpResp.Body) + _ = httpResp.Body.Close() }() // TODO(bwplotka): Pass logger and emit debug on error? @@ -329,17 +337,17 @@ func (c *Client) Endpoint() string { return c.urlString } -// Read reads from a remote endpoint. -func (c *Client) Read(ctx context.Context, query *prompb.Query) (*prompb.QueryResult, error) { +// Read reads from a remote endpoint. The sortSeries parameter is only respected in the case of a sampled response; +// chunked responses arrive already sorted by the server. +func (c *Client) Read(ctx context.Context, query *prompb.Query, sortSeries bool) (storage.SeriesSet, error) { c.readQueries.Inc() defer c.readQueries.Dec() req := &prompb.ReadRequest{ // TODO: Support batching multiple queries into one read request, // as the protobuf interface allows for it. - Queries: []*prompb.Query{ - query, - }, + Queries: []*prompb.Query{query}, + AcceptedResponseTypes: AcceptedResponseTypes, } data, err := proto.Marshal(req) if err != nil { @@ -358,7 +366,6 @@ func (c *Client) Read(ctx context.Context, query *prompb.Query) (*prompb.QueryRe httpReq.Header.Set("X-Prometheus-Remote-Read-Version", "0.1.0") ctx, cancel := context.WithTimeout(ctx, c.timeout) - defer cancel() ctx, span := otel.Tracer("").Start(ctx, "Remote Read", trace.WithSpanKind(trace.SpanKindClient)) defer span.End() @@ -366,24 +373,58 @@ func (c *Client) Read(ctx context.Context, query *prompb.Query) (*prompb.QueryRe start := time.Now() httpResp, err := c.Client.Do(httpReq.WithContext(ctx)) if err != nil { + cancel() return nil, fmt.Errorf("error sending request: %w", err) } - defer func() { - io.Copy(io.Discard, httpResp.Body) - httpResp.Body.Close() - }() - c.readQueriesDuration.Observe(time.Since(start).Seconds()) - c.readQueriesTotal.WithLabelValues(strconv.Itoa(httpResp.StatusCode)).Inc() - - compressed, err = io.ReadAll(httpResp.Body) - if err != nil { - return nil, fmt.Errorf("error reading response. HTTP status code: %s: %w", httpResp.Status, err) - } if httpResp.StatusCode/100 != 2 { - return nil, fmt.Errorf("remote server %s returned HTTP status %s: %s", c.urlString, httpResp.Status, strings.TrimSpace(string(compressed))) + // Make an attempt at getting an error message. + body, _ := io.ReadAll(httpResp.Body) + _ = httpResp.Body.Close() + + cancel() + return nil, fmt.Errorf("remote server %s returned http status %s: %s", c.urlString, httpResp.Status, string(body)) } + contentType := httpResp.Header.Get("Content-Type") + + switch { + case strings.HasPrefix(contentType, "application/x-protobuf"): + c.readQueriesDuration.WithLabelValues("sampled").Observe(time.Since(start).Seconds()) + c.readQueriesTotal.WithLabelValues("sampled", strconv.Itoa(httpResp.StatusCode)).Inc() + ss, err := c.handleSampledResponse(req, httpResp, sortSeries) + cancel() + return ss, err + case strings.HasPrefix(contentType, "application/x-streamed-protobuf; proto=prometheus.ChunkedReadResponse"): + c.readQueriesDuration.WithLabelValues("chunked").Observe(time.Since(start).Seconds()) + + s := NewChunkedReader(httpResp.Body, c.chunkedReadLimit, nil) + return NewChunkedSeriesSet(s, httpResp.Body, query.StartTimestampMs, query.EndTimestampMs, func(err error) { + code := strconv.Itoa(httpResp.StatusCode) + if !errors.Is(err, io.EOF) { + code = "aborted_stream" + } + c.readQueriesTotal.WithLabelValues("chunked", code).Inc() + cancel() + }), nil + default: + c.readQueriesDuration.WithLabelValues("unsupported").Observe(time.Since(start).Seconds()) + c.readQueriesTotal.WithLabelValues("unsupported", strconv.Itoa(httpResp.StatusCode)).Inc() + cancel() + return nil, fmt.Errorf("unsupported content type: %s", contentType) + } +} + +func (c *Client) handleSampledResponse(req *prompb.ReadRequest, httpResp *http.Response, sortSeries bool) (storage.SeriesSet, error) { + compressed, err := io.ReadAll(httpResp.Body) + if err != nil { + return nil, fmt.Errorf("error reading response. HTTP status code: %s: %w", httpResp.Status, err) + } + defer func() { + _, _ = io.Copy(io.Discard, httpResp.Body) + _ = httpResp.Body.Close() + }() + uncompressed, err := snappy.Decode(nil, compressed) if err != nil { return nil, fmt.Errorf("error reading response: %w", err) @@ -399,5 +440,8 @@ func (c *Client) Read(ctx context.Context, query *prompb.Query) (*prompb.QueryRe return nil, fmt.Errorf("responses: want %d, got %d", len(req.Queries), len(resp.Results)) } - return resp.Results[0], nil + // This client does not batch queries so there's always only 1 result. + res := resp.Results[0] + + return FromQueryResult(sortSeries, res), nil } diff --git a/storage/remote/client_test.go b/storage/remote/client_test.go index 9184ce100..c8b3d487e 100644 --- a/storage/remote/client_test.go +++ b/storage/remote/client_test.go @@ -23,9 +23,15 @@ import ( "testing" "time" + "github.com/gogo/protobuf/proto" + "github.com/golang/snappy" config_util "github.com/prometheus/common/config" "github.com/prometheus/common/model" "github.com/stretchr/testify/require" + + "github.com/prometheus/prometheus/config" + "github.com/prometheus/prometheus/prompb" + "github.com/prometheus/prometheus/tsdb/chunkenc" ) var longErrMessage = strings.Repeat("error message", maxErrMsgLen) @@ -208,3 +214,226 @@ func TestClientCustomHeaders(t *testing.T) { require.True(t, called, "The remote server wasn't called") } + +func TestReadClient(t *testing.T) { + tests := []struct { + name string + query *prompb.Query + httpHandler http.HandlerFunc + expectedLabels []map[string]string + expectedSamples [][]model.SamplePair + expectedErrorContains string + sortSeries bool + }{ + { + name: "sorted sampled response", + httpHandler: sampledResponseHTTPHandler(t), + expectedLabels: []map[string]string{ + {"foo1": "bar"}, + {"foo2": "bar"}, + }, + expectedSamples: [][]model.SamplePair{ + { + {Timestamp: model.Time(0), Value: model.SampleValue(3)}, + {Timestamp: model.Time(5), Value: model.SampleValue(4)}, + }, + { + {Timestamp: model.Time(0), Value: model.SampleValue(1)}, + {Timestamp: model.Time(5), Value: model.SampleValue(2)}, + }, + }, + expectedErrorContains: "", + sortSeries: true, + }, + { + name: "unsorted sampled response", + httpHandler: sampledResponseHTTPHandler(t), + expectedLabels: []map[string]string{ + {"foo2": "bar"}, + {"foo1": "bar"}, + }, + expectedSamples: [][]model.SamplePair{ + { + {Timestamp: model.Time(0), Value: model.SampleValue(1)}, + {Timestamp: model.Time(5), Value: model.SampleValue(2)}, + }, + { + {Timestamp: model.Time(0), Value: model.SampleValue(3)}, + {Timestamp: model.Time(5), Value: model.SampleValue(4)}, + }, + }, + expectedErrorContains: "", + sortSeries: false, + }, + { + name: "chunked response", + query: &prompb.Query{ + StartTimestampMs: 4000, + EndTimestampMs: 12000, + }, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/x-streamed-protobuf; proto=prometheus.ChunkedReadResponse") + + flusher, ok := w.(http.Flusher) + require.True(t, ok) + + cw := NewChunkedWriter(w, flusher) + l := []prompb.Label{ + {Name: "foo", Value: "bar"}, + } + + chunks := buildTestChunks(t) + for i, c := range chunks { + cSeries := prompb.ChunkedSeries{Labels: l, Chunks: []prompb.Chunk{c}} + readResp := prompb.ChunkedReadResponse{ + ChunkedSeries: []*prompb.ChunkedSeries{&cSeries}, + QueryIndex: int64(i), + } + + b, err := proto.Marshal(&readResp) + require.NoError(t, err) + + _, err = cw.Write(b) + require.NoError(t, err) + } + }), + expectedLabels: []map[string]string{ + {"foo": "bar"}, + {"foo": "bar"}, + {"foo": "bar"}, + }, + // This is the output of buildTestChunks minus the samples outside the query range. + expectedSamples: [][]model.SamplePair{ + { + {Timestamp: model.Time(4000), Value: model.SampleValue(4)}, + }, + { + {Timestamp: model.Time(5000), Value: model.SampleValue(1)}, + {Timestamp: model.Time(6000), Value: model.SampleValue(2)}, + {Timestamp: model.Time(7000), Value: model.SampleValue(3)}, + {Timestamp: model.Time(8000), Value: model.SampleValue(4)}, + {Timestamp: model.Time(9000), Value: model.SampleValue(5)}, + }, + { + {Timestamp: model.Time(10000), Value: model.SampleValue(2)}, + {Timestamp: model.Time(11000), Value: model.SampleValue(3)}, + {Timestamp: model.Time(12000), Value: model.SampleValue(4)}, + }, + }, + expectedErrorContains: "", + }, + { + name: "unsupported content type", + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "foobar") + }), + expectedErrorContains: "unsupported content type", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + server := httptest.NewServer(test.httpHandler) + defer server.Close() + + u, err := url.Parse(server.URL) + require.NoError(t, err) + + conf := &ClientConfig{ + URL: &config_util.URL{URL: u}, + Timeout: model.Duration(5 * time.Second), + ChunkedReadLimit: config.DefaultChunkedReadLimit, + } + c, err := NewReadClient("test", conf) + require.NoError(t, err) + + query := &prompb.Query{} + if test.query != nil { + query = test.query + } + + ss, err := c.Read(context.Background(), query, test.sortSeries) + if test.expectedErrorContains != "" { + require.ErrorContains(t, err, test.expectedErrorContains) + return + } + + require.NoError(t, err) + + i := 0 + + for ss.Next() { + require.NoError(t, ss.Err()) + s := ss.At() + + l := s.Labels() + require.Len(t, test.expectedLabels[i], l.Len()) + for k, v := range test.expectedLabels[i] { + require.True(t, l.Has(k)) + require.Equal(t, v, l.Get(k)) + } + + it := s.Iterator(nil) + j := 0 + + for valType := it.Next(); valType != chunkenc.ValNone; valType = it.Next() { + require.NoError(t, it.Err()) + + ts, v := it.At() + expectedSample := test.expectedSamples[i][j] + + require.Equal(t, int64(expectedSample.Timestamp), ts) + require.Equal(t, float64(expectedSample.Value), v) + + j++ + } + + require.Len(t, test.expectedSamples[i], j) + + i++ + } + + require.NoError(t, ss.Err()) + }) + } +} + +func sampledResponseHTTPHandler(t *testing.T) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/x-protobuf") + + resp := prompb.ReadResponse{ + Results: []*prompb.QueryResult{ + { + Timeseries: []*prompb.TimeSeries{ + { + Labels: []prompb.Label{ + {Name: "foo2", Value: "bar"}, + }, + Samples: []prompb.Sample{ + {Value: float64(1), Timestamp: int64(0)}, + {Value: float64(2), Timestamp: int64(5)}, + }, + Exemplars: []prompb.Exemplar{}, + }, + { + Labels: []prompb.Label{ + {Name: "foo1", Value: "bar"}, + }, + Samples: []prompb.Sample{ + {Value: float64(3), Timestamp: int64(0)}, + {Value: float64(4), Timestamp: int64(5)}, + }, + Exemplars: []prompb.Exemplar{}, + }, + }, + }, + }, + } + b, err := proto.Marshal(&resp) + require.NoError(t, err) + + _, err = w.Write(snappy.Encode(nil, b)) + require.NoError(t, err) + } +} diff --git a/storage/remote/codec.go b/storage/remote/codec.go index c9220ca42..80bb81150 100644 --- a/storage/remote/codec.go +++ b/storage/remote/codec.go @@ -540,6 +540,220 @@ func (c *concreteSeriesIterator) Err() error { return nil } +// chunkedSeriesSet implements storage.SeriesSet. +type chunkedSeriesSet struct { + chunkedReader *ChunkedReader + respBody io.ReadCloser + mint, maxt int64 + cancel func(error) + + current storage.Series + err error +} + +func NewChunkedSeriesSet(chunkedReader *ChunkedReader, respBody io.ReadCloser, mint, maxt int64, cancel func(error)) storage.SeriesSet { + return &chunkedSeriesSet{ + chunkedReader: chunkedReader, + respBody: respBody, + mint: mint, + maxt: maxt, + cancel: cancel, + } +} + +// Next return true if there is a next series and false otherwise. It will +// block until the next series is available. +func (s *chunkedSeriesSet) Next() bool { + res := &prompb.ChunkedReadResponse{} + + err := s.chunkedReader.NextProto(res) + if err != nil { + if !errors.Is(err, io.EOF) { + s.err = err + _, _ = io.Copy(io.Discard, s.respBody) + } + + _ = s.respBody.Close() + s.cancel(err) + + return false + } + + s.current = &chunkedSeries{ + ChunkedSeries: prompb.ChunkedSeries{ + Labels: res.ChunkedSeries[0].Labels, + Chunks: res.ChunkedSeries[0].Chunks, + }, + mint: s.mint, + maxt: s.maxt, + } + + return true +} + +func (s *chunkedSeriesSet) At() storage.Series { + return s.current +} + +func (s *chunkedSeriesSet) Err() error { + return s.err +} + +func (s *chunkedSeriesSet) Warnings() annotations.Annotations { + return nil +} + +type chunkedSeries struct { + prompb.ChunkedSeries + mint, maxt int64 +} + +var _ storage.Series = &chunkedSeries{} + +func (s *chunkedSeries) Labels() labels.Labels { + b := labels.NewScratchBuilder(0) + return s.ToLabels(&b, nil) +} + +func (s *chunkedSeries) Iterator(it chunkenc.Iterator) chunkenc.Iterator { + csIt, ok := it.(*chunkedSeriesIterator) + if ok { + csIt.reset(s.Chunks, s.mint, s.maxt) + return csIt + } + return newChunkedSeriesIterator(s.Chunks, s.mint, s.maxt) +} + +type chunkedSeriesIterator struct { + chunks []prompb.Chunk + idx int + cur chunkenc.Iterator + valType chunkenc.ValueType + mint, maxt int64 + + err error +} + +var _ chunkenc.Iterator = &chunkedSeriesIterator{} + +func newChunkedSeriesIterator(chunks []prompb.Chunk, mint, maxt int64) *chunkedSeriesIterator { + it := &chunkedSeriesIterator{} + it.reset(chunks, mint, maxt) + return it +} + +func (it *chunkedSeriesIterator) Next() chunkenc.ValueType { + if it.err != nil { + return chunkenc.ValNone + } + if len(it.chunks) == 0 { + return chunkenc.ValNone + } + + for it.valType = it.cur.Next(); it.valType != chunkenc.ValNone; it.valType = it.cur.Next() { + atT := it.AtT() + if atT > it.maxt { + it.chunks = nil // Exhaust this iterator so follow-up calls to Next or Seek return fast. + return chunkenc.ValNone + } + if atT >= it.mint { + return it.valType + } + } + + if it.idx >= len(it.chunks)-1 { + it.valType = chunkenc.ValNone + } else { + it.idx++ + it.resetIterator() + it.valType = it.Next() + } + + return it.valType +} + +func (it *chunkedSeriesIterator) Seek(t int64) chunkenc.ValueType { + if it.err != nil { + return chunkenc.ValNone + } + if len(it.chunks) == 0 { + return chunkenc.ValNone + } + + startIdx := it.idx + it.idx += sort.Search(len(it.chunks)-startIdx, func(i int) bool { + return it.chunks[startIdx+i].MaxTimeMs >= t + }) + if it.idx > startIdx { + it.resetIterator() + } else { + ts := it.cur.AtT() + if ts >= t { + return it.valType + } + } + + for it.valType = it.cur.Next(); it.valType != chunkenc.ValNone; it.valType = it.cur.Next() { + ts := it.cur.AtT() + if ts > it.maxt { + it.chunks = nil // Exhaust this iterator so follow-up calls to Next or Seek return fast. + return chunkenc.ValNone + } + if ts >= t && ts >= it.mint { + return it.valType + } + } + + it.valType = chunkenc.ValNone + return it.valType +} + +func (it *chunkedSeriesIterator) resetIterator() { + if it.idx < len(it.chunks) { + chunk := it.chunks[it.idx] + + decodedChunk, err := chunkenc.FromData(chunkenc.Encoding(chunk.Type), chunk.Data) + if err != nil { + it.err = err + return + } + + it.cur = decodedChunk.Iterator(nil) + } else { + it.cur = chunkenc.NewNopIterator() + } +} + +func (it *chunkedSeriesIterator) reset(chunks []prompb.Chunk, mint, maxt int64) { + it.chunks = chunks + it.mint = mint + it.maxt = maxt + it.idx = 0 + if len(chunks) > 0 { + it.resetIterator() + } +} + +func (it *chunkedSeriesIterator) At() (ts int64, v float64) { + return it.cur.At() +} + +func (it *chunkedSeriesIterator) AtHistogram(h *histogram.Histogram) (int64, *histogram.Histogram) { + return it.cur.AtHistogram(h) +} + +func (it *chunkedSeriesIterator) AtFloatHistogram(fh *histogram.FloatHistogram) (int64, *histogram.FloatHistogram) { + return it.cur.AtFloatHistogram(fh) +} + +func (it *chunkedSeriesIterator) AtT() int64 { + return it.cur.AtT() +} + +func (it *chunkedSeriesIterator) Err() error { + return it.err +} + // validateLabelsAndMetricName validates the label names/values and metric names returned from remote read, // also making sure that there are no labels with duplicate names. func validateLabelsAndMetricName(ls []prompb.Label) error { @@ -612,15 +826,6 @@ func FromLabelMatchers(matchers []*prompb.LabelMatcher) ([]*labels.Matcher, erro return result, nil } -// LabelProtosToMetric unpack a []*prompb.Label to a model.Metric. -func LabelProtosToMetric(labelPairs []*prompb.Label) model.Metric { - metric := make(model.Metric, len(labelPairs)) - for _, l := range labelPairs { - metric[model.LabelName(l.Name)] = model.LabelValue(l.Value) - } - return metric -} - // DecodeWriteRequest from an io.Reader into a prompb.WriteRequest, handling // snappy decompression. // Used also by documentation/examples/remote_storage. diff --git a/storage/remote/codec_test.go b/storage/remote/codec_test.go index 279d10e41..404f1add7 100644 --- a/storage/remote/codec_test.go +++ b/storage/remote/codec_test.go @@ -16,6 +16,7 @@ package remote import ( "bytes" "fmt" + "io" "sync" "testing" @@ -24,6 +25,7 @@ import ( "github.com/prometheus/common/model" "github.com/stretchr/testify/require" + "github.com/prometheus/prometheus/config" "github.com/prometheus/prometheus/model/histogram" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/model/metadata" @@ -705,3 +707,270 @@ func (c *mockChunkIterator) Next() bool { func (c *mockChunkIterator) Err() error { return nil } + +func TestChunkedSeriesIterator(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + chks := buildTestChunks(t) + + it := newChunkedSeriesIterator(chks, 2000, 12000) + + require.NoError(t, it.err) + require.NotNil(t, it.cur) + + // Initial next; advance to first valid sample of first chunk. + res := it.Next() + require.Equal(t, chunkenc.ValFloat, res) + require.NoError(t, it.Err()) + + ts, v := it.At() + require.Equal(t, int64(2000), ts) + require.Equal(t, float64(2), v) + + // Next to the second sample of the first chunk. + res = it.Next() + require.Equal(t, chunkenc.ValFloat, res) + require.NoError(t, it.Err()) + + ts, v = it.At() + require.Equal(t, int64(3000), ts) + require.Equal(t, float64(3), v) + + // Attempt to seek to the first sample of the first chunk (should return current sample). + res = it.Seek(0) + require.Equal(t, chunkenc.ValFloat, res) + + ts, v = it.At() + require.Equal(t, int64(3000), ts) + require.Equal(t, float64(3), v) + + // Seek to the end of the first chunk. + res = it.Seek(4000) + require.Equal(t, chunkenc.ValFloat, res) + + ts, v = it.At() + require.Equal(t, int64(4000), ts) + require.Equal(t, float64(4), v) + + // Next to the first sample of the second chunk. + res = it.Next() + require.Equal(t, chunkenc.ValFloat, res) + require.NoError(t, it.Err()) + + ts, v = it.At() + require.Equal(t, int64(5000), ts) + require.Equal(t, float64(1), v) + + // Seek to the second sample of the third chunk. + res = it.Seek(10999) + require.Equal(t, chunkenc.ValFloat, res) + require.NoError(t, it.Err()) + + ts, v = it.At() + require.Equal(t, int64(11000), ts) + require.Equal(t, float64(3), v) + + // Attempt to seek to something past the last sample (should return false and exhaust the iterator). + res = it.Seek(99999) + require.Equal(t, chunkenc.ValNone, res) + require.NoError(t, it.Err()) + + // Attempt to next past the last sample (should return false as the iterator is exhausted). + res = it.Next() + require.Equal(t, chunkenc.ValNone, res) + require.NoError(t, it.Err()) + }) + + t.Run("invalid chunk encoding error", func(t *testing.T) { + chks := buildTestChunks(t) + + // Set chunk type to an invalid value. + chks[0].Type = 8 + + it := newChunkedSeriesIterator(chks, 0, 14000) + + res := it.Next() + require.Equal(t, chunkenc.ValNone, res) + + res = it.Seek(1000) + require.Equal(t, chunkenc.ValNone, res) + + require.ErrorContains(t, it.err, "invalid chunk encoding") + require.Nil(t, it.cur) + }) + + t.Run("empty chunks", func(t *testing.T) { + emptyChunks := make([]prompb.Chunk, 0) + + it1 := newChunkedSeriesIterator(emptyChunks, 0, 1000) + require.Equal(t, chunkenc.ValNone, it1.Next()) + require.Equal(t, chunkenc.ValNone, it1.Seek(1000)) + require.NoError(t, it1.Err()) + + var nilChunks []prompb.Chunk + + it2 := newChunkedSeriesIterator(nilChunks, 0, 1000) + require.Equal(t, chunkenc.ValNone, it2.Next()) + require.Equal(t, chunkenc.ValNone, it2.Seek(1000)) + require.NoError(t, it2.Err()) + }) +} + +func TestChunkedSeries(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + chks := buildTestChunks(t) + + s := chunkedSeries{ + ChunkedSeries: prompb.ChunkedSeries{ + Labels: []prompb.Label{ + {Name: "foo", Value: "bar"}, + {Name: "asdf", Value: "zxcv"}, + }, + Chunks: chks, + }, + } + + require.Equal(t, labels.FromStrings("asdf", "zxcv", "foo", "bar"), s.Labels()) + + it := s.Iterator(nil) + res := it.Next() // Behavior is undefined w/o the initial call to Next. + + require.Equal(t, chunkenc.ValFloat, res) + require.NoError(t, it.Err()) + + ts, v := it.At() + require.Equal(t, int64(0), ts) + require.Equal(t, float64(0), v) + }) +} + +func TestChunkedSeriesSet(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + buf := &bytes.Buffer{} + flusher := &mockFlusher{} + + w := NewChunkedWriter(buf, flusher) + r := NewChunkedReader(buf, config.DefaultChunkedReadLimit, nil) + + chks := buildTestChunks(t) + l := []prompb.Label{ + {Name: "foo", Value: "bar"}, + } + + for i, c := range chks { + cSeries := prompb.ChunkedSeries{Labels: l, Chunks: []prompb.Chunk{c}} + readResp := prompb.ChunkedReadResponse{ + ChunkedSeries: []*prompb.ChunkedSeries{&cSeries}, + QueryIndex: int64(i), + } + + b, err := proto.Marshal(&readResp) + require.NoError(t, err) + + _, err = w.Write(b) + require.NoError(t, err) + } + + ss := NewChunkedSeriesSet(r, io.NopCloser(buf), 0, 14000, func(error) {}) + require.NoError(t, ss.Err()) + require.Nil(t, ss.Warnings()) + + res := ss.Next() + require.True(t, res) + require.NoError(t, ss.Err()) + + s := ss.At() + require.Equal(t, 1, s.Labels().Len()) + require.True(t, s.Labels().Has("foo")) + require.Equal(t, "bar", s.Labels().Get("foo")) + + it := s.Iterator(nil) + it.Next() + ts, v := it.At() + require.Equal(t, int64(0), ts) + require.Equal(t, float64(0), v) + + numResponses := 1 + for ss.Next() { + numResponses++ + } + require.Equal(t, numTestChunks, numResponses) + require.NoError(t, ss.Err()) + }) + + t.Run("chunked reader error", func(t *testing.T) { + buf := &bytes.Buffer{} + flusher := &mockFlusher{} + + w := NewChunkedWriter(buf, flusher) + r := NewChunkedReader(buf, config.DefaultChunkedReadLimit, nil) + + chks := buildTestChunks(t) + l := []prompb.Label{ + {Name: "foo", Value: "bar"}, + } + + for i, c := range chks { + cSeries := prompb.ChunkedSeries{Labels: l, Chunks: []prompb.Chunk{c}} + readResp := prompb.ChunkedReadResponse{ + ChunkedSeries: []*prompb.ChunkedSeries{&cSeries}, + QueryIndex: int64(i), + } + + b, err := proto.Marshal(&readResp) + require.NoError(t, err) + + b[0] = 0xFF // Corruption! + + _, err = w.Write(b) + require.NoError(t, err) + } + + ss := NewChunkedSeriesSet(r, io.NopCloser(buf), 0, 14000, func(error) {}) + require.NoError(t, ss.Err()) + require.Nil(t, ss.Warnings()) + + res := ss.Next() + require.False(t, res) + require.ErrorContains(t, ss.Err(), "proto: illegal wireType 7") + }) +} + +// mockFlusher implements http.Flusher. +type mockFlusher struct{} + +func (f *mockFlusher) Flush() {} + +const ( + numTestChunks = 3 + numSamplesPerTestChunk = 5 +) + +func buildTestChunks(t *testing.T) []prompb.Chunk { + startTime := int64(0) + chks := make([]prompb.Chunk, 0, numTestChunks) + + time := startTime + + for i := 0; i < numTestChunks; i++ { + c := chunkenc.NewXORChunk() + + a, err := c.Appender() + require.NoError(t, err) + + minTimeMs := time + + for j := 0; j < numSamplesPerTestChunk; j++ { + a.Append(time, float64(i+j)) + time += int64(1000) + } + + chks = append(chks, prompb.Chunk{ + MinTimeMs: minTimeMs, + MaxTimeMs: time, + Type: prompb.Chunk_XOR, + Data: c.Bytes(), + }) + } + + return chks +} diff --git a/storage/remote/read.go b/storage/remote/read.go index e54b14f1e..2ec48784d 100644 --- a/storage/remote/read.go +++ b/storage/remote/read.go @@ -165,11 +165,11 @@ func (q *querier) Select(ctx context.Context, sortSeries bool, hints *storage.Se return storage.ErrSeriesSet(fmt.Errorf("toQuery: %w", err)) } - res, err := q.client.Read(ctx, query) + res, err := q.client.Read(ctx, query, sortSeries) if err != nil { return storage.ErrSeriesSet(fmt.Errorf("remote_read: %w", err)) } - return newSeriesSetFilter(FromQueryResult(sortSeries, res), added) + return newSeriesSetFilter(res, added) } // addExternalLabels adds matchers for each external label. External labels diff --git a/storage/remote/read_handler_test.go b/storage/remote/read_handler_test.go index a68187268..4cd4647e7 100644 --- a/storage/remote/read_handler_test.go +++ b/storage/remote/read_handler_test.go @@ -179,7 +179,7 @@ func BenchmarkStreamReadEndpoint(b *testing.B) { require.Equal(b, 2, recorder.Code/100) var results []*prompb.ChunkedReadResponse - stream := NewChunkedReader(recorder.Result().Body, DefaultChunkedReadLimit, nil) + stream := NewChunkedReader(recorder.Result().Body, config.DefaultChunkedReadLimit, nil) for { res := &prompb.ChunkedReadResponse{} @@ -280,7 +280,7 @@ func TestStreamReadEndpoint(t *testing.T) { require.Equal(t, "", recorder.Result().Header.Get("Content-Encoding")) var results []*prompb.ChunkedReadResponse - stream := NewChunkedReader(recorder.Result().Body, DefaultChunkedReadLimit, nil) + stream := NewChunkedReader(recorder.Result().Body, config.DefaultChunkedReadLimit, nil) for { res := &prompb.ChunkedReadResponse{} err := stream.NextProto(res) diff --git a/storage/remote/read_test.go b/storage/remote/read_test.go index 357bdba1f..d63cefc3f 100644 --- a/storage/remote/read_test.go +++ b/storage/remote/read_test.go @@ -27,6 +27,7 @@ import ( "github.com/prometheus/prometheus/config" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/prompb" + "github.com/prometheus/prometheus/storage" "github.com/prometheus/prometheus/util/annotations" "github.com/prometheus/prometheus/util/testutil" ) @@ -198,7 +199,7 @@ type mockedRemoteClient struct { b labels.ScratchBuilder } -func (c *mockedRemoteClient) Read(_ context.Context, query *prompb.Query) (*prompb.QueryResult, error) { +func (c *mockedRemoteClient) Read(_ context.Context, query *prompb.Query, sortSeries bool) (storage.SeriesSet, error) { if c.got != nil { return nil, fmt.Errorf("expected only one call to remote client got: %v", query) } @@ -227,7 +228,7 @@ func (c *mockedRemoteClient) Read(_ context.Context, query *prompb.Query) (*prom q.Timeseries = append(q.Timeseries, &prompb.TimeSeries{Labels: s.Labels}) } } - return q, nil + return FromQueryResult(sortSeries, q), nil } func (c *mockedRemoteClient) reset() { diff --git a/storage/remote/storage.go b/storage/remote/storage.go index afa2d411a..05634f179 100644 --- a/storage/remote/storage.go +++ b/storage/remote/storage.go @@ -115,6 +115,7 @@ func (s *Storage) ApplyConfig(conf *config.Config) error { c, err := NewReadClient(name, &ClientConfig{ URL: rrConf.URL, Timeout: rrConf.RemoteTimeout, + ChunkedReadLimit: rrConf.ChunkedReadLimit, HTTPClientConfig: rrConf.HTTPClientConfig, Headers: rrConf.Headers, }) diff --git a/web/api/v1/api_test.go b/web/api/v1/api_test.go index ba38ddc97..261ed6b61 100644 --- a/web/api/v1/api_test.go +++ b/web/api/v1/api_test.go @@ -1074,6 +1074,9 @@ func setupRemote(s storage.Storage) *httptest.Server { } } + w.Header().Set("Content-Type", "application/x-protobuf") + w.Header().Set("Content-Encoding", "snappy") + if err := remote.EncodeReadResponse(&resp, w); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return From 9da75328ea76f4ebb9a02009b93331a8374f38cf Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Wed, 28 Aug 2024 11:15:42 -0400 Subject: [PATCH 19/20] fix(utf8): ensure correct validation when legacy mode turned on (#14736) fix(utf8): ensure correct validation when legacy mode turned on This depends on the included update of the prometheus/common dependency. --------- Signed-off-by: Owen Williams --- go.mod | 4 +- go.sum | 8 ++-- model/labels/labels_common.go | 19 ++++++-- model/labels/labels_test.go | 78 ++++++++++++++++++++++++++++++++- promql/parser/printer.go | 2 +- scrape/scrape.go | 8 +++- scrape/scrape_test.go | 37 ++++++++++++++++ storage/remote/write_handler.go | 5 ++- 8 files changed, 146 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index 50d560bc3..af327c64a 100644 --- a/go.mod +++ b/go.mod @@ -52,9 +52,9 @@ require ( github.com/oklog/ulid v1.3.1 github.com/ovh/go-ovh v1.6.0 github.com/prometheus/alertmanager v0.27.0 - github.com/prometheus/client_golang v1.19.1 + github.com/prometheus/client_golang v1.20.0 github.com/prometheus/client_model v0.6.1 - github.com/prometheus/common v0.55.0 + github.com/prometheus/common v0.56.0 github.com/prometheus/common/assets v0.2.0 github.com/prometheus/common/sigv4 v0.1.0 github.com/prometheus/exporter-toolkit v0.11.0 diff --git a/go.sum b/go.sum index bd4aa4f6b..933ef9420 100644 --- a/go.sum +++ b/go.sum @@ -608,8 +608,8 @@ github.com/prometheus/client_golang v1.3.0/go.mod h1:hJaj2vgQTGQmVCsAACORcieXFeD github.com/prometheus/client_golang v1.4.0/go.mod h1:e9GMxYsXl05ICDXkRhurwBS4Q3OK1iX/F2sw+iXX5zU= github.com/prometheus/client_golang v1.7.1/go.mod h1:PY5Wy2awLA44sXw4AOSfFBetzPP4j5+D6mVACh+pe2M= github.com/prometheus/client_golang v1.11.0/go.mod h1:Z6t4BnS23TR94PD6BsDNk8yVqroYurpAkEiz0P2BEV0= -github.com/prometheus/client_golang v1.19.1 h1:wZWJDwK+NameRJuPGDhlnFgx8e8HN3XHQeLaYJFJBOE= -github.com/prometheus/client_golang v1.19.1/go.mod h1:mP78NwGzrVks5S2H6ab8+ZZGJLZUq1hoULYBAYBw1Ho= +github.com/prometheus/client_golang v1.20.0 h1:jBzTZ7B099Rg24tny+qngoynol8LtVYlA2bqx3vEloI= +github.com/prometheus/client_golang v1.20.0/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE= github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190115171406-56726106282f/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= @@ -625,8 +625,8 @@ github.com/prometheus/common v0.9.1/go.mod h1:yhUN8i9wzaXS3w1O07YhxHEBxD+W35wd8b github.com/prometheus/common v0.10.0/go.mod h1:Tlit/dnDKsSWFlCLTWaA1cyBgKHSMdTB80sz/V91rCo= github.com/prometheus/common v0.26.0/go.mod h1:M7rCNAaPfAosfx8veZJCuw84e35h3Cfd9VFqTh1DIvc= github.com/prometheus/common v0.29.0/go.mod h1:vu+V0TpY+O6vW9J44gczi3Ap/oXXR10b+M/gUGO4Hls= -github.com/prometheus/common v0.55.0 h1:KEi6DK7lXW/m7Ig5i47x0vRzuBsHuvJdi5ee6Y3G1dc= -github.com/prometheus/common v0.55.0/go.mod h1:2SECS4xJG1kd8XF9IcM1gMX6510RAEL65zxzNImwdc8= +github.com/prometheus/common v0.56.0 h1:UffReloqkBtvtQEYDg2s+uDPGRrJyC6vZWPGXf6OhPY= +github.com/prometheus/common v0.56.0/go.mod h1:7uRPFSUTbfZWsJ7MHY56sqt7hLQu3bxXHDnNhl8E9qI= github.com/prometheus/common/assets v0.2.0 h1:0P5OrzoHrYBOSM1OigWL3mY8ZvV2N4zIE/5AahrSrfM= github.com/prometheus/common/assets v0.2.0/go.mod h1:D17UVUE12bHbim7HzwUvtqm6gwBEaDQ0F+hIGbFbccI= github.com/prometheus/common/sigv4 v0.1.0 h1:qoVebwtwwEhS85Czm2dSROY5fTo2PAPEVdDeppTwGX4= diff --git a/model/labels/labels_common.go b/model/labels/labels_common.go index 6db86b03c..d7bdc1e07 100644 --- a/model/labels/labels_common.go +++ b/model/labels/labels_common.go @@ -95,12 +95,23 @@ func (ls *Labels) UnmarshalYAML(unmarshal func(interface{}) error) error { } // IsValid checks if the metric name or label names are valid. -func (ls Labels) IsValid() bool { +func (ls Labels) IsValid(validationScheme model.ValidationScheme) bool { err := ls.Validate(func(l Label) error { - if l.Name == model.MetricNameLabel && !model.IsValidMetricName(model.LabelValue(l.Value)) { - return strconv.ErrSyntax + if l.Name == model.MetricNameLabel { + // If the default validation scheme has been overridden with legacy mode, + // we need to call the special legacy validation checker. + if validationScheme == model.LegacyValidation && model.NameValidationScheme == model.UTF8Validation && !model.IsValidLegacyMetricName(string(model.LabelValue(l.Value))) { + return strconv.ErrSyntax + } + if !model.IsValidMetricName(model.LabelValue(l.Value)) { + return strconv.ErrSyntax + } } - if !model.LabelName(l.Name).IsValid() || !model.LabelValue(l.Value).IsValid() { + if validationScheme == model.LegacyValidation && model.NameValidationScheme == model.UTF8Validation { + if !model.LabelName(l.Name).IsValidLegacy() || !model.LabelValue(l.Value).IsValid() { + return strconv.ErrSyntax + } + } else if !model.LabelName(l.Name).IsValid() || !model.LabelValue(l.Value).IsValid() { return strconv.ErrSyntax } return nil diff --git a/model/labels/labels_test.go b/model/labels/labels_test.go index d8910cdc8..920890831 100644 --- a/model/labels/labels_test.go +++ b/model/labels/labels_test.go @@ -21,6 +21,7 @@ import ( "strings" "testing" + "github.com/prometheus/common/model" "github.com/stretchr/testify/require" "gopkg.in/yaml.v2" ) @@ -272,11 +273,86 @@ func TestLabels_IsValid(t *testing.T) { }, } { t.Run("", func(t *testing.T) { - require.Equal(t, test.expected, test.input.IsValid()) + require.Equal(t, test.expected, test.input.IsValid(model.LegacyValidation)) }) } } +func TestLabels_ValidationModes(t *testing.T) { + for _, test := range []struct { + input Labels + globalMode model.ValidationScheme + callMode model.ValidationScheme + expected bool + }{ + { + input: FromStrings( + "__name__", "test.metric", + "hostname", "localhost", + "job", "check", + ), + globalMode: model.UTF8Validation, + callMode: model.UTF8Validation, + expected: true, + }, + { + input: FromStrings( + "__name__", "test", + "\xc5 bad utf8", "localhost", + "job", "check", + ), + globalMode: model.UTF8Validation, + callMode: model.UTF8Validation, + expected: false, + }, + { + // Setting the common model to legacy validation and then trying to check for UTF-8 on a + // per-call basis is not supported. + input: FromStrings( + "__name__", "test.utf8.metric", + "hostname", "localhost", + "job", "check", + ), + globalMode: model.LegacyValidation, + callMode: model.UTF8Validation, + expected: false, + }, + { + input: FromStrings( + "__name__", "test", + "hostname", "localhost", + "job", "check", + ), + globalMode: model.LegacyValidation, + callMode: model.LegacyValidation, + expected: true, + }, + { + input: FromStrings( + "__name__", "test.utf8.metric", + "hostname", "localhost", + "job", "check", + ), + globalMode: model.UTF8Validation, + callMode: model.LegacyValidation, + expected: false, + }, + { + input: FromStrings( + "__name__", "test", + "host.name", "localhost", + "job", "check", + ), + globalMode: model.UTF8Validation, + callMode: model.LegacyValidation, + expected: false, + }, + } { + model.NameValidationScheme = test.globalMode + require.Equal(t, test.expected, test.input.IsValid(test.callMode)) + } +} + func TestLabels_Equal(t *testing.T) { labels := FromStrings( "aaa", "111", diff --git a/promql/parser/printer.go b/promql/parser/printer.go index 5613956f7..69f5f082d 100644 --- a/promql/parser/printer.go +++ b/promql/parser/printer.go @@ -88,7 +88,7 @@ func (node *AggregateExpr) getAggOpStr() string { func joinLabels(ss []string) string { for i, s := range ss { // If the label is already quoted, don't quote it again. - if s[0] != '"' && s[0] != '\'' && s[0] != '`' && !model.IsValidLegacyMetricName(model.LabelValue(s)) { + if s[0] != '"' && s[0] != '\'' && s[0] != '`' && !model.IsValidLegacyMetricName(string(model.LabelValue(s))) { ss[i] = fmt.Sprintf("\"%s\"", s) } } diff --git a/scrape/scrape.go b/scrape/scrape.go index 222cc62f9..5fcd623c0 100644 --- a/scrape/scrape.go +++ b/scrape/scrape.go @@ -111,6 +111,7 @@ type scrapeLoopOptions struct { interval time.Duration timeout time.Duration scrapeClassicHistograms bool + validationScheme model.ValidationScheme mrc []*relabel.Config cache *scrapeCache @@ -186,6 +187,7 @@ func newScrapePool(cfg *config.ScrapeConfig, app storage.Appendable, offsetSeed options.PassMetadataInContext, metrics, options.skipOffsetting, + opts.validationScheme, ) } sp.metrics.targetScrapePoolTargetLimit.WithLabelValues(sp.config.JobName).Set(float64(sp.config.TargetLimit)) @@ -346,6 +348,7 @@ func (sp *scrapePool) restartLoops(reuseCache bool) { cache: cache, interval: interval, timeout: timeout, + validationScheme: validationScheme, }) ) if err != nil { @@ -853,6 +856,7 @@ type scrapeLoop struct { interval time.Duration timeout time.Duration scrapeClassicHistograms bool + validationScheme model.ValidationScheme // Feature flagged options. enableNativeHistogramIngestion bool @@ -1160,6 +1164,7 @@ func newScrapeLoop(ctx context.Context, passMetadataInContext bool, metrics *scrapeMetrics, skipOffsetting bool, + validationScheme model.ValidationScheme, ) *scrapeLoop { if l == nil { l = log.NewNopLogger() @@ -1211,6 +1216,7 @@ func newScrapeLoop(ctx context.Context, appendMetadataToWAL: appendMetadataToWAL, metrics: metrics, skipOffsetting: skipOffsetting, + validationScheme: validationScheme, } sl.ctx, sl.cancel = context.WithCancel(ctx) @@ -1631,7 +1637,7 @@ loop: err = errNameLabelMandatory break loop } - if !lset.IsValid() { + if !lset.IsValid(sl.validationScheme) { err = fmt.Errorf("invalid metric name or label names: %s", lset.String()) break loop } diff --git a/scrape/scrape_test.go b/scrape/scrape_test.go index 9b3061521..b703f21d4 100644 --- a/scrape/scrape_test.go +++ b/scrape/scrape_test.go @@ -684,6 +684,7 @@ func newBasicScrapeLoop(t testing.TB, ctx context.Context, scraper scraper, app false, newTestScrapeMetrics(t), false, + model.LegacyValidation, ) } @@ -826,6 +827,7 @@ func TestScrapeLoopRun(t *testing.T) { false, scrapeMetrics, false, + model.LegacyValidation, ) // The loop must terminate during the initial offset if the context @@ -970,6 +972,7 @@ func TestScrapeLoopMetadata(t *testing.T) { false, scrapeMetrics, false, + model.LegacyValidation, ) defer cancel() @@ -1065,6 +1068,40 @@ func TestScrapeLoopFailWithInvalidLabelsAfterRelabel(t *testing.T) { require.Equal(t, 0, seriesAdded) } +func TestScrapeLoopFailLegacyUnderUTF8(t *testing.T) { + // Test that scrapes fail when default validation is utf8 but scrape config is + // legacy. + model.NameValidationScheme = model.UTF8Validation + defer func() { + model.NameValidationScheme = model.LegacyValidation + }() + s := teststorage.New(t) + defer s.Close() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + sl := newBasicScrapeLoop(t, ctx, &testScraper{}, s.Appender, 0) + sl.validationScheme = model.LegacyValidation + + slApp := sl.appender(ctx) + total, added, seriesAdded, err := sl.append(slApp, []byte("{\"test.metric\"} 1\n"), "", time.Time{}) + require.ErrorContains(t, err, "invalid metric name or label names") + require.NoError(t, slApp.Rollback()) + require.Equal(t, 1, total) + require.Equal(t, 0, added) + require.Equal(t, 0, seriesAdded) + + // When scrapeloop has validation set to UTF-8, the metric is allowed. + sl.validationScheme = model.UTF8Validation + + slApp = sl.appender(ctx) + total, added, seriesAdded, err = sl.append(slApp, []byte("{\"test.metric\"} 1\n"), "", time.Time{}) + require.NoError(t, err) + require.Equal(t, 1, total) + require.Equal(t, 1, added) + require.Equal(t, 1, seriesAdded) +} + func makeTestMetrics(n int) []byte { // Construct a metrics string to parse sb := bytes.Buffer{} diff --git a/storage/remote/write_handler.go b/storage/remote/write_handler.go index 9ea3f2bf9..58fb668cc 100644 --- a/storage/remote/write_handler.go +++ b/storage/remote/write_handler.go @@ -28,6 +28,7 @@ import ( "github.com/golang/snappy" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" + "github.com/prometheus/common/model" "github.com/prometheus/prometheus/config" "github.com/prometheus/prometheus/model/exemplar" @@ -239,7 +240,7 @@ func (h *writeHandler) write(ctx context.Context, req *prompb.WriteRequest) (err // TODO(bwplotka): Even as per 1.0 spec, this should be a 400 error, while other samples are // potentially written. Perhaps unify with fixed writeV2 implementation a bit. - if !ls.Has(labels.MetricName) || !ls.IsValid() { + if !ls.Has(labels.MetricName) || !ls.IsValid(model.NameValidationScheme) { level.Warn(h.logger).Log("msg", "Invalid metric names or labels", "got", ls.String()) samplesWithInvalidLabels++ continue @@ -380,7 +381,7 @@ func (h *writeHandler) appendV2(app storage.Appender, req *writev2.Request, rs * // Validate series labels early. // NOTE(bwplotka): While spec allows UTF-8, Prometheus Receiver may impose // specific limits and follow https://prometheus.io/docs/specs/remote_write_spec_2_0/#invalid-samples case. - if !ls.Has(labels.MetricName) || !ls.IsValid() { + if !ls.Has(labels.MetricName) || !ls.IsValid(model.NameValidationScheme) { badRequestErrs = append(badRequestErrs, fmt.Errorf("invalid metric name or labels, got %v", ls.String())) samplesWithInvalidLabels += len(ts.Samples) + len(ts.Histograms) continue From c586c15ae6a0fdb4d02abb9b850bcfedb44644e9 Mon Sep 17 00:00:00 2001 From: machine424 Date: Wed, 15 Nov 2023 11:41:12 +0100 Subject: [PATCH 20/20] fix(discovery): make discovery manager notify consumers of dropped targets for still defined jobs scrape/manager_test.go: add a test to check that the manager gets notified for targets that got dropped by discovery to reproduce: https://github.com/prometheus/prometheus/issues/12858#issuecomment-1732318102 Signed-off-by: machine424 --- discovery/manager.go | 41 ++-- discovery/manager_test.go | 5 +- scrape/manager.go | 2 +- scrape/manager_test.go | 413 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 446 insertions(+), 15 deletions(-) diff --git a/discovery/manager.go b/discovery/manager.go index 48bea85bb..b7ebdb7e0 100644 --- a/discovery/manager.go +++ b/discovery/manager.go @@ -212,9 +212,7 @@ func (m *Manager) ApplyConfig(cfg map[string]Configs) error { m.metrics.FailedConfigs.Set(float64(failedCount)) var ( - wg sync.WaitGroup - // keep shows if we keep any providers after reload. - keep bool + wg sync.WaitGroup newProviders []*Provider ) for _, prov := range m.providers { @@ -228,13 +226,12 @@ func (m *Manager) ApplyConfig(cfg map[string]Configs) error { continue } newProviders = append(newProviders, prov) - // refTargets keeps reference targets used to populate new subs' targets + // refTargets keeps reference targets used to populate new subs' targets as they should be the same. var refTargets map[string]*targetgroup.Group prov.mu.Lock() m.targetsMtx.Lock() for s := range prov.subs { - keep = true refTargets = m.targets[poolKey{s, prov.name}] // Remove obsolete subs' targets. if _, ok := prov.newSubs[s]; !ok { @@ -267,7 +264,9 @@ func (m *Manager) ApplyConfig(cfg map[string]Configs) error { // While startProvider does pull the trigger, it may take some time to do so, therefore // we pull the trigger as soon as possible so that downstream managers can populate their state. // See https://github.com/prometheus/prometheus/pull/8639 for details. - if keep { + // This also helps making the downstream managers drop stale targets as soon as possible. + // See https://github.com/prometheus/prometheus/pull/13147 for details. + if len(m.providers) > 0 { select { case m.triggerSend <- struct{}{}: default: @@ -288,7 +287,9 @@ func (m *Manager) StartCustomProvider(ctx context.Context, name string, worker D name: {}, }, } + m.mtx.Lock() m.providers = append(m.providers, p) + m.mtx.Unlock() m.startProvider(ctx, p) } @@ -403,19 +404,33 @@ func (m *Manager) allGroups() map[string][]*targetgroup.Group { tSets := map[string][]*targetgroup.Group{} n := map[string]int{} + m.mtx.RLock() m.targetsMtx.Lock() - defer m.targetsMtx.Unlock() - for pkey, tsets := range m.targets { - for _, tg := range tsets { - // Even if the target group 'tg' is empty we still need to send it to the 'Scrape manager' - // to signal that it needs to stop all scrape loops for this target set. - tSets[pkey.setName] = append(tSets[pkey.setName], tg) - n[pkey.setName] += len(tg.Targets) + for _, p := range m.providers { + p.mu.RLock() + for s := range p.subs { + // Send empty lists for subs without any targets to make sure old stale targets are dropped by consumers. + // See: https://github.com/prometheus/prometheus/issues/12858 for details. + if _, ok := tSets[s]; !ok { + tSets[s] = []*targetgroup.Group{} + n[s] = 0 + } + if tsets, ok := m.targets[poolKey{s, p.name}]; ok { + for _, tg := range tsets { + tSets[s] = append(tSets[s], tg) + n[s] += len(tg.Targets) + } + } } + p.mu.RUnlock() } + m.targetsMtx.Unlock() + m.mtx.RUnlock() + for setName, v := range n { m.metrics.DiscoveredTargets.WithLabelValues(setName).Set(float64(v)) } + return tSets } diff --git a/discovery/manager_test.go b/discovery/manager_test.go index be07edbdb..707c3931d 100644 --- a/discovery/manager_test.go +++ b/discovery/manager_test.go @@ -939,11 +939,13 @@ func TestTargetSetTargetGroupsPresentOnConfigChange(t *testing.T) { discoveryManager.ApplyConfig(c) // Original targets should be present as soon as possible. + // An empty list should be sent for prometheus2 to drop any stale targets syncedTargets = <-discoveryManager.SyncCh() mu.Unlock() - require.Len(t, syncedTargets, 1) + require.Len(t, syncedTargets, 2) verifySyncedPresence(t, syncedTargets, "prometheus", "{__address__=\"foo:9090\"}", true) require.Len(t, syncedTargets["prometheus"], 1) + require.Empty(t, syncedTargets["prometheus2"]) // prometheus2 configs should be ready on second sync. syncedTargets = <-discoveryManager.SyncCh() @@ -1275,6 +1277,7 @@ func TestCoordinationWithReceiver(t *testing.T) { Targets: []model.LabelSet{{"__instance__": "1"}}, }, }, + "mock1": {}, }, }, { diff --git a/scrape/manager.go b/scrape/manager.go index 6d4e8707b..e3dba5f0e 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -142,7 +142,7 @@ func (m *Manager) UnregisterMetrics() { func (m *Manager) reloader() { reloadIntervalDuration := m.opts.DiscoveryReloadInterval - if reloadIntervalDuration < model.Duration(5*time.Second) { + if reloadIntervalDuration == model.Duration(0) { reloadIntervalDuration = model.Duration(5 * time.Second) } diff --git a/scrape/manager_test.go b/scrape/manager_test.go index c8d9bd698..a2a3bba6f 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -20,6 +20,7 @@ import ( "net/http/httptest" "net/url" "os" + "sort" "strconv" "sync" "testing" @@ -36,6 +37,7 @@ import ( "github.com/prometheus/prometheus/config" "github.com/prometheus/prometheus/discovery" + _ "github.com/prometheus/prometheus/discovery/file" "github.com/prometheus/prometheus/discovery/targetgroup" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/model/relabel" @@ -869,3 +871,414 @@ func TestUnregisterMetrics(t *testing.T) { manager.UnregisterMetrics() } } + +func applyConfig( + t *testing.T, + config string, + scrapeManager *Manager, + discoveryManager *discovery.Manager, +) { + t.Helper() + + cfg := loadConfiguration(t, config) + require.NoError(t, scrapeManager.ApplyConfig(cfg)) + + c := make(map[string]discovery.Configs) + scfgs, err := cfg.GetScrapeConfigs() + require.NoError(t, err) + for _, v := range scfgs { + c[v.JobName] = v.ServiceDiscoveryConfigs + } + require.NoError(t, discoveryManager.ApplyConfig(c)) +} + +func runManagers(t *testing.T, ctx context.Context) (*discovery.Manager, *Manager) { + t.Helper() + + reg := prometheus.NewRegistry() + sdMetrics, err := discovery.RegisterSDMetrics(reg, discovery.NewRefreshMetrics(reg)) + require.NoError(t, err) + discoveryManager := discovery.NewManager( + ctx, + log.NewNopLogger(), + reg, + sdMetrics, + discovery.Updatert(100*time.Millisecond), + ) + scrapeManager, err := NewManager( + &Options{DiscoveryReloadInterval: model.Duration(100 * time.Millisecond)}, + nil, + nopAppendable{}, + prometheus.NewRegistry(), + ) + require.NoError(t, err) + go discoveryManager.Run() + go scrapeManager.Run(discoveryManager.SyncCh()) + return discoveryManager, scrapeManager +} + +func writeIntoFile(t *testing.T, content, filePattern string) *os.File { + t.Helper() + + file, err := os.CreateTemp("", filePattern) + require.NoError(t, err) + _, err = file.WriteString(content) + require.NoError(t, err) + return file +} + +func requireTargets( + t *testing.T, + scrapeManager *Manager, + jobName string, + waitToAppear bool, + expectedTargets []string, +) { + t.Helper() + + require.Eventually(t, func() bool { + targets, ok := scrapeManager.TargetsActive()[jobName] + if !ok { + if waitToAppear { + return false + } + t.Fatalf("job %s shouldn't be dropped", jobName) + } + if expectedTargets == nil { + return targets == nil + } + if len(targets) != len(expectedTargets) { + return false + } + sTargets := []string{} + for _, t := range targets { + sTargets = append(sTargets, t.String()) + } + sort.Strings(expectedTargets) + sort.Strings(sTargets) + for i, t := range sTargets { + if t != expectedTargets[i] { + return false + } + } + return true + }, 1*time.Second, 100*time.Millisecond) +} + +// TestTargetDisappearsAfterProviderRemoved makes sure that when a provider is dropped, (only) its targets are dropped. +func TestTargetDisappearsAfterProviderRemoved(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + myJob := "my-job" + myJobSDTargetURL := "my:9876" + myJobStaticTargetURL := "my:5432" + + sdFileContent := fmt.Sprintf(`[{"targets": ["%s"]}]`, myJobSDTargetURL) + sDFile := writeIntoFile(t, sdFileContent, "*targets.json") + + baseConfig := ` +scrape_configs: +- job_name: %s + static_configs: + - targets: ['%s'] + file_sd_configs: + - files: ['%s'] +` + + discoveryManager, scrapeManager := runManagers(t, ctx) + defer scrapeManager.Stop() + + applyConfig( + t, + fmt.Sprintf( + baseConfig, + myJob, + myJobStaticTargetURL, + sDFile.Name(), + ), + scrapeManager, + discoveryManager, + ) + // Make sure the jobs targets are taken into account + requireTargets( + t, + scrapeManager, + myJob, + true, + []string{ + fmt.Sprintf("http://%s/metrics", myJobSDTargetURL), + fmt.Sprintf("http://%s/metrics", myJobStaticTargetURL), + }, + ) + + // Apply a new config where a provider is removed + baseConfig = ` +scrape_configs: +- job_name: %s + static_configs: + - targets: ['%s'] +` + applyConfig( + t, + fmt.Sprintf( + baseConfig, + myJob, + myJobStaticTargetURL, + ), + scrapeManager, + discoveryManager, + ) + // Make sure the corresponding target was dropped + requireTargets( + t, + scrapeManager, + myJob, + false, + []string{ + fmt.Sprintf("http://%s/metrics", myJobStaticTargetURL), + }, + ) + + // Apply a new config with no providers + baseConfig = ` +scrape_configs: +- job_name: %s +` + applyConfig( + t, + fmt.Sprintf( + baseConfig, + myJob, + ), + scrapeManager, + discoveryManager, + ) + // Make sure the corresponding target was dropped + requireTargets( + t, + scrapeManager, + myJob, + false, + nil, + ) +} + +// TestOnlyProviderStaleTargetsAreDropped makes sure that when a job has only one provider with multiple targets +// and when the provider can no longer discover some of those targets, only those stale targets are dropped. +func TestOnlyProviderStaleTargetsAreDropped(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + jobName := "my-job" + jobTarget1URL := "foo:9876" + jobTarget2URL := "foo:5432" + + sdFile1Content := fmt.Sprintf(`[{"targets": ["%s"]}]`, jobTarget1URL) + sdFile2Content := fmt.Sprintf(`[{"targets": ["%s"]}]`, jobTarget2URL) + sDFile1 := writeIntoFile(t, sdFile1Content, "*targets.json") + sDFile2 := writeIntoFile(t, sdFile2Content, "*targets.json") + + baseConfig := ` +scrape_configs: +- job_name: %s + file_sd_configs: + - files: ['%s', '%s'] +` + discoveryManager, scrapeManager := runManagers(t, ctx) + defer scrapeManager.Stop() + + applyConfig( + t, + fmt.Sprintf(baseConfig, jobName, sDFile1.Name(), sDFile2.Name()), + scrapeManager, + discoveryManager, + ) + + // Make sure the job's targets are taken into account + requireTargets( + t, + scrapeManager, + jobName, + true, + []string{ + fmt.Sprintf("http://%s/metrics", jobTarget1URL), + fmt.Sprintf("http://%s/metrics", jobTarget2URL), + }, + ) + + // Apply the same config for the same job but with a non existing file to make the provider + // unable to discover some targets + applyConfig( + t, + fmt.Sprintf(baseConfig, jobName, sDFile1.Name(), "/idontexistdoi.json"), + scrapeManager, + discoveryManager, + ) + + // The old target should get dropped + requireTargets( + t, + scrapeManager, + jobName, + false, + []string{fmt.Sprintf("http://%s/metrics", jobTarget1URL)}, + ) +} + +// TestProviderStaleTargetsAreDropped makes sure that when a job has only one provider and when that provider +// should no longer discover targets, the targets of that provider are dropped. +// See: https://github.com/prometheus/prometheus/issues/12858 +func TestProviderStaleTargetsAreDropped(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + jobName := "my-job" + jobTargetURL := "foo:9876" + + sdFileContent := fmt.Sprintf(`[{"targets": ["%s"]}]`, jobTargetURL) + sDFile := writeIntoFile(t, sdFileContent, "*targets.json") + + baseConfig := ` +scrape_configs: +- job_name: %s + file_sd_configs: + - files: ['%s'] +` + discoveryManager, scrapeManager := runManagers(t, ctx) + defer scrapeManager.Stop() + + applyConfig( + t, + fmt.Sprintf(baseConfig, jobName, sDFile.Name()), + scrapeManager, + discoveryManager, + ) + + // Make sure the job's targets are taken into account + requireTargets( + t, + scrapeManager, + jobName, + true, + []string{ + fmt.Sprintf("http://%s/metrics", jobTargetURL), + }, + ) + + // Apply the same config for the same job but with a non existing file to make the provider + // unable to discover some targets + applyConfig( + t, + fmt.Sprintf(baseConfig, jobName, "/idontexistdoi.json"), + scrapeManager, + discoveryManager, + ) + + // The old target should get dropped + requireTargets( + t, + scrapeManager, + jobName, + false, + nil, + ) +} + +// TestOnlyStaleTargetsAreDropped makes sure that when a job has multiple providers, when aone of them should no, +// longer discover targets, only the stale targets of that provier are dropped. +func TestOnlyStaleTargetsAreDropped(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + myJob := "my-job" + myJobSDTargetURL := "my:9876" + myJobStaticTargetURL := "my:5432" + otherJob := "other-job" + otherJobTargetURL := "other:1234" + + sdFileContent := fmt.Sprintf(`[{"targets": ["%s"]}]`, myJobSDTargetURL) + sDFile := writeIntoFile(t, sdFileContent, "*targets.json") + + baseConfig := ` +scrape_configs: +- job_name: %s + static_configs: + - targets: ['%s'] + file_sd_configs: + - files: ['%s'] +- job_name: %s + static_configs: + - targets: ['%s'] +` + + discoveryManager, scrapeManager := runManagers(t, ctx) + defer scrapeManager.Stop() + + // Apply the initial config with an existing file + applyConfig( + t, + fmt.Sprintf( + baseConfig, + myJob, + myJobStaticTargetURL, + sDFile.Name(), + otherJob, + otherJobTargetURL, + ), + scrapeManager, + discoveryManager, + ) + // Make sure the jobs targets are taken into account + requireTargets( + t, + scrapeManager, + myJob, + true, + []string{ + fmt.Sprintf("http://%s/metrics", myJobSDTargetURL), + fmt.Sprintf("http://%s/metrics", myJobStaticTargetURL), + }, + ) + requireTargets( + t, + scrapeManager, + otherJob, + true, + []string{fmt.Sprintf("http://%s/metrics", otherJobTargetURL)}, + ) + + // Apply the same config with a non existing file for myJob + applyConfig( + t, + fmt.Sprintf( + baseConfig, + myJob, + myJobStaticTargetURL, + "/idontexistdoi.json", + otherJob, + otherJobTargetURL, + ), + scrapeManager, + discoveryManager, + ) + + // Only the SD target should get dropped for myJob + requireTargets( + t, + scrapeManager, + myJob, + false, + []string{ + fmt.Sprintf("http://%s/metrics", myJobStaticTargetURL), + }, + ) + // The otherJob should keep its target + requireTargets( + t, + scrapeManager, + otherJob, + false, + []string{fmt.Sprintf("http://%s/metrics", otherJobTargetURL)}, + ) +}