From b6312f4a8ed7bba536abe9efeb65360935f98cd6 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 10 Apr 2024 12:54:25 -0400 Subject: [PATCH] Add spss and limit to search request hash (#3557) * Add spss and limit to search request hash Signed-off-by: Joe Elliott * changelog Signed-off-by: Joe Elliott * lint Signed-off-by: Joe Elliott --------- Signed-off-by: Joe Elliott --- CHANGELOG.md | 1 + modules/frontend/search_handlers_test.go | 4 ++-- modules/frontend/search_sharder.go | 19 ++++++++++------ modules/frontend/search_sharder_test.go | 29 ++++++++++++++++-------- 4 files changed, 34 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 081b041002a..0643dbc91b9 100644 --- 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 index 2b2d50a4043..cbb53a2873a 100644 --- 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 index f025250e10b..e8753afff4f 100644 --- 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 index f4bd190aada..b229951ffbb 100644 --- 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) }