From d121db7a65fe76650ed177c4a4ad44f1e3c6a145 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 12 Jan 2023 15:20:50 +0100 Subject: [PATCH] federate: Fix PeekBack usage In most cases, there is no sample at `maxt`, so `PeekBack` has to be used. So far, `PeekBack` did not return a float histogram, and we disregarded even any returned normal histogram. This fixes both, and also tweaks the unit test to discover the problem (by using an earlier timestamp than "now" for the samples in the TSDB). Signed-off-by: beorn7 --- storage/buffer.go | 6 ++++-- storage/buffer_test.go | 2 +- web/federate.go | 12 ++++++++---- web/federate_test.go | 8 ++++---- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/storage/buffer.go b/storage/buffer.go index dc9e9bca3..92767cdd7 100644 --- a/storage/buffer.go +++ b/storage/buffer.go @@ -68,9 +68,11 @@ func (b *BufferedSeriesIterator) ReduceDelta(delta int64) bool { // PeekBack returns the nth previous element of the iterator. If there is none buffered, // ok is false. -func (b *BufferedSeriesIterator) PeekBack(n int) (t int64, v float64, h *histogram.Histogram, ok bool) { +func (b *BufferedSeriesIterator) PeekBack(n int) ( + t int64, v float64, h *histogram.Histogram, fh *histogram.FloatHistogram, ok bool, +) { s, ok := b.buf.nthLast(n) - return s.t, s.v, s.h, ok + return s.t, s.v, s.h, s.fh, ok } // Buffer returns an iterator over the buffered data. Invalidates previously diff --git a/storage/buffer_test.go b/storage/buffer_test.go index aac958397..44d11f0ed 100644 --- a/storage/buffer_test.go +++ b/storage/buffer_test.go @@ -107,7 +107,7 @@ func TestBufferedSeriesIterator(t *testing.T) { require.Equal(t, ev, v, "value mismatch") } prevSampleEq := func(ets int64, ev float64, eok bool) { - ts, v, _, ok := it.PeekBack(1) + ts, v, _, _, ok := it.PeekBack(1) require.Equal(t, eok, ok, "exist mismatch") require.Equal(t, ets, ts, "timestamp mismatch") require.Equal(t, ev, v, "value mismatch") diff --git a/web/federate.go b/web/federate.go index 1589c893e..93526771b 100644 --- a/web/federate.go +++ b/web/federate.go @@ -116,7 +116,8 @@ Loop: var ( t int64 v float64 - h *histogram.FloatHistogram + h *histogram.Histogram + fh *histogram.FloatHistogram ok bool ) valueType := it.Seek(maxt) @@ -124,12 +125,15 @@ Loop: case chunkenc.ValFloat: t, v = it.At() case chunkenc.ValFloatHistogram, chunkenc.ValHistogram: - t, h = it.AtFloatHistogram() + t, fh = it.AtFloatHistogram() default: - t, v, _, ok = it.PeekBack(1) + t, v, h, fh, ok = it.PeekBack(1) if !ok { continue Loop } + if h != nil { + fh = h.ToFloat() + } } // The exposition formats do not support stale markers, so drop them. This // is good enough for staleness handling of federated data, as the @@ -141,7 +145,7 @@ Loop: vec = append(vec, promql.Sample{ Metric: s.Labels(), - Point: promql.Point{T: t, V: v, H: h}, + Point: promql.Point{T: t, V: v, H: fh}, }) } if ws := set.Warnings(); len(ws) > 0 { diff --git a/web/federate_test.go b/web/federate_test.go index b0def6f79..944b0f1ec 100644 --- a/web/federate_test.go +++ b/web/federate_test.go @@ -340,16 +340,16 @@ func TestFederationWithNativeHistograms(t *testing.T) { l := labels.FromStrings("__name__", "test_metric", "foo", fmt.Sprintf("%d", i)) expL := labels.FromStrings("__name__", "test_metric", "instance", "", "foo", fmt.Sprintf("%d", i)) if i%3 == 0 { - _, err = app.Append(0, l, 101*60*1000, float64(i*100)) + _, err = app.Append(0, l, 100*60*1000, float64(i*100)) expVec = append(expVec, promql.Sample{ - Point: promql.Point{T: 101 * 60 * 1000, V: float64(i * 100)}, + Point: promql.Point{T: 100 * 60 * 1000, V: float64(i * 100)}, Metric: expL, }) } else { hist.ZeroCount++ - _, err = app.AppendHistogram(0, l, 101*60*1000, hist.Copy(), nil) + _, err = app.AppendHistogram(0, l, 100*60*1000, hist.Copy(), nil) expVec = append(expVec, promql.Sample{ - Point: promql.Point{T: 101 * 60 * 1000, H: hist.ToFloat()}, + Point: promql.Point{T: 100 * 60 * 1000, H: hist.ToFloat()}, Metric: expL, }) }