Add spss and limit to search request hash (#3557)

* Add spss and limit to search request hash Signed-off-by: Joe Elliott <[email protected]> * changelog Signed-off-by: Joe Elliott <[email protected]> * lint Signed-off-by: Joe Elliott <[email protected]> --------- Signed-off-by: Joe Elliott <[email protected]>
grafana · Apr 10, 2024 · b6312f4 · b6312f4
1 parent 4d5c10f
commit b6312f4
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -21,6 +21,7 @@
 * [BUGFIX] Add support for dashes, quotes and spaces in attribute names in autocomplete [#3458](https://github.com/grafana/tempo/pull/3458) (@mapno)
 * [BUGFIX] Correctly handle 429s in GRPC search streaming. [#3469](https://github.com/grafana/tempo/pull/3469) (@joe-ellitot)
 * [BUGFIX] Correctly cancel GRPC and HTTP contexts in the frontend to prevent having to rely on http write timeout. [#3443](https://github.com/grafana/tempo/pull/3443) (@joe-elliott)
+* [BUGFIX] Add spss and limit to the frontend cache key to prevent the return of incorrect results. [#3557](https://github.com/grafana/tempo/pull/3557) (@joe-elliott)
 
 ## v2.4.1
 

diff --git a/modules/frontend/search_handlers_test.go b/modules/frontend/search_handlers_test.go
@@ -548,7 +548,7 @@ func TestSearchAccessesCache(t *testing.T) {
 
 	// setup query
 	query := "{}"
-	hash := hashForTraceQLQuery(query)
+	hash := hashForSearchRequest(&tempopb.SearchRequest{Query: query, Limit: 3, SpansPerSpanSet: 2})
 	start := uint32(10)
 	end := uint32(20)
 	cacheKey := searchJobCacheKey(tenant, hash, int64(start), int64(end), meta, 0, 1)
@@ -558,7 +558,7 @@ func TestSearchAccessesCache(t *testing.T) {
 	require.Equal(t, 0, len(bufs))
 
 	// execute query
-	path := fmt.Sprintf("/?start=%d&end=%d&q=%s", start, end, query) // encapsulates block above
+	path := fmt.Sprintf("/?start=%d&end=%d&q=%s&limit=3&spss=2", start, end, query) // encapsulates block above
 	req := httptest.NewRequest("GET", path, nil)
 	ctx := req.Context()
 	ctx = user.InjectOrgID(ctx, tenant)

diff --git a/modules/frontend/search_sharder.go b/modules/frontend/search_sharder.go
@@ -268,7 +268,7 @@ func backendRange(start, end uint32, queryBackendAfter time.Duration) (uint32, u
 func buildBackendRequests(ctx context.Context, tenantID string, parent *http.Request, searchReq *tempopb.SearchRequest, metas []*backend.BlockMeta, bytesPerRequest int, reqCh chan<- *http.Request, errFn func(error)) {
 	defer close(reqCh)
 
-	queryHash := hashForTraceQLQuery(searchReq.Query)
+	queryHash := hashForSearchRequest(searchReq)
 
 	for _, m := range metas {
 		pages := pagesPerRequest(m, bytesPerRequest)
@@ -321,22 +321,27 @@ func buildBackendRequests(ctx context.Context, tenantID string, parent *http.Req
 	}
 }
 
-// hashForTraceQLQuery returns a uint64 hash of the query. if the query is invalid it returns a 0 hash.
+// hashForSearchRequest returns a uint64 hash of the query. if the query is invalid it returns a 0 hash.
 // before hashing the query is forced into a canonical form so equivalent queries will hash to the same value.
-func hashForTraceQLQuery(query string) uint64 {
-	if query == "" {
+func hashForSearchRequest(searchRequest *tempopb.SearchRequest) uint64 {
+	if searchRequest.Query == "" {
 		return 0
 	}
 
-	ast, err := traceql.Parse(query)
+	ast, err := traceql.Parse(searchRequest.Query)
 	if err != nil { // this should never occur. if we've made this far we've already validated the query can parse. however, for sanity, just fail to cache if we can't parse
 		return 0
 	}
 
 	// forces the query into a canonical form
-	query = ast.String()
+	query := ast.String()
 
-	return fnv1a.HashString64(query)
+	// add the query, limit and spss to the hash
+	hash := fnv1a.HashString64(query)
+	hash = fnv1a.AddUint64(hash, uint64(searchRequest.Limit))
+	hash = fnv1a.AddUint64(hash, uint64(searchRequest.SpansPerSpanSet))
+
+	return hash
 }
 
 // pagesPerRequest returns an integer value that indicates the number of pages

diff --git a/modules/frontend/search_sharder_test.go b/modules/frontend/search_sharder_test.go
@@ -700,28 +700,37 @@ func TestMaxDuration(t *testing.T) {
 
 func TestHashTraceQLQuery(t *testing.T) {
 	// exact same queries should have the same hash
-	h1 := hashForTraceQLQuery("{ span.foo = `bar` }")
-	h2 := hashForTraceQLQuery("{ span.foo = `bar` }")
+	h1 := hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` }"})
+	h2 := hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` }"})
 	require.Equal(t, h1, h2)
 
 	// equivalent queries should have the same hash
-	h1 = hashForTraceQLQuery("{ span.foo = `bar`     }")
-	h2 = hashForTraceQLQuery("{ span.foo = `bar` }")
+	h1 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar`     }"})
+	h2 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` }"})
 	require.Equal(t, h1, h2)
 
-	h1 = hashForTraceQLQuery("{ (span.foo = `bar`) || (span.bar = `foo`) }")
-	h2 = hashForTraceQLQuery("{ span.foo = `bar` || span.bar = `foo` }")
+	h1 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ (span.foo = `bar`) || (span.bar = `foo`) }"})
+	h2 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` || span.bar = `foo` }"})
 	require.Equal(t, h1, h2)
 
 	// different queries should have different hashes
-	h1 = hashForTraceQLQuery("{ span.foo = `bar` }")
-	h2 = hashForTraceQLQuery("{ span.foo = `baz` }")
+	h1 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` }"})
+	h2 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `baz` }"})
 	require.NotEqual(t, h1, h2)
 
 	// invalid queries should return 0
-	h1 = hashForTraceQLQuery("{ span.foo = `bar` ")
+	h1 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` "})
 	require.Equal(t, uint64(0), h1)
 
-	h1 = hashForTraceQLQuery("")
+	h1 = hashForSearchRequest(&tempopb.SearchRequest{Query: ""})
 	require.Equal(t, uint64(0), h1)
+
+	// same queries with different spss and limit should have the different hash
+	h1 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` }", Limit: 1})
+	h2 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` }", Limit: 2})
+	require.NotEqual(t, h1, h2)
+
+	h1 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` }", SpansPerSpanSet: 1})
+	h2 = hashForSearchRequest(&tempopb.SearchRequest{Query: "{ span.foo = `bar` }", SpansPerSpanSet: 2})
+	require.NotEqual(t, h1, h2)
 }