Fix range calculation on recent searches (#2581)

* hint Signed-off-by: Joe Elliott <[email protected]> * more info Signed-off-by: Joe Elliott <[email protected]> * fix and test Signed-off-by: Joe Elliott <[email protected]> * remove debug Signed-off-by: Joe Elliott <[email protected]> --------- Signed-off-by: Joe Elliott <[email protected]>
grafana · Jun 22, 2023 · c633387 · c633387
1 parent 4773ec4
commit c633387
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 5 deletions.
diff --git a/modules/frontend/searchsharding.go b/modules/frontend/searchsharding.go
@@ -135,7 +135,7 @@ func (s searchSharder) RoundTrip(r *http.Request) (*http.Response, error) {
 
 	// build request to search ingester based on query_ingesters_until config and time range
 	// pass subCtx in requests so we can cancel and exit early
-	ingesterReq, err := s.ingesterRequest(subCtx, tenantID, r, searchReq)
+	ingesterReq, err := s.ingesterRequest(subCtx, tenantID, r, *searchReq)
 	if err != nil {
 		return nil, err
 	}
@@ -433,10 +433,10 @@ func pagesPerRequest(m *backend.BlockMeta, bytesPerRequest int) int {
 // that covers the ingesters. If nil is returned for the http.Request then there is no ingesters query.
 // since this function modifies searchReq.Start and End we are taking a value instead of a pointer to prevent it from
 // unexpectedly changing the passed searchReq.
-func (s *searchSharder) ingesterRequest(ctx context.Context, tenantID string, parent *http.Request, searchReq *tempopb.SearchRequest) (*http.Request, error) {
+func (s *searchSharder) ingesterRequest(ctx context.Context, tenantID string, parent *http.Request, searchReq tempopb.SearchRequest) (*http.Request, error) {
 	// request without start or end, search only in ingester
 	if searchReq.Start == 0 || searchReq.End == 0 {
-		return buildIngesterRequest(ctx, tenantID, parent, searchReq)
+		return buildIngesterRequest(ctx, tenantID, parent, &searchReq)
 	}
 
 	now := time.Now()
@@ -463,7 +463,7 @@ func (s *searchSharder) ingesterRequest(ctx context.Context, tenantID string, pa
 	searchReq.Start = ingesterStart
 	searchReq.End = ingesterEnd
 
-	return buildIngesterRequest(ctx, tenantID, parent, searchReq)
+	return buildIngesterRequest(ctx, tenantID, parent, &searchReq)
 }
 
 func buildIngesterRequest(ctx context.Context, tenantID string, parent *http.Request, searchReq *tempopb.SearchRequest) (*http.Request, error) {

diff --git a/modules/frontend/searchsharding_test.go b/modules/frontend/searchsharding_test.go
@@ -8,6 +8,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"reflect"
 	"strconv"
 	"strings"
 	"sync"
@@ -388,7 +389,8 @@ func TestIngesterRequest(t *testing.T) {
 		searchReq, err := api.ParseSearchRequest(req)
 		require.NoError(t, err)
 
-		actualReq, err := s.ingesterRequest(context.Background(), "test", req, searchReq)
+		copyReq := searchReq
+		actualReq, err := s.ingesterRequest(context.Background(), "test", req, *searchReq)
 		if tc.expectedError != nil {
 			assert.Equal(t, tc.expectedError, err)
 			continue
@@ -399,6 +401,10 @@ func TestIngesterRequest(t *testing.T) {
 		} else {
 			assert.Equal(t, tc.expectedURI, actualReq.RequestURI)
 		}
+
+		// it may seem odd to test that the searchReq is not modified, but this is to prevent an issue that
+		// occurs if the ingesterRequest method is changed to take a searchReq pointer
+		require.True(t, reflect.DeepEqual(copyReq, searchReq))
 	}
 }