From 34dcb86fe775dcb77b1d21287cf03e60027f84c1 Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Sun, 14 Jul 2024 14:54:26 -0700 Subject: [PATCH] Ensure that TimeFilter's adjustments to a QueryRange Range are valid Query range is a bit of an odd beast. It requires a start, end, and a step. Promql's calculations assume that the datapoint times are start plus a multiple of step; if they aren't you are subject to LookbackDelta constraints. Specifically with TimeFilter -- where we are setting either an absolute or relative time barrier we were causing some issues as we were previously truncating the time -- instead of finding the first multiple of step within our timerange. To explain the issue; lets consider the following query: ``` Start: t10 End: t25 Step: 3 ``` If this were to run normally we'd get results with data at 10,13,16,19,22,25 If we at the same time have an absoluteTimeFilter @ t20 -- we'd (prior to this patch) get: 10, 13, 16, 19, 20, 23. For shorter-range queries (i.e. within a day) this is generally not an issue because the step isn't generally long enough to exceed the LookbackDelta default of 5m. But as you look at *longer* time ranges this problem is easier and easier to hit (in #659 I was able to reproduce easily with a ~3 day query -- where step was ~9m. Fixes #659 --- pkg/promclient/multi_api_test.go | 6 +-- pkg/promclient/timefilter.go | 14 ++++++- pkg/promclient/timefilter_test.go | 66 +++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 5 deletions(-) diff --git a/pkg/promclient/multi_api_test.go b/pkg/promclient/multi_api_test.go index 5e5bdf71d..33f1c458c 100644 --- a/pkg/promclient/multi_api_test.go +++ b/pkg/promclient/multi_api_test.go @@ -16,7 +16,7 @@ type stubAPI struct { labelNames func() []string labelValues func(label string) model.LabelValues query func() model.Value - queryRange func() model.Value + queryRange func(q string, r v1.Range) model.Value series func() []model.LabelSet getValue func() model.Value metadata func() map[string][]v1.Metadata @@ -51,7 +51,7 @@ func (s *stubAPI) QueryRange(ctx context.Context, query string, r v1.Range) (mod if s.queryRange == nil { return nil, nil, nil } - return s.queryRange(), nil, nil + return s.queryRange(query, r), nil, nil } // Series finds series by label matchers. @@ -159,7 +159,7 @@ func TestMultiAPIMerging(t *testing.T) { getSample(model.LabelSet{model.MetricNameLabel: "testmetric"}), } }, - queryRange: func() model.Value { + queryRange: func(_ string, _ v1.Range) model.Value { return model.Vector{ getSample(model.LabelSet{model.MetricNameLabel: "testmetric"}), } diff --git a/pkg/promclient/timefilter.go b/pkg/promclient/timefilter.go index 77485aa14..01e4243db 100644 --- a/pkg/promclient/timefilter.go +++ b/pkg/promclient/timefilter.go @@ -69,7 +69,12 @@ func (tf *AbsoluteTimeFilter) QueryRange(ctx context.Context, query string, r v1 if tf.Truncate { if !tf.Start.IsZero() && r.Start.Before(tf.Start) { - r.Start = tf.Start + remainder := tf.Start.Sub(r.Start) % r.Step + if remainder > 0 { + r.Start = tf.Start.Add(r.Step - remainder) + } else { + r.Start = tf.Start + } } if !tf.End.IsZero() && r.End.After(tf.End) { r.End = tf.End @@ -193,7 +198,12 @@ func (tf *RelativeTimeFilter) QueryRange(ctx context.Context, query string, r v1 if tf.Truncate { if !tfStart.IsZero() && r.Start.Before(tfStart) { - r.Start = tfStart + remainder := tfStart.Sub(r.Start) % r.Step + if remainder > 0 { + r.Start = tfStart.Add(r.Step - remainder) + } else { + r.Start = tfStart + } } if !tfEnd.IsZero() && r.End.After(tfEnd) { r.End = tfEnd diff --git a/pkg/promclient/timefilter_test.go b/pkg/promclient/timefilter_test.go index e55b31bd4..e6abaf829 100644 --- a/pkg/promclient/timefilter_test.go +++ b/pkg/promclient/timefilter_test.go @@ -7,6 +7,7 @@ import ( "time" v1 "github.com/prometheus/client_golang/api/prometheus/v1" + "github.com/prometheus/common/model" ) type timeFilterTestCase struct { @@ -200,3 +201,68 @@ func TestRelativeTimeFilter(t *testing.T) { }) } + +// For time truncation we need to ensure that any new range's start aligns with a multiple of the step from the overall query start +// otherwise we get into a LOT of trouble with LookbackDelta as the timestamps of the result won't align properly +func TestAbsoluteTimeFilterStepAlignment(t *testing.T) { + var r v1.Range + stub := &stubAPI{ + queryRange: func(_ string, rng v1.Range) model.Value { + r = rng + return nil + }, + } + filterStart, _ := time.Parse(time.RFC3339, "2024-07-01T00:00:00Z") + api := &AbsoluteTimeFilter{ + API: stub, + Start: filterStart, + Truncate: true, + } + + start := time.Unix(1719738435, 0) + end := time.Unix(1719879274, 0) + + api.QueryRange(context.TODO(), "foo", v1.Range{ + Start: start, + End: end, + Step: 563 * time.Second, + }) + + remainder := r.Start.Sub(start) % r.Step + if remainder > 0 { + t.Fatalf("unexpected step misalignment!") + } +} + +// For time truncation we need to ensure that any new range's start aligns with a multiple of the step from the overall query start +// otherwise we get into a LOT of trouble with LookbackDelta as the timestamps of the result won't align properly +func TestRelativeTimeFilterStepAlignment(t *testing.T) { + var r v1.Range + stub := &stubAPI{ + queryRange: func(_ string, rng v1.Range) model.Value { + r = rng + return nil + }, + } + dur, _ := time.ParseDuration("-2h") + api := &RelativeTimeFilter{ + API: stub, + Start: &dur, + Truncate: true, + } + + now := time.Now() + start := now.Add(-1 * time.Hour * 24) + end := now + + api.QueryRange(context.TODO(), "foo", v1.Range{ + Start: start, + End: end, + Step: 563 * time.Second, + }) + + remainder := r.Start.Sub(start) % r.Step + if remainder > 0 { + t.Fatalf("unexpected step misalignment!") + } +}