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 jacksontj#659 I was able to
reproduce easily with a ~3 day query -- where step was ~9m.

Fixes jacksontj#659

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"}),
 			}

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

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!")
+	}
+}