From 3509c46e41f6a72b57b82285c522871680010658 Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Thu, 26 Sep 2024 14:20:19 +1000 Subject: [PATCH] query-tee: fix behaviour of `-proxy.compare-skip-recent-samples` for long-running queries (#9416) * query-tee: fix behaviour of `-proxy.compare-skip-recent-samples` for long-running queries * Add changelog entry --- CHANGELOG.md | 1 + tools/querytee/proxy_endpoint.go | 25 ++--- tools/querytee/proxy_endpoint_test.go | 2 +- tools/querytee/proxy_test.go | 2 +- tools/querytee/response_comparator.go | 103 ++++++++++++--------- tools/querytee/response_comparator_test.go | 15 +-- 6 files changed, 83 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53728aed19e..f8a7ac8739b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -231,6 +231,7 @@ * [ENHANCEMENT] Optionally consider equivalent error messages the same when comparing responses. Enabled by default, disable with `-proxy.require-exact-error-match=true`. #9143 #9350 #9366 * [BUGFIX] Ensure any errors encountered while forwarding a request to a backend (eg. DNS resolution failures) are logged. #8419 * [BUGFIX] The comparison of the results should not fail when either side contains extra samples from within SkipRecentSamples duration. #8920 +* [BUGFIX] When `-proxy.compare-skip-recent-samples` is enabled, compare sample timestamps with the time the query requests were made, rather than the time at which the comparison is occurring. #9416 ### Documentation diff --git a/tools/querytee/proxy_endpoint.go b/tools/querytee/proxy_endpoint.go index 51ddecb1740..f49f0374c96 100644 --- a/tools/querytee/proxy_endpoint.go +++ b/tools/querytee/proxy_endpoint.go @@ -24,7 +24,7 @@ import ( ) type ResponsesComparator interface { - Compare(expected, actual []byte) (ComparisonResult, error) + Compare(expected, actual []byte, queryEvaluationTime time.Time) (ComparisonResult, error) } type ProxyEndpoint struct { @@ -102,14 +102,15 @@ func (p *ProxyEndpoint) selectBackends() []ProxyBackendInterface { func (p *ProxyEndpoint) executeBackendRequests(req *http.Request, backends []ProxyBackendInterface, resCh chan *backendResponse) { var ( - wg = sync.WaitGroup{} - err error - body []byte - responses = make([]*backendResponse, 0, len(backends)) - responsesMtx = sync.Mutex{} - timingMtx = sync.Mutex{} - query = req.URL.RawQuery - logger, ctx = spanlogger.NewWithLogger(req.Context(), p.logger, "Incoming proxied request") + wg = sync.WaitGroup{} + err error + body []byte + responses = make([]*backendResponse, 0, len(backends)) + responsesMtx = sync.Mutex{} + timingMtx = sync.Mutex{} + query = req.URL.RawQuery + logger, ctx = spanlogger.NewWithLogger(req.Context(), p.logger, "Incoming proxied request") + evaluationTime = time.Now() ) defer logger.Finish() @@ -258,7 +259,7 @@ func (p *ProxyEndpoint) executeBackendRequests(req *http.Request, backends []Pro expectedResponse, actualResponse = actualResponse, expectedResponse } - result, err := p.compareResponses(expectedResponse, actualResponse) + result, err := p.compareResponses(expectedResponse, actualResponse, evaluationTime) if result == ComparisonFailed { level.Error(logger).Log( "msg", "response comparison failed", @@ -314,7 +315,7 @@ func (p *ProxyEndpoint) waitBackendResponseForDownstream(resCh chan *backendResp return firstResponse } -func (p *ProxyEndpoint) compareResponses(expectedResponse, actualResponse *backendResponse) (ComparisonResult, error) { +func (p *ProxyEndpoint) compareResponses(expectedResponse, actualResponse *backendResponse, queryEvaluationTime time.Time) (ComparisonResult, error) { if expectedResponse.err != nil { return ComparisonFailed, fmt.Errorf("skipped comparison of response because the request to the preferred backend failed: %w", expectedResponse.err) } @@ -335,7 +336,7 @@ func (p *ProxyEndpoint) compareResponses(expectedResponse, actualResponse *backe return ComparisonSkipped, fmt.Errorf("skipped comparison of response because the response from the secondary backend contained an unexpected content type '%s', expected 'application/json'", actualResponse.contentType) } - return p.comparator.Compare(expectedResponse.body, actualResponse.body) + return p.comparator.Compare(expectedResponse.body, actualResponse.body, queryEvaluationTime) } type backendResponse struct { diff --git a/tools/querytee/proxy_endpoint_test.go b/tools/querytee/proxy_endpoint_test.go index 86f59834ee9..2658f171d51 100644 --- a/tools/querytee/proxy_endpoint_test.go +++ b/tools/querytee/proxy_endpoint_test.go @@ -704,7 +704,7 @@ type mockComparator struct { comparisonError error } -func (m *mockComparator) Compare(_, _ []byte) (ComparisonResult, error) { +func (m *mockComparator) Compare(_, _ []byte, _ time.Time) (ComparisonResult, error) { return m.comparisonResult, m.comparisonError } diff --git a/tools/querytee/proxy_test.go b/tools/querytee/proxy_test.go index e0ead99df9a..85b04e121c8 100644 --- a/tools/querytee/proxy_test.go +++ b/tools/querytee/proxy_test.go @@ -47,7 +47,7 @@ var testRoutes = []Route{ type testComparator struct{} -func (testComparator) Compare(_, _ []byte) (ComparisonResult, error) { +func (testComparator) Compare(_, _ []byte, _ time.Time) (ComparisonResult, error) { return ComparisonSuccess, nil } diff --git a/tools/querytee/response_comparator.go b/tools/querytee/response_comparator.go index 8ec556be21e..f992a279fe8 100644 --- a/tools/querytee/response_comparator.go +++ b/tools/querytee/response_comparator.go @@ -22,7 +22,7 @@ import ( ) // SamplesComparatorFunc helps with comparing different types of samples coming from /api/v1/query and /api/v1/query_range routes. -type SamplesComparatorFunc func(expected, actual json.RawMessage, opts SampleComparisonOptions) error +type SamplesComparatorFunc func(expected, actual json.RawMessage, queryEvaluationTime time.Time, opts SampleComparisonOptions) error type SamplesResponse struct { Status string @@ -64,7 +64,7 @@ func (s *SamplesComparator) RegisterSamplesType(samplesType string, comparator S s.sampleTypesComparator[samplesType] = comparator } -func (s *SamplesComparator) Compare(expectedResponse, actualResponse []byte) (ComparisonResult, error) { +func (s *SamplesComparator) Compare(expectedResponse, actualResponse []byte, queryEvaluationTime time.Time) (ComparisonResult, error) { var expected, actual SamplesResponse err := json.Unmarshal(expectedResponse, &expected) @@ -102,7 +102,7 @@ func (s *SamplesComparator) Compare(expectedResponse, actualResponse []byte) (Co return ComparisonFailed, fmt.Errorf("resultType %s not registered for comparison", expected.Data.ResultType) } - if err := comparator(expected.Data.Result, actual.Data.Result, s.opts); err != nil { + if err := comparator(expected.Data.Result, actual.Data.Result, queryEvaluationTime, s.opts); err != nil { return ComparisonFailed, err } @@ -208,7 +208,7 @@ func formatAnnotationsForErrorMessage(warnings []string) string { return "[" + strings.Join(formatted, ", ") + "]" } -func compareMatrix(expectedRaw, actualRaw json.RawMessage, opts SampleComparisonOptions) error { +func compareMatrix(expectedRaw, actualRaw json.RawMessage, queryEvaluationTime time.Time, opts SampleComparisonOptions) error { var expected, actual model.Matrix err := json.Unmarshal(expectedRaw, &expected) @@ -220,7 +220,7 @@ func compareMatrix(expectedRaw, actualRaw json.RawMessage, opts SampleComparison return err } - if allMatrixSamplesWithinRecentSampleWindow(expected, opts) && allMatrixSamplesWithinRecentSampleWindow(actual, opts) { + if allMatrixSamplesWithinRecentSampleWindow(expected, queryEvaluationTime, opts) && allMatrixSamplesWithinRecentSampleWindow(actual, queryEvaluationTime, opts) { return nil } @@ -240,7 +240,7 @@ func compareMatrix(expectedRaw, actualRaw json.RawMessage, opts SampleComparison } actualMetric := actual[actualMetricIndex] - err := compareMatrixSamples(expectedMetric, actualMetric, opts) + err := compareMatrixSamples(expectedMetric, actualMetric, queryEvaluationTime, opts) if err != nil { return fmt.Errorf("%w\nExpected result for series:\n%v\n\nActual result for series:\n%v", err, expectedMetric, actualMetric) } @@ -249,9 +249,9 @@ func compareMatrix(expectedRaw, actualRaw json.RawMessage, opts SampleComparison return nil } -func compareMatrixSamples(expected, actual *model.SampleStream, opts SampleComparisonOptions) error { +func compareMatrixSamples(expected, actual *model.SampleStream, queryEvaluationTime time.Time, opts SampleComparisonOptions) error { expectedSamplesTail, actualSamplesTail, err := comparePairs(expected.Values, actual.Values, func(p1 model.SamplePair, p2 model.SamplePair) error { - err := compareSamplePair(p1, p2, opts) + err := compareSamplePair(p1, p2, queryEvaluationTime, opts) if err != nil { return fmt.Errorf("float sample pair does not match for metric %s: %w", expected.Metric, err) } @@ -262,7 +262,7 @@ func compareMatrixSamples(expected, actual *model.SampleStream, opts SampleCompa } expectedHistogramSamplesTail, actualHistogramSamplesTail, err := comparePairs(expected.Histograms, actual.Histograms, func(p1 model.SampleHistogramPair, p2 model.SampleHistogramPair) error { - err := compareSampleHistogramPair(p1, p2, opts) + err := compareSampleHistogramPair(p1, p2, queryEvaluationTime, opts) if err != nil { return fmt.Errorf("histogram sample pair does not match for metric %s: %w", expected.Metric, err) } @@ -282,11 +282,11 @@ func compareMatrixSamples(expected, actual *model.SampleStream, opts SampleCompa } skipAllRecentFloatSamples := canSkipAllSamples(func(p model.SamplePair) bool { - return time.Since(p.Timestamp.Time())-opts.SkipRecentSamples < 0 + return queryEvaluationTime.Sub(p.Timestamp.Time())-opts.SkipRecentSamples < 0 }, expectedSamplesTail, actualSamplesTail) skipAllRecentHistogramSamples := canSkipAllSamples(func(p model.SampleHistogramPair) bool { - return time.Since(p.Timestamp.Time())-opts.SkipRecentSamples < 0 + return queryEvaluationTime.Sub(p.Timestamp.Time())-opts.SkipRecentSamples < 0 }, expectedHistogramSamplesTail, actualHistogramSamplesTail) if skipAllRecentFloatSamples && skipAllRecentHistogramSamples { @@ -356,20 +356,20 @@ func canSkipAllSamples[S ~[]M, M any](skip func(M) bool, ss ...S) bool { return true } -func allMatrixSamplesWithinRecentSampleWindow(m model.Matrix, opts SampleComparisonOptions) bool { +func allMatrixSamplesWithinRecentSampleWindow(m model.Matrix, queryEvaluationTime time.Time, opts SampleComparisonOptions) bool { if opts.SkipRecentSamples == 0 { return false } for _, series := range m { for _, sample := range series.Values { - if time.Since(sample.Timestamp.Time()) > opts.SkipRecentSamples { + if queryEvaluationTime.Sub(sample.Timestamp.Time()) > opts.SkipRecentSamples { return false } } for _, sample := range series.Histograms { - if time.Since(sample.Timestamp.Time()) > opts.SkipRecentSamples { + if queryEvaluationTime.Sub(sample.Timestamp.Time()) > opts.SkipRecentSamples { return false } } @@ -378,7 +378,7 @@ func allMatrixSamplesWithinRecentSampleWindow(m model.Matrix, opts SampleCompari return true } -func compareVector(expectedRaw, actualRaw json.RawMessage, opts SampleComparisonOptions) error { +func compareVector(expectedRaw, actualRaw json.RawMessage, queryEvaluationTime time.Time, opts SampleComparisonOptions) error { var expected, actual model.Vector err := json.Unmarshal(expectedRaw, &expected) @@ -391,7 +391,7 @@ func compareVector(expectedRaw, actualRaw json.RawMessage, opts SampleComparison return err } - if allVectorSamplesWithinRecentSampleWindow(expected, opts) && allVectorSamplesWithinRecentSampleWindow(actual, opts) { + if allVectorSamplesWithinRecentSampleWindow(expected, queryEvaluationTime, opts) && allVectorSamplesWithinRecentSampleWindow(actual, queryEvaluationTime, opts) { return nil } @@ -413,13 +413,18 @@ func compareVector(expectedRaw, actualRaw json.RawMessage, opts SampleComparison actualMetric := actual[actualMetricIndex] if expectedMetric.Histogram == nil && actualMetric.Histogram == nil { - err := compareSamplePair(model.SamplePair{ - Timestamp: expectedMetric.Timestamp, - Value: expectedMetric.Value, - }, model.SamplePair{ - Timestamp: actualMetric.Timestamp, - Value: actualMetric.Value, - }, opts) + err := compareSamplePair( + model.SamplePair{ + Timestamp: expectedMetric.Timestamp, + Value: expectedMetric.Value, + }, + model.SamplePair{ + Timestamp: actualMetric.Timestamp, + Value: actualMetric.Value, + }, + queryEvaluationTime, + opts, + ) if err != nil { return fmt.Errorf("float sample pair does not match for metric %s: %w", expectedMetric.Metric, err) } @@ -428,13 +433,18 @@ func compareVector(expectedRaw, actualRaw json.RawMessage, opts SampleComparison } else if expectedMetric.Histogram == nil && actualMetric.Histogram != nil { return fmt.Errorf("sample pair does not match for metric %s: expected float value but got histogram", expectedMetric.Metric) } else { // Expected value is a histogram and the actual value is a histogram. - err := compareSampleHistogramPair(model.SampleHistogramPair{ - Timestamp: expectedMetric.Timestamp, - Histogram: expectedMetric.Histogram, - }, model.SampleHistogramPair{ - Timestamp: actualMetric.Timestamp, - Histogram: actualMetric.Histogram, - }, opts) + err := compareSampleHistogramPair( + model.SampleHistogramPair{ + Timestamp: expectedMetric.Timestamp, + Histogram: expectedMetric.Histogram, + }, + model.SampleHistogramPair{ + Timestamp: actualMetric.Timestamp, + Histogram: actualMetric.Histogram, + }, + queryEvaluationTime, + opts, + ) if err != nil { return fmt.Errorf("histogram sample pair does not match for metric %s: %w", expectedMetric.Metric, err) } @@ -444,13 +454,13 @@ func compareVector(expectedRaw, actualRaw json.RawMessage, opts SampleComparison return nil } -func allVectorSamplesWithinRecentSampleWindow(v model.Vector, opts SampleComparisonOptions) bool { +func allVectorSamplesWithinRecentSampleWindow(v model.Vector, queryEvaluationTime time.Time, opts SampleComparisonOptions) bool { if opts.SkipRecentSamples == 0 { return false } for _, sample := range v { - if time.Since(sample.Timestamp.Time()) > opts.SkipRecentSamples { + if queryEvaluationTime.Sub(sample.Timestamp.Time()) > opts.SkipRecentSamples { return false } } @@ -458,7 +468,7 @@ func allVectorSamplesWithinRecentSampleWindow(v model.Vector, opts SampleCompari return true } -func compareScalar(expectedRaw, actualRaw json.RawMessage, opts SampleComparisonOptions) error { +func compareScalar(expectedRaw, actualRaw json.RawMessage, queryEvaluationTime time.Time, opts SampleComparisonOptions) error { var expected, actual model.Scalar err := json.Unmarshal(expectedRaw, &expected) if err != nil { @@ -470,20 +480,25 @@ func compareScalar(expectedRaw, actualRaw json.RawMessage, opts SampleComparison return err } - return compareSamplePair(model.SamplePair{ - Timestamp: expected.Timestamp, - Value: expected.Value, - }, model.SamplePair{ - Timestamp: actual.Timestamp, - Value: actual.Value, - }, opts) + return compareSamplePair( + model.SamplePair{ + Timestamp: expected.Timestamp, + Value: expected.Value, + }, + model.SamplePair{ + Timestamp: actual.Timestamp, + Value: actual.Value, + }, + queryEvaluationTime, + opts, + ) } -func compareSamplePair(expected, actual model.SamplePair, opts SampleComparisonOptions) error { +func compareSamplePair(expected, actual model.SamplePair, queryEvaluationTime time.Time, opts SampleComparisonOptions) error { if expected.Timestamp != actual.Timestamp { return fmt.Errorf("expected timestamp %v but got %v", expected.Timestamp, actual.Timestamp) } - if opts.SkipRecentSamples > 0 && time.Since(expected.Timestamp.Time()) < opts.SkipRecentSamples { + if opts.SkipRecentSamples > 0 && queryEvaluationTime.Sub(expected.Timestamp.Time()) < opts.SkipRecentSamples { return nil } if !compareSampleValue(float64(expected.Value), float64(actual.Value), opts) { @@ -507,12 +522,12 @@ func compareSampleValue(first, second float64, opts SampleComparisonOptions) boo return math.Abs(first-second) <= opts.Tolerance } -func compareSampleHistogramPair(expected, actual model.SampleHistogramPair, opts SampleComparisonOptions) error { +func compareSampleHistogramPair(expected, actual model.SampleHistogramPair, queryEvaluationTime time.Time, opts SampleComparisonOptions) error { if expected.Timestamp != actual.Timestamp { return fmt.Errorf("expected timestamp %v but got %v", expected.Timestamp, actual.Timestamp) } - if opts.SkipRecentSamples > 0 && time.Since(expected.Timestamp.Time()) < opts.SkipRecentSamples { + if opts.SkipRecentSamples > 0 && queryEvaluationTime.Sub(expected.Timestamp.Time()) < opts.SkipRecentSamples { return nil } diff --git a/tools/querytee/response_comparator_test.go b/tools/querytee/response_comparator_test.go index 90419c3b556..4603c087a3b 100644 --- a/tools/querytee/response_comparator_test.go +++ b/tools/querytee/response_comparator_test.go @@ -699,7 +699,7 @@ Count: 2.000000, Sum: 3.000000, Buckets: [[0,2):2] @[1]`, }, } { t.Run(tc.name, func(t *testing.T) { - err := compareMatrix(tc.expected, tc.actual, SampleComparisonOptions{}) + err := compareMatrix(tc.expected, tc.actual, time.Now(), SampleComparisonOptions{}) if tc.err == "" { require.NoError(t, err) return @@ -1078,7 +1078,7 @@ func TestCompareVector(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - err := compareVector(tc.expected, tc.actual, SampleComparisonOptions{}) + err := compareVector(tc.expected, tc.actual, time.Now(), SampleComparisonOptions{}) if tc.err == nil { require.NoError(t, err) return @@ -1115,7 +1115,7 @@ func TestCompareScalar(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - err := compareScalar(tc.expected, tc.actual, SampleComparisonOptions{}) + err := compareScalar(tc.expected, tc.actual, time.Now(), SampleComparisonOptions{}) if tc.err == nil { require.NoError(t, err) return @@ -1127,8 +1127,9 @@ func TestCompareScalar(t *testing.T) { } func TestCompareSamplesResponse(t *testing.T) { - now := model.Now().String() - overAnHourAgo := model.Now().Add(-61 * time.Minute).String() + nowT := model.TimeFromUnixNano(time.Date(2099, 1, 2, 3, 4, 5, 6, time.UTC).UnixNano()) + now := nowT.String() + overAnHourAgo := nowT.Add(-61 * time.Minute).String() for _, tc := range []struct { name string @@ -2172,7 +2173,7 @@ func TestCompareSamplesResponse(t *testing.T) { UseRelativeError: tc.useRelativeError, SkipRecentSamples: tc.skipRecentSamples, }) - result, err := samplesComparator.Compare(tc.expected, tc.actual) + result, err := samplesComparator.Compare(tc.expected, tc.actual, nowT.Time()) if tc.err == nil { require.NoError(t, err) require.Equal(t, ComparisonSuccess, result) @@ -2342,7 +2343,7 @@ func TestCompareSampleHistogramPair(t *testing.T) { field.mutator(expected.Histogram, model.FloatString(testCase.expected)) field.mutator(actual.Histogram, model.FloatString(testCase.actual)) - err := compareSampleHistogramPair(expected, actual, opts) + err := compareSampleHistogramPair(expected, actual, time.Now(), opts) if testCase.shouldFail { expectedError := field.expectedErrorMessageGenerator(&expected, &actual)